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