[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <18d61c52-be6f-4ef3-b020-d597ba7cdaeb@daynix.com>
Date: Thu, 19 Sep 2024 14:51:43 +0200
From: Akihiko Odaki <akihiko.odaki@...nix.com>
To: gur.stavi@...wei.com
Cc: andrew@...nix.com, corbet@....net, davem@...emloft.net,
edumazet@...gle.com, jasowang@...hat.com, kuba@...nel.org,
kvm@...r.kernel.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
mst@...hat.com, netdev@...r.kernel.org, pabeni@...hat.com, shuah@...nel.org,
virtualization@...ts.linux-foundation.org, willemdebruijn.kernel@...il.com,
xuanzhuo@...ux.alibaba.com, yuri.benditovich@...nix.com
Subject: Re: [PATCH RFC v3 2/9] virtio_net: Add functions for hashing
On 2024/09/16 10:01, gur.stavi@...wei.com wrote:
>> +
>> +static inline void virtio_net_toeplitz(struct virtio_net_toeplitz_state *state,
>> + const __be32 *input, size_t len)
>>
>> The function calculates a hash value but its name does not make it
>> clear. Consider adding a 'calc'.
>>
>> +{
>> + u32 key;
>> +
>> + while (len) {
>> + state->key++;
>> + key = be32_to_cpu(*state->key);
>>
>> You perform be32_to_cpu to support both CPU endianities.
>> If you will follow with an unconditional swab32, you could run the
>> following loop on a more natural 0 to 31 always referring to bit 0
>> and avoiding !!(key & bit):
>>
>> key = swab32(be32_to_cpu(*state->key));
>> for (i = 0; i < 32; i++, key >>= 1) {
>> if (be32_to_cpu(*input) & 1)
>> state->hash ^= state->key_buffer;
>> state->key_buffer = (state->key_buffer << 1) | (key & 1);
>> }
>>
>
> Fixing myself, in previous version 'input' was tested against same bit.
> Advantage is less clear now, replacing !! with extra shift.
> However, since little endian CPUs are more common, the combination of
> swab32(be32_to_cpu(x) will actually become a nop.
> Similar tactic may be applied to 'input' by assigning it to local
> variable. This may produce more efficient version but not necessary
> easier to understand.
>
> key = bswap32(be32_to_cpu(*state->key));
> for (u32 bit = BIT(31); bit; bit >>= 1, key >>= 1) {
> if (be32_to_cpu(*input) & bit)
> state->hash ^= state->key_buffer;
> state->key_buffer =
> (state->key_buffer << 1) | (key & 1);
> }
This unfortunately does not work. swab32() works at *byte*-level but we
need to reverse the order of *bits*. bitrev32() is what we need, but it
cannot eliminate be32_to_cpu().
Regards,
Akihiko Odaki
Powered by blists - more mailing lists