Hi Ivan,
> -----Original Message-----
> From: Ivan Malov <ivan.ma...@oktetlabs.ru>
> Sent: Monday, October 4, 2021 2:06 PM
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
>
> Hi Ori,
>
> On 04/10/2021 08:45, Ori Kam wrote:
> > Hi Ivan,
> >
> >> -----Original Message-----
> >> From: Ivan Malov <ivan.ma...@oktetlabs.ru>
> >> Sent: Sunday, October 3, 2021 9:11 PM
> >> Subject: Re: [PATCH v1 02/12] ethdev: add eswitch port item to flow
> >> API
> >>
> >>
> >>
> >> On 03/10/2021 15:40, Ori Kam wrote:
> >>> Hi Andrew and Ivan,
> >>>
> >>>> -----Original Message-----
> >>>> From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> >>>> Sent: Friday, October 1, 2021 4:47 PM
> >>>> Subject: [PATCH v1 02/12] ethdev: add eswitch port item to flow API
> >>>>
> >>>> From: Ivan Malov <ivan.ma...@oktetlabs.ru>
> >>>>
> >>>> For use with "transfer" flows. Supposed to match traffic entering
> >>>> the e-switch from the external world (network, guests) via the port
> >>>> which is logically connected with the given ethdev.
> >>>>
> >>>> Must not be combined with attributes "ingress" / "egress".
> >>>>
> >>>> This item is meant to use the same structure as ethdev item.
> >>>>
> >>>
> >>> In case the app is not working with representors, meaning each
> >>> switch port is mapped to ethdev.
> >>> both items (ethdev and eswitch port ) have the same meaning?
> >>
> >> No. Ethdev means ethdev, and e-switch port is the point where this
> >> ethdev is plugged to. For example, "transfer + ESWITCH_PORT" for a
> >> regular PF ethdev typically means the network port (maybe you can
> >> recall the idea that a PF ethdev "represents" the network port it's
> associated with).
> >>
> >> I believe, that diagrams which these patches add to
> >> "doc/guides/prog_guide/rte_flow.rst" may come in handy to understand
> >> the meaning. Also, you can take a look at our larger diagram from the
> >> Sep 14 gathering.
> >>
> >
> > Lets look at the following system:
> > E-Switch has 3 ports - PF, VF1, VF2
> > The ports are distributed as follows:
> > DPDK application:
> > ethdev(0) pf,
> > ethdev(1) representor to VF1
> > ethdev(2) representor to VF2
> > ethdev(3) VF1
> >
> > VM:
> > VF2
> >
> > As we know all representors are realy connected to the PF(at least in
> > this example)
>
> This example tries to say that the e-switch has 3 ports in total, and, given
> your explanation, one may indeed agree that *in this example* representors
> re-use e-switch port of ethdev=0 (with some metadata to distinguish
> packets, etc.). But one can hardly assume that *all* representors with any
> vendor's NIC are connected to the e-switch the same way. It's vendor
> specific. Well, at least, applications don't have this knowledge and don't
> need
> to.
>
> >
> > So matching on ethdev(3) means matching on traffic sent from DPDK port
> 3 right?
>
> Item ETHDEV (ethdev_id=3) matches traffic sent by DPDK port 3. Looks like
> we're on the same page here.
>
Good.
> > And matching on eswitch_port(3) means matching in traffic that goes
> > into VF1 which is the same traffic as ethdev(3) right?
>
> I didn't catch the thought about "the same traffic". Direction is not the
> same.
> Item ESWITCH_PORT (ethdev_id=3) matches traffic sent by DPDK port 1.
>
This is the critical part for my understanding.
Matching on ethdev_id(3) means matching on traffic that is coming from DPDK
port3.
So from E-Switch view point it is traffic that goes into VF1?
While matching on E-Switch_port(3) means matching on traffic coming from VF1?
And by the same logic matching on ethdev_id(1) means matching on taffic that
was sent
from DPDK port 1 and matching on E-Switch_port(1) means matching on traffic
coming from
VF1
So in this case eswitch_port(3) is equal ot eswitch_port(1) right?
While ethdev(1) is not equal to ethdev(3)
And just to complete the picture, matching on ethdev(2) will result in traffic
coming from the dpdk port and matching on eswitch_port(2) will match
on traffic coming from VF2
> Yes, in this case neither of the ports (1, 3) is truly "external" (they both
> interface the DPDK application), but, the thing is, they're "external" *to
> each
> other* in the sense that they sit at the opposite ends of the wire.
>
> >
> > Matching on ethdev(1) means matching on the PF port in the E-Switch but
> with some
> > metadata that marks the traffic as coming from DPDK port 1 and not from
> VF1 E-Switch
> > port right?
>
> That's vendor specific. The application doesn't have to know how exactly
> this particular ethdev is connected to the e-switch - whether it re-uses
> the PF's e-switch port or has its own. The e-switch port that connects
> the ethdev with the e-switch is just assumed to exist logically.
>
> >
> > While matching on eswitch_port(2) means matching on traffic coming from
> the VM right?
>
> Right.
>
I think the my above question will clear everything for me.
> >
> >>>
> >>>> Signed-off-by: Ivan Malov <ivan.ma...@oktetlabs.ru>
> >>>> Signed-off-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
> >>>> ---
> >>>> app/test-pmd/cmdline_flow.c | 27
> +++++++++++++++++++++
> >>>> doc/guides/prog_guide/rte_flow.rst | 22 +++++++++++++++++
> >>>> 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 | 12 ++++++++-
> >>>> 6 files changed, 66 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-
> >> pmd/cmdline_flow.c
> >>>> index e05b0d83d2..188d0ee39d 100644
> >>>> --- a/app/test-pmd/cmdline_flow.c
> >>>> +++ b/app/test-pmd/cmdline_flow.c
> >>>> @@ -308,6 +308,8 @@ enum index {
> >>>> ITEM_POL_POLICY,
> >>>> ITEM_ETHDEV,
> >>>> ITEM_ETHDEV_ID,
> >>>> + ITEM_ESWITCH_PORT,
> >>>> + ITEM_ESWITCH_PORT_ETHDEV_ID,
> >>>
> >>> Like my comment from previous patch, I'm not sure the correct
> >>> term for ETHDEV is ID is should be port.
> >>
> >> Please see my reply in the previous thread. "ethdev" here is an
> >> "anchor", a "beacon" of sorts which allows either to refer namely to
> >> this ethdev or to the e-switch port associated with it.
> >>
> >>>
> >>>>
> >>>> /* Validate/create actions. */
> >>>> ACTIONS,
> >>>> @@ -1003,6 +1005,7 @@ static const enum index next_item[] = {
> >>>> ITEM_INTEGRITY,
> >>>> ITEM_CONNTRACK,
> >>>> ITEM_ETHDEV,
> >>>> + ITEM_ESWITCH_PORT,
> >>>> END_SET,
> >>>> ZERO,
> >>>> };
> >>>> @@ -1377,6 +1380,12 @@ static const enum index item_ethdev[] = {
> >>>> ZERO,
> >>>> };
> >>>>
> >>>> +static const enum index item_eswitch_port[] = {
> >>>> + ITEM_ESWITCH_PORT_ETHDEV_ID,
> >>>> + ITEM_NEXT,
> >>>> + ZERO,
> >>>> +};
> >>>> +
> >>>> static const enum index next_action[] = {
> >>>> ACTION_END,
> >>>> ACTION_VOID,
> >>>> @@ -3632,6 +3641,21 @@ static const struct token token_list[] = {
> >>>> item_param),
> >>>> .args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
> >> id)),
> >>>> },
> >>>> + [ITEM_ESWITCH_PORT] = {
> >>>> + .name = "eswitch_port",
> >>>> + .help = "match traffic at e-switch going from the
> >>>> external port
> >>>> associated with the given ethdev",
> >>>
> >>> Missing the word logically since if we are talking about representor the
> >> connected port
> >>> is the PF while we want to match traffic on one of the FVs.
> >>
> >> Doesn't the word "external" say it all?
> >>
> >> Representor Ethdev <--> Admin ethdev's PF <--> E-Switch <--> VF
> >> (external / the most remote endpoint).
> >>
> >
> > Until the last comment External had totally different meaning to me.
> > I think you should add some place the meaning of external or use
> > the most remote endpoint.
>
> We'll keep this in mind. Given your above example (when both VF and its
> representor) are plugged to the DPDK application, "external" may sound a
> bit off, but maybe it can be retained and just clarified as the "most
> remote endpoint" in the e-switch with respect to the ethdev in question.
>
+1 to the most remote or finding different wording.
> >
> >>>
> >>>> + .priv = PRIV_ITEM(ESWITCH_PORT,
> >>>> + sizeof(struct rte_flow_item_ethdev)),
> >>>> + .next = NEXT(item_eswitch_port),
> >>>> + .call = parse_vc,
> >>>> + },
> >>>> + [ITEM_ESWITCH_PORT_ETHDEV_ID] = {
> >>>> + .name = "ethdev_id",
> >>>> + .help = "ethdev ID",
> >>>> + .next = NEXT(item_eswitch_port,
> >>>> NEXT_ENTRY(COMMON_UNSIGNED),
> >>>> + item_param),
> >>>> + .args = ARGS(ARGS_ENTRY(struct rte_flow_item_ethdev,
> >> id)),
> >>>> + },
> >>>> /* Validate/create actions. */
> >>>> [ACTIONS] = {
> >>>> .name = "actions",
> >>>> @@ -8370,6 +8394,9 @@ flow_item_default_mask(const struct
> >>>> rte_flow_item *item)
> >>>> case RTE_FLOW_ITEM_TYPE_ETHDEV:
> >>>> mask = &rte_flow_item_ethdev_mask;
> >>>> break;
> >>>> + case RTE_FLOW_ITEM_TYPE_ESWITCH_PORT:
> >>>> + mask = &rte_flow_item_ethdev_mask;
> >>>> + break;
> >>>
> >>> Not sure maybe merged the two cases?
> >>
> >> A noble idea.
> >>
> >>>
> >>>> default:
> >>>> break;
> >>>> }
> >>>> diff --git a/doc/guides/prog_guide/rte_flow.rst
> >>>> b/doc/guides/prog_guide/rte_flow.rst
> >>>> index ab628d9139..292bb42410 100644
> >>>> --- a/doc/guides/prog_guide/rte_flow.rst
> >>>> +++ b/doc/guides/prog_guide/rte_flow.rst
> >>>> @@ -1460,6 +1460,28 @@ Use this with attribute **transfer**.
> Attributes
> >>>> **ingress** and
> >>>> | ``mask`` | ``id`` | zeroed for wildcard match |
> >>>> +----------+----------+---------------------------+
> >>>>
> >>>> +Item: ``ESWITCH_PORT``
> >>>> +^^^^^^^^^^^^^^^^^^^^^^
> >>>> +
> >>>> +Matches traffic at e-switch going from the external port associated
> >>>> +with the given ethdev, for example, traffic from net. port or guest.
> >>>
> >>> Maybe replace external with e-switch?
> >>
> >> The word "e-switch" is already here. Basically, all "external" ports are
> >> "e-switch external ports" = ports which connect the e-switch with
> >> non-DPDK world: network ports, VFs passed to guests, etc.
> >>
> >
> > Please see comment above, the word external is not clearly defined. >
> >>>
> >>>> +
> >>>> +::
> >>>> +
> >>>> + * (Ethdev) ~~~~~~~~~~~~ (Internal Port) ~~~~ [] <<<< (External
> >> Port)
> >>>> + * : SW : Logical Net /
> >>>> Guest :
> >>>> + * : :
> >>>> :
> >>>> + * | ---- PMD Layer ---- | ------------ E-Switch Layer
> >>>> ------------ |
> >>>> + *
> >>>> + * [] shows the effective ("transfer") standpoint, the match
> >>>> engine;
> >>>> + * << shows the traffic flow in question hitting the match engine;
> >>>> + * ~~ shows logical interconnects between the endpoints.
> >>>> +
> >>>
> >>> I'm not sure I understand this diagram.
> >>
> >> Thanks for the feedback. I have no way with drawing fancy diagrams, so
> >> this one may not be ideal, yes. But could you please elaborate on your
> >> concerns. Maybe you understand it the right way but just unsure. If you
> >> describe what you see when looking at the diagram, I'll be able to
> >> either confirm your vision or debunk any misunderstanding and possibly
> >> improve the drawing.
> >>
> >
> >
> > I will try,
> > Ethdev is the port that the application sees in DPDK right?
>
> Exactly.
>
> > Internal port is what the PMD sees, for example in case of representor it
> will see the PF port.
>
> Vendor-specific, but, yes, let's assume that.
>
> > External (also due to a drawing in one of your comments) is the E-Switch
> end point.
> > So this drawing shows that the app select the external port that is
> connected to the
> > internal port which is connected to the ethdev port?
>
> So what's the problem?
>
> Ethdev ----- E-switch port A -- ... --- E-switch port B------ VF
>
> So, saying "transfer + ESWITCH_PORT" here implies the e-switch port B.
> How do we name it? "External"? "The most remote"? Any other options?
>
> >
> > Thanks,
> > Ori
> >>>
> >>>> +Use this with attribute **transfer**. Attributes **ingress** and
> >>>> +**egress** (`Attribute: Traffic direction`_) must not be used.
> >>>> +
> >>>> +This item is meant to use the same structure as `Item: ETHDEV`_.
> >>>> +
> >>>> Actions
> >>>> ~~~~~~~
> >>>>
> >>>> diff --git a/doc/guides/rel_notes/release_21_11.rst
> >>>> b/doc/guides/rel_notes/release_21_11.rst
> >>>> index 91631adb4e..b2b27de3f0 100644
> >>>> --- a/doc/guides/rel_notes/release_21_11.rst
> >>>> +++ b/doc/guides/rel_notes/release_21_11.rst
> >>>> @@ -167,7 +167,7 @@ API Changes
> >>>> Also, make sure to start the actual text at the margin.
> >>>>
> >> =======================================================
> >>>>
> >>>> -* ethdev: Added item ``ETHDEV`` to flow API.
> >>>> +* ethdev: Added items ``ETHDEV``, ``ESWITCH_PORT`` to flow API.
> >>>>
> >>>> * cryptodev: The API rte_cryptodev_pmd_is_valid_dev is modified to
> >>>> rte_cryptodev_is_valid_dev as it can be used by the application as
> diff --
> >> git
> >>>> a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> index 6d5de5457c..9a5c2a2d82 100644
> >>>> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> >>>> @@ -3824,6 +3824,10 @@ This section lists supported pattern items
> and
> >>>> their attributes, if any.
> >>>>
> >>>> - ``id {unsigned}``: ethdev ID
> >>>>
> >>>> +- ``eswitch_port``: match traffic at e-switch going from the external
> >>>> +port associated with the given ethdev
> >>>> +
> >>>> + - ``ethdev_id {unsigned}``: ethdev ID
> >>>> +
> >>>> Actions list
> >>>> ^^^^^^^^^^^^
> >>>>
> >>>> diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c index
> >>>> 84eb61cb6c..c4aea5625f 100644
> >>>> --- a/lib/ethdev/rte_flow.c
> >>>> +++ b/lib/ethdev/rte_flow.c
> >>>> @@ -101,6 +101,7 @@ static const struct rte_flow_desc_data
> >>>> rte_flow_desc_item[] = {
> >>>> MK_FLOW_ITEM(INTEGRITY, sizeof(struct rte_flow_item_integrity)),
> >>>> MK_FLOW_ITEM(CONNTRACK, sizeof(uint32_t)),
> >>>> MK_FLOW_ITEM(ETHDEV, sizeof(struct rte_flow_item_ethdev)),
> >>>> + MK_FLOW_ITEM(ESWITCH_PORT, 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
> >>>> 880502098e..1a7e4c2e3d 100644
> >>>> --- a/lib/ethdev/rte_flow.h
> >>>> +++ b/lib/ethdev/rte_flow.h
> >>>> @@ -583,6 +583,16 @@ enum rte_flow_item_type {
> >>>> * @see struct rte_flow_item_ethdev
> >>>> */
> >>>> RTE_FLOW_ITEM_TYPE_ETHDEV,
> >>>> +
> >>>> + /**
> >>>> + * [META]
> >>>> + *
> >>>> + * Matches traffic at e-switch going from the external port
> >>>> associated
> >>>> + * with the given ethdev, for example, traffic from net. port
> >>>> or guest.
> >>>> + *
> >>>> + * @see struct rte_flow_item_ethdev
> >>>> + */
> >>>> + RTE_FLOW_ITEM_TYPE_ESWITCH_PORT,
> >>>> };
> >>>>
> >>>> /**
> >>>> @@ -1813,7 +1823,7 @@ static const struct rte_flow_item_conntrack
> >>>> rte_flow_item_conntrack_mask = {
> >>>> * @b EXPERIMENTAL: this structure may change without prior notice
> >>>> *
> >>>> * Provides an ethdev ID for use with items which are as follows:
> >>>> - * RTE_FLOW_ITEM_TYPE_ETHDEV.
> >>>> + * RTE_FLOW_ITEM_TYPE_ETHDEV,
> >>>> RTE_FLOW_ITEM_TYPE_ESWITCH_PORT.
> >>>> */
> >>>> struct rte_flow_item_ethdev {
> >>>> uint16_t id; /**< Ethdev ID */
> >>>> --
> >>>> 2.30.2
> >>>
> >>> Best,
> >>> Ori
> >>>
> >>
> >> --
> >> Ivan M
>
> --
> Ivan M
Thanks,
Ori
