24/05/2022 04:50, Spike Du:
> From: Thomas Monjalon <tho...@monjalon.net>
> > 23/05/2022 05:01, Spike Du:
> > > From: Stephen Hemminger <step...@networkplumber.org>
> > > > Spike Du <spi...@nvidia.com> wrote:
> > > > > --- a/lib/ethdev/rte_ethdev.h
> > > > > +++ b/lib/ethdev/rte_ethdev.h
> > > > > @@ -1249,7 +1249,16 @@ struct rte_eth_rxconf {
> > > > > */
> > > > > union rte_eth_rxseg *rx_seg;
> > > > >
> > > > > - uint64_t reserved_64s[2]; /**< Reserved for future fields */
> > > > > + /**
> > > > > + * Per-queue Rx limit watermark defined as percentage of Rx
> > > > > queue
> > > > > + * size. If Rx queue receives traffic higher than this
> > > > > percentage,
> > > > > + * the event RTE_ETH_EVENT_RX_LWM is triggered.
> > > > > + */
> > > > > + uint8_t lwm;
> > > > > +
> > > > > + uint8_t reserved_bits[3];
> > > > > + uint32_t reserved_32s;
> > > > > + uint64_t reserved_64s;
> > > >
> > > > Ok but, this is an ABI risk about this because reserved stuff was
> > > > never required before.
> >
> > An ABI compatibility issue would be for an application compiled with an old
> > DPDK, and loading a new DPDK at runtime.
> > Let's think what would happen in such a case.
> >
> > > > Whenever is a reserved field is introduced the code (in this case
> > > > rte_ethdev_configure).
> >
> > rte_eth_rx_queue_setup() is called with rx_conf->lwm not initialized.
> > Then the library and drivers may interpret a wrong value.
> >
> > > > Best practice would have been to have the code require all reserved
> > > > fields be
> > > > 0 in earlier releases. In this case an application is like to define
> > > > a watermark of zero; how will your code handle it.
> > >
> > > Having watermark of 0 is desired, which is the default. LWM of 0 means
> > > the Rx Queue's watermark is not monitored, hence no LWM event is
> > generated.
> >
> > The problem is to have a value not initialized.
> > I think the best approach is to not expose the LWM value through this
> > configuration structure.
> > If the need is to get the current value, we should better add a field in the
> > struct rte_eth_rxq_info.
>
> At least from all the dpdk app/example code, rxconf is initialized to 0 then
> setup
> The Rx queue, if user follows these examples we should not have ABI issue.
> Since many people are concerned about rxconf change, it's ok to remove the LWM
> Field there.
> Yes, I think we can add lwm into rte_eth_rxq_info. If we can set Rx queue's
> attribute,
> We should have a way to get it.
Unfortunately we cannot rely on examples for ABI compatibility.
My suggestion of moving the field in rte_eth_rxq_info
is not obvious because it could change the size of the struct.
But thanks to __rte_cache_min_aligned, it is OK.
Running pahole on this struct shows we have 50 bytes free:
/* size: 128, cachelines: 2, members: 6 */
/* padding: 50 */
The other option would be to get the LWM value with a "get" function.
What others prefer?
