On Sat, Mar 09, 2019 at 03:53:41PM +0000, l...@sdf.org 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
