> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Tuesday, May 24, 2022 5:46 AM
> To: Stephen Hemminger <step...@networkplumber.org>; Spike Du
> <spi...@nvidia.com>
> Cc: dev@dpdk.org; Matan Azrad <ma...@nvidia.com>; Slava Ovsiienko
> <viachesl...@nvidia.com>; Ori Kam <or...@nvidia.com>; Raslan Darawsheh
> <rasl...@nvidia.com>; ferruh.yi...@amd.com;
> andrew.rybche...@oktetlabs.ru
> Subject: Re: [RFC v2 3/7] ethdev: introduce Rx queue based limit watermark
>
> External email: Use caution opening links or attachments
>
>
> 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.
>
> [...]
> >
> > > Why introduce new query/set operations? This should just be part of
> > > the overall device configuration.
>
> Thanks to the "set" function, we can avoid the ABI compat issue.
>
> > Due to different implementation. LWM can be a dynamic configuration
> which can help user design a flexible flow control.
> > User may feel ok with LWM of 80% to get high throughput, or later on with
> 50% to throttle the traffic responsively by handling LWM event in order to
> reduce drop.
> > Some driver like mlx5 may implement LWM event as one-time shot. When
> > you receive LWM event, you need to reconfigure LWM in order to receive
> the event again, thus you will not likely to be overwhelmed by the events.
> > These all require set operation.
>
> Yes it is better to allow dynamic watermark configuration, not using the
> function rte_eth_rx_queue_setup().
>
> > For the query operation. The rte_event API
> rte_eth_dev_callback_process() is per-port API, it doesn't carry much
> information when an event happens.
> > When a LWM event happens, we need to know in which Rx queue it
> happens or optionally what's the current LWM percentage of this queue.
> > The query operation serves this purpose.
>
> Yes "query" has to be called in the event handler because event structure is
> not specific to any event type.
>
>
