Re: [RFC v1 3/7] ethdev: introduce Rx queue based limit watermark

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 5/6/22 06:56, Spike Du wrote:
> LWM(limit watermark) is a per Rx queue attribute that notifies dpdk

dpdk is not necessary about.

I'm not sure that "attribute" can notify application. Please,
reword the description.

> application event of RTE_ETH_EVENT_RXQ_LIMIT_REACHED when the Rx
> queue's usable descriptor is under the watermark.
> To simplify its configuration, LWM is a percentage of Rx queue
> descriptor size with valid value of [0,99].
> Setting LWM to 0 means disable it.

... which is the default.

Can I request notification when no descriptors left?
1 seems to be close to the answer, but not in the case of big
Rx rings.

> Add LWM's configuration handle in eth_dev_ops.

handle sounds bad here. May be "driver callback" or "driver
method".

> 
> Signed-off-by: Spike Du <spiked@nvidia.com>
> ---
>   lib/ethdev/ethdev_driver.h |  7 +++++++
>   lib/ethdev/rte_ethdev.c    | 28 ++++++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev.h    | 30 +++++++++++++++++++++++++++++-
>   lib/ethdev/version.map     |  3 +++
>   4 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 69d9dc2..1e9cdbf 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -470,6 +470,10 @@ typedef int (*eth_rx_queue_setup_t)(struct rte_eth_dev *dev,
>   				    const struct rte_eth_rxconf *rx_conf,
>   				    struct rte_mempool *mb_pool);
>   
> +typedef int (*eth_rx_queue_set_lwm_t)(struct rte_eth_dev *dev,
> +				      uint16_t rx_queue_id,
> +				      uint8_t lwm);
> +

Please, add full description including parameters and return
values.

>   /** @internal Setup a transmit queue of an Ethernet device. */
>   typedef int (*eth_tx_queue_setup_t)(struct rte_eth_dev *dev,
>   				    uint16_t tx_queue_id,
> @@ -1283,6 +1287,9 @@ struct eth_dev_ops {
>   
>   	/** Dump private info from device */
>   	eth_dev_priv_dump_t eth_dev_priv_dump;
> +
> +	/** Set Rx queue limit watermark */
> +	eth_rx_queue_set_lwm_t rx_queue_set_lwm;
>   };
>   
>   /**
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 29a3d80..1e4fc6a 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -4414,6 +4414,34 @@ int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
>   							queue_idx, tx_rate));
>   }
>   
> +int rte_eth_rx_queue_set_lwm(uint16_t port_id, uint16_t queue_idx,
> +			     uint8_t lwm)
> +{
> +	struct rte_eth_dev *dev;
> +	struct rte_eth_dev_info dev_info;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (queue_idx > dev_info.max_rx_queues) {

It should be >=

> +		RTE_ETHDEV_LOG(ERR,
> +			"Set queue rate limit:port %u: invalid queue ID=%u\n",
> +			port_id, queue_idx);
> +		return -EINVAL;
> +	}
> +
> +	if (lwm > 99)
> +		return -EINVAL;
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_set_lwm, -ENOTSUP);
> +	return eth_err(port_id, (*dev->dev_ops->rx_queue_set_lwm)(dev,
> +							     queue_idx, lwm));
> +}
> +
>   RTE_INIT(eth_dev_init_fp_ops)
>   {
>   	uint32_t i;
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 04cff8e..f29e53b 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1249,8 +1249,12 @@ struct rte_eth_rxconf {
>   	 */
>   	union rte_eth_rxseg *rx_seg;
>   
> -	uint64_t reserved_64s[2]; /**< Reserved for future fields */
> +	uint64_t reserved_64s;
> +	uint32_t reserved_32s;
> +	uint32_t lwm:8;
> +	uint32_t reserved_bits:24;

I strong dislike bit fields for such purpose. It should
be uint8_t field.

Since we break ABI below anyway, we can break it here as well.

>   	void *reserved_ptrs[2];   /**< Reserved for future fields */
> +

No unrelated changes, please.

>   };
>   
>   /**
> @@ -3668,6 +3672,29 @@ int rte_eth_dev_set_vlan_ether_type(uint16_t port_id,
>    */
>   int rte_eth_dev_set_vlan_pvid(uint16_t port_id, uint16_t pvid, int on);
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Set Rx queue based limit watermark.
> + *
> + * @param port_id
> + *  The port identifier of the Ethernet device.
> + * @param queue_idx
> + *  The index of the receive queue
> + * @param lwm
> + *  The limit watermark percentage of Rx queue descriptor size.
> + *  The valid range is [0,99].
> + *  Setting 0 means disable limit watermark.
> + *
> + * @return
> + *   - (0) if successful.
> + *   - negative if failed.

Please, be precise with negative return values specification
and its meaning.

> + */
> +__rte_experimental
> +int rte_eth_rx_queue_set_lwm(uint16_t port_id, uint16_t queue_idx,
> +				uint8_t lwm);
> +
>   typedef void (*buffer_tx_error_fn)(struct rte_mbuf **unsent, uint16_t count,
>   		void *userdata);
>   
> @@ -3873,6 +3900,7 @@ enum rte_eth_event_type {
>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> +	RTE_ETH_EVENT_RXQ_LIMIT_REACHED,/**< RX queue limit reached */

RX -> Rx

as I understand it is an ABI breakage.

>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>   };
>   
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 20391ab..8b85ad8 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -279,6 +279,9 @@ EXPERIMENTAL {
>   	rte_flow_async_action_handle_create;
>   	rte_flow_async_action_handle_destroy;
>   	rte_flow_async_action_handle_update;
> +
> +	# added in 22.07
> +	rte_eth_rx_queue_set_lwm;
>   };
>   
>   INTERNAL {