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 <m...@smartsharesystems.com> wrote: > > From: Bili Dong [mailto:qobili...@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 <qobili...@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 > > >
