On Sun, Dec 5, 2021 at 11:30 PM Stephen Hemminger
<step...@networkplumber.org> wrote:
>
> On Sun, 5 Dec 2021 12:33:57 +0530
> Jerin Jacob <jerinjac...@gmail.com> wrote:
>
> > On Sat, Dec 4, 2021 at 11:08 PM Stephen Hemminger
> > <step...@networkplumber.org> wrote:
> > >
> > > On Sat, 4 Dec 2021 22:54:58 +0530
> > > <jer...@marvell.com> wrote:
> > >
> > > > + /**
> > > > + * 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;
> > > > + uint8_t reserved_8s[7];
> > > > + uint64_t reserved_64s[1]; /**< Reserved for future fields */
> > > > void *reserved_ptrs[2]; /**< Reserved for future fields */
> > >
> > > Not sure you can claim ABI compatibility because the previous versions of
> > > DPDK
> > > did not enforce that reserved fields must be zero. The Linux kernel
> > > learned this when adding flags for new system calls; reserved fields only
> > > work if you enforce that application must set them to zero.
> >
> > In this case it rte_eth_dev_info is an out parameter and implementation of
> > rte_eth_dev_info_get() already memseting to 0.
> > Do you still see any other ABI issue?
> >
> > See rte_eth_dev_info_get()
> > /*
> > * Init dev_info before port_id check since caller does not have
> > * return status and does not know if get is successful or not.
> > */
> > memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>
> The concern was from the misreading comment. It talks about what application
> should do.
> Could you reword the comment so that it describes what pfc_queue_tc_max is
> here
The comment is at rte_eth_dev_info::pfc_queue_tc_max. So it is implied
that get pararamter.
current comment
---
+ * 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.
---
Is updating to following help to clarify. If so, I will send v2, if
not, Please suggest.
---
+ * When set to non zero value by the driver, 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.
---
> and move the flow control set part of the comment to where the API for that
> is.
The comment is needed for rte_eth_dev_priority_flow_ctrl_set() and
rte_eth_dev_priority_flow_ctrl_queue_set().
Instead of duplicating the comments, I added the comment at
rte_eth_dev_info::pfc_queue_tc_max and
added "@see struct rte_eth_dev_info::pfc_queue_tc_max priority flow
control usage models"
in rte_eth_dev_priority_flow_ctrl_set() and
rte_eth_dev_priority_flow_ctrl_queue_set().
