On Tue, Jan 25, 2022 at 11:05 PM Ferruh Yigit <ferruh.yi...@intel.com> wrote:
>
> On 1/13/2022 10:27 AM, jer...@marvell.com wrote:
> > From: Jerin Jacob <jer...@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 <jer...@marvell.com>
> > Signed-off-by: Sunil Kumar Kori <sk...@marvell.com>
> > ---
> >
> > A driver implemtion based on this API is at
> > https://patches.dpdk.org/project/dpdk/patch/20220111081831.881374-1-sk...@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.
