[openib-general] Re: [PATCH] ipoib_flush_paths

[Michael S. Tsirkin]

Quoting r. Roland Dreier <rdreier at cisco.com>:
> Subject: Re: [PATCH] ipoib_flush_paths
> 
>     Michael> Actually, it turned out to be the simplest solution - and
>     Michael> quite elegant since there's no room for mistakes: if
>     Michael> query is going to be running this means module is still
>     Michael> loaded so we can take a reference to it without races.
> 
> Yes, this is suprisingly clean.
> 
>     Michael> As a bonus, and assertion inside __module_get increases
>     Michael> the chance to catch races where user forgets to cancel
>     Michael> the query - much nicer than crashing randomly.
> 
> Actually I think __module_get() will do the wrong thing if called
> during module unloading -- it doesn't test module_is_live().  In other
> words, calling __module_get() without already holding a ref has a
> race: __try_stop_module() can see the ref count as 0, then
> __module_get() can increment it, and then __try_stop_module() sets the
> module state to GOING and returns.
> 
> So the right thing to do is BUG_ON(!try_module_get(owner))

A bit more code but better debugging then. OK.

> Also, I don't think that a consumer of ib_sa() would ever pass an
> owner other than THIS_MODULE.  So how about if we keep the API the
> same and just do the THIS_MODULE stuff in an inline wrapper?

Good idea.

> Like the following...  it ends up being a pretty big diff, but just
> because I moved some comments around and so on.  Also I put the
> try_module_get() stuff out of line into call_sa_callback(), because
> the compiled code ends up smaller that way.
> 
> Does anyone disagree with this patch?  Michael, are you happy with
> this tweaked version of yours?

I'm happy, go ahead.

-- 
MST