Hi, David,
On Thu, Aug 26, 2021 at 1:01 AM David Gow davidgow@google.com wrote:
On Thu, Aug 26, 2021 at 9:26 AM Isabella Basso isabellabdoamaral@usp.br 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@ns.sciencehorizons.ne...
Co-developed-by: Augusto Durães Camargo augusto.duraes33@gmail.com Signed-off-by: Augusto Durães Camargo augusto.duraes33@gmail.com Co-developed-by: Enzo Ferreira ferreiraenzoa@gmail.com Signed-off-by: Enzo Ferreira ferreiraenzoa@gmail.com Signed-off-by: Isabella Basso isabellabdoamaral@usp.br
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