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