Re: [dpdk-dev] [PATCH v2] Implement memcmp using AVX/SSE instructions.

[Ravi Kerur <rkerur@gmail.com>]

Hi Konstantin,


On Mon, May 11, 2015 at 2:51 AM, Ananyev, Konstantin <
konstantin.ananyev@intel.com> wrote:

> Hi Ravi,
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ravi Kerur
> > Sent: Friday, May 08, 2015 11:55 PM
> > To: Matt Laswell
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] Implement memcmp using AVX/SSE
> instructions.
> >
> > On Fri, May 8, 2015 at 3:29 PM, Matt Laswell <laswell@infiniteio.com>
> wrote:
> >
> > >
> > >
> > > On Fri, May 8, 2015 at 4:19 PM, Ravi Kerur <rkerur@gmail.com> wrote:
> > >
> > >> This patch replaces memcmp in librte_hash with rte_memcmp which is
> > >> implemented with AVX/SSE instructions.
> > >>
> > >> +static inline int
> > >> +rte_memcmp(const void *_src_1, const void *_src_2, size_t n)
> > >> +{
> > >> +       const uint8_t *src_1 = (const uint8_t *)_src_1;
> > >> +       const uint8_t *src_2 = (const uint8_t *)_src_2;
> > >> +       int ret = 0;
> > >> +
> > >> +       if (n & 0x80)
> > >> +               return rte_cmp128(src_1, src_2);
> > >> +
> > >> +       if (n & 0x40)
> > >> +               return rte_cmp64(src_1, src_2);
> > >> +
> > >> +       if (n & 0x20) {
> > >> +               ret = rte_cmp32(src_1, src_2);
> > >> +               n -= 0x20;
> > >> +               src_1 += 0x20;
> > >> +               src_2 += 0x20;
> > >> +       }
> > >>
> > >>
> > > Pardon me for butting in, but this seems incorrect for the first two
> cases
> > > listed above, as the function as written will only compare the first
> 128 or
> > > 64 bytes of each source and return the result.  The pattern expressed
> in
> > > the 32 byte case appears more correct, as it compares the first 32
> bytes
> > > and then lets later pieces of the function handle the smaller remaining
> > > bits of the sources. Also, if this function is to handle arbitrarily
> large
> > > source data, the 128 byte case needs to be in a loop.
> > >
> > > What am I missing?
> > >
> >
> > Current max hash key length supported is 64 bytes, hence no comparison is
> > done after 64 bytes. 128 bytes comparison is added to measure performance
> > only and there is no use-case as of now. With the current use-cases its
> not
> > required but if there is a need to handle large arbitrary data upto 128
> > bytes it can be modified.
>
> So on x86 let say rte_memcmp(k1, k2, 65) might produce invalid results,
> right?
> While on PPC will work as expected (as it calls memcpu underneath)?
> That looks really weird to me.
> If you plan to use rte_memcmp only for hash comparisons, then probably
> you should put it somewhere into librte_hash and name it accordingly:
> rte_hash_key_cmp() or something.
> And put a big comment around it, that it only works with particular
> lengths.
> If you want it to be a generic function inside EAL, then it probably need
> to handle different lengths properly
> on all supported architectures.
> Konstantin
>
>
Let me just explain it here and probably add it to document as well.

rte_memcmp is not

1. a replacement to memcmp

2.  restricted to hash key comparison

rte_memcmp is

1. optimized comparison for 16 to 128 bytes, v1 patch series had this
support. Changed some of the logic in v2 due to concerns raised for
unavailable use-cases beyond 64 bytes comparison. With minor tuning over
the weekend I am able to get better performance for anything between 16 to
128 bytes comparison.

2. will be specific to DPDK  i.e. currently all memcmp usage in DPDK are
for equality or inequality hence "less than" or "greater than"
implementation in rte_memcmp doesn't make sense and will be removed in
subsequent patches, it will return 0 or 1 for equal/unequal cases.

rte_hash will be the first candidate to move to rte_memcmp and subsequently
rte_lpm6 which uses 16 bytes comparison will be moved

Later on RING_SIZE which uses large size for comparison will be moved. I am
currently studying/understanding that logic and will make changes to
rte_memcmp to support that.

I don't want to make lot of changes in one shot and see that patch series
die a slow death with no takers.

Thanks,
Ravi

>
> > >
> > > --
> > > Matt Laswell
> > > infinite io, inc.
> > > laswell@infiniteio.com
> > >
> > >
>