Re: [PATCH akpm/next] lib: crc32: conditionally constify crc32 lookup table

[Daniel Borkmann]

On 01/03/2015 12:35 AM, Andrew Morton wrote:
> On Wed, 31 Dec 2014 20:03:28 +0100 Daniel Borkmann <dborkman@xxxxxxxxxx> wrote:
>
> > Commit 8f243af42ade ("sections: fix const sections for crc32 table")
>
> The 8f243af42ade changelog is rather poor :(.  With the help of this
> changelog I can now see what 8f243af42ade was doing.  I must have been
> asleep at the time.
>
> > removed the compile-time generated crc32 tables from the RO sections,
> > because it conflicts with the definition of __cacheline_aligned
> > which puts all such aligned data into .data..cacheline_aligned section
> > optimized for wasting less space, and causes const align issues with
> > some GCC versions (see #52181, for example).
>
> (searches several bugzilla databases)
>
> "https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52181"; would be more
> reader-friendly.

Okay, will add it like that.

> > We can fix that in two steps: 1) by using the ____cacheline_aligned
> > version, which only aligns the data but doesn't move it into specific
> > sections, 2) test GCC and in problematic cases fall back to the current
> > code, otherwise use const and proper alignment for the lookup tables.
> >
> > After patch tables are in RO:
> >
> > $ nm -v lib/crc32.o | grep -1 -E "crc32c?table"
> > 0000000000000000 t arch_local_irq_enable
> > 0000000000000000 r crc32ctable_le
> > 0000000000000000 t crc32_exit
> > --
> > 0000000000000960 t test_buf
> > 0000000000002000 r crc32table_be
> > 0000000000004000 r crc32table_le
> > 000000001d1056e5 A __crc_crc32_be
> >
> > Signed-off-by: Daniel Borkmann <dborkman@xxxxxxxxxx>
> > Cc: Joe Mario <jmario@xxxxxxxxxx>
> > ---
> >   Makefile                   |  5 +++++
> >   lib/Makefile               |  3 +++
> >   lib/gen_crc32table.c       | 21 +++++++++++++++------
> >   scripts/gcc-const-align.sh | 21 +++++++++++++++++++++
> >   4 files changed, 44 insertions(+), 6 deletions(-)
>
> Seems a lot of fuss.  Why are these tables cacheline aligned anyway?
> To avoid one cache miss (most of the time, presumably) in a 16k table.
> Pretty marginal benefit, I suspect.

I guess, it actually came in with the slice-by-8 algorithm (e.g. used
in SCTP checksumming if no offloading is available) that was added back
then, that is, commit 324eb0f17d9dc ("crc32: add slice-by-8 algorithm
to existing code").

> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -171,6 +171,9 @@ obj-$(CONFIG_FONT_SUPPORT) += fonts/
> >   hostprogs-y	:= gen_crc32table
> >   clean-files	:= crc32table.h
> >
> > +# We need to transfer this flag to the host compiler if present
> > +HOSTCFLAGS_gen_crc32table.o := $(findstring -DCC_HAVE_CONST_ALIGN,$(KBUILD_CFLAGS))
> > +
> >   $(obj)/crc32.o: $(obj)/crc32table.h
> >
> >   quiet_cmd_crc32 = GEN     $@
> > diff --git a/lib/gen_crc32table.c b/lib/gen_crc32table.c
> > index 71fcfcd..2f06893 100644
> > --- a/lib/gen_crc32table.c
> > +++ b/lib/gen_crc32table.c
> > @@ -21,6 +21,14 @@
> >   # define BE_TABLE_SIZE (1 << CRC_BE_BITS)
> >   #endif
> >
> > +#ifdef CC_HAVE_CONST_ALIGN
> > +# define TABLE_CONST_ATTR "const"
> > +# define TABLE_ALIGNMENT  "____cacheline_aligned"
> > +#else
> > +# define TABLE_CONST_ATTR ""
> > +# define TABLE_ALIGNMENT  "__cacheline_aligned"
> > +#endif
>
> Pity out poor readers, trying to work out what all this does and why it
> is here.  It is totally unobvious that this is working around some gcc
> bug.  Can we please have a nice comment which explains everything?

Will add a comment, sure.

Thanks, Andrew!

I'll send out v2 with your feedback tomorrow.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/