Re: [PATCH 1/5] lib/sort: Make swap functions more generic

[Andy Shevchenko]

On Sat, Mar 09, 2019 at 03:53:41PM +0000, lkml@xxxxxxx wrote:
> Andy Shevchenko wrote:
> > On Thu, Feb 21, 2019 at 06:30:28AM +0000, George Spelvin wrote:
> >> +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> > 
> > Why #ifdef is better than if (IS_ENABLED()) ?
> 
> Because CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS is bool and not
> tristate.  IS_ENABLED tests for 'y' or 'm' but we don't need it
> for something that's only on or off.

There is IS_BUILTIN(), though it's a common practice to use IS_ENABLED() even
for boolean options (I think because of naming of the macro).

> Looking through the kernel, I see both, but #ifdef or #if defined()
> are definitely in the majority:
> 
> lib/lzo/lzo1x_compress.c:#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && defined(LZO_USE_CTZ64)
> lib/siphash.c:#ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> lib/string.c:#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> lib/strncpy_from_user.c:#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> lib/zlib_inflate/inffast.c:#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> 
> I see a few IS_ENABLED uses in include/crypto/ and kernel/bpf/.
> 
> It makes no real difference; #ifdef is simpler to me.


> static bool __attribute_const__
> is_aligned(const void *base, size_t size, unsigned char align)
> {
> 	unsigned char lsbits = (unsigned char)size;
> #ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> 	(void)base;
> #else
> 	lsbits |= (unsigned char)(uintptr_t)base;
> #endif
> 	return (lsbits & (align - 1)) == 0;
> }

> Any preference?

This one looks better in a sense we don't suppress the warnings when it's not
needed.

> > For such primitives that operates on top of an arrays we usually append 's' to
> > the name. Currently the name is misleading.
> > 
> > Perhaps u32s_swap().
> 
> I don't worry much about the naming of static helper functions.
> If they were exported, it would be a whole lot more important!
> 
> I find "u32s" confusing; I keep reading the "s" as "signed" rather
> than a plural.

For signedness we use prefixes, for plural â suffixes. I don't see the point of
confusion. And this is in use in kernel a lot.

> How about one of:
> swap_bytes / swap_ints / swap_longs
> swap_1 / swap_4 / swap_8

longs are ambiguous, so I would prefer bit-sized types.

> > Shouldn't simple memcpy cover these case for both 32- and 64-bit architectures?
> 
> This isn't a memcpy, it's a memory *swap*.  To do it with memcpy
> requires:
> 	memcpy(temp_buffer, a, size);
> 	memcpy(a, b, size);
> 	memcpy(b, temp_buffer, size);
> 
> This is 1.5x as much memory access, and you have to find a large
> enough temp_buffer.  (I didn't think a variable-length array on
> the stack would make people happy.)
> 
> Also, although it is a predictable branch, memcpy() has to check the
> alignment of its inputs each call.  The reason for these helpers is
> to factor that out.

Makes sense.

-- 
With Best Regards,
Andy Shevchenko