DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Bruce Richardson" <bruce.richardson@intel.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Cc: "Thomas Monjalon" <thomas@monjalon.net>,
	"Stephen Hemminger" <stephen@networkplumber.org>,
	"Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>,
	<dev@dpdk.org>, "Morrissey, Sean" <sean.morrissey@intel.com>,
	<honnappa.nagarahalli@arm.com>, <jerinj@marvell.com>,
	<ajit.khaparde@broadcom.com>, <olivier.matz@6wind.com>,
	<maxime.coquelin@redhat.com>, <david.marchand@redhat.com>,
	<ktraynor@redhat.com>
Subject: RE: [PATCH v5 0/2] Add config file support for l3fwd
Date: Fri, 25 Feb 2022 13:50:30 +0100	[thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86EFC@smartserver.smartshare.dk> (raw)
In-Reply-To: <YhiyETnBQuI3MURQ@bricha3-MOBL.ger.corp.intel.com>

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 25 February 2022 11.40
> 
> 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.

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

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.


  parent reply	other threads:[~2022-02-25 12:50 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-16 15:01 [PATCH v1 " Sean Morrissey
2021-12-16 15:01 ` [PATCH v1 1/2] examples/l3fwd: add config file support for LPM/FIB Sean Morrissey
2021-12-16 15:01 ` [PATCH v1 2/2] examples/l3fwd: add config file support for EM Sean Morrissey
2021-12-20 11:08 ` [PATCH v2 0/2] Add config file support for l3fwd Sean Morrissey
2021-12-20 11:08   ` [PATCH v2 1/2] examples/l3fwd: add config file support for LPM/FIB Sean Morrissey
2021-12-20 15:42     ` Ananyev, Konstantin
2021-12-20 11:08   ` [PATCH v2 2/2] examples/l3fwd: add config file support for EM Sean Morrissey
2021-12-20 15:53     ` Ananyev, Konstantin
2021-12-21 12:30   ` [PATCH v3 0/2] Add config file support for l3fwd Sean Morrissey
2021-12-21 12:30     ` [PATCH v3 1/2] examples/l3fwd: add config file support for LPM/FIB Sean Morrissey
2021-12-21 12:30     ` [PATCH v3 2/2] examples/l3fwd: add config file support for EM Sean Morrissey
2022-01-10 14:00       ` Ananyev, Konstantin
2022-01-26 12:44     ` [PATCH v4 0/2] Add config file support for l3fwd Sean Morrissey
2022-01-26 12:44       ` [PATCH v4 1/2] examples/l3fwd: add config file support for LPM/FIB Sean Morrissey
2022-01-26 12:44       ` [PATCH v4 2/2] examples/l3fwd: add config file support for EM Sean Morrissey
2022-02-04 19:59       ` [PATCH v5 0/2] Add config file support for l3fwd Sean Morrissey
2022-02-04 19:59         ` [PATCH v5 1/2] examples/l3fwd: add config file support for LPM/FIB Sean Morrissey
2022-02-08  2:21           ` Han, YingyaX
2022-02-04 19:59         ` [PATCH v5 2/2] examples/l3fwd: add config file support for EM Sean Morrissey
2022-02-04 22:26         ` [PATCH v5 0/2] Add config file support for l3fwd Stephen Hemminger
2022-02-06 15:16           ` Ananyev, Konstantin
2022-02-08  3:04             ` Stephen Hemminger
2022-02-08 10:44               ` Ananyev, Konstantin
2022-02-08 16:15                 ` Medvedkin, Vladimir
2022-02-08 17:49                   ` Stephen Hemminger
2022-02-08 18:10                     ` Bruce Richardson
2022-02-09 12:00                     ` Ananyev, Konstantin
2022-02-09 13:54                       ` Bruce Richardson
2022-02-09 16:00                         ` Medvedkin, Vladimir
2022-02-22  9:59                       ` Thomas Monjalon
2022-02-22 10:39                         ` Ananyev, Konstantin
2022-02-22 13:46                           ` Thomas Monjalon
2022-02-22 15:13                             ` Ananyev, Konstantin
2022-02-22 16:48                               ` Thomas Monjalon
2022-02-24 11:06                                 ` Ananyev, Konstantin
2022-02-24 13:46                                   ` Thomas Monjalon
2022-02-24 13:58                                     ` Bruce Richardson
2022-02-25 10:36                                       ` Ananyev, Konstantin
2022-02-25 10:40                                         ` Bruce Richardson
2022-02-25 12:21                                           ` Ananyev, Konstantin
2022-02-25 12:50                                           ` Morten Brørup [this message]
2022-02-25  5:18                                     ` Honnappa Nagarahalli
2022-03-01 14:49         ` [PATCH v6 " Sean Morrissey
2022-03-01 14:49           ` [PATCH v6 1/2] examples/l3fwd: add config file support for LPM/FIB Sean Morrissey
2022-03-01 14:49           ` [PATCH v6 2/2] examples/l3fwd: add config file support for EM Sean Morrissey
2022-03-08  8:57           ` [PATCH v6 0/2] Add config file support for l3fwd Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=98CBD80474FA8B44BF855DF32C47DC35D86EFC@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=ajit.khaparde@broadcom.com \
    --cc=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=honnappa.nagarahalli@arm.com \
    --cc=jerinj@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=ktraynor@redhat.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=olivier.matz@6wind.com \
    --cc=sean.morrissey@intel.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    --cc=vladimir.medvedkin@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).