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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Oct 29, 2019 at 03:47:42PM +0000, Parav Pandit wrote:
> 
> 
> > From: Jason Gunthorpe <jgg@mellanox.com>
> > Sent: Tuesday, October 29, 2019 10:34 AM
> > 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 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.

> > It looks just as incorrect to have the one line call
> 
> No, its not incorrect. Because device.c provides functionality to
> register/unregister handler and dispatch event.  While cache layer
> deals with cache updates etc, and uses wq service provided device.c
> If we rename ib_dispatch_event() to ib_dispatch_cache_event() it
> make sense to move to cache.c However we have non cache specific
> events there too, so current code split between cache.c and device.c
> looks appropriate.

It always looks bad to have a single line function call like that,
especially just for spurious reasons like file placement or
functioning naming. It shows something is being done wrong

Jason