Re: [Xen-devel] [PATCH 3/4] Support accelerated network plugin modules

[Muli Ben-Yehuda <muli@xxxxxxxxxx>]

On Wed, May 09, 2007 at 12:58:06PM +0100, Kieran Mansley wrote:

> > > +#define NETFRONT_LOCK_AND_CALL_ACCELERATOR_HOOK(_np, _hook,
> _args...)   \
> > > +        do {
> \
> > > +                unsigned _flags;
> \
> > > +                spin_lock_irqsave(&(_np)->accelerator_lock,
> _flags);    \
> > > +                if((_np)->accelerator && (_np)-
> >accel_vif_state.hooks)  \
> > > +                        (_np)->accel_vif_state.hooks->_hook(_args);
> \
> > > +                spin_unlock_irqrestore(&(_np)->accelerator_lock,
> _flags); \
> > > +        } while(0)
> > 
> > Please get rid of these macros - it's not exactly a lot of code to
> > duplicate and it makes it obvious what's going on.
> 
> The first macro I'm happy to get rid of - I noticed after Keir
> commented on the use of caps in the name that it's no longer used.
> The second I think is enough code that it would unnecessarily
> clutter the existing functions.  For this reason I'd rather leave it
> in (with a lower-case name).

It's a matter of taste, but I'd prefer it if it was obvious when
looking at the code that the hook is being called with a spinlock
held. That's actually another thing - why must every hook be called
with the spinlock held? if it's to protect the accelerator from going
away, what's actually needed is a ref count (struct kref) on the
accelerator.

Cheers,
Muli

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel