From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f42.google.com (mail-lf0-f42.google.com [209.85.215.42]) by dpdk.org (Postfix) with ESMTP id 9C4DC9205 for ; Fri, 30 Oct 2015 20:34:54 +0100 (CET) Received: by lffz202 with SMTP id z202so37651018lff.3 for ; Fri, 30 Oct 2015 12:34:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=5CrWE4Ev5AZ8NxY4BYroayrvDPePg/mWWMTbT0aJyz0=; b=fdSLO4NXvlpSUMqLx4hsQ1P3JHHEDRtRjjEtwLKWzd/HPlsXD+71ABQ0/JoXoDNSUh 4fCD9SJeNXu0wtOm44dtEr18ocoKv4ugpx7OvUv01e0jwB4jw7yWm18Zv4hJuwu3LxP9 dihSAmDzGcJlcXRtb8y/B38gyimz7MPT7kFZMOWrgIUjVVcXJHA1WUd6flTgY8QjpAbq UoWEAOCai2QfTQsOwUvTh3P5nnzqUvbToU8p9hr+XqFXguoLROMuZUW1jVOj4Khfby4U 8RqhPb9Ovu+kS4veMiAOdeGEpjbmvzumGlFeATXWwniubDaxgNf9WkSZtALWS8FLGtN7 q76Q== MIME-Version: 1.0 X-Received: by 10.25.18.39 with SMTP id h39mr3242356lfi.7.1446233694235; Fri, 30 Oct 2015 12:34:54 -0700 (PDT) Received: by 10.114.199.4 with HTTP; Fri, 30 Oct 2015 12:34:54 -0700 (PDT) In-Reply-To: <20151030175927.GA21104@mhcomputing.net> References: <20151030120018.GA4904@bricha3-MOBL3> <20151030175927.GA21104@mhcomputing.net> Date: Fri, 30 Oct 2015 22:34:54 +0300 Message-ID: From: Vladimir Medvedkin To: Matthew Hall Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] lpm patches X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Oct 2015 19:34:54 -0000 Hi all, 2015-10-30 20:59 GMT+03:00 Matthew Hall : > 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