<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 12 (filtered medium)"><style></style></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><a name="_MailEndCompose"><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Hi Bili,<o:p></o:p></span></a></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>OK. I’m not familiar with this library, but the other function assumes 8 byte aligned data, so this should be OK too.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>And with the required function signature, I now understand why you use endian conversions this way.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Acked-by: Morten Brørup <mb@smartsharesystems.com><o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span lang=DA style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Med venlig hilsen / Kind regards,<o:p></o:p></span></p><p class=MsoNormal><span lang=DA style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>-Morten Brørup<o:p></o:p></span></p><p class=MsoNormal><span lang=DA style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><div style='border:none;border-left:solid blue 1.5pt;padding:0cm 0cm 0cm 4.0pt'><div><div style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm'><p class=MsoNormal><b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>From:</span></b><span style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'> Bili Dong [mailto:qobilidop@gmail.com] <br><b>Sent:</b> Wednesday, 15 February 2023 22.40<br><b>To:</b> Morten Brørup<br><b>Cc:</b> yipeng1.wang@intel.com; sameh.gobriel@intel.com; bruce.richardson@intel.com; vladimir.medvedkin@intel.com; cristian.dumitrescu@intel.com; dev@dpdk.org<br><b>Subject:</b> Re: [PATCH v3] hash: add XOR32 hash function<o:p></o:p></span></p></div></div><p class=MsoNormal><o:p> </o:p></p><div><p class=MsoNormal>Hi Morten,<o:p></o:p></p><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Thanks for your comments!<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>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?<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>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.<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>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?<o:p></o:p></p></div><div><p class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal>Thanks,<o:p></o:p></p></div><div><p class=MsoNormal>Bili<o:p></o:p></p></div></div><p class=MsoNormal><o:p> </o:p></p><div><div><p class=MsoNormal>On Wed, Feb 15, 2023 at 3:39 AM Morten Brørup <<a href="mailto:mb@smartsharesystems.com">mb@smartsharesystems.com</a>> wrote:<o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><p class=MsoNormal style='margin-bottom:12.0pt'>> 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" target="_blank">https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/net/rte_ip.h#L162</a><br><br><o:p></o:p></p></blockquote></div></div></div></body></html>