Re: [PATCH v5 0/2] Add config file support for l3fwd

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

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

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