Hi Ivan, please see inline comments.
Thanks, Tomer
> -----Original Message-----
> From: Ivan Malov <ivan.ma...@arknetworks.am>
> Sent: Tuesday, 8 August 2023 2:03
> To: Tomer Shmilovich <tshmilov...@nvidia.com>
> Cc: Ori Kam <or...@nvidia.com>; NBU-Contact-Thomas Monjalon
> (EXTERNAL) <tho...@monjalon.net>; Ferruh Yigit <ferruh.yi...@amd.com>;
> Andrew Rybchenko <andrew.rybche...@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 <tshmilov...@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
> >
> >
