On Tue, Mar 05, 2019 at 10:31:50AM +0200, Gal Pressman wrote:
> On 04-Mar-19 22:45, Jason Gunthorpe wrote:
> > On Wed, Feb 27, 2019 at 10:41:03AM +0200, Leon Romanovsky wrote:
> >> On Wed, Feb 27, 2019 at 09:58:10AM +0200, Gal Pressman wrote:
> >>> On 26-Feb-19 20:03, Steve Wise wrote:
> >>>>> +#define efa_stat_inc(dev, stat) \
> >>>>> + do { \
> >>>>> + typeof(dev) _dev = dev; \
> >>>>> + unsigned long flags; \
> >>>>> + \
> >>>>> + spin_lock_irqsave(&_dev->stats_lock, flags); \
> >>>>> + (stat)++; \
> >>>>> + spin_unlock_irqrestore(&_dev->stats_lock, flags); \
> >>>>> + } while (0)
> >>>>> +
> >>>>
> >>>>
> >>>> Would this be more safe as a static inline function where you explicitly
> >>>> type the parameters?
> >>>
> >>> The typeof looks like an overkill.
> >>> I prefer to keep it as a define, but I will thin it down a little.
> >
> > You should always avoid define functions - do it only if you need to
> > meta-program something..
> >
> >> Generally speaking, we are not super excited to see abstractions over
> >> basic kernel primitives, especially if such abstractions don't do a lot,
> >> but hiding locks.
> >
> > I'm confused what this anyhow.. This looks a lot like an atomic64_t incr.
> >
> > The atomic version is surely faster than the spinlock version, so this
> > should probably change..
>
> The main thing we wanted to avoid is adding another ATOMIC_INIT() for every new
> counter along the way, I'll change that.
Open coding atomic64 is worse than a list of ATOMIC_INIT's.
Jason
