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
