On Tue, Oct 29, 2019 at 06:11:01PM +0000, Parav Pandit wrote:
>
>
> > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > Sent: Tuesday, October 29, 2019 10:55 AM
> > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA
> > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens
> > <danielj@xxxxxxxxxxxx>
> > Subject: Re: [PATCH rdma-next v1 1/2] IB/core: Let IB core distribute cache
> > update events
> >
> > On Tue, Oct 29, 2019 at 03:47:42PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > > Sent: Tuesday, October 29, 2019 10:34 AM
> > > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> > > > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>;
> > RDMA
> > > > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens
> > > > <danielj@xxxxxxxxxxxx>
> > > > Subject: Re: [PATCH rdma-next v1 1/2] IB/core: Let IB core
> > > > distribute cache update events
> > > >
> > > > On Tue, Oct 29, 2019 at 03:32:46PM +0000, Parav Pandit wrote:
> > > >
> > > > > > > +/**
> > > > > > > + * ib_dispatch_event - Dispatch an asynchronous event
> > > > > > > + * @event:Event to dispatch
> > > > > > > + *
> > > > > > > + * Low-level drivers must call ib_dispatch_event() to
> > > > > > > +dispatch the
> > > > > > > + * event to all registered event handlers when an
> > > > > > > +asynchronous event
> > > > > > > + * occurs.
> > > > > > > + */
> > > > > > > +void ib_dispatch_event(struct ib_event *event) {
> > > > > > > + ib_enqueue_cache_update_event(event);
> > > > > > > +}
> > > > > > > EXPORT_SYMBOL(ib_dispatch_event);
> > > > > >
> > > > > > Why not just move this into cache.c?
> > > > > >
> > > > > Same thought same to me when I had to add one liner call.
> > > > > However the issue was device.c has the code for the event
> > > > registration/unregistration and calling the handlers unrelated to cache.
> > > > > So moving ib_dispatch_event() to cache.c looked incorrect to me.
> > > >
> > > > Well, maybe we can move the wq code from the cache.c into here?
> > >
> > > I prefer to keep all cache specific code in cache.c because there is
> > > where we flush the ib_wq, queue work there.
> >
> > I think that is because the cache is not subscribing to a notifier, it really should,
> > then things order properly and the flush is not needed.
> >
> Cache shouldn't subscribe to the notifier, that is the race
> condition issue.
Notifiers are ordered, as long as the cache subscribes first to the
notifier list it is not a 'race condition'.
This is still somewhat problematic because 'ib_dispatch_event' still
calls the handlers under a spinlock (ie the handlers are still
non-blocking contexts)
I'm suggesting to swap it for a normal blocking notifier chain and
guarentee the cache is the first entry in the chain.
Jason
