On 5/16/23 13:59, Kevin Traynor wrote:
> By default OVS configures 2048 descriptors for tx and rx queues
> on DPDK devices. It also allows the user to configure those values.
>
> If the values used are not acceptable to the device then queue setup
> would fail.
>
> The device exposes it's max/min/alignment requirements and OVS
> applies some limits also. Use these to ensure an acceptable value
> is used for the number of descriptors on a device tx/rx.
>
> If the default or user value is not acceptable, adjust to a suitable
> value and log.
>
> Reported-at: https://bugzilla.redhat.com/2119876
> Signed-off-by: Kevin Traynor <ktray...@redhat.com>
> ---
> lib/netdev-dpdk.c | 61 ++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 50 insertions(+), 11 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 2d9afc493..e5fc1aa30 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1911,8 +1911,30 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const
> struct smap *args)
> static void
> dpdk_process_queue_size(struct netdev *netdev, const struct smap *args,
> - const char *flag, int default_size, int *new_size)
> + struct rte_eth_dev_info *info, bool is_rx)
> {
> - int queue_size = smap_get_int(args, flag, default_size);
> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> + struct rte_eth_desc_lim *lim;
> + int default_size, queue_size, cur_size, new_requested_size;
> + int *cur_requested_size;
> + bool reconfig = false;
>
> + if (is_rx) {
> + default_size = NIC_PORT_DEFAULT_RXQ_SIZE;
> + new_requested_size = smap_get_int(args, "n_rxq_desc", default_size);
> + cur_size = dev->rxq_size;
> + cur_requested_size = &dev->requested_rxq_size;
> + lim = info ? &info->rx_desc_lim : NULL;
> +
The empty line here seems unnecessary.
> + } else {
> + default_size = NIC_PORT_DEFAULT_TXQ_SIZE;
> + new_requested_size = smap_get_int(args, "n_txq_desc", default_size);
> + cur_size = dev->txq_size;
> + cur_requested_size = &dev->requested_txq_size;
> + lim = info ? &info->tx_desc_lim : NULL;
> + }
> +
> + queue_size = new_requested_size;
> +
> + /* Check for OVS limits. */
> if (queue_size <= 0 || queue_size > NIC_PORT_MAX_Q_SIZE
> || !is_pow2(queue_size)) {
> @@ -1920,7 +1942,24 @@ dpdk_process_queue_size(struct netdev *netdev, const
> struct smap *args,
> }
>
> - if (queue_size != *new_size) {
> - *new_size = queue_size;
> + if (lim) {
> + /* Check for device limits. */
> + if (lim->nb_align) {
> + queue_size = ROUND_UP(queue_size, lim->nb_align);
> + }
> + queue_size = MIN(queue_size, lim->nb_max);
> + queue_size = MAX(queue_size, lim->nb_min);
> + }
> +
> + *cur_requested_size = queue_size;
> +
> + if (cur_size != queue_size) {
> netdev_request_reconfigure(netdev);
> + reconfig = true;
> + }
> + if (new_requested_size != queue_size) {
> + VLOG(reconfig ? VLL_INFO : VLL_DBG,
> + "Interface %s cannot set %s descriptor size to %d. "
> + "Adjusted to %d.", dev->up.name, is_rx ? "rx": "tx" ,
> + new_requested_size, queue_size);
This message is a bit misleading. It says that it can't set the
descriptor size. But it's not the descriptor size, it's a number
of descriptors. Or a descriptor ring size, but that's too much
of the unnecessary terminology.
And we should use netdev_get_name instead of accessing the name
directly.
How about this:
VLOG(reconfig ? VLL_INFO : VLL_DBG,
"%s: Unable to set the number of %s descriptors to %d. "
"Adjusted to %d.", netdev_get_name(netdev),
is_rx ? "rx": "tx", new_requested_size, queue_size);
?
> }
> }
> @@ -1938,7 +1977,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args,
> {RTE_ETH_FC_RX_PAUSE, RTE_ETH_FC_FULL }
> };
> + struct rte_eth_dev_info info;
> const char *new_devargs;
> const char *vf_mac;
> int err = 0;
> + int ret;
>
> ovs_mutex_lock(&dpdk_mutex);
> @@ -1947,11 +1988,4 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args,
> dpdk_set_rxq_config(dev, args);
>
> - dpdk_process_queue_size(netdev, args, "n_rxq_desc",
> - NIC_PORT_DEFAULT_RXQ_SIZE,
> - &dev->requested_rxq_size);
> - dpdk_process_queue_size(netdev, args, "n_txq_desc",
> - NIC_PORT_DEFAULT_TXQ_SIZE,
> - &dev->requested_txq_size);
> -
> new_devargs = smap_get(args, "dpdk-devargs");
>
> @@ -2073,4 +2107,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args,
> }
>
> + ret = rte_eth_dev_info_get(dev->port_id, &info);
> +
> + dpdk_process_queue_size(netdev, args, !ret ? &info : NULL, true);
> + dpdk_process_queue_size(netdev, args, !ret ? &info : NULL, false);
Any reason to move this after the flow control?
If flow control is not supported we jump over this part of the code and not
configuring the queue size. You can test that by adding a net_null device.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
