Re: [dpdk-dev] [PATCH v2 1/2] ethdev: support queue-based priority flow control

[Jerin Jacob <jerinjacobk@gmail.com>]

On Tue, Jan 25, 2022 at 11:05 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 1/13/2022 10:27 AM, jerinj@marvell.com wrote:
> > From: Jerin Jacob <jerinj@marvell.com>
> >
> > Based on device support and use-case need, there are two different ways
> > to enable PFC. The first case is the port level PFC configuration, in
> > this case, rte_eth_dev_priority_flow_ctrl_set() API shall be used to
> > configure the PFC, and PFC frames will be generated using based on VLAN
> > TC value.
> >
> > The second case is the queue level PFC configuration, in this
> > case, Any packet field content can be used to steer the packet to the
> > specific queue using rte_flow or RSS and then use
> > rte_eth_dev_priority_flow_ctrl_queue_set() to set the TC mapping on each
> > queue. Based on congestion selected on the specific queue, configured TC
> > shall be used to generate PFC frames.
> > > Operation of these modes are mutually exclusive, when driver sets
> > non zero value for rte_eth_dev_info::pfc_queue_tc_max,
> > application must use queue level PFC configuration via
> > rte_eth_dev_priority_flow_ctrl_queue_set() API instead of port level
> > PFC configuration via rte_eth_dev_priority_flow_ctrl_set() API to
> > realize PFC configuration.
> >
> > This patch enables the configuration for second case a.k.a queue
> > based PFC also updates rte_eth_dev_priority_flow_ctrl_set()
> > implmentaion to adheher to rte_eth_dev_info::pfc_queue_tc_max
> > handling.
> >
> > Also updated libabigail.abignore to ignore the update
> > to reserved fields in rte_eth_dev_info.
> >
> > Signed-off-by: Jerin Jacob <jerinj@marvell.com>
> > Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
> > ---
> >
> > A driver implemtion based on this API is at
> > https://patches.dpdk.org/project/dpdk/patch/20220111081831.881374-1-skori@marvell.com/
> >
> > RFC..v1
> >   - Added queue based PFC config API instead port based
> >
> > v1..v2

> > +
> >   /**
> >    * Tunnel type for device-specific classifier configuration.
> >    * @see rte_eth_udp_tunnel
> > @@ -1841,8 +1863,30 @@ struct rte_eth_dev_info {
> >        * embedded managed interconnect/switch.
> >        */
> >       struct rte_eth_switch_info switch_info;
> > -
> > -     uint64_t reserved_64s[2]; /**< Reserved for future fields */
> > +     /**
> > +      * Maximum supported traffic class as per PFC (802.1Qbb) specification.
> > +      *
> > +      * Based on device support and use-case need, there are two different
> > +      * ways to enable PFC. The first case is the port level PFC
> > +      * configuration, in this case, rte_eth_dev_priority_flow_ctrl_set()
> > +      * API shall be used to configure the PFC, and PFC frames will be
> > +      * generated using based on VLAN TC value.
> > +      * The second case is the queue level PFC configuration, in this case,
> > +      * Any packet field content can be used to steer the packet to the
> > +      * specific queue using rte_flow or RSS and then use
> > +      * rte_eth_dev_priority_flow_ctrl_queue_set() to set the TC mapping
> > +      * on each queue. Based on congestion selected on the specific queue,
> > +      * configured TC shall be used to generate PFC frames.
> > +      *
> > +      * When set to non zero value, application must use queue level
> > +      * PFC configuration via rte_eth_dev_priority_flow_ctrl_queue_set() API
> > +      * instead of port level PFC configuration via
> > +      * rte_eth_dev_priority_flow_ctrl_set() API to realize
> > +      * PFC configuration.
> > +      */
> > +     uint8_t pfc_queue_tc_max;
>
>
> 'rte_eth_dev_info_get()' is one of the APIs that anyone using ethdev needs to use.
>
> Instead of expanding it with less used features, what do you think to have a
> specific API to get the 'pfc_queue_tc_max'?


OK. rte_eth_dev_info_get() was general theme used in ethdev API. Fine
for new API
specifically for this.

>
> It also can be used by application to detect queue based PFC is supported by
> driver or not.

OK.

> Assume API is 'rte_eth_dev_priority_flow_ctrl_get()', if it returns '-ENOTSUP'
> application can know that PMD doesn't support queue based PFC.

OK.

>
>
> > +     uint8_t reserved_8s[7];
> > +     uint64_t reserved_64s[1]; /**< Reserved for future fields */
> >       void *reserved_ptrs[2];   /**< Reserved for future fields */
> >   };
> >
> > @@ -4109,6 +4153,9 @@ int rte_eth_dev_flow_ctrl_set(uint16_t port_id,
> >    * Configure the Ethernet priority flow control under DCB environment
> >    * for Ethernet device.
> >    *
> > + * @see struct rte_eth_dev_info::pfc_queue_tc_max priority
> > + * flow control usage models.
> > + *
> >    * @param port_id
> >    * The port identifier of the Ethernet device.
> >    * @param pfc_conf
> > @@ -4119,10 +4166,40 @@ int rte_eth_dev_flow_ctrl_set(uint16_t port_id,
> >    *   - (-ENODEV)  if *port_id* invalid.
> >    *   - (-EINVAL)  if bad parameter
> >    *   - (-EIO)     if flow control setup failure or device is removed.
> > + *
> >    */
> >   int rte_eth_dev_priority_flow_ctrl_set(uint16_t port_id,
> > -                             struct rte_eth_pfc_conf *pfc_conf);
> > +                                    struct rte_eth_pfc_conf *pfc_conf);
>
> Above syntax changes are not needed.
>
> DPDK coding convention is using tabs (mostly two) for multi line function
> decleration/definition, please be consistant with usage, this patch has
> multiple variations.

Ack.

>
> >
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice.
> > + *
> > + * Configure the Ethernet priority flow control for a given queue
> > + * for Ethernet device.
> > + *
> > + * @see struct rte_eth_dev_info::pfc_queue_tc_max priority flow control
> > + * usage models.
> > + *
> > + * @note When an ethdev port switches to PFC mode, the unconfigured
>
> Doesit mean queue based PFC mode?

Yes.  I will change to PFC mode -> queue-based PFC mode.

>
> > + * queues shall be configured by the driver with default values such as
> > + * lower priority value for TC etc.
> > + *
>
> I assume there is no way for application to know what the defaults values are,
> also not sure if application interested in this.

Yes. I don't think the application cares. But added in the doc to
clarify the state.

>
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param pfc_queue_conf
> > + *   The pointer to the structure of the priority flow control parameters
> > + *   for the queue.
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if hardware doesn't support priority flow control mode.
> > + *   - (-ENODEV)  if *port_id* invalid.
> > + *   - (-EINVAL)  if bad parameter
> > + *   - (-EIO)     if flow control setup queue failure
> > + */
> > +__rte_experimental
> > +int rte_eth_dev_priority_flow_ctrl_queue_set(uint16_t port_id,
> > +                                          struct rte_eth_pfc_queue_conf *pfc_queue_conf);
>
> I wonder if Rx/Tx queue id should be API arguments, to be consistent
> with some other APIs, and will it help application that configures queues
> in a loop.
> But I can see 'rx_pause' or 'tx_pause' (in config) can be valid or not
> based on the 'mode', so I understand to have queue ids in the struct.
> No strong opinion.

Yes. I keep it as is.