[dpdk-dev] [PATCH 1/3] app/testpmd: Moved cmdline_flow to librte_cmdline

george.dit at gmail.com george.dit at gmail.com
Wed Jan 24 12:57:00 CET 2018


Hi again,

I decided to simplify things for now, hence sent a new minimal patch
only for testpmd (not librte_cmdline), which allows external
applications to compile against it without errors.

Thanks again for you time,
Georgios

On Tue, Jan 16, 2018 at 6:54 PM, Adrien Mazarguil
<adrien.mazarguil at 6wind.com> wrote:
> On Tue, Jan 16, 2018 at 03:54:57PM +0100, george.dit at gmail.com wrote:
>> Hi Adrien,
>>
>> Thanks for your insights and sorry for omitting the cover letter (this is
>> my first patch in DPDK).
>
> No problem, don't worry about that. Remember to put as much context as close
> as possible to the related changes. The commit log of a patch is in fact a
> more appropriate place since a cover letter is simply a summary of a
> subsequent series and is discarded once applied.
>
>> I understand your concerns. The reason I proposed this patch is to
>> facilitate a more high level vendor agnostic API for configuring and
>> monitoring DPDK-based NICs.
>>
>> To avoid just copying thousands of lines of code into my application, do
>> you think it is feasible to move at least the components (struct context,
>> struct arg, struct token and the parse_* helpers) you mentioned and
>> restructure testpmd in a way that allows applications to re-use all of its
>> parsing features?
>
> Yes I think it's feasible, although at the cost of great effort because
> you'd need to untangle engine code from parser code to expose the former,
> the flow command being a mix of both. Proper APIs must be devised for that,
> hence my question: is it really worth the trouble?
>
> Other contributors already asked me how they could reuse the flow command
> parser to implement similar testpmd commands without copy/pasting the entire
> file, so I'm already convinced separating at least the engine part makes
> sense at the testpmd level. Moving it to librte_cmdline for external
> applications seems more complex though.
>
>> I could give it a try and come back with a new patch, otherwise I am
>> perfectly ok if you want to do it instead.
>
> While I'd certainly like to do it (at least at the testpmd level), it's
> unlikely to happen anytime soon due to other priorities.
>
> Feel free to take care of it if you're motivated enough, just keep in mind
> right now I don't think this should be exposed as a public API. I can change
> my mind if you manage to make it generic enough.
>
>> On Tue, Jan 16, 2018 at 3:31 PM, Adrien Mazarguil <
>> adrien.mazarguil at 6wind.com> wrote:
>>
>> > George,
>> >
>> > I missed the original RFC thread [1][2] (you should have used it as a cover
>> > letter for this series BTW) please see below.
>> >
>> > On Tue, Jan 16, 2018 at 10:24:25AM +0100, Olivier Matz wrote:
>> > > Hi,
>> > >
>> > > > On Tue, Jan 16, 2018 at 9:40 AM Olivier Matz <olivier.matz at 6wind.com>
>> > wrote:
>> > > >
>> > > > On Tue, Jan 16, 2018 at 08:45:32AM +0000, george.dit at gmail.com wrote:
>> > > > > Hi Georgios,
>> > > > >
>> > > > > On Mon, Jan 15, 2018 at 01:30:35AM +0000, Lu, Wenzhuo wrote:
>> > > > > > Hi,
>> > > > > >
>> > > > > > > -----Original Message-----
>> > > > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Georgios
>> > Katsikas
>> > > > > > > Sent: Saturday, January 13, 2018 5:01 AM
>> > > > > > > To: olivier.matz at 6wind.com
>> > > > > > > Cc: dev at dpdk.org; Georgios Katsikas <george.dit at gmail.com>
>> > > > > > > Subject: [dpdk-dev] [PATCH 1/3] app/testpmd: Moved cmdline_flow
>> > to
>> > > > > > > librte_cmdline
>> > > > > > >
>> > > > > > > Signed-off-by: Georgios Katsikas <george.dit at gmail.com>
>> > > > > > Looks like a good idea to move this code to the lib.
>> > > > > > cc Adrien the author of this file, app/test-pmd/cmdline_flow.c.
>> > > > >
>> > > > > If the command line parsing of rte_flow is something that has some
>> > > > > chances to be shared among multiple applications, I agree it makes
>> > sense
>> > > > > to move it in a library.
>> > > > >
>> > > > > However, my opinion is that it would be better to have a specific
>> > > > > library for it, like librte_flow_cmdline, because I'm not sure that
>> > > > > people linking with librte_cmdline always want to pull the rte_flow
>> > > > > parsing code.
>> >
>> > In my opinion the entire flow command parser has nothing to do outside
>> > testpmd, it's way too specific, even if another application finds it
>> > useful.
>> >
>> > Code duplication being a bad thing, your application could as well compile
>> > or even #include app/test-pmd/cmdline_flow.c directly (not pretty, I know)
>> > since it would have the same internal layout as testpmd. Testpmd's Makefile
>> > could be modified to spit it out as a separate library if needed.
>> >
>> > What could make more sense would be to extract the parser engine for
>> > librte_cmdline's dynamic tokens (e.g. struct context, struct arg, struct
>> > token) and possibly various generic helpers (e.g. parse_string,
>> > parse_mac_addr), but not enum index nor token_list[] and friends which
>> > define the layout of testpmd's flow command.
>> >
>> > This would enable other flow-like commands without duplicating the engine
>> > every time, however I'm still not sure if making it available outside
>> > testpmd is a good idea. Extracting and making it fully generic will require
>> > a considerable amount of work.
>> >
>> > > > Hi Lu, Oliver,
>> > > >
>> > > > Thanks for your feedback!
>> > > > You have a point here, flow commands are only a subset of the parser.
>> > > > Do you want me to create this new library and send another patch?
>> > >
>> > > Let's first wait for Adrien's feedback, he can have counter-arguments.
>> > >
>> > > > I guess I have to use librte_cmdline as a template/example for
>> > creating the
>> > > > librte_flow_cmdline library.
>> > >
>> > > It can be used as an example for Makefile and .map file.
>> >
>> > I'm not opposed to the idea of exporting the parser engine after making it
>> > properly generic, but please reconsider. Testpmd is an application made to
>> > validate PMD functionality. The flow command implementation, as neat as it
>> > is, remains a complex wrapper on top of the cmdline_parse API which wasn't
>> > originally designed for dynamic tokens. Its syntax may evolve without
>> > warning if deemed necessary. Making it public will subject it to exported
>> > API/ABI constraints and likely impede future evolution.
>> >
>> > Assuming your application is not dragging testpmd's legacy, I think it
>> > would
>> > be wiser to re-implement a simpler flow command look-alike on top of a more
>> > suitable command-line handling library if you like its syntax.
>> >
>> > [1] http://dpdk.org/ml/archives/dev/2018-January/086872.html
>> > [2] Message-ID: CAN9HtFDz+imqbCKfs6a0NE0W7iF8C+-KiNB0nCRywimspjfEDg at mail.
>> > gmail.com
>> >
>> > --
>> > Adrien Mazarguil
>> > 6WIND
>> >
>>
>>
>>
>> --
>> Georgios Katsikas
>> Industrial Ph.D. Student
>> Network Intelligence Group
>> Decision, Networks, and Analytics (DNA) Lab
>> RISE SICS
>> E-Mail:  georgios.katsikas at ri.se
>
> --
> Adrien Mazarguil
> 6WIND



-- 
Georgios Katsikas
Industrial Ph.D. Student
Network Intelligence Group
Decision, Networks, and Analytics (DNA) Lab
RISE SICS
E-Mail:  georgios.katsikas at ri.se


More information about the dev mailing list