ethdev: add flow API support for P4-programmable devices
Checks
Commit Message
This patch introduces the new "program" action type to enable flow API
support for P4-programmable devices.
In the case of P4-programmable devices, the device is initially blank.
The flow items and actions are defined by the user (outside of any
vendor control) through the P4 program, which is typically compiled
into firmware that is loaded on the device at init time. These flow
items and actions are then used during the run-time phase to add flows
on the device.
Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
---
Change log:
V1:
-Incorporated the feedback from the DPDK Summit 2023, sincere thanks
to the many colleagues who contributed!
-Based on Ori's suggestion, decided to reuse the existing "flex" flow
item instead of defining a new flow item, so only the new "program"
action type is required.
RFC:
-RFC link: https://mails.dpdk.org/archives/dev/2023-August/273703.html
lib/ethdev/rte_flow.h | 50 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
Comments
Hi Cristian,
> -----Original Message-----
> From: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> Sent: Friday, September 15, 2023 10:00 PM
> Subject: [PATCH] ethdev: add flow API support for P4-programmable devices
>
> This patch introduces the new "program" action type to enable flow API
> support for P4-programmable devices.
>
> In the case of P4-programmable devices, the device is initially blank.
> The flow items and actions are defined by the user (outside of any
> vendor control) through the P4 program, which is typically compiled
> into firmware that is loaded on the device at init time. These flow
> items and actions are then used during the run-time phase to add flows
> on the device.
>
> Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> ---
> Change log:
>
> V1:
> -Incorporated the feedback from the DPDK Summit 2023, sincere thanks
> to the many colleagues who contributed!
> -Based on Ori's suggestion, decided to reuse the existing "flex" flow
> item instead of defining a new flow item, so only the new "program"
> action type is required.
>
> RFC:
> -RFC link: https://mails.dpdk.org/archives/dev/2023-August/273703.html
>
> lib/ethdev/rte_flow.h | 50 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 50 insertions(+)
>
> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> index 2ebb76dbc0..9eef5027d0 100644
> --- a/lib/ethdev/rte_flow.h
> +++ b/lib/ethdev/rte_flow.h
> @@ -2981,6 +2981,15 @@ enum rte_flow_action_type {
> * @see struct rte_flow_action_indirect_list
> */
> RTE_FLOW_ACTION_TYPE_INDIRECT_LIST,
> +
> + /**
> + * Program action. These actions are defined by the program currently
> + * loaded on the device. For example, these actions are applicable to
> + * devices that can be programmed through the P4 language.
> + *
> + * @see struct rte_flow_action_prog.
> + */
> + RTE_FLOW_ACTION_TYPE_PROG,
> };
>
> /**
> @@ -4055,6 +4064,47 @@ struct rte_flow_indirect_update_flow_meter_mark
> {
> enum rte_color init_color;
> };
>
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice.
> + *
> + * Program action argument configuration parameters.
> + *
> + * The action argument field length must be non-zero. The action argument
> field
Why can't it be zero? I can see actions that don't have any arguments.
> + * value must be non-NULL, with the value bytes specified in network byte
> order.
> + *
> + * @see struct rte_flow_action_prog
> + */
> +struct rte_flow_action_prog_argument {
> + /** Argument name. */
> + const char *arg_name;
Maybe uint32 id?
> + /** Argument field length. */
> + uint32_t arg_length;
size?
> + /** Argument field value. */
> + const uint8_t *arg_value;
data?
I just wish to make it align with other names we have in rte_flow
for example raw_encap
In any case I think we can drop the arg prefix.
> +};
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice.
> + *
> + * RTE_FLOW_ACTION_TYPE_PROG
> + *
> + * Program action configuration parameters.
> + *
> + * Each action can have zero or more arguments.
> + *
> + * @see RTE_FLOW_ACTION_TYPE_PROG
> + */
> +struct rte_flow_action_prog {
> + /** Action name. */
> + const char *action_name;
> + /** Number of action arguments. */
> + uint32_t action_args_num;
> + /** Action arguments array. */
> + const struct rte_flow_action_prog_argument *action_args;
> +};
> +
> /* Mbuf dynamic field offset for metadata. */
> extern int32_t rte_flow_dynf_metadata_offs;
>
> --
> 2.34.1
Best,
Ori
Hi Ori,
Will implement your comments and send the V2 straight away. Answers inline.
> -----Original Message-----
> From: Ori Kam <orika@nvidia.com>
> Sent: Thursday, September 21, 2023 5:39 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; dev@dpdk.org;
> NBU-Contact-Thomas Monjalon (EXTERNAL) <thomas@monjalon.net>;
> david.marchand@redhat.com; jerinj@marvell.com; jerinjacobk@gmail.com;
> mb@smartsharesystems.com; ferruh.yigit@amd.com; Zhang, Helin
> <helin.zhang@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>;
> Mcnamara, John <john.mcnamara@intel.com>
> Cc: techboard@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: RE: [PATCH] ethdev: add flow API support for P4-programmable
> devices
>
> Hi Cristian,
>
> > -----Original Message-----
> > From: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > Sent: Friday, September 15, 2023 10:00 PM
> > Subject: [PATCH] ethdev: add flow API support for P4-programmable
> devices
> >
> > This patch introduces the new "program" action type to enable flow API
> > support for P4-programmable devices.
> >
> > In the case of P4-programmable devices, the device is initially blank.
> > The flow items and actions are defined by the user (outside of any
> > vendor control) through the P4 program, which is typically compiled
> > into firmware that is loaded on the device at init time. These flow
> > items and actions are then used during the run-time phase to add flows
> > on the device.
> >
> > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > ---
> > Change log:
> >
> > V1:
> > -Incorporated the feedback from the DPDK Summit 2023, sincere thanks
> > to the many colleagues who contributed!
> > -Based on Ori's suggestion, decided to reuse the existing "flex" flow
> > item instead of defining a new flow item, so only the new "program"
> > action type is required.
> >
> > RFC:
> > -RFC link: https://mails.dpdk.org/archives/dev/2023-August/273703.html
> >
> > lib/ethdev/rte_flow.h | 50
> +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 50 insertions(+)
> >
> > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > index 2ebb76dbc0..9eef5027d0 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -2981,6 +2981,15 @@ enum rte_flow_action_type {
> > * @see struct rte_flow_action_indirect_list
> > */
> > RTE_FLOW_ACTION_TYPE_INDIRECT_LIST,
> > +
> > + /**
> > + * Program action. These actions are defined by the program currently
> > + * loaded on the device. For example, these actions are applicable to
> > + * devices that can be programmed through the P4 language.
> > + *
> > + * @see struct rte_flow_action_prog.
> > + */
> > + RTE_FLOW_ACTION_TYPE_PROG,
> > };
> >
> > /**
> > @@ -4055,6 +4064,47 @@ struct
> rte_flow_indirect_update_flow_meter_mark
> > {
> > enum rte_color init_color;
> > };
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice.
> > + *
> > + * Program action argument configuration parameters.
> > + *
> > + * The action argument field length must be non-zero. The action argument
> > field
>
> Why can't it be zero? I can see actions that don't have any arguments.
Yes, actions with no arguments are definitely valid and allowed, but you probably
misread this, as it refers to the configuration of an action argument (for those
actions that have arguments) as opposed to the action.
I will rephrase this to make it easier to read.
>
> > + * value must be non-NULL, with the value bytes specified in network byte
> > order.
> > + *
> > + * @see struct rte_flow_action_prog
> > + */
> > +struct rte_flow_action_prog_argument {
> > + /** Argument name. */
> > + const char *arg_name;
>
> Maybe uint32 id?
The reason for having the argument name also specified here is purely related to
allowing the control plane to specify the action arguments in a potentially different
order than in the P4 program, or to skip some arguments, implying a default value
to be used for these arguments.
Since the arguments have a string identifier in the P4 program, we kept a string
name here for this reason. Having a numerical ID would imply the position of the
argument within the argument list in the P4 program, which is IMO less useful.
I agree we can remove this name, but then we lose the ability explained above.
Therefore, I am going to keep this for now, in case you agree with my explanation,
But we can remove it if you're strongly against it.
>
> > + /** Argument field length. */
> > + uint32_t arg_length;
>
> size?
OK, will change "size".
>
> > + /** Argument field value. */
> > + const uint8_t *arg_value;
>
> data?
>
> I just wish to make it align with other names we have in rte_flow
> for example raw_encap
> In any case I think we can drop the arg prefix.
>
OK, will drop the arg_ prefix in V2.
I am still in favor of using "value" instead of "data", as IMO people find argument
value more suggestive than argument data. And I also see "value" used in other
parts of the API, but happy to change it to "data" if you feel strongly about it.
> > +};
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice.
> > + *
> > + * RTE_FLOW_ACTION_TYPE_PROG
> > + *
> > + * Program action configuration parameters.
> > + *
> > + * Each action can have zero or more arguments.
> > + *
> > + * @see RTE_FLOW_ACTION_TYPE_PROG
> > + */
> > +struct rte_flow_action_prog {
> > + /** Action name. */
> > + const char *action_name;
> > + /** Number of action arguments. */
> > + uint32_t action_args_num;
> > + /** Action arguments array. */
> > + const struct rte_flow_action_prog_argument *action_args;
> > +};
> > +
> > /* Mbuf dynamic field offset for metadata. */
> > extern int32_t rte_flow_dynf_metadata_offs;
> >
> > --
> > 2.34.1
>
> Best,
> Ori
Regards,
Cristian
> From: Dumitrescu, Cristian [mailto:cristian.dumitrescu@intel.com]
> Sent: Monday, 25 September 2023 13.52
>
> Hi Ori,
>
> Will implement your comments and send the V2 straight away. Answers inline.
>
> > From: Ori Kam <orika@nvidia.com>
> > Sent: Thursday, September 21, 2023 5:39 PM
> >
> > Hi Cristian,
> >
> > > From: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > > Sent: Friday, September 15, 2023 10:00 PM
> > >
> > > This patch introduces the new "program" action type to enable flow API
> > > support for P4-programmable devices.
> > >
> > > In the case of P4-programmable devices, the device is initially blank.
> > > The flow items and actions are defined by the user (outside of any
> > > vendor control) through the P4 program, which is typically compiled
> > > into firmware that is loaded on the device at init time. These flow
> > > items and actions are then used during the run-time phase to add flows
> > > on the device.
> > >
> > > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> > > ---
[...]
> > > +struct rte_flow_action_prog_argument {
> > > + /** Argument name. */
> > > + const char *arg_name;
> >
> > Maybe uint32 id?
>
> The reason for having the argument name also specified here is purely related
> to
> allowing the control plane to specify the action arguments in a potentially
> different
> order than in the P4 program, or to skip some arguments, implying a default
> value
> to be used for these arguments.
>
> Since the arguments have a string identifier in the P4 program, we kept a
> string
> name here for this reason. Having a numerical ID would imply the position of
> the
> argument within the argument list in the P4 program, which is IMO less useful.
Having an index would imply the position within the argument list.
However, an ID is more like an enum, and does not imply any position.
The application would probably need to treat the possible string values like enum values anyway. In other words: It's not really a string, it's an enum represented by a string. So we might as well represent it by an integer type (uint32_t).
>
> I agree we can remove this name, but then we lose the ability explained above.
> Therefore, I am going to keep this for now, in case you agree with my
> explanation,
> But we can remove it if you're strongly against it.
Intuitively, a numerical ID seems easier and faster to handle than a string.
I don’t feel strongly about this. And I'm not really involved with RTE_FLOW or P4, so not really qualified. ;-)
@@ -2981,6 +2981,15 @@ enum rte_flow_action_type {
* @see struct rte_flow_action_indirect_list
*/
RTE_FLOW_ACTION_TYPE_INDIRECT_LIST,
+
+ /**
+ * Program action. These actions are defined by the program currently
+ * loaded on the device. For example, these actions are applicable to
+ * devices that can be programmed through the P4 language.
+ *
+ * @see struct rte_flow_action_prog.
+ */
+ RTE_FLOW_ACTION_TYPE_PROG,
};
/**
@@ -4055,6 +4064,47 @@ struct rte_flow_indirect_update_flow_meter_mark {
enum rte_color init_color;
};
+/**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice.
+ *
+ * Program action argument configuration parameters.
+ *
+ * The action argument field length must be non-zero. The action argument field
+ * value must be non-NULL, with the value bytes specified in network byte order.
+ *
+ * @see struct rte_flow_action_prog
+ */
+struct rte_flow_action_prog_argument {
+ /** Argument name. */
+ const char *arg_name;
+ /** Argument field length. */
+ uint32_t arg_length;
+ /** Argument field value. */
+ const uint8_t *arg_value;
+};
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this structure may change without prior notice.
+ *
+ * RTE_FLOW_ACTION_TYPE_PROG
+ *
+ * Program action configuration parameters.
+ *
+ * Each action can have zero or more arguments.
+ *
+ * @see RTE_FLOW_ACTION_TYPE_PROG
+ */
+struct rte_flow_action_prog {
+ /** Action name. */
+ const char *action_name;
+ /** Number of action arguments. */
+ uint32_t action_args_num;
+ /** Action arguments array. */
+ const struct rte_flow_action_prog_argument *action_args;
+};
+
/* Mbuf dynamic field offset for metadata. */
extern int32_t rte_flow_dynf_metadata_offs;