> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> Sent: Tuesday, October 22, 2019 2:55 PM
> To: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: Doug Ledford <dledford@xxxxxxxxxx>; Leon Romanovsky
> <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux-rdma@xxxxxxxxxxxxxxx>;
> Daniel Jurgens <danielj@xxxxxxxxxxxx>; Parav Pandit
> <parav@xxxxxxxxxxxx>
> 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().
