From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: "Zhang, Qi Z" <qi.z.zhang@intel.com>,
Ferruh Yigit <ferruh.yigit@amd.com>,
"Singh, Aman Deep" <aman.deep.singh@intel.com>,
"Zhang, Yuying" <yuying.zhang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>,
"orika@nvidia.com" <orika@nvidia.com>
Subject: RE: [PATCH v4] app/testpmd: enable cli for programmable action
Date: Thu, 12 Oct 2023 00:04:00 +0000 [thread overview]
Message-ID: <DM4PR11MB599469B70CD69F9F90BBE68ED7D3A@DM4PR11MB5994.namprd11.prod.outlook.com> (raw)
In-Reply-To: <DM4PR11MB5994F439970C6D81D667991ED7CCA@DM4PR11MB5994.namprd11.prod.outlook.com>
> -----Original Message-----
> From: Zhang, Qi Z <qi.z.zhang@intel.com>
> Sent: Wednesday, October 11, 2023 9:20 PM
> To: Ferruh Yigit <ferruh.yigit@amd.com>; Singh, Aman Deep
> <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> Cc: dev@dpdk.org; Dumitrescu, Cristian <cristian.dumitrescu@intel.com>;
> orika@nvidia.com
> Subject: RE: [PATCH v4] app/testpmd: enable cli for programmable action
>
>
>
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@amd.com>
> > Sent: Wednesday, October 11, 2023 6:21 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Singh, Aman Deep
> > <aman.deep.singh@intel.com>; Zhang, Yuying <yuying.zhang@intel.com>
> > Cc: dev@dpdk.org; Dumitrescu, Cristian
> > <cristian.dumitrescu@intel.com>; orika@nvidia.com
> > Subject: Re: [PATCH v4] app/testpmd: enable cli for programmable
> > action
> >
> > On 10/11/2023 3:24 AM, Zhang, Qi Z wrote:
> > >
> > >> -----Original Message-----
> > >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> > >> Sent: Tuesday, October 10, 2023 6:49 PM
> > >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Singh, Aman Deep
> > >> <aman.deep.singh@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>
> > >> Cc: dev@dpdk.org; Dumitrescu, Cristian
> > >> <cristian.dumitrescu@intel.com>; orika@nvidia.com
> > >> Subject: Re: [PATCH v4] app/testpmd: enable cli for programmable
> > >> action
> > >>
> > >> On 10/7/2023 11:47 AM, Qi Zhang wrote:
> > >>> Parsing command line for rte_flow_action_prog.
> > >>>
> > >>> Syntax:
> > >>>
> > >>> "prog name <name> [arguments <arg_name_0> <arg_value_0> \
> > >>> <arg_name_1> <arg_value1> ... end]"
> > >>>
> > >>
> > >> Can you please put full rte flow command in the commit log? Like
> > >> what is the 'pattern' for above command?
> > >
> > > The pattern part should be independent of the action part,
> > >
> > > though for our P4 device, we will prefer use rte_flow_flex_item,
> > > something
> > like:
> > >
> > > flow create 0 pattern flex item is xxx pattern is xxx / flex item is
> > > xxx pattern
> > is / actions prog name ......
> > >
> > > but it does not limit PMD to support flow like below
> > >
> >
> > I think agreement was to use flex pattern, and my understand is
> > "struct rte_flow_item_flex" will be used to present the table_id.
> >
> > Without not using flex, how driver will detect which table to update?
> >
> >
> > > flow create 0 pattern eth / ipv4 src is 1.1.1.1 / actions prog name ......
> > >
> > > So I think it may not be necessary to highlight the pattern format here.
> > >
> >
> > Complete samples helps a lot to user, can you please include the full
> > rte flow command, you can have flex pattern sample and if you want add
> > more samples with other patterns but we need to clarify it first.
>
> Agree, I have added full sample on v6 in testpmd document as below:
>
> ...
> A rule use Programmable Action to perform a customized tunnel header
> encap for specific IP packets
>
> testpmd> flow create 0 ingress pattern eth / ipv4 src is 1.2.3.4 / end actions
> prog name cust_tun_encap arguments tunn_id 55AA meta0 2E meta1 9000
> end / end"
> ...
>
> The reason I did not include a sample with a flex item is that, in the context of
> our P4 device, there is no requirement to utilize the
> 'rte_flow_item_flex_handle' within 'rte_flow_item_flex.'
> However, the current testpmd does not offer support for this configuration.
>
> Therefore, it may be necessary to introduce a new syntax, such as ... pattern
> flex pattern is xxxx / flex pattern is xxx / end ..., which would also map to
> 'rte_flow_item_flex'.
>
> >
> >
> > >>
> > >>
> > >>> Use parse_string0 to parse name string.
> > >>> Use parse_hex to parse hex string.
> > >>> Use struct action_prog_data to store parsed result.
> > >>>
> > >>> Example:
> > >>>
> > >>> Action with 2 arguments:
> > >>>
> > >>> "prog name action0 arguments field0 03FF field1 55AA end"
> > >>>
> > >>> Action without argument:
> > >>>
> > >>> "prog name action1"
> > >>>
> > >>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > >>>
> > >>
> > >> Is there an existing driver implementation, checking it helps to
> > >> understand feature implementation?
> > >
> > > This work is still ongoing, currently we target to upstream on DPDK
> > > 24.03
> > >
> >
> > If you won't have driver yet, do you have a way to test these commands?
> > Or is this implementation just theoretical at this stage?
I may not get your point in my previous reply, to verify the correctness of the new commands before the driver is ready,
Actually, I added some dump code in port_flow_create to verify each prog action and its arguments' size , value
>
> Yes, internally, we are very close to have an implementation that will leverage
> this new API, The JSON file loaded by CPFL PMD contains the mapping rule
> that direct PMD how to handling an action_prog, currently we didn't see any
> gap of the new API.
>
> >
> >
> > >>
> > >>
> > >>> ---
> > >>>
> > >>> v4:
> > >>> - be more generous on the max size of name and value.
> > >>>
> > >>> v3:
> > >>> - refine struct action_prog_data
> > >>> - enlarge the max size
> > >>>
> > >>> v2:
> > >>> - fix title
> > >>> - minor coding style refine.
> > >>>
> > >>> app/test-pmd/cmdline_flow.c | 232
> > >>> ++++++++++++++++++++++++++++++++++++
> > >>> 1 file changed, 232 insertions(+)
> > >>>
> > >>
> > >> Hi Qi,
> > >>
> > >> Can you please update documentation too,
> > >> `doc/guides/testpmd_app_ug/testpmd_funcs.rst`, `Flow rules
> > >> management` section.
> > >
> > > Sure.
> > >
> > >>
> > >>
> > >>> diff --git a/app/test-pmd/cmdline_flow.c
> > >>> b/app/test-pmd/cmdline_flow.c index 21828c144c..ae5556e704 100644
> > >>> --- a/app/test-pmd/cmdline_flow.c
> > >>> +++ b/app/test-pmd/cmdline_flow.c
> > >>> @@ -719,6 +719,13 @@ enum index {
> > >>> ACTION_IPV6_EXT_PUSH,
> > >>> ACTION_IPV6_EXT_PUSH_INDEX,
> > >>> ACTION_IPV6_EXT_PUSH_INDEX_VALUE,
> > >>> + ACTION_PROG,
> > >>> + ACTION_PROG_NAME,
> > >>> + ACTION_PROG_NAME_STRING,
> > >>> + ACTION_PROG_ARGUMENTS,
> > >>> + ACTION_PROG_ARG_NAME,
> > >>> + ACTION_PROG_ARG_VALUE,
> > >>> + ACTION_PROG_ARG_END,
> > >>> };
> > >>>
> > >>> /** Maximum size for pattern in struct rte_flow_item_raw. */ @@
> > >>> -749,6 +756,23 @@ struct action_rss_data {
> > >>> uint16_t queue[ACTION_RSS_QUEUE_NUM]; };
> > >>>
> > >>> +#define ACTION_PROG_NAME_SIZE_MAX 256 #define
> > >> ACTION_PROG_ARG_NUM_MAX
> > >>> +16 #define ACTION_PROG_ARG_VALUE_SIZE_MAX 64
> > >>> +
> > >>> +/** Storage for struct rte_flow_action_prog including external data.
> > >>> +*/ struct action_prog_data {
> > >>> + struct rte_flow_action_prog conf;
> > >>> + struct {
> > >>> + char name[ACTION_PROG_NAME_SIZE_MAX];
> > >>> + struct rte_flow_action_prog_argument
> > >> args[ACTION_PROG_ARG_NUM_MAX];
> > >>> + struct {
> > >>> + char names[ACTION_PROG_NAME_SIZE_MAX];
> > >>> + uint8_t
> > >> value[ACTION_PROG_ARG_VALUE_SIZE_MAX];
> > >>> + } arg_data[ACTION_PROG_ARG_NUM_MAX];
> > >>> + } data;
> > >>> +};
> > >>> +
> > >>> /** Maximum data size in struct rte_flow_action_raw_encap. */
> > >>> #define ACTION_RAW_ENCAP_MAX_DATA 512 #define
> > >> RAW_ENCAP_CONFS_MAX_NUM
> > >>> 8 @@ -2169,6 +2193,7 @@ static const enum index next_action[] = {
> > >>> ACTION_QUOTA_QU,
> > >>> ACTION_IPV6_EXT_REMOVE,
> > >>> ACTION_IPV6_EXT_PUSH,
> > >>> + ACTION_PROG,
> > >>> ZERO,
> > >>> };
> > >>>
> > >>> @@ -2510,6 +2535,13 @@ static const enum index
> > >> action_represented_port[] = {
> > >>> ZERO,
> > >>> };
> > >>>
> > >>> +static const enum index action_prog[] = {
> > >>> + ACTION_PROG_NAME,
> > >>> + ACTION_PROG_ARGUMENTS,
> > >>> + ACTION_NEXT,
> > >>> + ZERO,
> > >>> +};
> > >>> +
> > >>> static int parse_set_raw_encap_decap(struct context *, const
> > >>> struct token
> > *,
> > >>> const char *, unsigned int,
> > >>> void *, unsigned int);
> > >>> @@ -2786,6 +2818,18 @@ static int
> > >>> parse_qu_mode_name(struct context *ctx, const struct token *token,
> > >>> const char *str, unsigned int len, void *buf,
> > >>> unsigned int size);
> > >>> +static int
> > >>> +parse_vc_action_prog(struct context *, const struct token *,
> > >>> + const char *, unsigned int, void *,
> > >>> + unsigned int);
> > >>> +static int
> > >>> +parse_vc_action_prog_arg_name(struct context *, const struct token *,
> > >>> + const char *, unsigned int, void *,
> > >>> + unsigned int);
> > >>> +static int
> > >>> +parse_vc_action_prog_arg_value(struct context *, const struct token *,
> > >>> + const char *, unsigned int, void *,
> > >>> + unsigned int);
> > >>> static int comp_none(struct context *, const struct token *,
> > >>> unsigned int, char *, unsigned int); static int
> > >>> comp_boolean(struct context *, const struct token *, @@ -7518,6
> > >>> +7562,48 @@ static const struct token token_list[] = {
> > >>> .args = ARGS(ARGS_ENTRY(struct rte_flow_item_tx_queue,
> > >>> tx_queue)),
> > >>> },
> > >>> + [ACTION_PROG] = {
> > >>> + .name = "prog",
> > >>> + .help = "match a programmable action",
> > >>> + .priv = PRIV_ACTION(PROG, sizeof(struct action_prog_data)),
> > >>> + .next = NEXT(action_prog),
> > >>> + .call = parse_vc_action_prog,
> > >>> + },
> > >>> + [ACTION_PROG_NAME] = {
> > >>> + .name = "name",
> > >>> + .help = "programble action name",
> > >>>
> > >>
> > >> Can you please remind me again what was the 'name' filed of "struct
> > >> rte_flow_action_prog" was for?
> > >
> > > The 'name' field serves as a means for the driver to identify an
> > > action
> > schema, enabling it to verify if the number of parameters and the size
> > of each parameter value align with the P4 definition.
> > > Subsequently, the driver translates these values into
> > > hardware-specific
> > configurations. If there is a misalignment, the PMD will return a failure.
> > >
> >
> > As I understand it is used for kind of unique action handler, but we
> > have rte_flow handler already, I am still not clear why an action
> > handler is required.
> >
> > Why driver is not using rte flow handler?
>
> Not sure if I have understood your question correctly.
> But, the "name" field you're referring to is not related with an action handler,
> it's more like an action type.
> Just like 'RTE_FLOW_ACTION_TYPE_XXX,' which is predefined, but this action
> type is defined as device-specific.
>
> >
> > Again driver implementation would clear more the intended usage.
>
> At this time, I hope above sample testpmd command can be helpful in this
> matter.
>
> >
> > >>
> > >>
> > >>> + .next = NEXT(action_prog,
> > >> NEXT_ENTRY(ACTION_PROG_NAME_STRING)),
> > >>> + .args = ARGS(ARGS_ENTRY(struct action_prog_data,
> > >> data.name)),
> > >>> + },
> > >>> + [ACTION_PROG_NAME_STRING] = {
> > >>> + .name = "{string}",
> > >>> + .type = "STRING",
> > >>> + .help = "programmable action name string",
> > >>> + .call = parse_string0,
> > >>> + },
> > >>> + [ACTION_PROG_ARGUMENTS] = {
> > >>> + .name = "arguments",
> > >>> + .help = "programmable action name",
> > >>> + .next = NEXT(action_prog,
> > >> NEXT_ENTRY(ACTION_PROG_ARG_NAME)),
> > >>> + .call = parse_vc_conf,
> > >>> + },
> > >>> + [ACTION_PROG_ARG_NAME] = {
> > >>> + .name = "{string}",
> > >>> + .help = "programmable action argument name",
> > >>> + .next = NEXT(NEXT_ENTRY(ACTION_PROG_ARG_VALUE)),
> > >>> + .call = parse_vc_action_prog_arg_name,
> > >>> + },
> > >>> + [ACTION_PROG_ARG_VALUE] = {
> > >>> + .name = "{hex}",
> > >>> + .help = "programmable action argument value",
> > >>> + .next = NEXT(NEXT_ENTRY(ACTION_PROG_ARG_END,
> > >> ACTION_PROG_ARG_NAME)),
> > >>> + .call = parse_vc_action_prog_arg_value,
> > >>> + },
> > >>> + [ACTION_PROG_ARG_END] = {
> > >>> + .name = "end",
> > >>> + .help = "end of the programmable action arguments",
> > >>> + },
> > >>> +
> > >>>
> > >>
> > >> Does this means two 'end' required if multiple args provided, like:
> > >> prog name "name" arguments field0 03FF field1 1 end / end I am
> > >> aware there is variable length of key/value, and need a marker to
> > >> stop, but this end specific for action is not used,
> > >
> > > Actually I borrowed the idea from the queue group in the RSS action:
> > >
> > > 'actions rss queues 0 1 2 3 end / end ..."
> > >
> > > The difference is that the 'end' check was previously hidden within
> > > the
> > 'parse_vc_action_rss_queue' function, while in my implementation, I've
> > defined it as a distinct state.
> > >
> >
> >
> > My concern is if ACTION_xxx_END tokens be confusing or redundant if we
> > have more of them.
> >
> > Is there a benefit to have it as token, comparing to have it in the
> > parser functions as RSS does?
>
> In my opinion, adding more 'ACTION_xxx_END' won't negatively impact code
> readability.
> It enhances the transparency of the syntax and may even facilitate potential
> automation.
next prev parent reply other threads:[~2023-10-12 0:04 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-05 10:02 [PATCH 1/2] " Qi Zhang
2023-10-05 11:42 ` [PATCH v2] " Qi Zhang
2023-10-05 4:32 ` Stephen Hemminger
2023-10-06 2:37 ` Zhang, Qi Z
2023-10-06 11:07 ` [PATCH v3] " Qi Zhang
2023-10-06 12:35 ` Dumitrescu, Cristian
2023-10-07 1:50 ` Zhang, Qi Z
2023-10-07 10:47 ` [PATCH v4] " Qi Zhang
2023-10-08 0:06 ` Dumitrescu, Cristian
2023-10-10 10:49 ` Ferruh Yigit
2023-10-11 2:24 ` Zhang, Qi Z
2023-10-11 10:20 ` Ferruh Yigit
2023-10-11 13:19 ` Zhang, Qi Z
2023-10-12 0:04 ` Zhang, Qi Z [this message]
2023-10-12 1:32 ` Stephen Hemminger
2023-10-27 11:06 ` Zhang, Qi Z
2023-10-11 11:58 ` [PATCH v5] " Qi Zhang
2023-10-11 12:03 ` [PATCH v6] " Qi Zhang
2024-02-08 1:10 ` Ferruh Yigit
2024-04-18 15:39 ` Ferruh Yigit
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=DM4PR11MB599469B70CD69F9F90BBE68ED7D3A@DM4PR11MB5994.namprd11.prod.outlook.com \
--to=qi.z.zhang@intel.com \
--cc=aman.deep.singh@intel.com \
--cc=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=orika@nvidia.com \
--cc=yuying.zhang@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).