Re: lpm patches

[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?

> * 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

> * 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.

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.