From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 84F9EA034C; Fri, 25 Feb 2022 13:50:33 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 733B44115C; Fri, 25 Feb 2022 13:50:33 +0100 (CET) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id BF8794068B for ; Fri, 25 Feb 2022 13:50:31 +0100 (CET) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v5 0/2] Add config file support for l3fwd Date: Fri, 25 Feb 2022 13:50:30 +0100 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86EFC@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v5 0/2] Add config file support for l3fwd Thread-Index: AdgqNBrSEorhjCTLQ7CC7R3TGatIygAD9ZAg References: <20220126124459.2469838-1-sean.morrissey@intel.com> <2262420.IPqQCg1nHW@thomas> <2122832.NgBsaNRSFp@thomas> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" , "Ananyev, Konstantin" Cc: "Thomas Monjalon" , "Stephen Hemminger" , "Medvedkin, Vladimir" , , "Morrissey, Sean" , , , , , , , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Friday, 25 February 2022 11.40 >=20 > On Fri, Feb 25, 2022 at 10:36:29AM +0000, Ananyev, Konstantin wrote: > > > > > On Thu, Feb 24, 2022 at 02:46:24PM +0100, Thomas Monjalon wrote: > > > > 24/02/2022 12:06, Ananyev, Konstantin: > > > > > > > > > > > > > >> Or have a generic library for reading LPM > entries. L3fwd is supposed > > > > > > > > > > > > > >> to be as small as possible (it no longer > is), and the real work should > > > > > > > > > > > > > >> be done by libraries to make it easier to > build other applications. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I never heard users ask about such thing, > > > > > > > > > > > > > > but if there is a demand for that, then I > suppose it could be considered. > > > > > > > > > > > > > > CC-ing LPM/FIB maintainers to comment. > > > > > > > > > > > > > > Though I believe it should be a subject of > separate patch and discussion > > > > > > > > > > > > > > (I think many questions will arise - what > format should be, how to support > > > > > > > > > > > > > > different types of user-data, to make it > generic enough, etc.). > > > > > > > > > > > > > > > > > > > > > > > > > > Agree, it is very application specific, so it > could be really difficult > > > > > > > > > > > > > to make it generic. > > > > > > > > > > > > > > > > > > > > > > > > But several other also have LPM tables, so why > not have common code for other applications. > > > > > > > > > > > > > > > > > > > > > > > > examples/l3fwd-power/main.c > > > > > > > > > > > > examples/ipsec-secgw/rt.c > > > > > > > > > > > > examples/ip_fragmentation/main.c > > > > > > > > > > > > examples/l3fwd/l3fwd_lpm.c > > > > > > > > > > > > examples/ip_reassembly/main.c > > > > > > > > > > > > > > > > > > > > > > Ah yes, that's good point. > > > > > > > > > > > All these examples (except ipsec-secgw) started as > l3fwd clones, > > > > > > > > > > > so all of them have hard-coded LPM (and EM) tables > too. > > > > > > > > > > > Yes it would be good thing to address that problem > too, > > > > > > > > > > > and have some common code (and common routes file > format) for all of them. > > > > > > > > > > > I don't know is that a good idea to introduce = parse > file function in LPM/FIB library > > > > > > > > > > > itself, might be better to have something like > examples/common/lpm_parse*. > > > > > > > > > > > Anyway, this is an extra effort, and I think = no-one > has time for it in 22.03 timeframe. > > > > > > > > > > > My suggestion would be for 22.03 go ahead with > current l3fwd patches, > > > > > > > > > > > then later we can consider to make it common and > update other examples. > > > > > > > > > > > > > > > > > > > > I don't think this patch is urgent. > > > > > > > > > > I suggest taking time to have common code for all > examples > > > > > > > > > > and target a merge in DPDK 22.07. > > > > > > > > > > > > > > > > > > Well, yes, from one perspective it not really a > critical one, > > > > > > > > > we do live with hard-coded routes inside l3fwd for > nearly 10 year by now. > > > > > > > > > Though l3fwd is one of mostly used examples inside = DPDK > and > > > > > > > > > it is quite a pain to patch/rebuild it each time > someone needs to run > > > > > > > > > l3fwd with a different routing table. > > > > > > > > > Merging this patch will allow people to use l3fwd for > more realistic test > > > > > > > > > scenarios in a painless manner. > > > > > > > > > So I believe this patch is really helpful and should = be > beneficial for the whole community. > > > > > > > > > Looking from that perspective, I don't see why it has > to be "all or nothing" attitude here. > > > > > > > > > Why we can't move one step at a time instead? > > > > > > > > > That would allow to split and effort in terms of > development/testing/upstreaming/etc. > > > > > > > > > > > > > > > > When a feature is merged, there is less incentives to > rework. > > > > > > > > That's why, when a feature is not urgent, > > > > > > > > it is better to wait for the complete work. > > > > > > > > > > > > > > That's true till some extent, though from other side > > > > > > > even without further rework that patch improves situation > > > > > > > from what we have right now. > > > > > > > So I don't see any harm here. > > > > > > > > > > > > It is adding a lot of code to an example which is already = too > big. > > > > > > There are a lot of complain about the size of l3fwd. > > > > > > That's why I think it makes sense to require this extra code > > > > > > (not demonstrating anything, but just for testing > convenience) > > > > > > outside of the example. > > > > > > > > > > Ok, so your main concern is l3fwd code size increase, right? > > > > > Then would it help if for we'll move file parsing code into a > separate file(s) > > > > > (under examples/l3fwd) for now? > > > > > Something like examples/l3fwd/(lpm_em)_route_parse.c. > > > > > > > > Yes it would help to isolate the config file parsing code. > > > > What others think? > > > > > > > I still would like config code for loading an LPM table or FIB > table to be > > > put inside the relevant libraries themselves, rather than having = it > in the > > > examples themselves (even if shared between them). > > > > Honestly, I don't see any good reasons for that: > > I presume users of these libraries already have their own routing > > config files with their own format requirements and I suppose > > these formats differ a lot (depending on use-case). I can confirm this assumption. Our appliances use our own configuration file handling, which supports = full, partial and diff configurations. Furthermore, it uses a schema = with type definitions for simple syntax checking, as well as a = programmable validator to check system-wide configuration validity and = integrity. >=20 > Yes, I agree that all existing users of the libraries will have this = in > place. However, for new users, this may be useful for bootstrapping > things, > and it also makes it generally useable for both example apps, and any > apps > in the "app" folder, i.e. if l3fwd gets moved there. (Something I = agree > with, as it's now more than just example code). >=20 Don't put any config file parsers or similar in the fast path libraries. = It is unwanted bloat! If you want to standardize on a DPDK specific config file format for the = DPDK provided examples and applications, which I do agree with the = benefits of having, then provide a separate library to interact with the = underlying fast path libraries.