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.
[...]
>
> > 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.
