Hi Bili,

 

OK. I’m not familiar with this library, but the other function assumes 8 byte aligned data, so this should be OK too.

 

And with the required function signature, I now understand why you use endian conversions this way.

 

Acked-by: Morten Brørup <mb@smartsharesystems.com>

 

 

Med venlig hilsen / Kind regards,

-Morten Brørup

 

From: Bili Dong [mailto:qobilidop@gmail.com] 
Sent: Wednesday, 15 February 2023 22.40
To: Morten Brørup
Cc: yipeng1.wang@intel.com; sameh.gobriel@intel.com; bruce.richardson@intel.com; vladimir.medvedkin@intel.com; cristian.dumitrescu@intel.com; dev@dpdk.org
Subject: Re: [PATCH v3] hash: add XOR32 hash function

 

Hi Morten,

 

Thanks for your comments!

 

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?

 

The hash function signature has to conform to https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/table/rte_swx_hash_func.h#L31, 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.

 

I'm not sure about the byte alignment assumptions used in hash functions. My implementation basically follows the existing CRC32 hash: https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/hash/rte_hash_crc.h#L168, and I don't see byte alignment handled there. Maybe someone more familiar with lib/hash/ could provide some context on this?

 

Thanks,

Bili

 

On Wed, Feb 15, 2023 at 3:39 AM Morten Brørup <mb@smartsharesystems.com> wrote:

	> From: Bili Dong [mailto:qobilidop@gmail.com]
	> Sent: Wednesday, 15 February 2023 12.07
	> 
	> An XOR32 hash is needed in the Software Switch (SWX) Pipeline for its
	> use case in P4. We implement it in this patch so it could be easily
	> registered in the pipeline later.
	> 
	> Signed-off-by: Bili Dong <qobilidop@gmail.com>
	> ---
	
	[...]
	
	> +#define LEFT8b_MASK rte_cpu_to_be_32(0xff000000)
	> +#define LEFT16b_MASK rte_cpu_to_be_32(0xffff0000)
	> +
	> +/**
	> + * Calculate XOR32 hash on user-supplied byte array.
	> + *
	> + * @param data
	> + *   Data to perform hash on.
	> + * @param data_len
	> + *   How many bytes to use to calculate hash value.
	> + * @param init_val
	> + *   Value to initialise hash generator.
	> + * @return
	> + *   32bit calculated hash value.
	> + */
	> +static inline uint32_t
	> +rte_hash_xor(const void *data, uint32_t data_len, uint32_t init_val)
	> +{
	> +     uint32_t i;
	> +     uintptr_t pd = (uintptr_t) data;
	> +     init_val = rte_cpu_to_be_32(init_val);
	> +
	> +     for (i = 0; i < data_len / 4; i++) {
	> +             init_val ^= *(const uint32_t *)pd;
	> +             pd += 4;
	> +     }
	> +
	> +     if (data_len & 0x2) {
	> +             init_val ^= *(const uint32_t *)pd & LEFT16b_MASK;
	> +             pd += 2;
	> +     }
	> +
	> +     if (data_len & 0x1)
	> +             init_val ^= *(const uint32_t *)pd & LEFT8b_MASK;
	> +
	> +     init_val = rte_be_to_cpu_32(init_val);
	> +     return init_val;
	> +}
	
	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.
	
	I also suppose that the return type and the init_val parameter were meant to be rte_be32_t.
	
	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:
	https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/net/rte_ip.h#L162