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.
