RE: [dpdk-dev] [RFC PATCH] ethdev: support priority based flow control

[Ori Kam <orika@nvidia.com>]

Hi Jerin,

> -----Original Message-----
> From: Jerin Jacob <jerinjacobk@gmail.com>
> Sent: Tuesday, November 23, 2021 12:58 PM
> To: Ori Kam <orika@nvidia.com>
> 
> On Sun, Nov 21, 2021 at 3:20 PM Ori Kam <orika@nvidia.com> wrote:
> >
> > Hi Jerin,
> 
> Hi Ori,
> 
> >
> > Sorry for my late response,
> 
> Thanks for the review.
> 
> >
> > > -----Original Message-----
> > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > Sent: Wednesday, November 17, 2021 11:49 AM
> > > To: Jerin Jacob <jerinj@marvell.com>
> > > Subject: Re: [dpdk-dev] [RFC PATCH] ethdev: support priority based flow control
> > >
> > > On Tue, Oct 5, 2021 at 6:32 PM <jerinj@marvell.com> wrote:
> > > >
> > > > From: Jerin Jacob <jerinj@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 <jerinj@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.

I assume that you are trying to set the VLAN tag priority field right?

> 
> 
> >
> >
> > >
> > > > ---
> > > >
> > > > 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.
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?

> 
> 
> >
> > > > +.. _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