Re: [dpdk-dev] [PATCH] ethdev: support queue-based priority flow control

[Stephen Hemminger <stephen@networkplumber.org>]

On Sun, 5 Dec 2021 12:33:57 +0530
Jerin Jacob <jerinjacobk@gmail.com> wrote:

> On Sat, Dec 4, 2021 at 11:08 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Sat, 4 Dec 2021 22:54:58 +0530
> > <jerinj@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
and move the flow control set part of the comment to where the API for that is.