Re: [PATCH 1/6] hash.h: remove unused define directive

[Isabella B do Amaral]

Hi, David,

On Thu, Aug 26, 2021 at 1:01 AM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso <isabellabdoamaral@xxxxxx> wrote:
> >
> > The HAVE_ARCH_HASH_32 (single underscore) define hasn't been used for
> > any known supported architectures that have their own hash function
> > implementation (i.e. m68k, Microblaze, H8/300, pa-risc) since George's
> > patch [1], which introduced it.
> >
> > The supported 32-bit architectures from the list above have only been
> > making use of the (more general) HAVE_ARCH__HASH_32 define, which only
> > lacks the right shift operator, that wasn't targeted for optimizations
> > so far.
> >
> > [1] https://lore.kernel.org/lkml/20160525073311.5600.qmail@xxxxxxxxxxxxxxxxxxxxxx/
> >
> > Co-developed-by: Augusto Durães Camargo <augusto.duraes33@xxxxxxxxx>
> > Signed-off-by: Augusto Durães Camargo <augusto.duraes33@xxxxxxxxx>
> > Co-developed-by: Enzo Ferreira <ferreiraenzoa@xxxxxxxxx>
> > Signed-off-by: Enzo Ferreira <ferreiraenzoa@xxxxxxxxx>
> > Signed-off-by: Isabella Basso <isabellabdoamaral@xxxxxx>
> > ---
>
> I'm not familiar with the hash functions here, so take this with the
> appropriate heap of salt, but it took me a little while to understand
> exactly what this is doing.
>
> As I understand it:
> - There are separate __hash_32() and hash_32() functions.
> - Both of these have generic implementations, which can optionally be
> overridden by an architecture-specific optimised version.
> - There aren't any architectures which provide an optimised hash_32()
> implementation.
> - This patch therefore removes support for architecture-specific
> hash_32() implementations, and leaves only the generic implementation.
> - This generic implementation of hash_32() itself relies on
> __hash_32(), which may still be optimised.
>
> Could the commit description be updated to make this a bit clearer?

Sure, that makes perfect sense! Thank you very much for the feedback! Writing
those descriptions was quite a challenge for me, as I wasn't perfectly sure of
what the appropriate reasoning should be for the message. I'm also glad I was
able to get a grasp similar to yours. :)

> While we are getting rid of the HAVE_ARCH_HASH_32 #define, that seems
> to be a side-effect/implementation detail of removing support for
> architecture-specific hash_32() implementations...
>
> The other wild, out-there option would be to remove __hash_32()
> entirely and make everything use hash_32(), which then could have
> architecture-specific implementations. A quick grep reveals that
> there's only one use of __hash_32() outside of the hashing code itself
> (in fs/namei.c). This would be much more consistent with what
> hash_64() does, but also would be significantly more work, and
> potentially could have some implication (full_name_hash() performance
> maybe?) which I'm not aware of. So it's possibly not worth it.

I do agree with you that it seems a bit over the top, as I'm also really not
aware of the performance implications of such a change (and that seemed to be
what motivated most of the patch series that introduced the __hash_32() to
fs/namei.c), so I'd rather not mess with fs/namei.c based on consistency
reasons alone.

Thanks,
--
Isabella Basso