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

From: Parav Pandit <parav@mellanox.com>
To: Jason Gunthorpe <jgg@mellanox.com>, Leon Romanovsky <leon@kernel.org>
Cc: 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 05:08:56 +0000	[thread overview]
Message-ID: <AM0PR05MB4866B5ABA8F86D60A3FEE299D16B0@AM0PR05MB4866.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <20191022195518.GO22766@mellanox.com>


> -----Original Message-----
> From: Jason Gunthorpe <jgg@mellanox.com>
> Sent: Tuesday, October 22, 2019 2:55 PM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: Doug Ledford <dledford@redhat.com>; Leon Romanovsky
> <leonro@mellanox.com>; RDMA mailing list <linux-rdma@vger.kernel.org>;
> Daniel Jurgens <danielj@mellanox.com>; Parav Pandit
> <parav@mellanox.com>
> Subject: Re: [PATCH rdma-next 1/3] IB/core: Let IB core distribute cache
> update events
> 
> On Sun, Oct 20, 2019 at 09:54:25AM +0300, Leon Romanovsky wrote:
> > diff --git a/drivers/infiniband/core/device.c
> > b/drivers/infiniband/core/device.c
> > index 2f89c4d64b73..e9ab1289c224 100644
> > +++ b/drivers/infiniband/core/device.c
> > @@ -1951,15 +1951,7 @@ void ib_unregister_event_handler(struct
> > ib_event_handler *event_handler)  }
> > EXPORT_SYMBOL(ib_unregister_event_handler);
> >
> > -/**
> > - * 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)
> > +void ib_dispatch_cache_event_clients(struct ib_event *event)
> >  {
> 
> no kdoc for this?
>
I dropped the kdoc because it was an internal API, not exposed to other kernel modules. 
 And added for the external one below.

> >  	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()?
If so, yes, I think it should be fine.
Only event that I wanted to deliver faster was IB_EVENT_SRQ_LIMIT_REACHED.
However given no kernel consumer interested in it, doing fast event delivery for such event is not so useful.
We can slim down ib_is_cache_update_event().