RE: [RFC] ethdev: add group set miss actions API

[Tomer Shmilovich <tshmilovich@nvidia.com>]

Hi Ivan, please see inline comments.

Thanks, Tomer

> -----Original Message-----
> From: Ivan Malov <ivan.malov@arknetworks.am>
> Sent: Tuesday, 8 August 2023 2:03
> To: Tomer Shmilovich <tshmilovich@nvidia.com>
> Cc: Ori Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon
> (EXTERNAL) <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@amd.com>;
> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; dev@dpdk.org
> Subject: Re: [RFC] ethdev: add group set miss actions API
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Tomer,
> 
> This is a good proposal overall, but it's a bit questionable with regard to the
> transfer domain and precisely group 0.
> 
> Say, the user starts a DPDK application and plugs a PF to it, also plugs a
> representor for a VF. If I'm not mistaken, in this case, default behaviour is
> hardly "undefined" for group 0. Packets sent by a VF are expected to reach the
> representor (and vice versa). Also, packets arriving on the physical port are
> expected to hit the PF port, ethdev 0, for instance.
> 

True.

> Does this new API intend to re-define this? I mean, if the application fails to set
> the default action for group 0 (ENOTSUP), shall it assume that the behaviour
> will be as described above? And if it succeeds, then assume that such implicit
> interconnections cease functioning?
> 
> So, this API is something like "isolated mode"
> in the case of non-transfer API, but allowing to choose a "default" action
> rather than DROP?

You have a point. These are the default "miss actions" for all use cases as I understand them:
Transfer - miss group 0 - goes to the other side of the connection: rep --> VF, VF --> rep.
Transfer - miss group > 0 - goes to E-switch manager (proxy port).
Ingress - miss group 0 - goes to application when expected (i.e. promiscuous mode); otherwise drop/go to kernel in case of bifurcated driver.
Ingress - miss group > 0 - drop.
Egress - miss any group - goes to wire.

I suggest documenting these default "miss actions", and have the new function update the miss actions for a given group.
If an application sets the group's miss actions as none (i.e. actions[0].type == RTE_FLOW_ACTION_TYPE_END), the miss actions should be restored to the aforementioned default miss actions.

Also, a different PMD may define other default miss actions and they should be documented in the NIC doc.

> 
> Also, it is not quite clear how the new API is supposed to co-exist with the
> transfer proxy concept. Has this been properly considered?

All transfer groups are created on the proxy port, so I don't see any conflict when taking into consideration the above definition.

> 
> Thank you.
> 
> On Mon, 7 Aug 2023, Tomer Shmilovich wrote:
> 
> > Introduce new group set miss actions API:
> > rte_flow_group_set_miss_actions().
> >
> > A group's miss actions are a set of actions to be performed in case of
> > a miss on a group, i.e. when a packet didn't hit any flow rules in the
> > group.
> >
> > Currently, the expected behavior in this case is undefined.
> > In order to achieve such functionality, a user can add a flow rule
> > that matches on all traffic with the lowest priority in the group -
> > this is not explicit however, and can be overridden by another flow
> > rule with a lower priority.
> >
> > This new API function allows a user to set a group's miss actions in
> > an explicit way.
> >
> > Signed-off-by: Tomer Shmilovich <tshmilovich@nvidia.com>
> > ---
> > doc/guides/prog_guide/rte_flow.rst | 30 +++++++++++++++++++++++++
> > lib/ethdev/rte_flow.c              | 22 +++++++++++++++++++
> > lib/ethdev/rte_flow.h              | 35 ++++++++++++++++++++++++++++++
> > lib/ethdev/rte_flow_driver.h       |  7 ++++++
> > lib/ethdev/version.map             |  3 +++
> > 5 files changed, 97 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > b/doc/guides/prog_guide/rte_flow.rst
> > index 5bc998a433..590d2a770e 100644
> > --- a/doc/guides/prog_guide/rte_flow.rst
> > +++ b/doc/guides/prog_guide/rte_flow.rst
> > @@ -3758,6 +3758,36 @@ Information about the number of available
> resources can be retrieved via
> >                      struct rte_flow_queue_info *queue_info,
> >                      struct rte_flow_error *error);
> >
> > +Group Miss Actions
> > +~~~~~~~~~~~~~~~~~~
> > +
> > +In an application, many flow rules share common group attributes,
> > +meaning they can be grouped and classified together. A user can
> > +explicitly specify a set of actions performed on a packet when it did not
> match any flows rules in a group using the following API:
> > +
> > +.. code-block:: c
> > +
> > +      int
> > +      rte_flow_group_set_miss_actions(uint16_t port_id,
> > +                                      uint32_t group_id,
> > +                                      const struct rte_flow_group_attr *attr,
> > +                                      const struct rte_flow_action actions[],
> > +                                      struct rte_flow_error *error);
> > +
> > +For example, to configure a RTE_FLOW_TYPE_JUMP action as a miss action
> for ingress group 1:
> > +
> > +.. code-block:: c
> > +
> > +      struct rte_flow_group_attr attr = {.ingress = 1};
> > +      struct rte_flow_action act[] = {
> > +      /* Setting miss actions to jump to group 3 */
> > +          [0] = {.type = RTE_FLOW_ACTION_TYPE_JUMP,
> > +                 .conf = &(struct rte_flow_action_jump){.group = 3}},
> > +          [1] = {.type = RTE_FLOW_ACTION_TYPE_END},
> > +      };
> > +      struct rte_flow_error err;
> > +      rte_flow_group_set_miss_actions(port, 1, &attr, act, &err);
> > +
> > Flow templates
> > ~~~~~~~~~~~~~~
> >
> > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> > 271d854f78..a98d87265f 100644
> > --- a/lib/ethdev/rte_flow.c
> > +++ b/lib/ethdev/rte_flow.c
> > @@ -1973,6 +1973,28 @@ rte_flow_template_table_destroy(uint16_t
> port_id,
> >                                 NULL, rte_strerror(ENOTSUP)); }
> >
> > +int
> > +rte_flow_group_set_miss_actions(uint16_t port_id,
> > +                             uint32_t group_id,
> > +                             const struct rte_flow_group_attr *attr,
> > +                             const struct rte_flow_action actions[],
> > +                             struct rte_flow_error *error) {
> > +     struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > +     const struct rte_flow_ops *ops = rte_flow_ops_get(port_id,
> > +error);
> > +
> > +     if (unlikely(!ops))
> > +             return -rte_errno;
> > +     if (likely(!!ops->group_set_miss_actions)) {
> > +             return flow_err(port_id,
> > +                             ops->group_set_miss_actions(dev, group_id, attr, actions,
> error),
> > +                             error);
> > +     }
> > +     return rte_flow_error_set(error, ENOTSUP,
> > +                               RTE_FLOW_ERROR_TYPE_UNSPECIFIED,
> > +                               NULL, rte_strerror(ENOTSUP)); }
> > +
> > struct rte_flow *
> > rte_flow_async_create(uint16_t port_id,
> >                     uint32_t queue_id, diff --git
> > a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> > 86ed98c562..2d4fd49eb7 100644
> > --- a/lib/ethdev/rte_flow.h
> > +++ b/lib/ethdev/rte_flow.h
> > @@ -129,6 +129,12 @@ struct rte_flow_attr {
> >       uint32_t reserved:29; /**< Reserved, must be zero. */ };
> >
> > +struct rte_flow_group_attr {
> > +     uint32_t ingress:1;
> > +     uint32_t egress:1;
> > +     uint32_t transfer:1;
> > +};
> > +
> > /**
> >  * Matching pattern item types.
> >  *
> > @@ -5839,6 +5845,35 @@ rte_flow_template_table_destroy(uint16_t
> port_id,
> >               struct rte_flow_template_table *template_table,
> >               struct rte_flow_error *error);
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Set group miss actions.
> > + *
> > + * @param port_id
> > + *   Port identifier of Ethernet device.
> > + * @param group_id
> > + *   Identifier of a group to set miss actions for.
> > + * @param attr
> > + *   Group attributes.
> > + * @param actions
> > + *   List of group miss actions.
> > + * @param[out] error
> > + *   Perform verbose error reporting if not NULL.
> > + *   PMDs initialize this structure in case of error only.
> > + *
> > + * @return
> > + *   0 on success, a negative errno value otherwise and rte_errno is set.
> > + */
> > +__rte_experimental
> > +int
> > +rte_flow_group_set_miss_actions(uint16_t port_id,
> > +                             uint32_t group_id,
> > +                             const struct rte_flow_group_attr *attr,
> > +                             const struct rte_flow_action actions[],
> > +                             struct rte_flow_error *error);
> > +
> > /**
> >  * @warning
> >  * @b EXPERIMENTAL: this API may change without prior notice.
> > diff --git a/lib/ethdev/rte_flow_driver.h
> > b/lib/ethdev/rte_flow_driver.h index f9fb01b8a2..3ced086c47 100644
> > --- a/lib/ethdev/rte_flow_driver.h
> > +++ b/lib/ethdev/rte_flow_driver.h
> > @@ -227,6 +227,13 @@ struct rte_flow_ops {
> >               (struct rte_eth_dev *dev,
> >                struct rte_flow_template_table *template_table,
> >                struct rte_flow_error *err);
> > +     /** See rte_flow_group_set_miss_actions() */
> > +     int (*group_set_miss_actions)
> > +             (struct rte_eth_dev *dev,
> > +              uint32_t group_id,
> > +              const struct rte_flow_group_attr *attr,
> > +              const struct rte_flow_action actions[],
> > +              struct rte_flow_error *err);
> >       /** See rte_flow_async_create() */
> >       struct rte_flow *(*async_create)
> >               (struct rte_eth_dev *dev, diff --git
> > a/lib/ethdev/version.map b/lib/ethdev/version.map index
> > fc492ee839..bdd41ecb5e 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -312,6 +312,9 @@ EXPERIMENTAL {
> >       rte_flow_async_action_list_handle_query_update;
> >       rte_flow_async_actions_update;
> >       rte_flow_restore_info_dynflag;
> > +
> > +     # added in 23.11
> > +     rte_flow_group_set_miss_actions;
> > };
> >
> > INTERNAL {
> > --
> > 2.34.1
> >
> >