[ovs-dev] [PATCH v10 0/5] dpcls func ptrs & optimizations

Van Haaren, Harry harry.van.haaren at intel.com
Fri Jul 12 14:05:09 UTC 2019


> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> Sent: Friday, July 12, 2019 11:50 AM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>
> Cc: malvika.gupta at arm.com; Stokes, Ian <ian.stokes at intel.com>; Michal Orsák
> <michal.orsak at gmail.com>; dev at openvswitch.org
> Subject: Re: [PATCH v10 0/5] dpcls func ptrs & optimizations

<snip> lots of chatter

> >> All the flows fits into 5+1 case, i.e. optimized function
> >> dpcls_subtable_lookup_mf_u0w5_u1w1 used.
> >>
> >> Most interesting observation:
> >>
> >> * New version of dpcls lookup outperforms SMC in this setup even on
> >>   relatively small number of flows. With 8K flows dpcls faster than SMC
> >>   by 1.5% and by 5.7% with 256K flows.
> >>   Of course, SMC is 10% faster than dpcls with 8 flows, but it's not very
> >>   interesting because no-one can beat EMC in this area.
> >>
> >> I'd like to read the code more carefully tomorrow and probably give some
> >> more feedback.
> >>
> >> Best regards, Ilya Maximets.
> >
> > Thanks for your comments - please do prioritize feedback ASAP, because as
> > you know the 2.12 soft-freeze is already in effect.
> >
> > I'll work on Ian's comments on v10, but hold off sending v11 until there
> > is some feedback from you too :)

Thanks for the prompt review;


> Few comments:
> 
> 1. I don't really like that new dpcls depends on the internal structure of
>    'struct flowmap'. At least, we need to add a build assert to notify
>    anyone who will try to change it.

Agreed - adding a build-time assert is a good idea, will include in v11.


> 2. No need to expose so many internal stuff to every module that includes
>    'dpif-netdev.h'. Can we create 'dpif-netdev-private.h' instead and move
>    all the subtable related stuff there?

Good idea - keeps things a bit cleaner, will do for v11.


> 3. IMHO, it's better to apply some preprocessor optimizations to make it
>    easier to add new optimized functions and decrease code duplication.

Yes - had intended to MACRO them out when adding more -  V11 will include, thanks!


> 4. Please, add a space between 'ULLONG_FOR_EACH_1' and '('.

Sure - I see you've sent checkpatch improvements - nice one.


> 5. A lot of comments needs to be re-formatted according to coding style,
>    i.e. first letter capitalized + period at the end.

Yes - unfortunately I'm trained & hard-coded to DPDK standards... Will fix
as much as I see while rebasing.


> 6. "generic optimized" sounds weird.

I think it’s the best description of what it is that I can think of.

Generic in that it will work for any subtable & packet combination
Optimized in that it is specialized for a specific use-case at compile time.


[Pulling in from other email]
>> Ilya wrote:
>> One more thing. Could you, please, rename patches like:
>> dpif-netdev: Implement function pointers/subtable.

Yes will do.


> Here is some incremental I'm suggesting which covers comments 1, 3 and 4:

Thanks for the suggestions - working on it.

<snip> actual code changes (see previous email on mailing list archive).

Regards, -Harry


More information about the dev mailing list