Re: [PATCH v1 2/2] bitmap: Optimize memset() calls

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Aug 18, 2023 at 08:53:38AM +0200, Rasmus Villemoes wrote:
> On 17/08/2023 18.54, Andy Shevchenko wrote:

...

> > +#if BITS_PER_LONG == 64
> > +		memset64((uint64_t *)dst, 0, len);
> > +#else
> > +		memset32((uint32_t *)dst, 0, len);
> > +#endif
> >  }
> 
> So _if_ this is worth it at all,

Yury, I believe you have a set of performance tests for bitmaps, perhaps you
can give a try and see if we got a benefit here.

Yet, not all architectures have custom implementation of the optimized
memset()/memcpy(). But at least ARM, x86 do.

> all those new '#if BITS_PER_LONG == 64'
> suggests that we should instead have a new helper memset_long(), no?
> 
> In fact, string.h already has that:
> 
> static inline void *memset_l(unsigned long *p, unsigned long v,
>                 __kernel_size_t n)

My grep skills suddenly degraded, my thoughts were exactly like yours,
but I missed the _l instead of _long for which I grepped.

...

> Please just spell an all-ones long "~0UL", that also matches the
> small_const case.

OK!

...

> > -		memset(&dst[lim - off], 0, off*sizeof(unsigned long));
> > +		bitmap_zero(&dst[lim - off], off);
> 
> This... can't be right.

Indeed, I have in mind memset_long() and on the half-way replaced it with
bitmap_zero().

> bitmap_zero() still takes an argument which is
> the size in bits, while off is the whole number of words to shift. So if
> called with say shift=128, we'd have off==2, and that bitmap_zero()
> would, because bitmap_zero() rounds up to a whole number of words, end
> up clearing just one word.
> 
> Perhaps a chance to add some more test cases? Maybe we're not exercising
> any of the "shift more than BITS_PER_LONG" logic.
> 
> But rather than using bitmap_zero() here, forcing you to convert off to
> a number of bits and then bitmap_zero to divide again, if you do this at
> all, just change that memset() to memset_l().

Agree.

-- 
With Best Regards,
Andy Shevchenko