From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3944E41C9E; Wed, 15 Feb 2023 22:40:34 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B43CF40EF1; Wed, 15 Feb 2023 22:40:33 +0100 (CET) Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by mails.dpdk.org (Postfix) with ESMTP id EFA4240A7D for ; Wed, 15 Feb 2023 22:40:31 +0100 (CET) Received: by mail-ej1-f46.google.com with SMTP id k16so358476ejv.10 for ; Wed, 15 Feb 2023 13:40:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=rar17CBfKXVspr57ZENRI1n3yDrpDm+mBEIoP27W23k=; b=LrQAlDNPgoburRQJ1Iy0IKL9/o+HhWTKbYpOenVja8E2FkHYcBOyUJvSMg7FsqClT+ sKWOQWqMvYgazMx1NTpWjSSUoNdzEXZ4ADFuYRlG0hx43p61X78P3J4RJ9ykKl3L/wX/ kJmJw6Gyn+oNECkSyWnB85fQtu05zpMpS2WFS8/8SkIc8cKH28zrT/nAG9tA0mtX3EOb KPcMRL+OAXJW4Dq60G2J2X1S4t6OUMQeZxvbEUWlp74pB7g0Hojici2gMQxMZhLA6Wam rUL26zGWh7QNngZWmRRa3bEdLMxb5eMnbgemYhuhgd9+NW6D5KhIAW9AFCt0L0EzHw5Y iooA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=rar17CBfKXVspr57ZENRI1n3yDrpDm+mBEIoP27W23k=; b=SAHe9Nw8UAht6SFWZXhzNrauf8PyqVRzq+Np+tgUJRog5m8BaDmLADV1teiP965zV6 EV1sHsAO1z2Njtv9F3mczJ6IcVvl0/GiukvJbw51jGCQtnLueAZ5JfXUMla/1yC2szU+ XALWEvtuPGIW4kBXEkTYShgZU8mROUELpj0BpKzbbVlq8dJxiOPCin/fRv33ZjI6qXv2 Hpfasn9XRJjZuF7wC8qJGxpeXlBkYwwMhzM+PVrL+A9n8zVqFdErv/QMdzQhBWNkk/sz gz+1CESmwg5nv2HpzHmUEMb5sCZTmPwi1UQJ0SvALKaDJsX77I0J/0tj5QajF8/UaoxF ctZQ== X-Gm-Message-State: AO0yUKUj31ZsGaTcCVt9KtwS1lStPGfBoAlDLsCBQRrCxM7WN1xSZvmT THU6Sywq4VJ50VRYON/khb3b55Z5BeUeYYh6f+k= X-Google-Smtp-Source: AK7set9kLYPXydXWcjdu5BDz9VO52eQBbuD3Mzx05cTybZvHq0VWU52jsFxh7s87sEuGCSG+dBt2Zt3lvMtE6YfbK4I= X-Received: by 2002:a17:907:2cd2:b0:895:58be:963 with SMTP id hg18-20020a1709072cd200b0089558be0963mr1799719ejc.3.1676497230610; Wed, 15 Feb 2023 13:40:30 -0800 (PST) MIME-Version: 1.0 References: <20230215105442.3878441-1-qobilidop@gmail.com> <20230215110630.3885175-1-qobilidop@gmail.com> <98CBD80474FA8B44BF855DF32C47DC35D87740@smartserver.smartshare.dk> In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87740@smartserver.smartshare.dk> From: Bili Dong Date: Wed, 15 Feb 2023 13:39:54 -0800 Message-ID: Subject: Re: [PATCH v3] hash: add XOR32 hash function To: =?UTF-8?Q?Morten_Br=C3=B8rup?= Cc: yipeng1.wang@intel.com, sameh.gobriel@intel.com, bruce.richardson@intel.com, vladimir.medvedkin@intel.com, cristian.dumitrescu@intel.com, dev@dpdk.org Content-Type: multipart/alternative; boundary="000000000000e5c79c05f4c3edd8" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000e5c79c05f4c3edd8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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#L16= 8, 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=C3=B8rup 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 > > --- > > [...] > > > +#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 =3D (uintptr_t) data; > > + init_val =3D rte_cpu_to_be_32(init_val); > > + > > + for (i =3D 0; i < data_len / 4; i++) { > > + init_val ^=3D *(const uint32_t *)pd; > > + pd +=3D 4; > > + } > > + > > + if (data_len & 0x2) { > > + init_val ^=3D *(const uint32_t *)pd & LEFT16b_MASK; > > + pd +=3D 2; > > + } > > + > > + if (data_len & 0x1) > > + init_val ^=3D *(const uint32_t *)pd & LEFT8b_MASK; > > + > > + init_val =3D 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 > > > --000000000000e5c79c05f4c3edd8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Morten,

Thanks for your comment= s!

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 h= ave missed something but I think I used them (4 occurrences) in a semantica= lly meaningful way. Could you point me to the lines that are confusing?

The hash function signature has to conform to=C2=A0https://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 woul= d 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:=C2= =A0https://elixir.bootlin.com/dpdk/v22.11.1/source/lib/hash/r= te_hash_crc.h#L168, and I don't see byte alignment handled there. M= aybe someone more familiar with lib/hash/ could provide some context on thi= s?

Thanks,
Bili

On Wed, Feb 15, 202= 3 at 3:39 AM Morten Br=C3=B8rup <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<= br> > 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
> + *=C2=A0 =C2=A0Data to perform hash on.
> + * @param data_len
> + *=C2=A0 =C2=A0How many bytes to use to calculate hash value.
> + * @param init_val
> + *=C2=A0 =C2=A0Value to initialise hash generator.
> + * @return
> + *=C2=A0 =C2=A032bit calculated hash value.
> + */
> +static inline uint32_t
> +rte_hash_xor(const void *data, uint32_t data_len, uint32_t init_val)<= br> > +{
> +=C2=A0 =C2=A0 =C2=A0uint32_t i;
> +=C2=A0 =C2=A0 =C2=A0uintptr_t pd =3D (uintptr_t) data;
> +=C2=A0 =C2=A0 =C2=A0init_val =3D rte_cpu_to_be_32(init_val);
> +
> +=C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < data_len / 4; i++) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0init_val ^=3D *(const= uint32_t *)pd;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pd +=3D 4;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0if (data_len & 0x2) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0init_val ^=3D *(const= uint32_t *)pd & LEFT16b_MASK;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0pd +=3D 2;
> +=C2=A0 =C2=A0 =C2=A0}
> +
> +=C2=A0 =C2=A0 =C2=A0if (data_len & 0x1)
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0init_val ^=3D *(const= uint32_t *)pd & LEFT8b_MASK;
> +
> +=C2=A0 =C2=A0 =C2=A0init_val =3D rte_be_to_cpu_32(init_val);
> +=C2=A0 =C2=A0 =C2=A0return 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 usin= g 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 t= o be rte_be32_t.

Also, please document that the byte array must be 32 bit aligned. Alternati= vely, implement support for unaligned data. You can find inspiration for ha= ndling of unaligned data in the __rte_raw_cksum() function:
https://elixir.bootlin.com/dpdk= /v22.11.1/source/lib/net/rte_ip.h#L162


--000000000000e5c79c05f4c3edd8--