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

From: Jason Gunthorpe <jgg@mellanox.com>
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
Date: Wed, 23 Oct 2019 17:40:41 +0000	[thread overview]
Message-ID: <20191023174037.GY22766@mellanox.com> (raw)
In-Reply-To: <AM0PR05MB4866B5ABA8F86D60A3FEE299D16B0@AM0PR05MB4866.eurprd05.prod.outlook.com>

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??

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..

Jason