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

[Ravi Kerur]

Hi Konstantin,


On Mon, May 11, 2015 at 12:35 PM, Ananyev, Konstantin <
konstantin.ananyev at intel.com> wrote:

>
> Hi Ravi,
>
> >
> > From: Ravi Kerur [mailto:rkerur at gmail.com]
> > Sent: Monday, May 11, 2015 6:43 PM
> > To: Ananyev, Konstantin
> > Cc: Matt Laswell; dev at dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2] Implement memcmp using AVX/SSE
> instructions.
> >
> > Hi Konstantin,
> >
> >
> > On Mon, May 11, 2015 at 2:51 AM, Ananyev, Konstantin <
> konstantin.ananyev at intel.com> wrote:
> > Hi Ravi,
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ravi Kerur
> > > Sent: Friday, May 08, 2015 11:55 PM
> > > To: Matt Laswell
> > > Cc: dev at 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 at infiniteio.com>
> wrote:
> > >
> > > >
> > > >
> > > > On Fri, May 8, 2015 at 4:19 PM, Ravi Kerur <rkerur at 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.
>
> From what I see in v2 it supposed to work correctly for len in [0,64] and
> len=128, right?
> Not sure I get it: so for v1 it was able to handle any length correctly,
> but then you removed it?
> If so, I wonder what was the reason? Make it faster?
>

My initial discussion was with Zhilong(John) from Intel and we decided to
implement up to 128 bytes comparison and use rte_hash and rte_lpm6 as a
candidate for testing. When I sent out v1 patch, Bruce comments were on
use-case for 128 bytes comparison and was it really required? Hence I
decided in v2 to support only up to 64 bytes and added 128 bytes only for
performance measurement.

Personally I think support for up to 128 bytes comparison is required,
there might not be use-cases today but it will definitely be useful.


> Another thing that looks strange to me:
> While all rte_cmp*() uses actual data values for comparison results,
> rte_memcmp_remainder() return value depends not only on data values but
> also on data locations:
>
> +static inline int
> +rte_memcmp_remainder(const uint8_t *src_1u, const uint8_t *src_2u, size_t
> n)
> +{
> ...
> exit:
> +
> +       return src_1u < src_2u ? -1 : 1;
> +}
>
>
This is a bug and its not supposed to be there. I will fix it. Thanks for
catching it.


> If you just test for equal/not equal that doesn't really matter.
> If this is supposed to be a 'proper' comparison function, then the result
> is sort of unpredictable.
>
> > 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.
>
> If you don't plan your function to follow memcmp() semantics and syntax,
> why to name it rte_memcmp()?
> I  think that will make a lot of confusion around.
> Why not to name it differently(and put a clear comment in the declaration
> of course)?
>

Following memcmp semantics is not hard but there are no use-cases for it in
DPDK currently. Keeping it specific to DPDK usage simplifies code as well.
I can change the name to "rte_compare" and add comments to the function.
Will it work?


>
> >
> > 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.
>
> Sorry, didn't get you here.
>

Once rte_hash, rte_lpm6 changes and new compare function code are reviewed
and accepted I plan to move to different components (RING_SIZE is currently
defined to be from 256 to 16384 bytes) and memcmp function being used in
test_ring, test_pmd_ring and other functions. I did not want to add all
component changes into one patch series as it causes high review latency or
patch series just dies down silently. Instead make patches small and
incremental in every series, hope this clarifies.

Thanks,
Ravi

Konstantin
>
> >
> > 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 at infiniteio.com
> > > >
> > > >
>
>