Re: [dpdk-dev] lpm patches

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

On Fri, Oct 30, 2015 at 10:34:54PM +0300, Vladimir Medvedkin wrote:
> Hi all,
> 
> 2015-10-30 20:59 GMT+03:00 Matthew Hall <mhall@mhcomputing.net>:
> 
> > On Fri, Oct 30, 2015 at 12:00:18PM +0000, Bruce Richardson wrote:
> > > Matthew's patches were attachments, I don't think they came through in
> > patchwork
> > > correctly :-(, but that is the relevant link there anyway.]
> >
> > Let me know if there is something I can do better there. I was having a
> > difficult time figuring out how to preserve the thread ID in the middle of
> > the
> > thread and not cause a new thread. The git email workflows are very
> > confusing
> > and I figured it was better to send something as soon as I could.
> >
> > > * Some patches increase the next-hop to 16 bits, others to 24-bits. In
> > both cases
> > >   a single entry still only occupies 32-bits, so can be read/written to
> > >   atomically
> >
> > I went with 24 because it was the biggest amount I could get that still had
> > this property.
> >
> > > * Only Michal's set appears to take into account ABI versioning, which is
> > >   a difficult problem for this lib, with inlined functions.
> >
> > Agreed. His patches are the most professional from this perspective. This
> > is
> > why I was trying to contribute to you and to him so we get the most
> > professional result for the customers.
> >
> > > * Matthew's patchset moves the lookup functions to be non-inlined, which
> > will
> > >   make future updates easier from ABI compatibility - at the cost of
> > lookup
> > >   performance.
> >
> > This point is optional for me. I did it, because without it, it was totally
> > impossible for me to work on the code in a debugger as I am a security
> > engineering guy not a crazy embedded C coder or kernel hacker.
> >
> > > * Vladimir's patchset merges in the tbl24 and tbl8 entries into a single
> > data
> > >   type.
> >
> > I really liked this feature of Vladimir's patches, it makes it easier to
> > maintain and less confusing. I had a lot of headaches keeping all those
> > structs straight with the separate types, but I didn't know we had the
> > chance
> > for a great big MEGA-REFACTOR. I love this community!
> >
> > > * That patchset also introduces an extra optional 32-bit field "as_num",
> > allowing
> > >   64-bit lpm table entries - obviously at a cost of increased
> > memory/cache
> > >   footprint.
> >
> > Is there a way we could test it? Vladimir, did you test the performance? If
> > so, what happened?
> >
> Performance regression depends on the traffic pattern and cache size.
> 
> >
> > > * Stephen's patchset includes a range of other fixes e.g. for more
> > efficient
> > >   management of the rules array, and dynamic allocation of the TBL8s.
> > > * Matthew's patchset also includes change to LPM for IPv6, which I'm
> > considering
> > >   out-of-scope for now, so as to focus on LPM v4 only.
> >
> > Any chance that is inconsistent betwen LPM4 and LPM6 really hoses me,
> > because
> > I am writing green-field code which treats both protocols as first-class
> > citizens and I'd really not like to have totally inconsistent and inferior
> > support in one versus the other.
> >
> > > * Increase next hops to be the full 24 bits, so as to allow maximum
> > flexibility
> > >   and not waste the extra 8 bits of space in the 32-bit entries.
> >
> > +1
> >
> +1. Split of next hop and forwarding class I can do in app logic.
> 
> >
> > > * Move the lookup functions which work on multiple packets to be
> > non-inlined
> >
> > Open to opinions on the performance of this. I am not an expert on this
> > area.
> >
> > > * Merge in the tbl24 and tbl8 structures to make the code that little
> > bit shorter
> >
> > +1
> >
> > > * Look to pull in as many of Stephen's other improvements as possible -
> > though
> > >   this may be in a separate patchset to the other changes.
> >
> > +1. Perhaps if we get a pre-release on a branch with everything else, we
> > could
> > see if Stephen is willing to rebase his non-duplicate changes.
> >
> > > * I'm uncertain as to the extra 32-bit as_num field. Adding it as an
> > extra
> > >   #define is trivial, but adds to the compile-time config. Having it as
> > a run-time
> > >   option is possible, but likely will make the code a lot more
> > complicated, as
> > >   we no longer have arrays of a fixed size.
> > >
> > > Naturally, with whatever solution is come up with, ABI compatibility
> > must be
> > > taken into account and functions versionned appropriately!
> >
> > Normally I am not a big define guy. But it seems like a define is good
> > here.
> > Somebody is going to need to know beforehand if they are making a Core
> > Router
> > where they want this, or a Security Inspection system like mine, etc.
> >
> For example in case of core router as_num feature can be necessary for
> netflow. It can be necessary in case of security device such as my ddos
> mitigation system.
> 
> >
> > So it seems easier than doing a bunch of crazy size-juggling in the code.
> >
> > > do we want to have some of these changes in 2.2?
> >
> > Personally I am OK to wait as I have it working in my copy. I am just
> > trying
> > to be a good citizen of the community and contribute back when I see some
> > core
> > engineers going after the same code.
> >
> > In particular, for me, having LPM4 only with no LPM6 is not worth much so
> > I'd
> > be happy to wait for a single upgrade to both of them.
> >
> > > Matthew, Stephen, Vladimir, Michal, Thomas - thoughts on this?
> > > [do I accurately sum up the situation?]
> >
> > This email was top-quality and very well done by you guys.
> >
> > Matthew.
> >
> 
> Regards,
> Vladimir

Thanks for the feedback guys. Looks like we need to find a way to get the ability
to have either 32-bit or 64-bit LPM entries supported, as well as the additional
enhancements proposed :-)

We'll see what we can do in 2.3 timeframe.

Regards,
/Bruce