On 24/05/2023 19:15, Ilya Maximets wrote:
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.
yep, I think I removed an assignment at some stage and forgot to remove
the blank line.
+ } 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);
?
That reads better, thanks.
}
}
@@ -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.
That's a good point. IIRC, I moved it because I added the call to
info_get(), so I wanted to give the device a chance to be attached
first. I guess I can move it to after the devargs stuff but before the
other code to avoid this issue.
Thanks for the comments, I'll respin a v5.
Kevin.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
