Re: Generic flow string parser

Re: Generic flow string parser

[Thomas Monjalon]

This thread is an API suggestion, it should be discussed in
the developer mailing list (did the Cc here).

29/04/2023 16:23, Cliff Burdick:
> > Would rather the flow parser was rewritten as well. Doing open coded
> > parser is much more error prone and hard to extend versus writing the
> > parser in yacc/lex (ie bison/flex).
> 
> I agree, and that's kind of why the original suggestion of using testpmd
> came from. Writing a new parser is obviously the better choice and would
> have been great if testpmd started that way, but a significant amount of
> time was invested in that method. Since it works and is tested, it didn't
> seem like a bad request to build off that and bring that code into an rte_
> API. I'd imagine building a proper parser would not just require the parser
> piece, but also making sure all the tests now use that, and also the legacy
> testpmd was converted. It seemed unlikely all of this could be done in a
> reasonable amount of time and a lot of input from many people. Given the
> amount of debugging I (and others) have spent on figuring on why a flow
> spec didn't work properly, this could be a huge timesaver for new projects
> like Tom mentioned.
> 
> On Fri, Apr 28, 2023 at 5:04 PM Stephen Hemminger <
> stephen@networkplumber.org> wrote:
> 
> > On Fri, 28 Apr 2023 12:13:26 -0700
> > Cliff Burdick <shaklee3@gmail.com> wrote:
> >
> > > Hi Stephen, it would definitely not be worthwhile to repeat everything
> > > that's already tested with testpmd. I was thinking that given that there
> > > already is a "flow_parse" function that does almost everything needed,
> > > something like that could be exposed. If we think of the testpmd flow
> > > string as a sort of "IR" for string flow specification, that would allow
> > > others to implement higher-level transform of a schema like JSON or YAML
> > > into the testpmd language. Due to the complexity of testpmd and how it's
> > > the source of true for testing flows, I think it's too great of an ask to
> > > have testpmd support a new type of parsing. My only suggestion would be
> > > to take what already exists and expose it in a public API that is included
> > > in a DPDK install.

So the only things we need are 2 functions, if I understand well:

int rte_flow_to_text(const struct rte_flow*);
struct rte_flow *rte_flow_from_text(const char *);

Here I assume the output of rte_flow_from_text() would be a created flow,
meaning it calls rte_flow_create() under the hood.
Is it what you wish?
Or should it fill port ID, attributes, patterns and actions?

Re: Generic flow string parser

[Cliff Burdick]

[-- Attachment #1: Type: text/plain, Size: 4321 bytes --]

> So the only things we need are 2 functions, if I understand well:
>
> int rte_flow_to_text(const struct rte_flow*);
> struct rte_flow *rte_flow_from_text(const char *);
>
> Here I assume the output of rte_flow_from_text() would be a created flow,
> meaning it calls rte_flow_create() under the hood.
> Is it what you wish?
> Or should it fill port ID, attributes, patterns and actions?

I think it should follow closely with what "flow_parse" already does:
https://github.com/DPDK/dpdk/blob/d03446724972d2a1bb645ce7f3e64f5ef0203d61/app/test-pmd/cmdline_flow.c#L11304

Namely, just do the part of populating attributes, patterns, and actions
from a string. It's then up to the user if they want to create, validate,
or do something else with it (like see how it populated the structures).
The flow_parse function appears to take an opaque pointer that's specific
to a structure inside of cmdline_flow.c and assign the attributes, actions,
and patterns to members of that result struct. I don't know the reason for
this, but when calling the function the user doesn't know how many patterns
or actions their string will generate. They would either need to pass in
structures that are allocated larger than needed, or have a separate API
that returns how many actions and patterns are needed for a string, then
they need to allocate the correct size themselves. I'm assuming it's not
ideal to have the library itself do dynamic memory allocations for the
correct size.

On Sat, Apr 29, 2023 at 2:40 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> This thread is an API suggestion, it should be discussed in
> the developer mailing list (did the Cc here).
>
> 29/04/2023 16:23, Cliff Burdick:
> > > Would rather the flow parser was rewritten as well. Doing open coded
> > > parser is much more error prone and hard to extend versus writing the
> > > parser in yacc/lex (ie bison/flex).
> >
> > I agree, and that's kind of why the original suggestion of using testpmd
> > came from. Writing a new parser is obviously the better choice and would
> > have been great if testpmd started that way, but a significant amount of
> > time was invested in that method. Since it works and is tested, it didn't
> > seem like a bad request to build off that and bring that code into an
> rte_
> > API. I'd imagine building a proper parser would not just require the
> parser
> > piece, but also making sure all the tests now use that, and also the
> legacy
> > testpmd was converted. It seemed unlikely all of this could be done in a
> > reasonable amount of time and a lot of input from many people. Given the
> > amount of debugging I (and others) have spent on figuring on why a flow
> > spec didn't work properly, this could be a huge timesaver for new
> projects
> > like Tom mentioned.
> >
> > On Fri, Apr 28, 2023 at 5:04 PM Stephen Hemminger <
> > stephen@networkplumber.org> wrote:
> >
> > > On Fri, 28 Apr 2023 12:13:26 -0700
> > > Cliff Burdick <shaklee3@gmail.com> wrote:
> > >
> > > > Hi Stephen, it would definitely not be worthwhile to repeat
> everything
> > > > that's already tested with testpmd. I was thinking that given that
> there
> > > > already is a "flow_parse" function that does almost everything
> needed,
> > > > something like that could be exposed. If we think of the testpmd flow
> > > > string as a sort of "IR" for string flow specification, that would
> allow
> > > > others to implement higher-level transform of a schema like JSON or
> YAML
> > > > into the testpmd language. Due to the complexity of testpmd and how
> it's
> > > > the source of true for testing flows, I think it's too great of an
> ask to
> > > > have testpmd support a new type of parsing. My only suggestion would
> be
> > > > to take what already exists and expose it in a public API that is
> included
> > > > in a DPDK install.
>
> So the only things we need are 2 functions, if I understand well:
>
> int rte_flow_to_text(const struct rte_flow*);
> struct rte_flow *rte_flow_from_text(const char *);
>
> Here I assume the output of rte_flow_from_text() would be a created flow,
> meaning it calls rte_flow_create() under the hood.
> Is it what you wish?
> Or should it fill port ID, attributes, patterns and actions?
>
>
>

[-- Attachment #2: Type: text/html, Size: 5266 bytes --]

Re: Generic flow string parser

[Cliff Burdick]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

On Sat, Apr 29, 2023 at 2:49 PM Cliff Burdick <shaklee3@gmail.com> wrote:

> > So the only things we need are 2 functions, if I understand well:
> >
> > int rte_flow_to_text(const struct rte_flow*);
> > struct rte_flow *rte_flow_from_text(const char *);
> >
> > Here I assume the output of rte_flow_from_text() would be a created flow,
> > meaning it calls rte_flow_create() under the hood.
> > Is it what you wish?
> > Or should it fill port ID, attributes, patterns and actions?
> >
> > I think it should follow closely with what "flow_parse" already does:
> https://github.com/DPDK/dpdk/blob/d03446724972d2a1bb645ce7f3e64f5ef0203d61/app/test-pmd/cmdline_flow.c#L11304
> >
> > Namely, just do the part of populating attributes, patterns, and actions
> from a string. It's then up to the user if they want to create, validate,
> or do something else with it (like see how it populate> d the structures).
> The flow_parse function appears to take an opaque pointer that's specific
> to a structure inside of cmdline_flow.c and assign the attributes, actions,
> and patterns to members of that result struct. I don't know the reason for
> this, but when >calling the function the user doesn't know how many
> patterns or actions their string will generate. They would either need to
> pass in structures that are allocated larger than needed, or have a
> separate API that returns how many actions and patterns are needed for a
> string, then they need to allocate the correct size themselves. I'm
> assuming it's not ideal to have the library itself do dynamic memory
> allocations for the correct size.
>
>>
>> Tom, for what it's worth I'm on a quest to get this to work since I think
it's necessary. I'm just hacking through it like you did and I ran into the
same "template" keyword error. It's probably worthwhile to fix that
anyways. I'm maintaining a set of patches as I go. The general strategy has
been to remove testpmd's main function, compile it as a library, and link
against that.

[-- Attachment #2: Type: text/html, Size: 2769 bytes --]

Re: Generic flow string parser

[kumaraparameshwaran rathinavel]

[-- Attachment #1: Type: text/plain, Size: 3040 bytes --]

On Sun, Apr 30, 2023 at 3:10 AM Thomas Monjalon <thomas@monjalon.net> wrote:

> This thread is an API suggestion, it should be discussed in
> the developer mailing list (did the Cc here).
>
> 29/04/2023 16:23, Cliff Burdick:
> > > Would rather the flow parser was rewritten as well. Doing open coded
> > > parser is much more error prone and hard to extend versus writing the
> > > parser in yacc/lex (ie bison/flex).
> >
> > I agree, and that's kind of why the original suggestion of using testpmd
> > came from. Writing a new parser is obviously the better choice and would
> > have been great if testpmd started that way, but a significant amount of
> > time was invested in that method. Since it works and is tested, it didn't
> > seem like a bad request to build off that and bring that code into an
> rte_
> > API. I'd imagine building a proper parser would not just require the
> parser
> > piece, but also making sure all the tests now use that, and also the
> legacy
> > testpmd was converted. It seemed unlikely all of this could be done in a
> > reasonable amount of time and a lot of input from many people. Given the
> > amount of debugging I (and others) have spent on figuring on why a flow
> > spec didn't work properly, this could be a huge timesaver for new
> projects
> > like Tom mentioned.
> >
> > On Fri, Apr 28, 2023 at 5:04 PM Stephen Hemminger <
> > stephen@networkplumber.org> wrote:
> >
> > > On Fri, 28 Apr 2023 12:13:26 -0700
> > > Cliff Burdick <shaklee3@gmail.com> wrote:
> > >
> > > > Hi Stephen, it would definitely not be worthwhile to repeat
> everything
> > > > that's already tested with testpmd. I was thinking that given that
> there
> > > > already is a "flow_parse" function that does almost everything
> needed,
> > > > something like that could be exposed. If we think of the testpmd flow
> > > > string as a sort of "IR" for string flow specification, that would
> allow
> > > > others to implement higher-level transform of a schema like JSON or
> YAML
> > > > into the testpmd language. Due to the complexity of testpmd and how
> it's
> > > > the source of true for testing flows, I think it's too great of an
> ask to
> > > > have testpmd support a new type of parsing. My only suggestion would
> be
> > > > to take what already exists and expose it in a public API that is
> included
> > > > in a DPDK install.
>
> So the only things we need are 2 functions, if I understand well:
>
> int rte_flow_to_text(const struct rte_flow*);
> struct rte_flow *rte_flow_from_text(const char *);
>
> Here I assume the output of rte_flow_from_text() would be a created flow,
> meaning it calls rte_flow_create() under the hood.
> Is it what you wish?
> Or should it fill port ID, attributes, patterns and actions?
>
>
>> +1 It would be definitely useful to have a generic parser which could
>> re-use the test-pmd parser code as it has already done the heavy lifting. I
>> would be happy to contribute/help to get this going.
>>
>

[-- Attachment #2: Type: text/html, Size: 3910 bytes --]