Hi Jerin,
I think that we are not on the same page and I'm missing some critical info to
decide
on the best approch.
Can we please have a short meeting so you can explain to me about this feature?
I think it will be good if Thomas, Ferruh and Andrew could join.
Best,
Ori
> -----Original Message-----
> From: Jerin Jacob <jerinjac...@gmail.com>
> Sent: Thursday, November 25, 2021 1:12 PM
> To: Ori Kam <or...@nvidia.com>
> Subject: Re: [dpdk-dev] [RFC PATCH] ethdev: support priority based flow
> control
>
> On Wed, Nov 24, 2021 at 9:30 PM Ori Kam <or...@nvidia.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjac...@gmail.com>
> > > Sent: Wednesday, November 24, 2021 12:48 PM
> > >
> > > On Wed, Nov 24, 2021 at 3:01 PM Ori Kam <or...@nvidia.com> wrote:
> > > >
> > > > Hi Jerin,
> > > >
> > > > > -----Original Message-----
> > > > > From: Jerin Jacob <jerinjac...@gmail.com>
> > > > > Sent: Tuesday, November 23, 2021 12:58 PM
> > > > > To: Ori Kam <or...@nvidia.com>
> > > > >
> > > > > On Sun, Nov 21, 2021 at 3:20 PM Ori Kam <or...@nvidia.com> wrote:
> > > > > >
> > > > > > Hi Jerin,
> > > > >
> > > > > Hi Ori,
> > > > >
> > > > > >
> > > > > > Sorry for my late response,
> > > > >
> > > > > Thanks for the review.
> > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Jerin Jacob <jerinjac...@gmail.com>
> > > > > > > Sent: Wednesday, November 17, 2021 11:49 AM
> > > > > > > To: Jerin Jacob <jer...@marvell.com>
> > > > > > > Subject: Re: [dpdk-dev] [RFC PATCH] ethdev: support priority
> > > > > > > based flow control
> > > > > > >
> > > > > > > On Tue, Oct 5, 2021 at 6:32 PM <jer...@marvell.com> wrote:
> > > > > > > >
> > > > > > > > From: Jerin Jacob <jer...@marvell.com>
> > > > > > > >
> > > > > > > > rte_eth_dev_priority_flow_ctrl_set() based API is not generic
> > > > > > > > as it
> > > > > > > > can not support other than VLAN priority mapping to PFC traffic
> > > > > > > > class.
> > > > > > > >
> > > > > > > > Introducing RTE_FLOW_ACTION_TYPE_PFC_SET_TC rte_flow action to
> > > > > > > > set the traffic class as per 802.1Qbb specification. This will
> > > > > > > > enable,
> > > > > > > > Traffic class(8bit) to be selected based on any packet field
> > > > > > > > like DSCP.
> > > > > > > >
> > > > > > > > Also, making it as rte_flow action will enable fine control on
> > > > > > > > traffic class selection to a specific queue or VF etc.
> > > > > > > >
> > > > > > > > Signed-off-by: Jerin Jacob <jer...@marvell.com>
> > > > > > >
> > > > > > > Ping. If there are no comments on RFC, Planning to send v1 for
> > > > > > > 22.02.
> > > > > >
> > > > > > All the set type of functions are going to be deprecated.
> > > > > > you should use RTE_FLOW_ACTION_TYPE_MODIFY_FIELD.
> > > > > >
> > > > > > What is the item that you are matching on when using rte_flow? Is
> > > > > > it part of the tci in the vlan
> > > item?
> > > > >
> > > > > TC can be VLAN TCI field or DSCP field in IP header or any other field
> > > > > in packet.
> > > > > We need to set the traffic class as per 802.1Qbb specification, May I
> > > > > know how the "modify"
> > > > > attribute helps here. It should be a "set" operation. Right?
> > > > >
> > > >
> > > > Yes, in the rte_flow_action_modify_field there is what operation you
> > > > want to do,
> > > > in this case the action should be set.
> > >
> > > But RTE_FLOW_ACTION_TYPE_MODIFY_FIELD used for modify the packet
> > > content[1]. RIght?
> > > In this case, it is more of sideband data not anything on packet
> > > content. If so, explicit action
> > > makes sense. Right?
> > >
> >
> > It looks like I'm missing something,
> > If you don't want to change the packet and this is just data,
> > why not use tag/mark/flag/metadata?
> >
> > Who should get this data?
> > If the packet is hairpined and the packet is sent to wire this info should
> > be part
> > of the packet right?
>
> No. Here is what I envisioned for working this,
> User add riles like this.
>
> Patten: VLAN TCI is value X or DSCP value Y
> Action: RTE_FLOW_ACTION_TYPE_PFC_SET_TC with an value for TC(8bit
> defined in 802.1Qbb)
> Driver use this rule to enable TC (flow control) with that value for
> the given VLAN TCI == X
>
> tag/mark/flag/metadata used to embed something in mbuf. Here, This
> action establishes, For a given
> flow what TC value needs to be enabled(it does not need to be given in
> mbuf or packet for application to use).
> It just establishes the TC wiring for flow control enablement for a
> given pattern.
> Is it adding up?
>
> >
> > >
> > > [1]
> > > * RTE_FLOW_ACTION_TYPE_MODIFY_FIELD
> > > *
> > > * Modify a destination header field according to the specified
> > > * operation. Another field of the packet can be used as a source as well
> > > * as tag, mark, metadata, immediate value or a pointer to it.
> > >
> > > >
> > > > I assume that you are trying to set the VLAN tag priority field right?
> > >
> > > Both VLAN tag and/or DSCP field.
> > >
> >
> > Going back to the above comment so you are changing something in the packet.
>
> No. See above.
>
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Planning to submit the testpmd and cnxk ethdev driver changes
> > > > > > > > after receiving
> > > > > > > > the feedback on this.
> > > > > > > >
> > > > > > > >
> > > > > > > > doc/guides/prog_guide/rte_flow.rst | 24
> > > > > > > > ++++++++++++++++++++++++
> > > > > > > > lib/ethdev/rte_flow.c | 1 +
> > > > > > > > lib/ethdev/rte_flow.h | 27
> > > > > > > > +++++++++++++++++++++++++++
> > > > > > > > 3 files changed, 52 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/doc/guides/prog_guide/rte_flow.rst
> > > > > > > > b/doc/guides/prog_guide/rte_flow.rst
> > > > > > > > index 2b42d5ec8c..e59f8a2902 100644
> > > > > > > > --- a/doc/guides/prog_guide/rte_flow.rst
> > > > > > > > +++ b/doc/guides/prog_guide/rte_flow.rst
> > > > > > > > @@ -2999,6 +2999,30 @@ which is set in the packet meta-data
> > > > > > > > (i.e. struct
> > > > > ``rte_mbuf::sched::color``)
> > > > > > > > | ``meter_color`` | Packet color |
> > > > > > > > +-----------------+--------------+
> > > > > > > >
> > > > > > > > +Action: ``PFC_SET_TC``
> > > > > > > > +^^^^^^^^^^^^^^^^^^^^^^
> > > > > > > > +
> > > > > > > > +Set traffic class as per PFC (802.1Qbb) specification.
> > > > > > > > +
> > > > > > > > +This action must be used with any of the following action.
> > > > > > > > +
> > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_QUEUE``
> > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_RSS``
> > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_PF``
> > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_VF``
> > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_PHY_PORT``
> > > > > > > > +- ``RTE_FLOW_ACTION_TYPE_PORT_ID``
> > > > > > > > +
> > > > > >
> > > > > > Why? All the above actions are terminating actions so if I want ot
> > > > > > match on the value
> > > > > > it doesn't make sense to have it only on the last rule.
> > > > >
> > > > > In PFC, we are specifying, Given TC needs to steer to specific Queue,
> > > > > RSS, PF etc.
> > > > > Not sure how other actions are relevant for SET_TC action. Do you
> > > > > have any
> > > > > specific action in mind where SET_TC valid in addition to above
> > > > > actions
> > > > >
> > > > First what happens in case of egress traffic? There is no dest action.
> > >
> > > It will be invalid. I can change the documentation to specify egress
> > > direction is not valid.
> > > Thoughts?
> > >
> >
> > Why not? Isn't it possible that application will want to send some packet
> > with this value?
>
> This is Rx Flow control(8bit TC value defined in 802.1Qbb), Not
> relevant when using on Tx.
>
> >
> > > > Second what happens if for example the priority is based on the outer
> > > > tunnel
> > > > which I want decap and at a latter stage I want to do connection
> > > > tracking and only
> > > > if everything is correct I want to send this packet to a queue?
> > >
> > > Which is fine with the current scheme of things as per the documentation,
> > > "This action must be used with any of the following action." it does
> > > not preclude to
> > > use of any other action. If it is not clear, we can reword like below,
> > > ---
> > > This action must be used with any of the following action and not limited
> > > to
> > > using any of other actions in conjunction with the following action.
> > > ---
> > > Thoughts?
> > >
> >
> > Like stated above I can see use case where you want to set this value at
> > the start
> > of the pipe and then based on this value act.
> >
> > For example:
> > 1. decap the packet and based on the tunnel set this value and jump to
> > connection tracking group.
> > 2. run connection tracking and jump to next table
> > 3. Based on the connection tracking and the TC value send to some queue.
>
> Yes. It is possible to have decap + connection tracking +
> RTE_FLOW_ACTION_TYPE_PFC_SET_TC +
> [RTE_FLOW_ACTION_TYPE_QUEUE or RTE_FLOW_ACTION_TYPE_RSS or
> RTE_FLOW_ACTION_TYPE_PF or RTE_FLOW_ACTION_TYPE_VF or
> RTE_FLOW_ACTION_TYPE_PHY_PORT or RTE_FLOW_ACTION_TYPE_PORT_ID]
> cascaded actions.
>
> >
> > Best,
> > Ori
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > > > +.. _table_rte_flow_action_pfc_set_tc:
> > > > > > > > +
> > > > > > > > +.. table:: PFC_SET_PRIORITY
> > > > > > > > +
> > > > > > > > + +-----------------+-------------------------------------+
> > > > > > > > + | Field | Value |
> > > > > > > > + +=================+=====================================+
> > > > > > > > + | ``tc`` | Traffic class as per PFC (802.1Qbb) |
> > > > > > > > + +-----------------+-------------------------------------+
> > > > > > > > +
> > > > > > > > Negative types
> > > > > > > > ~~~~~~~~~~~~~~
> > > > > > > >
> > > > > > > > diff --git a/lib/ethdev/rte_flow.c b/lib/ethdev/rte_flow.c
> > > > > > > > index 8cb7a069c8..75c661159e 100644
> > > > > > > > --- a/lib/ethdev/rte_flow.c
> > > > > > > > +++ b/lib/ethdev/rte_flow.c
> > > > > > > > @@ -189,6 +189,7 @@ static const struct rte_flow_desc_data
> > > > > > > > rte_flow_desc_action[] = {
> > > > > > > > */
> > > > > > > > MK_FLOW_ACTION(INDIRECT, 0),
> > > > > > > > MK_FLOW_ACTION(CONNTRACK, sizeof(struct
> > > > > > > > rte_flow_action_conntrack)),
> > > > > > > > + MK_FLOW_ACTION(PFC_SET_TC, sizeof(struct
> > > > > > > > rte_flow_action_pfc_set_tc)),
> > > > > > > > };
> > > > > > > >
> > > > > > > > int
> > > > > > > > diff --git a/lib/ethdev/rte_flow.h b/lib/ethdev/rte_flow.h
> > > > > > > > index 7b1ed7f110..5298418e9e 100644
> > > > > > > > --- a/lib/ethdev/rte_flow.h
> > > > > > > > +++ b/lib/ethdev/rte_flow.h
> > > > > > > > @@ -2409,6 +2409,13 @@ enum rte_flow_action_type {
> > > > > > > > * See struct rte_flow_action_meter_color.
> > > > > > > > */
> > > > > > > > RTE_FLOW_ACTION_TYPE_METER_COLOR,
> > > > > > > > +
> > > > > > > > + /**
> > > > > > > > + * Set traffic class as per PFC (802.1Qbb)
> > > > > > > > specification.
> > > > > > > > + *
> > > > > > > > + * See struct rte_flow_action_pfc_set_tc.
> > > > > > > > + */
> > > > > > > > + RTE_FLOW_ACTION_TYPE_PFC_SET_TC,
> > > > > > > > };
> > > > > > > >
> > > > > > > > /**
> > > > > > > > @@ -3168,6 +3175,26 @@ struct rte_flow_action_meter_color {
> > > > > > > > enum rte_color color; /**< Packet color. */
> > > > > > > > };
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * @warning
> > > > > > > > + * @b EXPERIMENTAL: this structure may change without prior
> > > > > > > > notice
> > > > > > > > + *
> > > > > > > > + * RTE_FLOW_ACTION_TYPE_PFC_SET_TC
> > > > > > > > + *
> > > > > > > > + * Set traffic class as per PFC (802.1Qbb) specification.
> > > > > > > > + *
> > > > > > > > + * This action must be used any of the following action.
> > > > > > > > + * - RTE_FLOW_ACTION_TYPE_QUEUE,
> > > > > > > > + * - RTE_FLOW_ACTION_TYPE_RSS,
> > > > > > > > + * - RTE_FLOW_ACTION_TYPE_PF,
> > > > > > > > + * - RTE_FLOW_ACTION_TYPE_VF,
> > > > > > > > + * - RTE_FLOW_ACTION_TYPE_PHY_PORT,
> > > > > > > > + * - RTE_FLOW_ACTION_TYPE_PORT_ID
> > > > > > > > + */
> > > > > >
> > > > > > What does it mean? I must use it only on rules that have one of
> > > > > > the above actions?
> > > > >
> > > > > See above.
> > > > >
> > > > > >
> > > > > > > > +struct rte_flow_action_pfc_set_tc {
> > > > > > > > + uint8_t tc; /**< Traffic class as per PFC (802.1Qbb)
> > > > > > > > specification */
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > /**
> > > > > > > > * Field IDs for MODIFY_FIELD action.
> > > > > > > > */
> > > > > > > > --
> > > > > > > > 2.33.0
> > > > > > > >
> > > > > >
> > > > > > Best,
> > > > > > Ori
