On 12/18/24 14:44, Ilya Maximets wrote:
On 12/18/24 14:09, Maxime Coquelin wrote:
On 12/18/24 12:22, Kevin Traynor wrote:
On 17/12/2024 10:24, Maxime Coquelin wrote:
Hi Kevin,
On 12/6/24 21:26, Kevin Traynor wrote:
On 06/12/2024 15:58, Maxime Coquelin wrote:
This patch uses the new rte_vhost_driver_set_max_queue_num
API to set the maximum number of queue pairs supported by
the Vhost-user port.
This is required for VDUSE which needs to specify the
maximum number of queue pairs at creation time. Without it
128 queue pairs metadata would be allocated.
To configure it, a new 'vhost-max-queue-pairs' option is
introduced.
Signed-off-by: Maxime Coquelin <[email protected]>
Acked-by: Eelco Chaudron <[email protected]>
---
Note: depends on DPDK v24.11.
Changes in v4:
==============
- Change to vhost-queue-pairs-max to vhost-max-queue-pairs (Eelco)
- Remove mention to DPDK version in documentation (Eelco)
- Add missing parameter description in vswitch.xml (Eelco)
- Define min and max values for the new option (Maxime)
Changes in v3:
==============
- Introduce a new option to set the number of max queue pairs (Kevin)
- Add documentation for new option
Changes in v2:
==============
- Address checkpatch warnings.
---
Hi Maxime,
Testing with vhost-user backend with DPDK 24.11 worked well - in that it
called the API with the right number and didn't break anything :-)
A few comments on the code/docs below.
fyi - I initially tested on OVS main branch with 23.11 and I saw a loop
between the destroy_connection() callback triggering
netdev_dpdk_vhost_client_reconfigure(), repeat, etc when i started a VM
with 4 queues. So OVS having DPDK 24.11 support will need to be a
dependency I think (even aside from the experimental API issue).
Yes, I confirm 24.11 is also a functional dependency, as v23.11.2 miss
one fix that will be in upcoming v23.11.3:
As the 24.11 update patch is under review, we can just keep your
patchset for the main branch and make sure we apply in the correct order
(all going well - fingers crossed).
commit bc578c07f03ec3943fab88a5d156f28b98e1e652
Author: Maxime Coquelin <[email protected]>
Date: Thu Oct 3 10:11:10 2024 +0200
vhost: restrict set max queue pair API to VDUSE
[ upstream commit e1808999d36bb2e136a649f4651f36030aa468f1 ]
In order to avoid breaking Vhost-user live-migration, we want the
rte_vhost_driver_set_max_queue_num API to only be effective with
VDUSE.
Furthermore, this API is only really needed for VDUSE where the
device number of queues is defined by the backend. For Vhost-user,
this is defined by the frontend (e.g. QEMU), so the advantages of
restricting more the maximum number of queue pairs is limited to
a small memory gain (a handful of pointers).
Fixes: 4aa1f88ac13d ("vhost: add API to set max queue pairs")
Reported-by: Yu Jiang <[email protected]>
Signed-off-by: Maxime Coquelin <[email protected]>
Acked-by: David Marchand <[email protected]>
thanks,
Kevin.
Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++
lib/netdev-dpdk.c | 30 ++++++++++++++++++++++++
vswitchd/vswitch.xml | 10 ++++++++
3 files changed, 55 insertions(+)
diff --git a/Documentation/topics/dpdk/vhost-user.rst
b/Documentation/topics/dpdk/vhost-user.rst
index 7bba08ac2..656f7f69f 100644
--- a/Documentation/topics/dpdk/vhost-user.rst
+++ b/Documentation/topics/dpdk/vhost-user.rst
@@ -375,6 +375,21 @@ Tx retries max can be set for vhost-user-client ports::
Configurable vhost tx retries are not supported with vhost-user ports.
+vhost-user-client max queue pairs config
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+For vhost-user-client interfaces using the VDUSE backend, the maximum umber of
typo number
+queue pairs the Virtio device will support can be set at port creation time. If
+not set, the default value is 1 queue pair. This value is ignored for
+Vhost-user backends.
+
+Maximum number of queue pairs can be set for vhost-user-client-ports::
+
+ $ ovs-vsctl add-port br0 vduse0 \
+ -- set Interface vduse0 type=dpdkvhostuserclient \
+ options:vhost-server-path=/dev/vduse/vduse0 \
+ options:vhost-max-queue-pairs=4
+
.. _dpdk-testpmd:
DPDK in the Guest
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index dc52a2b56..a8b605113 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -19,6 +19,7 @@
#include <errno.h>
#include <signal.h>
+#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
@@ -153,6 +154,11 @@ typedef uint16_t dpdk_port_t;
/* Legacy default value for vhost tx retries. */
#define VHOST_ENQ_RETRY_DEF 8
+/* VDUSE-only, ignore for Vhost-user */
+#define VHOST_MAX_QUEUE_PAIRS_MIN 1
+#define VHOST_MAX_QUEUE_PAIRS_DEF VHOST_MAX_QUEUE_PAIRS_MIN
+#define VHOST_MAX_QUEUE_PAIRS_MAX 128
+
#define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
/* List of required flags advertised by the hardware that will be used
@@ -497,6 +503,9 @@ struct netdev_dpdk {
atomic_uint8_t vhost_tx_retries_max;
+ /* Ignored by DPDK for Vhost-user backends, only for VDUSE */
+ uint32_t vhost_max_queue_pairs;
+
I noticed this added a cacheline - perhaps we could use something
smaller and squash it in ?
/* --- cacheline 1 boundary (64 bytes) --- */
union {
OVS_CACHE_LINE_MARKER cacheline1; /* 64 1 */
struct {
struct ovs_mutex mutex; /* 64 48 */
struct dpdk_mp * dpdk_mp; /* 112 8 */
ovsrcu_index vid; /* 120 4 */
_Bool vhost_reconfigured; /* 124 1 */
atomic_uint8_t vhost_tx_retries_max; /* 125 1 */
/* XXX 2 bytes hole, try to pack */
/* --- cacheline 2 boundary (128 bytes) --- */
uint32_t vhost_max_queue_pairs; /* 128 4 */
uint8_t virtio_features_state; /* 132 1 */
}; /* 64 72 */
uint8_t pad55[128]; /* 64 128 */
}; /* 64 128 */
/* --- cacheline 3 boundary (192 bytes) --- */
Good catch, David already mentioned it to me off-list.
DPDK Vhost library only supports up to 128 queue pairs, so I think we
could use uint8_t type and so it would fit into the 1 byte hole.
Do you agree with this suggestion?
Yes, sounds good. It should be fine but you may want to confirm with
pahole as sometimes the comments can get stale.
Looks good with pahole:
/* --- cacheline 1 boundary (64 bytes) --- */
union {
OVS_CACHE_LINE_MARKER cacheline1; /* 64 1 */
struct {
struct ovs_mutex mutex; /* 64 48 */
struct dpdk_mp * dpdk_mp; /* 112 8 */
ovsrcu_index vid; /* 120 4 */
_Bool vhost_reconfigured; /* 124 1 */
atomic_uint8_t vhost_tx_retries_max; /* 125
1 */
uint8_t vhost_max_queue_pairs; /* 126 1 */
uint8_t virtio_features_state; /* 127 1 */
}; /* 64 64 */
uint8_t pad55[64]; /* 64 64 */
}; /* 64 64 */
/* --- cacheline 2 boundary (128 bytes) --- */
Why does it need to be in the first cache line? It's not on the hot path,
right?
Ok, what about moving it here (your reply can wait 2025 ;)):
union {
struct {
int requested_mtu; /* 704 4 */
int requested_n_txq; /* 708 4 */
int user_n_rxq; /* 712 4 */
int requested_n_rxq; /* 716 4 */
int requested_rxq_size; /* 720 4 */
int requested_txq_size; /* 724 4 */
int rxq_size; /* 728 4 */
int txq_size; /* 732 4 */
int requested_socket_id; /* 736 4 */
uint8_t vhost_max_queue_pairs; /* 740 1 */
/* XXX 3 bytes hole, try to pack */
uint64_t vhost_driver_flags; /* 744 8 */
struct rte_eth_fc_conf fc_conf; /* 752 20 */
/* XXX last struct has 2 bytes of padding */
/me goes back to PTO and Christmass preparations. :)
Enjoy your PTO,
Maxime
thanks,
Kevin.
/* Flags for virtio features recovery mechanism. */
uint8_t virtio_features_state;
@@ -1609,6 +1618,8 @@ vhost_common_construct(struct netdev *netdev)
atomic_init(&dev->vhost_tx_retries_max, VHOST_ENQ_RETRY_DEF);
+ dev->vhost_max_queue_pairs = VHOST_MAX_QUEUE_PAIRS_DEF;
+
return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
DPDK_DEV_VHOST, socket_id);
}
@@ -2491,6 +2502,7 @@ netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
const char *path;
int max_tx_retries, cur_max_tx_retries;
+ uint32_t max_queue_pairs;
ovs_mutex_lock(&dev->mutex);
if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
@@ -2498,6 +2510,15 @@ netdev_dpdk_vhost_client_set_config(struct netdev
*netdev,
if (!nullable_string_is_equal(path, dev->vhost_id)) {
free(dev->vhost_id);
dev->vhost_id = nullable_xstrdup(path);
+
+ max_queue_pairs = smap_get_int(args, "vhost-max-queue-pairs",
+ VHOST_MAX_QUEUE_PAIRS_DEF);
+ if (max_queue_pairs < VHOST_MAX_QUEUE_PAIRS_MIN
+ || max_queue_pairs > VHOST_MAX_QUEUE_PAIRS_MAX) {
+ max_queue_pairs = VHOST_MAX_QUEUE_PAIRS_DEF;
+ }
+ dev->vhost_max_queue_pairs = max_queue_pairs;
+
netdev_request_reconfigure(netdev);
}
}
@@ -2514,6 +2535,7 @@ netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
VLOG_INFO("Max Tx retries for vhost device '%s' set to %d",
netdev_get_name(netdev), max_tx_retries);
}
+
ovs_mutex_unlock(&dev->mutex);
return 0;
@@ -6400,6 +6422,14 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
*netdev)
goto unlock;
}
+ err = rte_vhost_driver_set_max_queue_num(dev->vhost_id,
+ dev->vhost_max_queue_pairs);
The log below is printed:
2024-12-06T18:28:51Z|00100|dpdk|INFO|VHOST_CONFIG: (/tmp/vhost0) Setting
max queue pairs to 1
It's kind of true, but it could be confusing when using vhost-user
backend. Maybe we should add an OVS info log before or after as a
reminder that the max queue pairs setting is only valid for VDUSE backend.
Yes, that's unfortunate that I added a log in
rte_vhost_driver_set_max_queue_num(), but only at DEBUG level:
+ if (!vsocket->is_vduse) {
+ VHOST_CONFIG_LOG(path, DEBUG,
+ "Keeping %u max queue pairs for
Vhost-user backend",
+ VHOST_MAX_QUEUE_PAIRS);
+ goto unlock_exit;
+ }
I can indeed a log before calling the API to avoid the confusion:
"Setting max queue pairs, only effective with VDUSE backends"
+ if (err) {
+ VLOG_ERR("rte_vhost_driver_set_max_queue_num failed for "
+ "vhost-user client port: %s\n", dev->up.name);
+ goto unlock;
+ }
+
err = rte_vhost_driver_start(dev->vhost_id);
if (err) {
VLOG_ERR("rte_vhost_driver_start failed for vhost user "
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 36cb4e495..0b5c5dcd6 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -3520,6 +3520,16 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch
options:peer=p1 \
</p>
</column>
+ <column name="options" key="vhost-max-queue-pairs"
+ type='{"type": "integer", "minInteger" : 1, "maxInteger": 128}'>
I already mention the max value is 128, so uint8_t will be enough for
vhost_max_queue_pairs field.
+ <p>
+ The value specifies the maximum number of queue pairs supported by
+ a vHost device. This is ignored for Vhost-user backends, only VDUSE
+ is supported.
+ Only supported by dpdkvhsotuserclient interfaces.
typo dpdkvhostuserclient
good catch, will fix.
+ </p>
It would be good to state the default here (like tx-retries-max below)
Indeed, will add in nex revision.
+ </column>
+
<column name="options" key="tx-retries-max"
type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'>
<p>
Thanks for the review,
Maxime
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
