Re: [PATCH rdma-next v1 1/2] IB/core: Let IB core distribute cache update events

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Oct 29, 2019 at 06:11:01PM +0000, Parav Pandit wrote:
> 
> 
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Tuesday, October 29, 2019 10:55 AM
> > To: Parav Pandit <parav@mellanox.com>
> > Cc: Leon Romanovsky <leon@kernel.org>; Doug Ledford
> > <dledford@redhat.com>; Leon Romanovsky <leonro@mellanox.com>; RDMA
> > mailing list <linux-rdma@vger.kernel.org>; Daniel Jurgens
> > <danielj@mellanox.com>
> > 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@mellanox.com>
> > > > Sent: Tuesday, October 29, 2019 10:34 AM
> > > > To: Parav Pandit <parav@mellanox.com>
> > > > Cc: Leon Romanovsky <leon@kernel.org>; Doug Ledford
> > > > <dledford@redhat.com>; Leon Romanovsky <leonro@mellanox.com>;
> > RDMA
> > > > mailing list <linux-rdma@vger.kernel.org>; Daniel Jurgens
> > > > <danielj@mellanox.com>
> > > > 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