> -----Original Message-----
> From: Jason Gunthorpe <jgg@xxxxxxxx>
> Sent: Tuesday, October 29, 2019 1:26 PM
> To: Parav Pandit <parav@xxxxxxxxxxxx>
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA
> mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens
> <danielj@xxxxxxxxxxxx>
> Subject: Re: [PATCH rdma-next v1 1/2] IB/core: Let IB core distribute cache
> update events
>
> On Tue, Oct 29, 2019 at 06:11:01PM +0000, Parav Pandit wrote:
> >
> >
> > > From: Jason Gunthorpe <jgg@xxxxxxxx>
> > > Sent: Tuesday, October 29, 2019 10:55 AM
> > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> > > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>;
> RDMA
> > > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens
> > > <danielj@xxxxxxxxxxxx>
> > > 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@xxxxxxxxxxxx>
> > > > > Sent: Tuesday, October 29, 2019 10:34 AM
> > > > > To: Parav Pandit <parav@xxxxxxxxxxxx>
> > > > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford
> > > > > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>;
> > > RDMA
> > > > > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Daniel Jurgens
> > > > > <danielj@xxxxxxxxxxxx>
> > > > > 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)
>
It is not problematic unless we ask ULPs to execute most of their work in the notifier context.
It is sub optimal at the moment.
> I'm suggesting to swap it for a normal blocking notifier chain and guarantee
Before writing this version, I considered that option.
However the blocker was qp's event_handler callback.
In below two call traces rw_semaphore will be triggered in interrupt context.
So wanted to split the QP's event_handler with device->event_handler.
After I split both, than running device's event_handler will be possible with cache as first entry in chain.
Shall I revise the series to split two handlers?
qedr_iw_event_handler
qedr_iw_qp_event
hns_roce_v2_aeq_int
hns_roce_qp_event
qp->event_handler
__ib_shared_qp_event_handler()
> the cache is the first entry in the chain.
>
> Jason
