<div dir="ltr">Hi Morten,<div><br></div><div>Thanks for your comments!</div><div><br></div><div>For endianness conversion, I double-checked my usages. I did use both rte_cpu_to_be_32() and rte_be_to_cpu_32(). I might have missed something but I think I used them (4 occurrences) in a semantically meaningful way. Could you point me to the lines that are confusing?</div><div><br></div><div>The hash function signature has to conform to <a href="https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/table/rte_swx_hash_func.h#L31">https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/table/rte_swx_hash_func.h#L31</a>, so I don't have the freedom to change the parameter type to rte_be32_t, although personally I agree with you and would prefer to make everything consistently big-endian here.</div><div><br></div><div>I'm not sure about the byte alignment assumptions used in hash functions. My implementation basically follows the existing CRC32 hash: <a href="https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/hash/rte_hash_crc.h#L168">https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/hash/rte_hash_crc.h#L168</a>, and I don't see byte alignment handled there. Maybe someone more familiar with lib/hash/ could provide some context on this?</div><div><br></div><div>Thanks,</div><div>Bili</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 15, 2023 at 3:39 AM Morten Brørup <<a href="mailto:mb@smartsharesystems.com">mb@smartsharesystems.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> From: Bili Dong [mailto:<a href="mailto:qobilidop@gmail.com" target="_blank">qobilidop@gmail.com</a>]<br>
> Sent: Wednesday, 15 February 2023 12.07<br>
> <br>
> An XOR32 hash is needed in the Software Switch (SWX) Pipeline for its<br>
> use case in P4. We implement it in this patch so it could be easily<br>
> registered in the pipeline later.<br>
> <br>
> Signed-off-by: Bili Dong <<a href="mailto:qobilidop@gmail.com" target="_blank">qobilidop@gmail.com</a>><br>
> ---<br>
<br>
[...]<br>
<br>
> +#define LEFT8b_MASK rte_cpu_to_be_32(0xff000000)<br>
> +#define LEFT16b_MASK rte_cpu_to_be_32(0xffff0000)<br>
> +<br>
> +/**<br>
> + * Calculate XOR32 hash on user-supplied byte array.<br>
> + *<br>
> + * @param data<br>
> + *   Data to perform hash on.<br>
> + * @param data_len<br>
> + *   How many bytes to use to calculate hash value.<br>
> + * @param init_val<br>
> + *   Value to initialise hash generator.<br>
> + * @return<br>
> + *   32bit calculated hash value.<br>
> + */<br>
> +static inline uint32_t<br>
> +rte_hash_xor(const void *data, uint32_t data_len, uint32_t init_val)<br>
> +{<br>
> +     uint32_t i;<br>
> +     uintptr_t pd = (uintptr_t) data;<br>
> +     init_val = rte_cpu_to_be_32(init_val);<br>
> +<br>
> +     for (i = 0; i < data_len / 4; i++) {<br>
> +             init_val ^= *(const uint32_t *)pd;<br>
> +             pd += 4;<br>
> +     }<br>
> +<br>
> +     if (data_len & 0x2) {<br>
> +             init_val ^= *(const uint32_t *)pd & LEFT16b_MASK;<br>
> +             pd += 2;<br>
> +     }<br>
> +<br>
> +     if (data_len & 0x1)<br>
> +             init_val ^= *(const uint32_t *)pd & LEFT8b_MASK;<br>
> +<br>
> +     init_val = rte_be_to_cpu_32(init_val);<br>
> +     return init_val;<br>
> +}<br>
<br>
I think that this function has swapped big endian and CPU endian everywhere. The result is the same, but the code would be much less confusing if using rte_cpu_32_to_be() when converting from CPU endian to big endian, and rte_be_to_cpu_32() when converting the other way.<br>
<br>
I also suppose that the return type and the init_val parameter were meant to be rte_be32_t.<br>
<br>
Also, please document that the byte array must be 32 bit aligned. Alternatively, implement support for unaligned data. You can find inspiration for handling of unaligned data in the __rte_raw_cksum() function:<br>
<a href="https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/net/rte_ip.h#L162" rel="noreferrer" target="_blank">https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/net/rte_ip.h#L162</a><br>
<br>
<br>
</blockquote></div>