RE: [PATCH rdma-next 1/3] IB/core: Let IB core distribute cache update events

[Parav Pandit <parav@mellanox.com>]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@mellanox.com>
> Sent: Wednesday, October 23, 2019 12:41 PM
> 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 1/3] IB/core: Let IB core distribute cache update
> events
> 
> On Wed, Oct 23, 2019 at 05:08:56AM +0000, Parav Pandit wrote:
> > > >  	unsigned long flags;
> > > >  	struct ib_event_handler *handler; @@ -1971,6 +1963,22 @@ void
> > > > ib_dispatch_event(struct ib_event *event)
> > > >
> > > >  	spin_unlock_irqrestore(&event->device->event_handler_lock,
> > > > flags); }
> > > > +
> > > > +/**
> > > > + * 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) {
> > > > +	if (ib_is_cache_update_event(event))
> > > > +		ib_enqueue_cache_update_event(event);
> > > > +	else
> > > > +		ib_dispatch_cache_event_clients(event);
> > > > +}
> > > >  EXPORT_SYMBOL(ib_dispatch_event);
> > >
> > > It seems like there is now some big mess here, many of the users of
> > > events, including cache, acctually do need a blocking context to do
> > > their work, while this function is supposed to be atomic context for the
> driver.
> > >
> > > So, after this change, many event types are now guarenteed to be
> > > called from a blocking context in a WQ - but we still go ahead and
> > > do silly things like launch more work to get into blocking contexts
> > > from the other users
> > >
> > > Thus I'm wondering if this wouldn't be better off just always
> > > pushing events into a wq and running the notifier subscriptions
> sequentially?
> > >
> > Are you saying we should drop the else part above and always do
> > ib_enqueue_cache_update_event()?
> 
> Yes, but also now saying that all notifier callbacks are called from work queues
> and can block for short periods.
> 
> This seems it would simplify many of the users??
> 
Yes, however that optimization should be a different series per ULP/consumer/event based.
Will send v1 through Leon's queue.

> And not using the ib_enqueue_cache_update_event() but a simple
> blocking_notifier_call_chain() with the cache always at the front
> 
> > Only event that I wanted to deliver faster was
> > IB_EVENT_SRQ_LIMIT_REACHED.
> 
> It might make sense to have an atomic event list for such things in future..
> 
Ok.

> Jason