[v2,1/4] ethdev: allow negative values in flow rule types
Checks
Commit Message
From: Gregory Etelson <getelson@mellanox.com>
RTE flow items & actions use positive values in item & action type.
Negative values are reserved for PMD private types. PMD
items & actions usually are not exposed to application and are not
used to create RTE flows.
The patch allows applications with access to PMD flow
items & actions ability to integrate RTE and PMD items & actions
and use them to create flow rule.
RTE flow library functions cannot work with PMD private items and
actions (elements) because RTE flow has no API to query PMD flow
object size. In the patch, PMD flow elements use object pointer.
RTE flow library functions handle PMD element object size as
size of a pointer. PMD handles its objects internally.
Signed-off-by: Gregory Etelson <getelson@mellanox.com>
Acked-by: Ori Kam <orika@mellanox.com>
---
v2:
* Update commit log
---
lib/librte_ethdev/rte_flow.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
Comments
On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson <getelson@nvidia.com> wrote:
> From: Gregory Etelson <getelson@mellanox.com>
>
> RTE flow items & actions use positive values in item & action type.
> Negative values are reserved for PMD private types. PMD
> items & actions usually are not exposed to application and are not
> used to create RTE flows.
>
> The patch allows applications with access to PMD flow
> items & actions ability to integrate RTE and PMD items & actions
> and use them to create flow rule.
>
While we are reviewing this, some quick comment/questions..
Doesn't this go against the above "PMD
items & actions usually are not exposed to application and are not
used to create RTE flows."?
Why would an application try to use PMD specific private types?
Isn't this contrary to having a standard API?
>
> RTE flow library functions cannot work with PMD private items and
> actions (elements) because RTE flow has no API to query PMD flow
> object size. In the patch, PMD flow elements use object pointer.
> RTE flow library functions handle PMD element object size as
> size of a pointer. PMD handles its objects internally.
>
> Signed-off-by: Gregory Etelson <getelson@mellanox.com>
> Acked-by: Ori Kam <orika@mellanox.com>
> ---
> v2:
> * Update commit log
> ---
> lib/librte_ethdev/rte_flow.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index f8fdd68fe9..9905426bc9 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -564,7 +564,12 @@ rte_flow_conv_item_spec(void *buf, const size_t size,
> }
> break;
> default:
> - off = rte_flow_desc_item[item->type].size;
> + /**
> + * allow PMD private flow item
> + */
> + off = (uint32_t)item->type <= INT_MAX ?
> + rte_flow_desc_item[item->type].size :
> + sizeof(void *);
> rte_memcpy(buf, data, (size > off ? off : size));
> break;
> }
> @@ -667,7 +672,12 @@ rte_flow_conv_action_conf(void *buf, const size_t
> size,
> }
> break;
> default:
> - off = rte_flow_desc_action[action->type].size;
> + /**
> + * allow PMD private flow action
> + */
> + off = (uint32_t)action->type <= INT_MAX ?
> + rte_flow_desc_action[action->type].size :
> + sizeof(void *);
> rte_memcpy(buf, action->conf, (size > off ? off : size));
> break;
> }
> @@ -709,8 +719,12 @@ rte_flow_conv_pattern(struct rte_flow_item *dst,
> unsigned int i;
>
> for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) {
> - if ((size_t)src->type >= RTE_DIM(rte_flow_desc_item) ||
> - !rte_flow_desc_item[src->type].name)
> + /**
> + * allow PMD private flow item
> + */
> + if (((uint32_t)src->type <= INT_MAX) &&
> + ((size_t)src->type >= RTE_DIM(rte_flow_desc_item)
> ||
> + !rte_flow_desc_item[src->type].name))
> return rte_flow_error_set
> (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ITEM,
> src,
> "cannot convert unknown item type");
> @@ -798,8 +812,12 @@ rte_flow_conv_actions(struct rte_flow_action *dst,
> unsigned int i;
>
> for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) {
> - if ((size_t)src->type >= RTE_DIM(rte_flow_desc_action) ||
> - !rte_flow_desc_action[src->type].name)
> + /**
> + * allow PMD private flow action
> + */
> + if (((uint32_t)src->type <= INT_MAX) &&
> + ((size_t)src->type >= RTE_DIM(rte_flow_desc_action) ||
> + !rte_flow_desc_action[src->type].name))
> return rte_flow_error_set
> (error, ENOTSUP,
> RTE_FLOW_ERROR_TYPE_ACTION,
> src, "cannot convert unknown action
> type");
> --
> 2.25.1
>
>
On 9/8/20 11:15 PM, Gregory Etelson wrote:
> From: Gregory Etelson <getelson@mellanox.com>
>
> RTE flow items & actions use positive values in item & action type.
> Negative values are reserved for PMD private types. PMD
> items & actions usually are not exposed to application and are not
> used to create RTE flows.
>
> The patch allows applications with access to PMD flow
> items & actions ability to integrate RTE and PMD items & actions
> and use them to create flow rule.
>
> RTE flow library functions cannot work with PMD private items and
> actions (elements) because RTE flow has no API to query PMD flow
> object size. In the patch, PMD flow elements use object pointer.
> RTE flow library functions handle PMD element object size as
> size of a pointer. PMD handles its objects internally.
>
> Signed-off-by: Gregory Etelson <getelson@mellanox.com>
> Acked-by: Ori Kam <orika@mellanox.com>
> ---
> v2:
> * Update commit log
> ---
> lib/librte_ethdev/rte_flow.c | 30 ++++++++++++++++++++++++------
> 1 file changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index f8fdd68fe9..9905426bc9 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -564,7 +564,12 @@ rte_flow_conv_item_spec(void *buf, const size_t size,
> }
> break;
> default:
> - off = rte_flow_desc_item[item->type].size;
> + /**
> + * allow PMD private flow item
> + */
> + off = (uint32_t)item->type <= INT_MAX ?
It looks inconsistent to cast to uint32_t and compare vs
INT_MAX. It should be either cast to 'unsigned int' or
compare vs INT32_MAX. I think cast to 'unsigned int' is
better for 'enum' values.
But may be it should be 'int' and < 0 in fact to match
the description that negative values are vendor-specific.
> + rte_flow_desc_item[item->type].size :
> + sizeof(void *);
> rte_memcpy(buf, data, (size > off ? off : size));
> break;
> }
> @@ -667,7 +672,12 @@ rte_flow_conv_action_conf(void *buf, const size_t size,
> }
> break;
> default:
> - off = rte_flow_desc_action[action->type].size;
> + /**
> + * allow PMD private flow action
> + */
> + off = (uint32_t)action->type <= INT_MAX ?
Same
> + rte_flow_desc_action[action->type].size :
> + sizeof(void *);
> rte_memcpy(buf, action->conf, (size > off ? off : size));
> break;
> }
> @@ -709,8 +719,12 @@ rte_flow_conv_pattern(struct rte_flow_item *dst,
> unsigned int i;
>
> for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) {
> - if ((size_t)src->type >= RTE_DIM(rte_flow_desc_item) ||
> - !rte_flow_desc_item[src->type].name)
> + /**
> + * allow PMD private flow item
> + */
> + if (((uint32_t)src->type <= INT_MAX) &&
Same
> + ((size_t)src->type >= RTE_DIM(rte_flow_desc_item) ||
> + !rte_flow_desc_item[src->type].name))
> return rte_flow_error_set
> (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ITEM, src,
> "cannot convert unknown item type");
> @@ -798,8 +812,12 @@ rte_flow_conv_actions(struct rte_flow_action *dst,
> unsigned int i;
>
> for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) {
> - if ((size_t)src->type >= RTE_DIM(rte_flow_desc_action) ||
> - !rte_flow_desc_action[src->type].name)
> + /**
> + * allow PMD private flow action
> + */
> + if (((uint32_t)src->type <= INT_MAX) &&
Same
> + ((size_t)src->type >= RTE_DIM(rte_flow_desc_action) ||
> + !rte_flow_desc_action[src->type].name))
> return rte_flow_error_set
> (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION,
> src, "cannot convert unknown action type");
>
On 9/15/20 7:36 AM, Ajit Khaparde wrote:
> On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson <getelson@nvidia.com
> <mailto:getelson@nvidia.com>> wrote:
>
> From: Gregory Etelson <getelson@mellanox.com
> <mailto:getelson@mellanox.com>>
>
> RTE flow items & actions use positive values in item & action type.
> Negative values are reserved for PMD private types. PMD
> items & actions usually are not exposed to application and are not
> used to create RTE flows.
>
> The patch allows applications with access to PMD flow
> items & actions ability to integrate RTE and PMD items & actions
> and use them to create flow rule.
>
> While we are reviewing this, some quick comment/questions..
>
> Doesn't this go against the above "PMD
> items & actions usually are not exposed to application and are not
> used to create RTE flows."?
> Why would an application try to use PMD specific private types?
> Isn't this contrary to having a standard API?
+1
Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow rule types
On 9/15/20 7:36 AM, Ajit Khaparde wrote:
On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson <getelson@nvidia.com<mailto:getelson@nvidia.com>> wrote:
From: Gregory Etelson <getelson@mellanox.com<mailto:getelson@mellanox.com>>
RTE flow items & actions use positive values in item & action type.
Negative values are reserved for PMD private types. PMD
items & actions usually are not exposed to application and are not
used to create RTE flows.
The patch allows applications with access to PMD flow
items & actions ability to integrate RTE and PMD items & actions
and use them to create flow rule.
While we are reviewing this, some quick comment/questions..
Doesn't this go against the above "PMD
items & actions usually are not exposed to application and are not
used to create RTE flows."?
Why would an application try to use PMD specific private types?
Isn't this contrary to having a standard API?
+1
PMD items and actions obtained in that patch are not intended to be used by application.
In general, application is not aware about content it receives from PMD. This is a special case when application
receives opaque element from PMD and passes it back.
> On 9/8/20 11:15 PM, Gregory Etelson wrote:
> > From: Gregory Etelson <getelson@mellanox.com>
> >
> > RTE flow items & actions use positive values in item & action type.
> > Negative values are reserved for PMD private types. PMD items &
> > actions usually are not exposed to application and are not used to
> > create RTE flows.
> >
> > The patch allows applications with access to PMD flow items & actions
> > ability to integrate RTE and PMD items & actions and use them to
> > create flow rule.
> >
> > RTE flow library functions cannot work with PMD private items and
> > actions (elements) because RTE flow has no API to query PMD flow
> > object size. In the patch, PMD flow elements use object pointer.
> > RTE flow library functions handle PMD element object size as size of a
> > pointer. PMD handles its objects internally.
> >
> > Signed-off-by: Gregory Etelson <getelson@mellanox.com>
> > Acked-by: Ori Kam <orika@mellanox.com>
> > ---
> > v2:
> > * Update commit log
> > ---
> > lib/librte_ethdev/rte_flow.c | 30 ++++++++++++++++++++++++------
> > 1 file changed, 24 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/librte_ethdev/rte_flow.c
> > b/lib/librte_ethdev/rte_flow.c index f8fdd68fe9..9905426bc9 100644
> > --- a/lib/librte_ethdev/rte_flow.c
> > +++ b/lib/librte_ethdev/rte_flow.c
> > @@ -564,7 +564,12 @@ rte_flow_conv_item_spec(void *buf, const size_t
> size,
> > }
> > break;
> > default:
> > - off = rte_flow_desc_item[item->type].size;
> > + /**
> > + * allow PMD private flow item
> > + */
> > + off = (uint32_t)item->type <= INT_MAX ?
>
> It looks inconsistent to cast to uint32_t and compare vs INT_MAX. It should
> be either cast to 'unsigned int' or compare vs INT32_MAX. I think cast to
> 'unsigned int' is better for 'enum' values.
> But may be it should be 'int' and < 0 in fact to match the description that
> negative values are vendor-specific.
>
I'll update PMD private item verification to
(int) item->type < 0
In the next patch release.
The same applies to PMD private actions verification
> > + rte_flow_desc_item[item->type].size :
> > + sizeof(void *);
> > rte_memcpy(buf, data, (size > off ? off : size));
> > break;
> > }
> > @@ -667,7 +672,12 @@ rte_flow_conv_action_conf(void *buf, const size_t
> size,
> > }
> > break;
> > default:
> > - off = rte_flow_desc_action[action->type].size;
> > + /**
> > + * allow PMD private flow action
> > + */
> > + off = (uint32_t)action->type <= INT_MAX ?
>
> Same
>
> > + rte_flow_desc_action[action->type].size :
> > + sizeof(void *);
> > rte_memcpy(buf, action->conf, (size > off ? off : size));
> > break;
> > }
> > @@ -709,8 +719,12 @@ rte_flow_conv_pattern(struct rte_flow_item *dst,
> > unsigned int i;
> >
> > for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) {
> > - if ((size_t)src->type >= RTE_DIM(rte_flow_desc_item) ||
> > - !rte_flow_desc_item[src->type].name)
> > + /**
> > + * allow PMD private flow item
> > + */
> > + if (((uint32_t)src->type <= INT_MAX) &&
>
> Same
>
> > + ((size_t)src->type >= RTE_DIM(rte_flow_desc_item) ||
> > + !rte_flow_desc_item[src->type].name))
> > return rte_flow_error_set
> > (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ITEM, src,
> > "cannot convert unknown item type"); @@
> > -798,8 +812,12 @@ rte_flow_conv_actions(struct rte_flow_action *dst,
> > unsigned int i;
> >
> > for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) {
> > - if ((size_t)src->type >= RTE_DIM(rte_flow_desc_action) ||
> > - !rte_flow_desc_action[src->type].name)
> > + /**
> > + * allow PMD private flow action
> > + */
> > + if (((uint32_t)src->type <= INT_MAX) &&
>
> Same
>
> > + ((size_t)src->type >= RTE_DIM(rte_flow_desc_action) ||
> > + !rte_flow_desc_action[src->type].name))
> > return rte_flow_error_set
> > (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION,
> > src, "cannot convert unknown action
> > type");
> >
From: Gregory Etelson
Sent: Tuesday, September 15, 2020 13:27
To: Andrew Rybchenko <arybchenko@solarflare.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>
Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; Ori Kam <orika@nvidia.com>; Gregory Etelson <getelson@mellanox.com>; Ori Kam <orika@mellanox.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow rule types
Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow rule types
On 9/15/20 7:36 AM, Ajit Khaparde wrote:
On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson <mailto:getelson@nvidia.com> wrote:
From: Gregory Etelson <mailto:getelson@mellanox.com>
RTE flow items & actions use positive values in item & action type.
Negative values are reserved for PMD private types. PMD
items & actions usually are not exposed to application and are not
used to create RTE flows.
The patch allows applications with access to PMD flow
items & actions ability to integrate RTE and PMD items & actions
and use them to create flow rule.
While we are reviewing this, some quick comment/questions..
Doesn't this go against the above "PMD
items & actions usually are not exposed to application and are not
used to create RTE flows."?
Why would an application try to use PMD specific private types?
Isn't this contrary to having a standard API?
+1
I would like to clarify the purpose and use of private elements patch.
That patch is prerequisite for [PATCH v2 2/4] ethdev: tunnel offload model patch.
The tunnel offload API provides unified hardware independent model to offload tunneled packets,
match on packet headers in hardware and to restore outer headers of partially offloaded packets.
The model implementation depends on hardware capabilities. For example, if hardware supports inner nat,
it can do nat first and postpone decap to the end, while other hardware that cannot do inner nat must decap first
and run nat actions afterwards. Such hardware has to save outer header in some hardware context,
register or memory, for application to restore a packet later, if needed. Also, in this case the exact solution
depends on PMD because of limited number of hardware contexts.
Although application working with DKDK can implement all these requirements with existing flow rules API,
it will have to address each hardware specifications separately.
To solve this limitation we selected design where application quires PMD for actions, or items,
that are optimal for a hardware that PMD represents. Result can be a mixture of RTE and PMD private elements -
it's up to PMD implementation. Application passes these elements back to PMD as a flow rule recipe
that's already optimal for underlying hardware.
If PMD has private elements in such rule items or actions, these private elements must not be rejected by RTE layer.
I hope it helps to understand what this model is trying to achieve.
Did that clarify your concerns ?
On 9/16/20 8:21 PM, Gregory Etelson wrote:
> From: Gregory Etelson
> Sent: Tuesday, September 15, 2020 13:27
> To: Andrew Rybchenko <arybchenko@solarflare.com>; Ajit Khaparde <ajit.khaparde@broadcom.com>
> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; Ori Kam <orika@nvidia.com>; Gregory Etelson <getelson@mellanox.com>; Ori Kam <orika@mellanox.com>; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow rule types
>
> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow rule types
> On 9/15/20 7:36 AM, Ajit Khaparde wrote:
> On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson <mailto:getelson@nvidia.com> wrote:
> From: Gregory Etelson <mailto:getelson@mellanox.com>
>
> RTE flow items & actions use positive values in item & action type.
> Negative values are reserved for PMD private types. PMD
> items & actions usually are not exposed to application and are not
> used to create RTE flows.
>
> The patch allows applications with access to PMD flow
> items & actions ability to integrate RTE and PMD items & actions
> and use them to create flow rule.
> While we are reviewing this, some quick comment/questions..
>
> Doesn't this go against the above "PMD
> items & actions usually are not exposed to application and are not
> used to create RTE flows."?
> Why would an application try to use PMD specific private types?
> Isn't this contrary to having a standard API?
>
> +1
>
> I would like to clarify the purpose and use of private elements patch.
> That patch is prerequisite for [PATCH v2 2/4] ethdev: tunnel offload model patch.
> The tunnel offload API provides unified hardware independent model to offload tunneled packets,
> match on packet headers in hardware and to restore outer headers of partially offloaded packets.
> The model implementation depends on hardware capabilities. For example, if hardware supports inner nat,
> it can do nat first and postpone decap to the end, while other hardware that cannot do inner nat must decap first
> and run nat actions afterwards. Such hardware has to save outer header in some hardware context,
> register or memory, for application to restore a packet later, if needed. Also, in this case the exact solution
> depends on PMD because of limited number of hardware contexts.
> Although application working with DKDK can implement all these requirements with existing flow rules API,
> it will have to address each hardware specifications separately.
> To solve this limitation we selected design where application quires PMD for actions, or items,
> that are optimal for a hardware that PMD represents. Result can be a mixture of RTE and PMD private elements -
> it's up to PMD implementation. Application passes these elements back to PMD as a flow rule recipe
> that's already optimal for underlying hardware.
> If PMD has private elements in such rule items or actions, these private elements must not be rejected by RTE layer.
>
> I hope it helps to understand what this model is trying to achieve.
> Did that clarify your concerns ?
There is a very simple question which I can't answer after
reading it.
Why these PMD specific actions and items do not bind
application to a specific vendor. If it binds, it should
be clearly stated in the description. If no, I'd like to
understand why since opaque actions/items are not really
well defined and hardly portable across vendors.
Hi Andrew,
> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Thursday, September 17, 2020 9:50 AM
>
> On 9/16/20 8:21 PM, Gregory Etelson wrote:
> > From: Gregory Etelson
> > Sent: Tuesday, September 15, 2020 13:27
> > To: Andrew Rybchenko <arybchenko@solarflare.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>
> > Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@nvidia.com>; Raslan
> Darawsheh <rasland@nvidia.com>; Ori Kam <orika@nvidia.com>; Gregory
> Etelson <getelson@mellanox.com>; Ori Kam <orika@mellanox.com>; NBU-
> Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow
> rule types
> >
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow
> rule types
> > On 9/15/20 7:36 AM, Ajit Khaparde wrote:
> > On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson
> <mailto:getelson@nvidia.com> wrote:
> > From: Gregory Etelson <mailto:getelson@mellanox.com>
> >
> > RTE flow items & actions use positive values in item & action type.
> > Negative values are reserved for PMD private types. PMD
> > items & actions usually are not exposed to application and are not
> > used to create RTE flows.
> >
> > The patch allows applications with access to PMD flow
> > items & actions ability to integrate RTE and PMD items & actions
> > and use them to create flow rule.
> > While we are reviewing this, some quick comment/questions..
> >
> > Doesn't this go against the above "PMD
> > items & actions usually are not exposed to application and are not
> > used to create RTE flows."?
> > Why would an application try to use PMD specific private types?
> > Isn't this contrary to having a standard API?
> >
> > +1
> >
> > I would like to clarify the purpose and use of private elements patch.
> > That patch is prerequisite for [PATCH v2 2/4] ethdev: tunnel offload model
> patch.
> > The tunnel offload API provides unified hardware independent model to
> offload tunneled packets,
> > match on packet headers in hardware and to restore outer headers of
> partially offloaded packets.
> > The model implementation depends on hardware capabilities. For example,
> if hardware supports inner nat,
> > it can do nat first and postpone decap to the end, while other hardware that
> cannot do inner nat must decap first
> > and run nat actions afterwards. Such hardware has to save outer header in
> some hardware context,
> > register or memory, for application to restore a packet later, if needed. Also,
> in this case the exact solution
> > depends on PMD because of limited number of hardware contexts.
> > Although application working with DKDK can implement all these
> requirements with existing flow rules API,
> > it will have to address each hardware specifications separately.
> > To solve this limitation we selected design where application quires PMD for
> actions, or items,
> > that are optimal for a hardware that PMD represents. Result can be a
> mixture of RTE and PMD private elements -
> > it's up to PMD implementation. Application passes these elements back to
> PMD as a flow rule recipe
> > that's already optimal for underlying hardware.
> > If PMD has private elements in such rule items or actions, these private
> elements must not be rejected by RTE layer.
> >
> > I hope it helps to understand what this model is trying to achieve.
> > Did that clarify your concerns ?
>
> There is a very simple question which I can't answer after
> reading it.
> Why these PMD specific actions and items do not bind
> application to a specific vendor. If it binds, it should
> be clearly stated in the description. If no, I'd like to
> understand why since opaque actions/items are not really
> well defined and hardly portable across vendors.
You are correct, when looking at this patch as a stand a lone
patch using such action / items does bind the application to specific PMD.
first sometimes it is required, for example one vendor may introduce private action
to support some key costumer, or enable feature that is not supported using standard rte flow API.
The main reason for this patch is the tunnel API[1] as stated in the reply
from Gregory, the tunnel API exposes a public function that returns a list of
actions / items. The list is generated by the PMD, so using the API is not binding
since it is generic, but the action / items returned are private but the application is not aware of those actions / items, from it's point of view it called a generic function
and got actions that are configured to do the requested job. All the application needs to do is send the actions / item as actions / item when calling flow create.
Does this answer your question?
[1] https://patches.dpdk.org/patch/76931/
> On 9/16/20 8:21 PM, Gregory Etelson wrote:
> > From: Gregory Etelson
> > Sent: Tuesday, September 15, 2020 13:27
> > To: Andrew Rybchenko <arybchenko@solarflare.com>; Ajit Khaparde
> > <ajit.khaparde@broadcom.com>
> > Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@nvidia.com>; Raslan
> > Darawsheh <rasland@nvidia.com>; Ori Kam <orika@nvidia.com>; Gregory
> > Etelson <getelson@mellanox.com>; Ori Kam <orika@mellanox.com>;
> > NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> > <ferruh.yigit@intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values
> > in flow rule types
> >
> > Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values
> > in flow rule types On 9/15/20 7:36 AM, Ajit Khaparde wrote:
> > On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson
> <mailto:getelson@nvidia.com> wrote:
> > From: Gregory Etelson <mailto:getelson@mellanox.com>
> >
> > RTE flow items & actions use positive values in item & action type.
> > Negative values are reserved for PMD private types. PMD items &
> > actions usually are not exposed to application and are not used to
> > create RTE flows.
> >
> > The patch allows applications with access to PMD flow items & actions
> > ability to integrate RTE and PMD items & actions and use them to
> > create flow rule.
> > While we are reviewing this, some quick comment/questions..
> >
> > Doesn't this go against the above "PMD items & actions usually are not
> > exposed to application and are not used to create RTE flows."?
> > Why would an application try to use PMD specific private types?
> > Isn't this contrary to having a standard API?
> >
> > +1
> >
> > I would like to clarify the purpose and use of private elements patch.
> > That patch is prerequisite for [PATCH v2 2/4] ethdev: tunnel offload model
> patch.
> > The tunnel offload API provides unified hardware independent model to
> > offload tunneled packets, match on packet headers in hardware and to
> restore outer headers of partially offloaded packets.
> > The model implementation depends on hardware capabilities. For
> > example, if hardware supports inner nat, it can do nat first and
> > postpone decap to the end, while other hardware that cannot do inner
> > nat must decap first and run nat actions afterwards. Such hardware has
> > to save outer header in some hardware context, register or memory, for
> application to restore a packet later, if needed. Also, in this case the exact
> solution depends on PMD because of limited number of hardware contexts.
> > Although application working with DKDK can implement all these
> > requirements with existing flow rules API, it will have to address each
> hardware specifications separately.
> > To solve this limitation we selected design where application quires
> > PMD for actions, or items, that are optimal for a hardware that PMD
> > represents. Result can be a mixture of RTE and PMD private elements -
> > it's up to PMD implementation. Application passes these elements back to
> PMD as a flow rule recipe that's already optimal for underlying hardware.
> > If PMD has private elements in such rule items or actions, these private
> elements must not be rejected by RTE layer.
> >
> > I hope it helps to understand what this model is trying to achieve.
> > Did that clarify your concerns ?
>
> There is a very simple question which I can't answer after reading it.
> Why these PMD specific actions and items do not bind application to a
> specific vendor. If it binds, it should be clearly stated in the description. If no,
> I'd like to understand why since opaque actions/items are not really well
> defined and hardly portable across vendors.
Tunnel Offload API does not bind application to a vendor.
One of the main goals of that model is to provide application with vendor/hardware independent solution.
PMD transfer to application an array of items. Application passes that array back to PMD as opaque data,
in rte_flow_create(), without reviewing the array content. Therefore, if there are internal PMD actions in the array,
they have no effect on application.
Consider the following application code example:
/* get PMD actions that implement tunnel offload */
rte_tunnel_decap_set(&tunnel, &pmd_actions, pmd_actions_num, error);
/* compile an array of actions to create flow rule */
memcpy(actions, pmd_actions, pmd_actions_num * sizeof(actions[0]));
memcpy(actions + pmd_actions_num, app_actions, app_actions_num * sizeof(actions[0]));
/* create flow rule*/
rte_flow_create(port_id, attr, pattern, actions, error);
vendor A provides pmd_actions_A = {va1, va2 …. vaN}
vendor B provides pmd_actions_B = {vb1}
Regardless of pmd_actions content, application code will not change.
However, each PMD will receive exact, hardware related, actions for tunnel offload.
Hi Ori,
On 9/17/20 10:47 AM, Ori Kam wrote:
> Hi Andrew,
>
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> Sent: Thursday, September 17, 2020 9:50 AM
>>
>> On 9/16/20 8:21 PM, Gregory Etelson wrote:
>>> From: Gregory Etelson
>>> Sent: Tuesday, September 15, 2020 13:27
>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Ajit Khaparde
>> <ajit.khaparde@broadcom.com>
>>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@nvidia.com>; Raslan
>> Darawsheh <rasland@nvidia.com>; Ori Kam <orika@nvidia.com>; Gregory
>> Etelson <getelson@mellanox.com>; Ori Kam <orika@mellanox.com>; NBU-
>> Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
>> <ferruh.yigit@intel.com>
>>> Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow
>> rule types
>>>
>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values in flow
>> rule types
>>> On 9/15/20 7:36 AM, Ajit Khaparde wrote:
>>> On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson
>> <mailto:getelson@nvidia.com> wrote:
>>> From: Gregory Etelson <mailto:getelson@mellanox.com>
>>>
>>> RTE flow items & actions use positive values in item & action type.
>>> Negative values are reserved for PMD private types. PMD
>>> items & actions usually are not exposed to application and are not
>>> used to create RTE flows.
>>>
>>> The patch allows applications with access to PMD flow
>>> items & actions ability to integrate RTE and PMD items & actions
>>> and use them to create flow rule.
>>> While we are reviewing this, some quick comment/questions..
>>>
>>> Doesn't this go against the above "PMD
>>> items & actions usually are not exposed to application and are not
>>> used to create RTE flows."?
>>> Why would an application try to use PMD specific private types?
>>> Isn't this contrary to having a standard API?
>>>
>>> +1
>>>
>>> I would like to clarify the purpose and use of private elements patch.
>>> That patch is prerequisite for [PATCH v2 2/4] ethdev: tunnel offload model
>> patch.
>>> The tunnel offload API provides unified hardware independent model to
>> offload tunneled packets,
>>> match on packet headers in hardware and to restore outer headers of
>> partially offloaded packets.
>>> The model implementation depends on hardware capabilities. For example,
>> if hardware supports inner nat,
>>> it can do nat first and postpone decap to the end, while other hardware that
>> cannot do inner nat must decap first
>>> and run nat actions afterwards. Such hardware has to save outer header in
>> some hardware context,
>>> register or memory, for application to restore a packet later, if needed. Also,
>> in this case the exact solution
>>> depends on PMD because of limited number of hardware contexts.
>>> Although application working with DKDK can implement all these
>> requirements with existing flow rules API,
>>> it will have to address each hardware specifications separately.
>>> To solve this limitation we selected design where application quires PMD for
>> actions, or items,
>>> that are optimal for a hardware that PMD represents. Result can be a
>> mixture of RTE and PMD private elements -
>>> it's up to PMD implementation. Application passes these elements back to
>> PMD as a flow rule recipe
>>> that's already optimal for underlying hardware.
>>> If PMD has private elements in such rule items or actions, these private
>> elements must not be rejected by RTE layer.
>>>
>>> I hope it helps to understand what this model is trying to achieve.
>>> Did that clarify your concerns ?
>>
>> There is a very simple question which I can't answer after
>> reading it.
>> Why these PMD specific actions and items do not bind
>> application to a specific vendor. If it binds, it should
>> be clearly stated in the description. If no, I'd like to
>> understand why since opaque actions/items are not really
>> well defined and hardly portable across vendors.
>
> You are correct, when looking at this patch as a stand a lone
> patch using such action / items does bind the application to specific PMD.
> first sometimes it is required, for example one vendor may introduce private action
> to support some key costumer, or enable feature that is not supported using standard rte flow API.
Good. So I understand it correctly.
> The main reason for this patch is the tunnel API[1] as stated in the reply
> from Gregory, the tunnel API exposes a public function that returns a list of
> actions / items. The list is generated by the PMD, so using the API is not binding
> since it is generic, but the action / items returned are private but the application is not aware of those actions / items, from it's point of view it called a generic function
> and got actions that are configured to do the requested job. All the application needs to do is send the actions / item as actions / item when calling flow create.
>
> Does this answer your question?
Yes, many thanks. I'm still trying to put the feature design in my head
and your explanations helped a lot. May be it is just the same words as
before, but in a bit different order and highlights.
> [1] https://patches.dpdk.org/patch/76931/
>
On 9/17/20 10:56 AM, Gregory Etelson wrote:
>> On 9/16/20 8:21 PM, Gregory Etelson wrote:
>>> From: Gregory Etelson
>>> Sent: Tuesday, September 15, 2020 13:27
>>> To: Andrew Rybchenko <arybchenko@solarflare.com>; Ajit Khaparde
>>> <ajit.khaparde@broadcom.com>
>>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@nvidia.com>; Raslan
>>> Darawsheh <rasland@nvidia.com>; Ori Kam <orika@nvidia.com>; Gregory
>>> Etelson <getelson@mellanox.com>; Ori Kam <orika@mellanox.com>;
>>> NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
>>> <ferruh.yigit@intel.com>
>>> Subject: RE: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values
>>> in flow rule types
>>>
>>> Subject: Re: [dpdk-dev] [PATCH v2 1/4] ethdev: allow negative values
>>> in flow rule types On 9/15/20 7:36 AM, Ajit Khaparde wrote:
>>> On Tue, Sep 8, 2020 at 1:16 PM Gregory Etelson
>> <mailto:getelson@nvidia.com> wrote:
>>> From: Gregory Etelson <mailto:getelson@mellanox.com>
>>>
>>> RTE flow items & actions use positive values in item & action type.
>>> Negative values are reserved for PMD private types. PMD items &
>>> actions usually are not exposed to application and are not used to
>>> create RTE flows.
>>>
>>> The patch allows applications with access to PMD flow items & actions
>>> ability to integrate RTE and PMD items & actions and use them to
>>> create flow rule.
>>> While we are reviewing this, some quick comment/questions..
>>>
>>> Doesn't this go against the above "PMD items & actions usually are not
>>> exposed to application and are not used to create RTE flows."?
>>> Why would an application try to use PMD specific private types?
>>> Isn't this contrary to having a standard API?
>>>
>>> +1
>>>
>>> I would like to clarify the purpose and use of private elements patch.
>>> That patch is prerequisite for [PATCH v2 2/4] ethdev: tunnel offload model
>> patch.
>>> The tunnel offload API provides unified hardware independent model to
>>> offload tunneled packets, match on packet headers in hardware and to
>> restore outer headers of partially offloaded packets.
>>> The model implementation depends on hardware capabilities. For
>>> example, if hardware supports inner nat, it can do nat first and
>>> postpone decap to the end, while other hardware that cannot do inner
>>> nat must decap first and run nat actions afterwards. Such hardware has
>>> to save outer header in some hardware context, register or memory, for
>> application to restore a packet later, if needed. Also, in this case the exact
>> solution depends on PMD because of limited number of hardware contexts.
>>> Although application working with DKDK can implement all these
>>> requirements with existing flow rules API, it will have to address each
>> hardware specifications separately.
>>> To solve this limitation we selected design where application quires
>>> PMD for actions, or items, that are optimal for a hardware that PMD
>>> represents. Result can be a mixture of RTE and PMD private elements -
>>> it's up to PMD implementation. Application passes these elements back to
>> PMD as a flow rule recipe that's already optimal for underlying hardware.
>>> If PMD has private elements in such rule items or actions, these private
>> elements must not be rejected by RTE layer.
>>>
>>> I hope it helps to understand what this model is trying to achieve.
>>> Did that clarify your concerns ?
>>
>> There is a very simple question which I can't answer after reading it.
>> Why these PMD specific actions and items do not bind application to a
>> specific vendor. If it binds, it should be clearly stated in the description. If no,
>> I'd like to understand why since opaque actions/items are not really well
>> defined and hardly portable across vendors.
>
> Tunnel Offload API does not bind application to a vendor.
> One of the main goals of that model is to provide application with vendor/hardware independent solution.
> PMD transfer to application an array of items. Application passes that array back to PMD as opaque data,
> in rte_flow_create(), without reviewing the array content. Therefore, if there are internal PMD actions in the array,
> they have no effect on application.
> Consider the following application code example:
>
> /* get PMD actions that implement tunnel offload */
> rte_tunnel_decap_set(&tunnel, &pmd_actions, pmd_actions_num, error);
>
> /* compile an array of actions to create flow rule */
> memcpy(actions, pmd_actions, pmd_actions_num * sizeof(actions[0]));
> memcpy(actions + pmd_actions_num, app_actions, app_actions_num * sizeof(actions[0]));
>
> /* create flow rule*/
> rte_flow_create(port_id, attr, pattern, actions, error);
>
> vendor A provides pmd_actions_A = {va1, va2 …. vaN}
> vendor B provides pmd_actions_B = {vb1}
> Regardless of pmd_actions content, application code will not change.
> However, each PMD will receive exact, hardware related, actions for tunnel offload.
>
Many thanks for explanations. I got it. I'll wait for the next version
to take a look at code once again.
@@ -564,7 +564,12 @@ rte_flow_conv_item_spec(void *buf, const size_t size,
}
break;
default:
- off = rte_flow_desc_item[item->type].size;
+ /**
+ * allow PMD private flow item
+ */
+ off = (uint32_t)item->type <= INT_MAX ?
+ rte_flow_desc_item[item->type].size :
+ sizeof(void *);
rte_memcpy(buf, data, (size > off ? off : size));
break;
}
@@ -667,7 +672,12 @@ rte_flow_conv_action_conf(void *buf, const size_t size,
}
break;
default:
- off = rte_flow_desc_action[action->type].size;
+ /**
+ * allow PMD private flow action
+ */
+ off = (uint32_t)action->type <= INT_MAX ?
+ rte_flow_desc_action[action->type].size :
+ sizeof(void *);
rte_memcpy(buf, action->conf, (size > off ? off : size));
break;
}
@@ -709,8 +719,12 @@ rte_flow_conv_pattern(struct rte_flow_item *dst,
unsigned int i;
for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) {
- if ((size_t)src->type >= RTE_DIM(rte_flow_desc_item) ||
- !rte_flow_desc_item[src->type].name)
+ /**
+ * allow PMD private flow item
+ */
+ if (((uint32_t)src->type <= INT_MAX) &&
+ ((size_t)src->type >= RTE_DIM(rte_flow_desc_item) ||
+ !rte_flow_desc_item[src->type].name))
return rte_flow_error_set
(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ITEM, src,
"cannot convert unknown item type");
@@ -798,8 +812,12 @@ rte_flow_conv_actions(struct rte_flow_action *dst,
unsigned int i;
for (i = 0, off = 0; !num || i != num; ++i, ++src, ++dst) {
- if ((size_t)src->type >= RTE_DIM(rte_flow_desc_action) ||
- !rte_flow_desc_action[src->type].name)
+ /**
+ * allow PMD private flow action
+ */
+ if (((uint32_t)src->type <= INT_MAX) &&
+ ((size_t)src->type >= RTE_DIM(rte_flow_desc_action) ||
+ !rte_flow_desc_action[src->type].name))
return rte_flow_error_set
(error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION,
src, "cannot convert unknown action type");