Hi,
> -----Original Message-----
> From: Ivan Malov <ivan.ma...@oktetlabs.ru>
> Sent: Wednesday, October 13, 2021 2:15
> To: Slava Ovsiienko <viachesl...@nvidia.com>; dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon <tho...@monjalon.net>; Ori Kam
> <or...@nvidia.com>; Xiaoyun Li <xiaoyun...@intel.com>; Ferruh Yigit
> <ferruh.yi...@intel.com>; Andrew Rybchenko
> <andrew.rybche...@oktetlabs.ru>
> Subject: Re: [dpdk-dev] [PATCH v3 01/12] ethdev: add port representor item
> to flow API
>
> Hi,
>
> On 12/10/2021 23:55, Slava Ovsiienko wrote:
> > Hi,
> >
> > Please, see below.
> >
> >> -----Original Message-----
> >> From: dev <dev-boun...@dpdk.org> On Behalf Of Ivan Malov
> >> Sent: Sunday, October 10, 2021 17:39
> >> To: dev@dpdk.org
> >> Cc: NBU-Contact-Thomas Monjalon <tho...@monjalon.net>; Ori Kam
> >> <or...@nvidia.com>; Xiaoyun Li <xiaoyun...@intel.com>; Ferruh Yigit
> >> <ferruh.yi...@intel.com>; Andrew Rybchenko
> >> <andrew.rybche...@oktetlabs.ru>
> >> Subject: [dpdk-dev] [PATCH v3 01/12] ethdev: add port representor
> >> item to flow API
> >>
> >> For use in "transfer" flows. Supposed to match traffic entering the
> >> embedded switch from the given ethdev.
> >>
> >> Must not be combined with direction attributes.
> >>
> >> Signed-off-by: Ivan Malov <ivan.ma...@oktetlabs.ru>
> >> ---
> >> app/test-pmd/cmdline_flow.c | 27 ++++++++++
> >
> > Should we separate testpmd changes into dedicated commit?
> > This patch intermixes RTE Flow API update and testpmd.
>
> I'm no fan of mixing things like that, but doing so appears to be rather
> logical.
> Each commit should be build testable. If the new defines have no users in the
> same commit, they are dummy. And I believe that readers typically want to
> see the use example in the same commit, too.
Sometime the testpmd change is larged and should be reviewed carefully.
But this series is not a case, and I see it is common practice to include
small updates of testpmd in the primary patch. I wonder why I do not follow
this practice 😉
OK, please, disregard my comment in previous mail.
With best regards, Slava
>
> >
> > With best regards,
> > Slava
> >
> >> doc/guides/prog_guide/rte_flow.rst | 59 +++++++++++++++++++++
> >> doc/guides/rel_notes/release_21_11.rst | 2 +
> >> doc/guides/testpmd_app_ug/testpmd_funcs.rst | 4 ++
> >> lib/ethdev/rte_flow.c | 1 +
> >> lib/ethdev/rte_flow.h | 27 ++++++++++
> >> 6 files changed, 120 insertions(+)
> >>
> >> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> >> index bb22294dd3..a912a8d815 100644
> >> --- a/app/test-pmd/cmdline_flow.c
> >> +++ b/app/test-pmd/cmdline_flow.c
> >> @@ -306,6 +306,8 @@ enum index {
> >> ITEM_POL_PORT,
> >> ITEM_POL_METER,
> >> ITEM_POL_POLICY,
> >> + ITEM_PORT_REPRESENTOR,
> >> + ITEM_PORT_REPRESENTOR_PORT_ID,
> >>
> >> /* Validate/create actions. */
> >> ACTIONS,
> >> @@ -1000,6 +1002,7 @@ static const enum index next_item[] = {
> >> ITEM_GENEVE_OPT,
> >> ITEM_INTEGRITY,
> >> ITEM_CONNTRACK,
> >> + ITEM_PORT_REPRESENTOR,
> >> END_SET,
> >> ZERO,
> >> };
> >> @@ -1368,6 +1371,12 @@ static const enum index item_integrity_lv[] = {
> >> ZERO,
> >> };
> >>
> >> +static const enum index item_port_representor[] = {
> >> + ITEM_PORT_REPRESENTOR_PORT_ID,
> >> + ITEM_NEXT,
> >> + ZERO,
> >> +};
> >> +
> >> static const enum index next_action[] = {
> >> ACTION_END,
> >> ACTION_VOID,
> >> @@ -3608,6 +3617,21 @@ static const struct token token_list[] = {
> >> item_param),
> >> .args = ARGS(ARGS_ENTRY(struct rte_flow_item_conntrack,
> >> flags)),
> >> },
> >> + [ITEM_PORT_REPRESENTOR] = {
> >> + .name = "port_representor",
> >> + .help = "match traffic entering the embedded switch from the
> >> given ethdev",
> >> + .priv = PRIV_ITEM(PORT_REPRESENTOR,
> >> + sizeof(struct rte_flow_item_ethdev)),
> >> + .next = NEXT(item_port_representor),
> >> + .call = parse_vc,
> >> + },
> >> + [ITEM_PORT_REPRESENTOR_PORT_ID] = {
> >> + .name = "port_id",
> >> + .help = "ethdev port ID",
> >> + .next = NEXT(item_port_representor,
> >> NEXT_ENTRY(COMMON_UNSIGNED),
> >> + item_param),
> >> + .args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
> >> port_id)),
> >> + },
> >> /* Validate/create actions. */
> >> [ACTIONS] = {
> >> .name = "actions",
> >> @@ -8343,6 +8367,9 @@ flow_item_default_mask(const struct
> >> rte_flow_item *item)
> >> case RTE_FLOW_ITEM_TYPE_PFCP:
> >> mask = &rte_flow_item_pfcp_mask;
> >> break;
> >> + case RTE_FLOW_ITEM_TYPE_PORT_REPRESENTOR:
> >> + mask = &rte_flow_item_ethdev_mask;
> >> + break;
> >> default:
> >> break;
> >> }
> >> diff --git a/doc/guides/prog_guide/rte_flow.rst
> >> b/doc/guides/prog_guide/rte_flow.rst
> >> index 2b42d5ec8c..91d5bdd712 100644
> >> --- a/doc/guides/prog_guide/rte_flow.rst
> >> +++ b/doc/guides/prog_guide/rte_flow.rst
> >> @@ -1425,6 +1425,65 @@ Matches a conntrack state after conntrack
> action.
> >> - ``flags``: conntrack packet state flags.
> >> - Default ``mask`` matches all state bits.
> >>
> >> +Item: ``PORT_REPRESENTOR``
> >> +^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> +
> >> +Matches traffic entering the embedded switch from the given ethdev.
> >> +
> >> +Term **ethdev** and the concept of **port representor** are
> synonymous.
> >> +The **represented port** is an *entity* plugged to the embedded switch
> >> +at the opposite end of the "wire" leading to the ethdev.
> >> +
> >> +::
> >> +
> >> + .--------------------.
> >> + | PORT_REPRESENTOR | Ethdev (Application Port Referred to by its
> ID)
> >> + '--------------------'
> >> + ||
> >> + \/
> >> + .----------------.
> >> + | Logical Port |
> >> + '----------------'
> >> + ||
> >> + ||
> >> + ||
> >> + \/
> >> + .----------.
> >> + | Switch |
> >> + '----------'
> >> + :
> >> + :
> >> + :
> >> + :
> >> + .----------------.
> >> + | Logical Port |
> >> + '----------------'
> >> + :
> >> + :
> >> + .--------------------.
> >> + | REPRESENTED_PORT | Net / Guest / Another Ethdev (Same
> >> Application)
> >> + '--------------------'
> >> +
> >> +
> >> +- Incompatible with `Attribute: Traffic direction`_.
> >> +- Requires `Attribute: Transfer`_.
> >> +
> >> +.. _table_rte_flow_item_ethdev:
> >> +
> >> +.. table:: ``struct rte_flow_item_ethdev``
> >> +
> >> + +----------+-------------+---------------------------+
> >> + | Field | Subfield | Value |
> >> + +==========+=============+===========================+
> >> + | ``spec`` | ``port_id`` | ethdev port ID |
> >> + +----------+-------------+---------------------------+
> >> + | ``last`` | ``port_id`` | upper range value |
> >> + +----------+-------------+---------------------------+
> >> + | ``mask`` | ``port_id`` | zeroed for wildcard match |
> >> + +----------+-------------+---------------------------+
> >> +
> >> +- Default ``mask`` provides exact match behaviour.
> >> +
> >> Actions
> >> ~~~~~~~
> >>
> >> diff --git a/doc/guides/rel_notes/release_21_11.rst
> >> b/doc/guides/rel_notes/release_21_11.rst
> >> index 89d4b33ef1..1261cb2bf3 100644
> >> --- a/doc/guides/rel_notes/release_21_11.rst
> >> +++ b/doc/guides/rel_notes/release_21_11.rst
> >> @@ -188,6 +188,8 @@ API Changes
> >> Also, make sure to start the actual text at the margin.
> >> =======================================================
> >>
> >> +* ethdev: Added item ``PORT_REPRESENTOR`` to flow API.
> >> +
> >> * kvargs: The experimental function ``rte_kvargs_strcmp()`` has been
> >> removed. Its usages have been replaced by a new function
> >> ``rte_kvargs_get_with_value()``.
> >> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >> index 8ead7a4a71..dcb9f47d98 100644
> >> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >> @@ -3795,6 +3795,10 @@ This section lists supported pattern items and
> >> their attributes, if any.
> >>
> >> - ``conntrack``: match conntrack state.
> >>
> >> +- ``port_representor``: match traffic entering the embedded switch from
> >> +the given ethdev
> >> +
> >> + - ``port_id {unsigned}``: ethdev port ID
> >> +
> >> Actions list
> >> ^^^^^^^^^^^^
> >>
> >> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> >> 8cb7a069c8..5e9317c6d1 100644
> >> --- a/lib/ethdev/rte_flow.c
> >> +++ b/lib/ethdev/rte_flow.c
> >> @@ -100,6 +100,7 @@ static const struct rte_flow_desc_data
> >> rte_flow_desc_item[] = {
> >> MK_FLOW_ITEM(GENEVE_OPT, sizeof(struct
> >> rte_flow_item_geneve_opt)),
> >> MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
> >> MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
> >> + MK_FLOW_ITEM(PORT_REPRESENTOR, sizeof(struct
> >> rte_flow_item_ethdev)),
> >> };
> >>
> >> /** Generate flow_action[] entry. */
> >> diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h index
> >> 7b1ed7f110..3625fd2c12 100644
> >> --- a/lib/ethdev/rte_flow.h
> >> +++ b/lib/ethdev/rte_flow.h
> >> @@ -574,6 +574,15 @@ enum rte_flow_item_type {
> >> * @see struct rte_flow_item_conntrack.
> >> */
> >> RTE_FLOW_ITEM_TYPE_CONNTRACK,
> >> +
> >> + /**
> >> + * [META]
> >> + *
> >> + * Matches traffic entering the embedded switch from the given
> >> ethdev.
> >> + *
> >> + * @see struct rte_flow_item_ethdev
> >> + */
> >> + RTE_FLOW_ITEM_TYPE_PORT_REPRESENTOR,
> >> };
> >>
> >> /**
> >> @@ -1799,6 +1808,24 @@ static const struct rte_flow_item_conntrack
> >> rte_flow_item_conntrack_mask = { }; #endif
> >>
> >> +/**
> >> + * @warning
> >> + * @b EXPERIMENTAL: this structure may change without prior notice
> >> + *
> >> + * Provides an ethdev port ID for use with the following items:
> >> + * RTE_FLOW_ITEM_TYPE_PORT_REPRESENTOR.
> >> + */
> >> +struct rte_flow_item_ethdev {
> >> + uint16_t port_id; /**< ethdev port ID */ };
> >> +
> >> +/** Default mask for items based on struct rte_flow_item_ethdev */
> >> +#ifndef __cplusplus static const struct rte_flow_item_ethdev
> >> +rte_flow_item_ethdev_mask = {
> >> + .port_id = 0xffff,
> >> +};
> >> +#endif
> >> +
> >> /**
> >> * Matching pattern item definition.
> >> *
> >> --
> >> 2.20.1
>
> --
> Ivan M
