| Message ID | 20241206145800.2626951-1-maxime.coquelin@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kevin Traynor |
| Headers | show |
| Series | [ovs-dev,v4] netdev-dpdk: Set Vhost port maximum number of queue pairs. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
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 <maxime.coquelin@redhat.com> > Acked-by: Eelco Chaudron <echaudro@redhat.com> > --- > > 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). 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) --- */ > /* 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. > + 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}'> > + <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 > + </p> It would be good to state the default here (like tx-retries-max below) > + </column> > + > <column name="options" key="tx-retries-max" > type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> > <p>
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 <maxime.coquelin@redhat.com> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> >> 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: commit bc578c07f03ec3943fab88a5d156f28b98e1e652 Author: Maxime Coquelin <maxime.coquelin@redhat.com> 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 <yux.jiang@intel.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Acked-by: David Marchand <david.marchand@redhat.com> > 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? > >> /* 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
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 <maxime.coquelin@redhat.com> >>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>> --- >>> >>> 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 <maxime.coquelin@redhat.com> > 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 <yux.jiang@intel.com> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > Acked-by: David Marchand <david.marchand@redhat.com> > > >> 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 >
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 <maxime.coquelin@redhat.com> >>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>> --- >>>> >>>> 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 <maxime.coquelin@redhat.com> >> 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 <yux.jiang@intel.com> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> Acked-by: David Marchand <david.marchand@redhat.com> >> >> >>> 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) --- */ > > 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 >> >
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 <maxime.coquelin@redhat.com> >>>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>>> --- >>>>> >>>>> 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 <maxime.coquelin@redhat.com> >>> 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 <yux.jiang@intel.com> >>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>> Acked-by: David Marchand <david.marchand@redhat.com> >>> >>> >>>> 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? /me goes back to PTO and Christmass preparations. :) > >> >> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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 <maxime.coquelin@redhat.com> >>>>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>>>> --- >>>>>> >>>>>> 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 <maxime.coquelin@redhat.com> >>>> 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 <yux.jiang@intel.com> >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>> Acked-by: David Marchand <david.marchand@redhat.com> >>>> >>>> >>>>> 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 >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
On 12/18/24 16:04, Maxime Coquelin wrote: > > > 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 <maxime.coquelin@redhat.com> >>>>>>> Acked-by: Eelco Chaudron <echaudro@redhat.com> >>>>>>> --- >>>>>>> >>>>>>> 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 <maxime.coquelin@redhat.com> >>>>> 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 <yux.jiang@intel.com> >>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >>>>> Acked-by: David Marchand <david.marchand@redhat.com> >>>>> >>>>> >>>>>> 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 */ > This location looks fine to me. Best regards, Ilya Maximets. >> /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 >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >> >
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 +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; + /* 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); + 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}'> + <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. + </p> + </column> + <column name="options" key="tx-retries-max" type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> <p>