Re: [dpdk-dev] [PATCH v4] Add toeplitz hash algorithm used by RSS

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Jun 19, 2015 at 07:14:15PM +0300, Vladimir Medvedkin wrote:
> Hi Bruce,
> 
> 2015-06-19 18:59 GMT+03:00 Richardson, Bruce <bruce.richardson@intel.com>:
> 
> >
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Vladimir Medvedkin
> > > Sent: Friday, June 19, 2015 3:56 PM
> > > To: dev@dpdk.org
> > > Subject: [dpdk-dev] [PATCH v4] Add toeplitz hash algorithm used by RSS
> > >
> > > v4 changes
> > > - Fix copyright
> > > - rename bswap_mask constant, add rte_ prefix
> > > - change rte_ipv[46]_tuple struct
> > > - change rte_thash_load_v6_addr prototype
> > >
> > > v3 changes
> > > - Rework API to be more generic
> > > - Add sctp_tag into tuple
> > >
> > > v2 changes
> > > - Add ipv6 support
> > > - Various style fixes
> > >
> >
> > Missing signoff line.
> >
> > > ---
> > >  lib/librte_hash/Makefile    |   1 +
> > >  lib/librte_hash/rte_thash.h | 202
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 203 insertions(+)
> > >  create mode 100644 lib/librte_hash/rte_thash.h
> > >
> > <...snip...>
> > > +
> > > +/* Byte swap mask used for converting IPv6 address 4-byte chunks to CPU
> > > byte order */
> > > +static const __m128i rte_thash_ipv6_bswap_mask = {0x0405060700010203,
> > > 0x0C0D0E0F08090A0B};
> > > +
> > > +#define RTE_THASH_V4_L3       2      /*calculate hash of ipv4 header
> > only*/
> > > +#define RTE_THASH_V4_L4       3      /*calculate hash of ipv4 +
> > transport
> > > headers*/
> > > +#define RTE_THASH_V6_L3       8      /*calculate hash of ipv6 header
> > only
> > > */
> > > +#define RTE_THASH_V6_L4       9      /*calculate hash of ipv6 +
> > transport
> > > headers */
> >
> > I'm still not seeing why these values need to be defined here, rather than
> > in a specific app.
> > Also, the choice of values for these defines seems strange to me? How were
> > they chosen?
> >
> This is a predefined values. They mean the length (in 4-bytes) of the
> input data
> in hashing. I think it's like defines in rte_ip.h, for example.
> 

I'm still not convined of the need for them in this file. If they are to be
kept though, they should instead have "LEN" in the name to indicate that they
are the lengths of the different keys. You could also make this clearer by
changing the L4 values to be computed using "sizeof()" given that the appropriate
tuple structures are present later in the header file to allow the sizes to be
correctly calculated.

/Bruce