DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bili Dong <qobilidop@gmail.com>
To: "Morten Brørup" <mb@smartsharesystems.com>
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
Date: Wed, 15 Feb 2023 13:39:54 -0800	[thread overview]
Message-ID: <CAFnY8dFrq21D332YSpZrb4ThZK62be3_i0oJ710bVqctNXyT+Q@mail.gmail.com> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87740@smartserver.smartshare.dk>

[-- Attachment #1: Type: text/plain, Size: 3342 bytes --]

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
>
>
>

[-- Attachment #2: Type: text/html, Size: 4504 bytes --]

  reply	other threads:[~2023-02-15 21:40 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-15 10:30 [PATCH] " Bili Dong
2023-02-15 10:54 ` [PATCH v2] " Bili Dong
2023-02-15 11:06   ` [PATCH v3] " Bili Dong
2023-02-15 11:39     ` Morten Brørup
2023-02-15 21:39       ` Bili Dong [this message]
2023-02-16  9:49         ` Morten Brørup
2023-02-20 13:49     ` Thomas Monjalon
2023-02-20 17:21       ` Bili Dong
2023-02-20 17:38         ` Thomas Monjalon
2023-02-20 17:51           ` Bruce Richardson
2023-02-20 17:54             ` Bili Dong
2023-02-20 18:19               ` Dumitrescu, Cristian
2023-02-20 18:50                 ` Bili Dong
2023-02-20 20:10     ` Medvedkin, Vladimir
2023-02-20 20:17       ` Bili Dong
2023-02-20 20:19     ` Dumitrescu, Cristian
2023-02-20 20:44       ` Bili Dong
2023-02-20 23:04         ` Stephen Hemminger
2023-02-21  1:38           ` Bili Dong
2023-02-21 16:47     ` [PATCH v4] " Bili Dong
2023-02-21 17:55       ` [PATCH v5] " Bili Dong
2023-02-21 19:37         ` [PATCH v6] " Bili Dong
2023-02-21 21:35           ` Bili Dong
2023-06-12 14:56           ` Thomas Monjalon
2023-06-15 17:14           ` Vladimir Medvedkin
2023-06-16 17:15             ` Bili Dong
2023-06-17 20:34               ` Mattias Rönnblom
2023-06-20 19:12           ` [PATCH v7 1/1] " Bili Dong
2023-06-28 19:19             ` Medvedkin, Vladimir
2023-06-29 17:33             ` [PATCH v8] " Bili Dong
2023-07-06 20:08               ` Stephen Hemminger
2023-07-10 22:01                 ` Bili Dong
2023-07-10 21:59               ` [PATCH v9] " Bili Dong
2023-09-29 15:38                 ` David Marchand
2023-10-03 16:51                 ` Medvedkin, Vladimir
2023-10-06  8:06                 ` David Marchand
2023-06-17 20:59 ` [PATCH] " Stephen Hemminger
2023-06-20 19:27   ` Bili Dong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFnY8dFrq21D332YSpZrb4ThZK62be3_i0oJ710bVqctNXyT+Q@mail.gmail.com \
    --to=qobilidop@gmail.com \
    --cc=bruce.richardson@intel.com \
    --cc=cristian.dumitrescu@intel.com \
    --cc=dev@dpdk.org \
    --cc=mb@smartsharesystems.com \
    --cc=sameh.gobriel@intel.com \
    --cc=vladimir.medvedkin@intel.com \
    --cc=yipeng1.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).