[openib-general] Re: RE: Re: [PATCH] ipoib_flush_paths

[Fabian Tillier]

On 4/6/06, Michael S. Tsirkin <mst at mellanox.co.il> wrote:
> Quoting r. Fabian Tillier <ftillier at silverstorm.com>:
> > Subject: Re: [openib-general] Re: RE: Re: [PATCH] ipoib_flush_paths
> >
> > On 4/6/06, Michael S. Tsirkin <mst at mellanox.co.il> wrote:
> > > Quoting r. Muli Ben-Yehuda <mulix at mulix.org>:
> > > > Subject: Re: [openib-general] Re: RE: Re: [PATCH] ipoib_flush_paths
> > > >
> > > > On Thu, Apr 06, 2006 at 04:38:33PM +0300, Michael S. Tsirkin wrote:
> > > >
> > > > > No, since we are keeping a callback pointer into that module.
> > > >
> > > > Sorry if I'm being dense but I don't see it in this patch. Point me at
> > > > it?
> > >
> > > You don't see it in the patch because SA already kept a callback pointer -
> > > that's the race I'm solving. Look in sa_query.c
> > >
> > >
> > > If I have
> > >
> > > struct query {
> > >        void (*callback)();
> > >        struct module *owner;
> > > }
> > >
> > > Then it is always safe to do
> > >
> > >        __get_module(query->owner);
> > >        query->callback();
> > >        put_module(query->owner);
> > >
> > > since it is the called module's responsibility to invalidate
> > > all such query objects before its unloaded.
> >
> > Wait, why are you doing __get_module just before the callback?  This
> > leaves the possibility of crashing - sure, you'll detect that things
> > went wrong, but you haven't solved the issue.  The whole point of the
> > reference is to prevent the crash.
>
> No, this problem is solved today in all ULPs by caller polling on flag
> (completion) that callback sets: all ULPs do this already, since the need to
> track resources irrespective of module being unloaded.
>
> > You need to call __get_module from the context of teh caller making the request.
>
> No, this would prevent module from being unloaded for extended periods
> of time - we don't want this.

I don't get it - as long as a query is outstanding (i.e. there will be
a callback invoked at some point in the future), it's unsafe for the
module to unload.  Taking a reference on the module when the request
is issued, rather than just before the callback is invoked, should
guarantee that __get_module succeeds (since you're in the context of
the caller).  Right now you're requiring the user to keep a parallel
reference count, when you could just handle it for them.  It would
also be completely fool proof - there would be no way for a module to
go away while a callback is outstanding, whether or not the callback
has started executing or not.

> All I must prevent is problem we missed previously: module being unloaded
> while callback is in progress.

No, you must prevent the module from being unloaded while a callback
is *outstanding*, which includes in progress but is a broader scope.

Using __get_module at the time the request is issued eliminates the
possibility of __try_get_module failing right before you invoke the
callback (though in practice __try_get_module would be eliminated
since you'd already hold a reference).

- Fab