| Message ID | 20250108113339.2367827-1-maxime.coquelin@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Kevin Traynor |
| Headers | show |
| Series | [ovs-dev,v5] netdev-dpdk: Set Vhost port maximum number of queue pairs. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 1/8/25 12:33, 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 v5: > ============== > - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) > - Rebased on top of latest main > 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. > --- > Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ > lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ > vswitchd/vswitch.xml | 13 ++++++++++ > 3 files changed, 59 insertions(+) Not a technical review, but a few small nitpicks below. Can probably be addressed on commit. Best regards, Ilya Maximets. > > diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst > index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ Period at the end of the comment. > +#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 > @@ -554,6 +560,9 @@ struct netdev_dpdk { > /* Socket ID detected when vHost device is brought up */ > int requested_socket_id; > > + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ Period. I see some other comments in this structure are not style compliant as well, but we shouldn't blindly follow this practice. :) Also, do we need to use capital V in vhost-user? Seems inconsistent with 'vHost device' spelling we use in other places. We seem to mostly use lowercase in logs and docs when mentioning 'vhost-user'. WDYT? > + uint8_t vhost_max_queue_pairs; > + > /* Denotes whether vHost port is client/server mode */ > uint64_t vhost_driver_flags; > > @@ -1606,6 +1615,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); > } > @@ -2488,6 +2499,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)) { > @@ -2495,6 +2507,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); > } > } > @@ -2511,6 +2532,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; > @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) > goto unlock; > } > > + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); 0-day bot complains about the line length, but is this log message even needed? Doesn't seem to be very useful. > + 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 275bcbec0..2cb503c37 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -3540,6 +3540,19 @@ 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 Same for the capital V. > + is supported. > + Only supported by dpdkvhostuserclient interfaces. > + </p> > + <p> > + Default value is 1. > + </p> > + </column> > + > <column name="options" key="tx-retries-max" > type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> > <p>
On 1/8/25 3:38 PM, Ilya Maximets wrote: > On 1/8/25 12:33, 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 v5: >> ============== >> - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) >> - Rebased on top of latest main >> 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. >> --- >> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ >> lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ >> vswitchd/vswitch.xml | 13 ++++++++++ >> 3 files changed, 59 insertions(+) > > Not a technical review, but a few small nitpicks below. > Can probably be addressed on commit. > > Best regards, Ilya Maximets. > >> >> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >> index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ > > Period at the end of the comment. Ack. > >> +#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 >> @@ -554,6 +560,9 @@ struct netdev_dpdk { >> /* Socket ID detected when vHost device is brought up */ >> int requested_socket_id; >> >> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ > > Period. I see some other comments in this structure are not style compliant > as well, but we shouldn't blindly follow this practice. :) > > Also, do we need to use capital V in vhost-user? Seems inconsistent with > 'vHost device' spelling we use in other places. We seem to mostly use lowercase > in logs and docs when mentioning 'vhost-user'. WDYT? I don't have a strong opinion. In DPDK, we generally use 'Vhost-user', but in OVS we indeed see 'vHost' 'or vhost-user'. I am fine with using vhost-user if this is the most common. > >> + uint8_t vhost_max_queue_pairs; >> + >> /* Denotes whether vHost port is client/server mode */ >> uint64_t vhost_driver_flags; >> >> @@ -1606,6 +1615,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); >> } >> @@ -2488,6 +2499,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)) { >> @@ -2495,6 +2507,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); >> } >> } >> @@ -2511,6 +2532,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; >> @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) >> goto unlock; >> } >> >> + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); > > 0-day bot complains about the line length, but is this log message even needed? > Doesn't seem to be very useful. I added it to try avoiding confusion. Indeed, it is unfortunate but rte_vhost_driver_set_max_queue_num() emits below INFO-level log before checking whether the backend is VDUSE or vhost-user: VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", max_queue_pairs); So without this log, I think it would be more confusing to the user when adding vhost-user ports. I can fix the log in the DPDK Vhost library, and even mark it as candidate to stable branch, but it will not be available at the time of the upcoming OVS release. > >> + 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 275bcbec0..2cb503c37 100644 >> --- a/vswitchd/vswitch.xml >> +++ b/vswitchd/vswitch.xml >> @@ -3540,6 +3540,19 @@ 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 > > Same for the capital V. OK, I am fine with 'vhost-user'. > >> + is supported. >> + Only supported by dpdkvhostuserclient interfaces. >> + </p> >> + <p> >> + Default value is 1. >> + </p> >> + </column> >> + >> <column name="options" key="tx-retries-max" >> type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> >> <p> > Thanks for the review, Maxime
On 08/01/2025 14:50, Maxime Coquelin wrote: > > > On 1/8/25 3:38 PM, Ilya Maximets wrote: >> On 1/8/25 12:33, 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 v5: >>> ============== >>> - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) >>> - Rebased on top of latest main >>> 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. >>> --- >>> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ >>> lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ >>> vswitchd/vswitch.xml | 13 ++++++++++ >>> 3 files changed, 59 insertions(+) >> >> Not a technical review, but a few small nitpicks below. >> Can probably be addressed on commit. >> >> Best regards, Ilya Maximets. >> >>> >>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>> index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ >> >> Period at the end of the comment. > > Ack. > >> >>> +#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 >>> @@ -554,6 +560,9 @@ struct netdev_dpdk { >>> /* Socket ID detected when vHost device is brought up */ >>> int requested_socket_id; >>> >>> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ >> >> Period. I see some other comments in this structure are not style compliant >> as well, but we shouldn't blindly follow this practice. :) >> >> Also, do we need to use capital V in vhost-user? Seems inconsistent with >> 'vHost device' spelling we use in other places. We seem to mostly use lowercase >> in logs and docs when mentioning 'vhost-user'. WDYT? > > I don't have a strong opinion. In DPDK, we generally use 'Vhost-user', > but in OVS we indeed see 'vHost' 'or vhost-user'. > > I am fine with using vhost-user if this is the most common. > >> >>> + uint8_t vhost_max_queue_pairs; >>> + >>> /* Denotes whether vHost port is client/server mode */ >>> uint64_t vhost_driver_flags; >>> >>> @@ -1606,6 +1615,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); >>> } >>> @@ -2488,6 +2499,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)) { >>> @@ -2495,6 +2507,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); >>> } >>> } >>> @@ -2511,6 +2532,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; >>> @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) >>> goto unlock; >>> } >>> >>> + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); >> >> 0-day bot complains about the line length, but is this log message even needed? >> Doesn't seem to be very useful. > > I added it to try avoiding confusion. Indeed, it is unfortunate but > rte_vhost_driver_set_max_queue_num() emits below INFO-level log before > checking whether the backend is VDUSE or vhost-user: > > VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", > max_queue_pairs); > > So without this log, I think it would be more confusing to the user when > adding vhost-user ports. > > I can fix the log in the DPDK Vhost library, and even mark it as > candidate to stable branch, but it will not be available at the time of > the upcoming OVS release. > I think that's ok. When we integrate new LTS 24.11.X release with updated log level we can remove the log from here. At the moment i think it's necessary otherwise it would be misleading for vhost-user users as I saw during testing of v4. >> >>> + 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 275bcbec0..2cb503c37 100644 >>> --- a/vswitchd/vswitch.xml >>> +++ b/vswitchd/vswitch.xml >>> @@ -3540,6 +3540,19 @@ 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 >> >> Same for the capital V. > > OK, I am fine with 'vhost-user'. > >> >>> + is supported. >>> + Only supported by dpdkvhostuserclient interfaces. >>> + </p> >>> + <p> >>> + Default value is 1. >>> + </p> >>> + </column> >>> + >>> <column name="options" key="tx-retries-max" >>> type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> >>> <p> >> > > Thanks for the review, > Maxime >
On 1/8/25 3:56 PM, Kevin Traynor wrote: > On 08/01/2025 14:50, Maxime Coquelin wrote: >> >> >> On 1/8/25 3:38 PM, Ilya Maximets wrote: >>> On 1/8/25 12:33, 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 v5: >>>> ============== >>>> - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) >>>> - Rebased on top of latest main >>>> 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. >>>> --- >>>> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ >>>> lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ >>>> vswitchd/vswitch.xml | 13 ++++++++++ >>>> 3 files changed, 59 insertions(+) >>> >>> Not a technical review, but a few small nitpicks below. >>> Can probably be addressed on commit. >>> >>> Best regards, Ilya Maximets. >>> >>>> >>>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>>> index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ >>> >>> Period at the end of the comment. >> >> Ack. >> >>> >>>> +#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 >>>> @@ -554,6 +560,9 @@ struct netdev_dpdk { >>>> /* Socket ID detected when vHost device is brought up */ >>>> int requested_socket_id; >>>> >>>> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ >>> >>> Period. I see some other comments in this structure are not style compliant >>> as well, but we shouldn't blindly follow this practice. :) >>> >>> Also, do we need to use capital V in vhost-user? Seems inconsistent with >>> 'vHost device' spelling we use in other places. We seem to mostly use lowercase >>> in logs and docs when mentioning 'vhost-user'. WDYT? >> >> I don't have a strong opinion. In DPDK, we generally use 'Vhost-user', >> but in OVS we indeed see 'vHost' 'or vhost-user'. >> >> I am fine with using vhost-user if this is the most common. >> >>> >>>> + uint8_t vhost_max_queue_pairs; >>>> + >>>> /* Denotes whether vHost port is client/server mode */ >>>> uint64_t vhost_driver_flags; >>>> >>>> @@ -1606,6 +1615,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); >>>> } >>>> @@ -2488,6 +2499,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)) { >>>> @@ -2495,6 +2507,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); >>>> } >>>> } >>>> @@ -2511,6 +2532,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; >>>> @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) >>>> goto unlock; >>>> } >>>> >>>> + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); >>> >>> 0-day bot complains about the line length, but is this log message even needed? >>> Doesn't seem to be very useful. >> >> I added it to try avoiding confusion. Indeed, it is unfortunate but >> rte_vhost_driver_set_max_queue_num() emits below INFO-level log before >> checking whether the backend is VDUSE or vhost-user: >> >> VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", >> max_queue_pairs); >> >> So without this log, I think it would be more confusing to the user when >> adding vhost-user ports. >> >> I can fix the log in the DPDK Vhost library, and even mark it as >> candidate to stable branch, but it will not be available at the time of >> the upcoming OVS release. >> > > I think that's ok. When we integrate new LTS 24.11.X release with > updated log level we can remove the log from here. At the moment i think > it's necessary otherwise it would be misleading for vhost-user users as > I saw during testing of v4. Thanks for the feedback Kevin, I agree with your proposal. Regarding the length exceeding 79 chars, we can change it to: "Setting max queue pairs, only effective with VDUSE" > >>> >>>> + 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 275bcbec0..2cb503c37 100644 >>>> --- a/vswitchd/vswitch.xml >>>> +++ b/vswitchd/vswitch.xml >>>> @@ -3540,6 +3540,19 @@ 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 >>> >>> Same for the capital V. >> >> OK, I am fine with 'vhost-user'. >> >>> >>>> + is supported. >>>> + Only supported by dpdkvhostuserclient interfaces. >>>> + </p> >>>> + <p> >>>> + Default value is 1. >>>> + </p> >>>> + </column> >>>> + >>>> <column name="options" key="tx-retries-max" >>>> type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> >>>> <p> >>> >> >> Thanks for the review, >> Maxime >> >
On 1/8/25 16:00, Maxime Coquelin wrote: > > > On 1/8/25 3:56 PM, Kevin Traynor wrote: >> On 08/01/2025 14:50, Maxime Coquelin wrote: >>> >>> >>> On 1/8/25 3:38 PM, Ilya Maximets wrote: >>>> On 1/8/25 12:33, 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 v5: >>>>> ============== >>>>> - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) >>>>> - Rebased on top of latest main >>>>> 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. >>>>> --- >>>>> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ >>>>> lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ >>>>> vswitchd/vswitch.xml | 13 ++++++++++ >>>>> 3 files changed, 59 insertions(+) >>>> >>>> Not a technical review, but a few small nitpicks below. >>>> Can probably be addressed on commit. >>>> >>>> Best regards, Ilya Maximets. >>>> >>>>> >>>>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>>>> index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ >>>> >>>> Period at the end of the comment. >>> >>> Ack. >>> >>>> >>>>> +#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 >>>>> @@ -554,6 +560,9 @@ struct netdev_dpdk { >>>>> /* Socket ID detected when vHost device is brought up */ >>>>> int requested_socket_id; >>>>> >>>>> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ >>>> >>>> Period. I see some other comments in this structure are not style compliant >>>> as well, but we shouldn't blindly follow this practice. :) >>>> >>>> Also, do we need to use capital V in vhost-user? Seems inconsistent with >>>> 'vHost device' spelling we use in other places. We seem to mostly use lowercase >>>> in logs and docs when mentioning 'vhost-user'. WDYT? >>> >>> I don't have a strong opinion. In DPDK, we generally use 'Vhost-user', >>> but in OVS we indeed see 'vHost' 'or vhost-user'. >>> >>> I am fine with using vhost-user if this is the most common. >>> >>>> >>>>> + uint8_t vhost_max_queue_pairs; >>>>> + >>>>> /* Denotes whether vHost port is client/server mode */ >>>>> uint64_t vhost_driver_flags; >>>>> >>>>> @@ -1606,6 +1615,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); >>>>> } >>>>> @@ -2488,6 +2499,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)) { >>>>> @@ -2495,6 +2507,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); >>>>> } >>>>> } >>>>> @@ -2511,6 +2532,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; >>>>> @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) >>>>> goto unlock; >>>>> } >>>>> >>>>> + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); >>>> >>>> 0-day bot complains about the line length, but is this log message even needed? >>>> Doesn't seem to be very useful. >>> >>> I added it to try avoiding confusion. Indeed, it is unfortunate but >>> rte_vhost_driver_set_max_queue_num() emits below INFO-level log before >>> checking whether the backend is VDUSE or vhost-user: >>> >>> VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", >>> max_queue_pairs); >>> >>> So without this log, I think it would be more confusing to the user when >>> adding vhost-user ports. >>> >>> I can fix the log in the DPDK Vhost library, and even mark it as >>> candidate to stable branch, but it will not be available at the time of >>> the upcoming OVS release. >>> >> >> I think that's ok. When we integrate new LTS 24.11.X release with >> updated log level we can remove the log from here. At the moment i think >> it's necessary otherwise it would be misleading for vhost-user users as >> I saw during testing of v4. > > Thanks for the feedback Kevin, I agree with your proposal. > > Regarding the length exceeding 79 chars, we can change it to: > "Setting max queue pairs, only effective with VDUSE" Looks fine. I didn't realize there is a confusing DPDK message. Though, an alternative solution might be to compare vhost_id with /dev/vduse and not call the API at all for non-vduse backends. It seems that is what vhost library is doing anyway. WDYT? > >> >>>> >>>>> + 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 275bcbec0..2cb503c37 100644 >>>>> --- a/vswitchd/vswitch.xml >>>>> +++ b/vswitchd/vswitch.xml >>>>> @@ -3540,6 +3540,19 @@ 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 >>>> >>>> Same for the capital V. >>> >>> OK, I am fine with 'vhost-user'. >>> >>>> >>>>> + is supported. >>>>> + Only supported by dpdkvhostuserclient interfaces. >>>>> + </p> >>>>> + <p> >>>>> + Default value is 1. >>>>> + </p> >>>>> + </column> >>>>> + >>>>> <column name="options" key="tx-retries-max" >>>>> type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> >>>>> <p> >>>> >>> >>> Thanks for the review, >>> Maxime >>> >> >
On 1/8/25 4:05 PM, Ilya Maximets wrote: > On 1/8/25 16:00, Maxime Coquelin wrote: >> >> >> On 1/8/25 3:56 PM, Kevin Traynor wrote: >>> On 08/01/2025 14:50, Maxime Coquelin wrote: >>>> >>>> >>>> On 1/8/25 3:38 PM, Ilya Maximets wrote: >>>>> On 1/8/25 12:33, 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 v5: >>>>>> ============== >>>>>> - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) >>>>>> - Rebased on top of latest main >>>>>> 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. >>>>>> --- >>>>>> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ >>>>>> lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ >>>>>> vswitchd/vswitch.xml | 13 ++++++++++ >>>>>> 3 files changed, 59 insertions(+) >>>>> >>>>> Not a technical review, but a few small nitpicks below. >>>>> Can probably be addressed on commit. >>>>> >>>>> Best regards, Ilya Maximets. >>>>> >>>>>> >>>>>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>>>>> index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ >>>>> >>>>> Period at the end of the comment. >>>> >>>> Ack. >>>> >>>>> >>>>>> +#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 >>>>>> @@ -554,6 +560,9 @@ struct netdev_dpdk { >>>>>> /* Socket ID detected when vHost device is brought up */ >>>>>> int requested_socket_id; >>>>>> >>>>>> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ >>>>> >>>>> Period. I see some other comments in this structure are not style compliant >>>>> as well, but we shouldn't blindly follow this practice. :) >>>>> >>>>> Also, do we need to use capital V in vhost-user? Seems inconsistent with >>>>> 'vHost device' spelling we use in other places. We seem to mostly use lowercase >>>>> in logs and docs when mentioning 'vhost-user'. WDYT? >>>> >>>> I don't have a strong opinion. In DPDK, we generally use 'Vhost-user', >>>> but in OVS we indeed see 'vHost' 'or vhost-user'. >>>> >>>> I am fine with using vhost-user if this is the most common. >>>> >>>>> >>>>>> + uint8_t vhost_max_queue_pairs; >>>>>> + >>>>>> /* Denotes whether vHost port is client/server mode */ >>>>>> uint64_t vhost_driver_flags; >>>>>> >>>>>> @@ -1606,6 +1615,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); >>>>>> } >>>>>> @@ -2488,6 +2499,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)) { >>>>>> @@ -2495,6 +2507,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); >>>>>> } >>>>>> } >>>>>> @@ -2511,6 +2532,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; >>>>>> @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) >>>>>> goto unlock; >>>>>> } >>>>>> >>>>>> + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); >>>>> >>>>> 0-day bot complains about the line length, but is this log message even needed? >>>>> Doesn't seem to be very useful. >>>> >>>> I added it to try avoiding confusion. Indeed, it is unfortunate but >>>> rte_vhost_driver_set_max_queue_num() emits below INFO-level log before >>>> checking whether the backend is VDUSE or vhost-user: >>>> >>>> VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", >>>> max_queue_pairs); >>>> >>>> So without this log, I think it would be more confusing to the user when >>>> adding vhost-user ports. >>>> >>>> I can fix the log in the DPDK Vhost library, and even mark it as >>>> candidate to stable branch, but it will not be available at the time of >>>> the upcoming OVS release. >>>> >>> >>> I think that's ok. When we integrate new LTS 24.11.X release with >>> updated log level we can remove the log from here. At the moment i think >>> it's necessary otherwise it would be misleading for vhost-user users as >>> I saw during testing of v4. >> >> Thanks for the feedback Kevin, I agree with your proposal. >> >> Regarding the length exceeding 79 chars, we can change it to: >> "Setting max queue pairs, only effective with VDUSE" > > Looks fine. I didn't realize there is a confusing DPDK message. > Though, an alternative solution might be to compare vhost_id with > /dev/vduse and not call the API at all for non-vduse backends. > It seems that is what vhost library is doing anyway. WDYT? We initially tried to not have to differenciate between vhost-user and VDUSE in OVS, but as it turns out this is not possible because above issue. So your proposal would also work, let me know what is your preference, I can prepare a patch only calling the rte_vhost_driver_set_max_queue_num API for VDUSE backends if you think this is the best way. Thanks, Maxime > >> >>> >>>>> >>>>>> + 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 275bcbec0..2cb503c37 100644 >>>>>> --- a/vswitchd/vswitch.xml >>>>>> +++ b/vswitchd/vswitch.xml >>>>>> @@ -3540,6 +3540,19 @@ 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 >>>>> >>>>> Same for the capital V. >>>> >>>> OK, I am fine with 'vhost-user'. >>>> >>>>> >>>>>> + is supported. >>>>>> + Only supported by dpdkvhostuserclient interfaces. >>>>>> + </p> >>>>>> + <p> >>>>>> + Default value is 1. >>>>>> + </p> >>>>>> + </column> >>>>>> + >>>>>> <column name="options" key="tx-retries-max" >>>>>> type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> >>>>>> <p> >>>>> >>>> >>>> Thanks for the review, >>>> Maxime >>>> >>> >> >
On 1/9/25 10:21, Maxime Coquelin wrote: > > > On 1/8/25 4:05 PM, Ilya Maximets wrote: >> On 1/8/25 16:00, Maxime Coquelin wrote: >>> >>> >>> On 1/8/25 3:56 PM, Kevin Traynor wrote: >>>> On 08/01/2025 14:50, Maxime Coquelin wrote: >>>>> >>>>> >>>>> On 1/8/25 3:38 PM, Ilya Maximets wrote: >>>>>> On 1/8/25 12:33, 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 v5: >>>>>>> ============== >>>>>>> - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) >>>>>>> - Rebased on top of latest main >>>>>>> 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. >>>>>>> --- >>>>>>> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ >>>>>>> lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ >>>>>>> vswitchd/vswitch.xml | 13 ++++++++++ >>>>>>> 3 files changed, 59 insertions(+) >>>>>> >>>>>> Not a technical review, but a few small nitpicks below. >>>>>> Can probably be addressed on commit. >>>>>> >>>>>> Best regards, Ilya Maximets. >>>>>> >>>>>>> >>>>>>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>>>>>> index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ >>>>>> >>>>>> Period at the end of the comment. >>>>> >>>>> Ack. >>>>> >>>>>> >>>>>>> +#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 >>>>>>> @@ -554,6 +560,9 @@ struct netdev_dpdk { >>>>>>> /* Socket ID detected when vHost device is brought up */ >>>>>>> int requested_socket_id; >>>>>>> >>>>>>> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ >>>>>> >>>>>> Period. I see some other comments in this structure are not style compliant >>>>>> as well, but we shouldn't blindly follow this practice. :) >>>>>> >>>>>> Also, do we need to use capital V in vhost-user? Seems inconsistent with >>>>>> 'vHost device' spelling we use in other places. We seem to mostly use lowercase >>>>>> in logs and docs when mentioning 'vhost-user'. WDYT? >>>>> >>>>> I don't have a strong opinion. In DPDK, we generally use 'Vhost-user', >>>>> but in OVS we indeed see 'vHost' 'or vhost-user'. >>>>> >>>>> I am fine with using vhost-user if this is the most common. >>>>> >>>>>> >>>>>>> + uint8_t vhost_max_queue_pairs; >>>>>>> + >>>>>>> /* Denotes whether vHost port is client/server mode */ >>>>>>> uint64_t vhost_driver_flags; >>>>>>> >>>>>>> @@ -1606,6 +1615,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); >>>>>>> } >>>>>>> @@ -2488,6 +2499,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)) { >>>>>>> @@ -2495,6 +2507,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); >>>>>>> } >>>>>>> } >>>>>>> @@ -2511,6 +2532,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; >>>>>>> @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) >>>>>>> goto unlock; >>>>>>> } >>>>>>> >>>>>>> + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); >>>>>> >>>>>> 0-day bot complains about the line length, but is this log message even needed? >>>>>> Doesn't seem to be very useful. >>>>> >>>>> I added it to try avoiding confusion. Indeed, it is unfortunate but >>>>> rte_vhost_driver_set_max_queue_num() emits below INFO-level log before >>>>> checking whether the backend is VDUSE or vhost-user: >>>>> >>>>> VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", >>>>> max_queue_pairs); >>>>> >>>>> So without this log, I think it would be more confusing to the user when >>>>> adding vhost-user ports. >>>>> >>>>> I can fix the log in the DPDK Vhost library, and even mark it as >>>>> candidate to stable branch, but it will not be available at the time of >>>>> the upcoming OVS release. >>>>> >>>> >>>> I think that's ok. When we integrate new LTS 24.11.X release with >>>> updated log level we can remove the log from here. At the moment i think >>>> it's necessary otherwise it would be misleading for vhost-user users as >>>> I saw during testing of v4. >>> >>> Thanks for the feedback Kevin, I agree with your proposal. >>> >>> Regarding the length exceeding 79 chars, we can change it to: >>> "Setting max queue pairs, only effective with VDUSE" >> >> Looks fine. I didn't realize there is a confusing DPDK message. >> Though, an alternative solution might be to compare vhost_id with >> /dev/vduse and not call the API at all for non-vduse backends. >> It seems that is what vhost library is doing anyway. WDYT? > > We initially tried to not have to differenciate between vhost-user and > VDUSE in OVS, but as it turns out this is not possible because above > issue. > > So your proposal would also work, let me know what is your > preference, I can prepare a patch only calling the > rte_vhost_driver_set_max_queue_num API for VDUSE backends if you think > this is the best way. I'd consider this as a stop gap solution until DPDK is fixed. If I'm choosing between: 1. Fighting the confusing logs with more logs, and 2. Checking for the device type and not bothering the user with some strangely looking logs. I'm for the latter. We can remove the check once DPDK stops printing confusing logs. Kevin, what do you think? > > Thanks, > Maxime > >> >>> >>>> >>>>>> >>>>>>> + 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 275bcbec0..2cb503c37 100644 >>>>>>> --- a/vswitchd/vswitch.xml >>>>>>> +++ b/vswitchd/vswitch.xml >>>>>>> @@ -3540,6 +3540,19 @@ 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 >>>>>> >>>>>> Same for the capital V. >>>>> >>>>> OK, I am fine with 'vhost-user'. >>>>> >>>>>> >>>>>>> + is supported. >>>>>>> + Only supported by dpdkvhostuserclient interfaces. >>>>>>> + </p> >>>>>>> + <p> >>>>>>> + Default value is 1. >>>>>>> + </p> >>>>>>> + </column> >>>>>>> + >>>>>>> <column name="options" key="tx-retries-max" >>>>>>> type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> >>>>>>> <p> >>>>>> >>>>> >>>>> Thanks for the review, >>>>> Maxime >>>>> >>>> >>> >> >
On 09/01/2025 11:16, Ilya Maximets wrote: > On 1/9/25 10:21, Maxime Coquelin wrote: >> >> >> On 1/8/25 4:05 PM, Ilya Maximets wrote: >>> On 1/8/25 16:00, Maxime Coquelin wrote: >>>> >>>> >>>> On 1/8/25 3:56 PM, Kevin Traynor wrote: >>>>> On 08/01/2025 14:50, Maxime Coquelin wrote: >>>>>> >>>>>> >>>>>> On 1/8/25 3:38 PM, Ilya Maximets wrote: >>>>>>> On 1/8/25 12:33, 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 v5: >>>>>>>> ============== >>>>>>>> - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) >>>>>>>> - Rebased on top of latest main >>>>>>>> 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. >>>>>>>> --- >>>>>>>> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ >>>>>>>> lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ >>>>>>>> vswitchd/vswitch.xml | 13 ++++++++++ >>>>>>>> 3 files changed, 59 insertions(+) >>>>>>> >>>>>>> Not a technical review, but a few small nitpicks below. >>>>>>> Can probably be addressed on commit. >>>>>>> >>>>>>> Best regards, Ilya Maximets. >>>>>>> >>>>>>>> >>>>>>>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>>>>>>> index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ >>>>>>> >>>>>>> Period at the end of the comment. >>>>>> >>>>>> Ack. >>>>>> >>>>>>> >>>>>>>> +#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 >>>>>>>> @@ -554,6 +560,9 @@ struct netdev_dpdk { >>>>>>>> /* Socket ID detected when vHost device is brought up */ >>>>>>>> int requested_socket_id; >>>>>>>> >>>>>>>> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ >>>>>>> >>>>>>> Period. I see some other comments in this structure are not style compliant >>>>>>> as well, but we shouldn't blindly follow this practice. :) >>>>>>> >>>>>>> Also, do we need to use capital V in vhost-user? Seems inconsistent with >>>>>>> 'vHost device' spelling we use in other places. We seem to mostly use lowercase >>>>>>> in logs and docs when mentioning 'vhost-user'. WDYT? >>>>>> >>>>>> I don't have a strong opinion. In DPDK, we generally use 'Vhost-user', >>>>>> but in OVS we indeed see 'vHost' 'or vhost-user'. >>>>>> >>>>>> I am fine with using vhost-user if this is the most common. >>>>>> >>>>>>> >>>>>>>> + uint8_t vhost_max_queue_pairs; >>>>>>>> + >>>>>>>> /* Denotes whether vHost port is client/server mode */ >>>>>>>> uint64_t vhost_driver_flags; >>>>>>>> >>>>>>>> @@ -1606,6 +1615,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); >>>>>>>> } >>>>>>>> @@ -2488,6 +2499,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)) { >>>>>>>> @@ -2495,6 +2507,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); >>>>>>>> } >>>>>>>> } >>>>>>>> @@ -2511,6 +2532,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; >>>>>>>> @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) >>>>>>>> goto unlock; >>>>>>>> } >>>>>>>> >>>>>>>> + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); >>>>>>> >>>>>>> 0-day bot complains about the line length, but is this log message even needed? >>>>>>> Doesn't seem to be very useful. >>>>>> >>>>>> I added it to try avoiding confusion. Indeed, it is unfortunate but >>>>>> rte_vhost_driver_set_max_queue_num() emits below INFO-level log before >>>>>> checking whether the backend is VDUSE or vhost-user: >>>>>> >>>>>> VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", >>>>>> max_queue_pairs); >>>>>> >>>>>> So without this log, I think it would be more confusing to the user when >>>>>> adding vhost-user ports. >>>>>> >>>>>> I can fix the log in the DPDK Vhost library, and even mark it as >>>>>> candidate to stable branch, but it will not be available at the time of >>>>>> the upcoming OVS release. >>>>>> >>>>> >>>>> I think that's ok. When we integrate new LTS 24.11.X release with >>>>> updated log level we can remove the log from here. At the moment i think >>>>> it's necessary otherwise it would be misleading for vhost-user users as >>>>> I saw during testing of v4. >>>> >>>> Thanks for the feedback Kevin, I agree with your proposal. >>>> >>>> Regarding the length exceeding 79 chars, we can change it to: >>>> "Setting max queue pairs, only effective with VDUSE" >>> >>> Looks fine. I didn't realize there is a confusing DPDK message. >>> Though, an alternative solution might be to compare vhost_id with >>> /dev/vduse and not call the API at all for non-vduse backends. >>> It seems that is what vhost library is doing anyway. WDYT? >> >> We initially tried to not have to differenciate between vhost-user and >> VDUSE in OVS, but as it turns out this is not possible because above >> issue. >> >> So your proposal would also work, let me know what is your >> preference, I can prepare a patch only calling the >> rte_vhost_driver_set_max_queue_num API for VDUSE backends if you think >> this is the best way. > > I'd consider this as a stop gap solution until DPDK is fixed. > If I'm choosing between: > 1. Fighting the confusing logs with more logs, and > 2. Checking for the device type and not bothering the user with some > strangely looking logs. > I'm for the latter. We can remove the check once DPDK stops printing > confusing logs. > > Kevin, what do you think? > I could live with either, but I agree that 2. is cleaner from a user view. Also, the OVS logs will stay consistent when the DPDK log changes are integrated, so the workaround is only internal to the code and not seen by the user. >> >> Thanks, >> Maxime >> >>> >>>> >>>>> >>>>>>> >>>>>>>> + 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 275bcbec0..2cb503c37 100644 >>>>>>>> --- a/vswitchd/vswitch.xml >>>>>>>> +++ b/vswitchd/vswitch.xml >>>>>>>> @@ -3540,6 +3540,19 @@ 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 >>>>>>> >>>>>>> Same for the capital V. >>>>>> >>>>>> OK, I am fine with 'vhost-user'. >>>>>> >>>>>>> >>>>>>>> + is supported. >>>>>>>> + Only supported by dpdkvhostuserclient interfaces. >>>>>>>> + </p> >>>>>>>> + <p> >>>>>>>> + Default value is 1. >>>>>>>> + </p> >>>>>>>> + </column> >>>>>>>> + >>>>>>>> <column name="options" key="tx-retries-max" >>>>>>>> type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> >>>>>>>> <p> >>>>>>> >>>>>> >>>>>> Thanks for the review, >>>>>> Maxime >>>>>> >>>>> >>>> >>> >> >
On 1/9/25 1:19 PM, Kevin Traynor wrote: > On 09/01/2025 11:16, Ilya Maximets wrote: >> On 1/9/25 10:21, Maxime Coquelin wrote: >>> >>> >>> On 1/8/25 4:05 PM, Ilya Maximets wrote: >>>> On 1/8/25 16:00, Maxime Coquelin wrote: >>>>> >>>>> >>>>> On 1/8/25 3:56 PM, Kevin Traynor wrote: >>>>>> On 08/01/2025 14:50, Maxime Coquelin wrote: >>>>>>> >>>>>>> >>>>>>> On 1/8/25 3:38 PM, Ilya Maximets wrote: >>>>>>>> On 1/8/25 12:33, 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 v5: >>>>>>>>> ============== >>>>>>>>> - Move vhost_max_queue_pairs field in struct netdev_dpdk (Ilya) >>>>>>>>> - Rebased on top of latest main >>>>>>>>> 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. >>>>>>>>> --- >>>>>>>>> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++ >>>>>>>>> lib/netdev-dpdk.c | 31 ++++++++++++++++++++++++ >>>>>>>>> vswitchd/vswitch.xml | 13 ++++++++++ >>>>>>>>> 3 files changed, 59 insertions(+) >>>>>>>> >>>>>>>> Not a technical review, but a few small nitpicks below. >>>>>>>> Can probably be addressed on commit. >>>>>>>> >>>>>>>> Best regards, Ilya Maximets. >>>>>>>> >>>>>>>>> >>>>>>>>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst >>>>>>>>> index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 */ >>>>>>>> >>>>>>>> Period at the end of the comment. >>>>>>> >>>>>>> Ack. >>>>>>> >>>>>>>> >>>>>>>>> +#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 >>>>>>>>> @@ -554,6 +560,9 @@ struct netdev_dpdk { >>>>>>>>> /* Socket ID detected when vHost device is brought up */ >>>>>>>>> int requested_socket_id; >>>>>>>>> >>>>>>>>> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ >>>>>>>> >>>>>>>> Period. I see some other comments in this structure are not style compliant >>>>>>>> as well, but we shouldn't blindly follow this practice. :) >>>>>>>> >>>>>>>> Also, do we need to use capital V in vhost-user? Seems inconsistent with >>>>>>>> 'vHost device' spelling we use in other places. We seem to mostly use lowercase >>>>>>>> in logs and docs when mentioning 'vhost-user'. WDYT? >>>>>>> >>>>>>> I don't have a strong opinion. In DPDK, we generally use 'Vhost-user', >>>>>>> but in OVS we indeed see 'vHost' 'or vhost-user'. >>>>>>> >>>>>>> I am fine with using vhost-user if this is the most common. >>>>>>> >>>>>>>> >>>>>>>>> + uint8_t vhost_max_queue_pairs; >>>>>>>>> + >>>>>>>>> /* Denotes whether vHost port is client/server mode */ >>>>>>>>> uint64_t vhost_driver_flags; >>>>>>>>> >>>>>>>>> @@ -1606,6 +1615,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); >>>>>>>>> } >>>>>>>>> @@ -2488,6 +2499,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)) { >>>>>>>>> @@ -2495,6 +2507,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); >>>>>>>>> } >>>>>>>>> } >>>>>>>>> @@ -2511,6 +2532,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; >>>>>>>>> @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) >>>>>>>>> goto unlock; >>>>>>>>> } >>>>>>>>> >>>>>>>>> + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); >>>>>>>> >>>>>>>> 0-day bot complains about the line length, but is this log message even needed? >>>>>>>> Doesn't seem to be very useful. >>>>>>> >>>>>>> I added it to try avoiding confusion. Indeed, it is unfortunate but >>>>>>> rte_vhost_driver_set_max_queue_num() emits below INFO-level log before >>>>>>> checking whether the backend is VDUSE or vhost-user: >>>>>>> >>>>>>> VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", >>>>>>> max_queue_pairs); >>>>>>> >>>>>>> So without this log, I think it would be more confusing to the user when >>>>>>> adding vhost-user ports. >>>>>>> >>>>>>> I can fix the log in the DPDK Vhost library, and even mark it as >>>>>>> candidate to stable branch, but it will not be available at the time of >>>>>>> the upcoming OVS release. >>>>>>> >>>>>> >>>>>> I think that's ok. When we integrate new LTS 24.11.X release with >>>>>> updated log level we can remove the log from here. At the moment i think >>>>>> it's necessary otherwise it would be misleading for vhost-user users as >>>>>> I saw during testing of v4. >>>>> >>>>> Thanks for the feedback Kevin, I agree with your proposal. >>>>> >>>>> Regarding the length exceeding 79 chars, we can change it to: >>>>> "Setting max queue pairs, only effective with VDUSE" >>>> >>>> Looks fine. I didn't realize there is a confusing DPDK message. >>>> Though, an alternative solution might be to compare vhost_id with >>>> /dev/vduse and not call the API at all for non-vduse backends. >>>> It seems that is what vhost library is doing anyway. WDYT? >>> >>> We initially tried to not have to differenciate between vhost-user and >>> VDUSE in OVS, but as it turns out this is not possible because above >>> issue. >>> >>> So your proposal would also work, let me know what is your >>> preference, I can prepare a patch only calling the >>> rte_vhost_driver_set_max_queue_num API for VDUSE backends if you think >>> this is the best way. >> >> I'd consider this as a stop gap solution until DPDK is fixed. >> If I'm choosing between: >> 1. Fighting the confusing logs with more logs, and >> 2. Checking for the device type and not bothering the user with some >> strangely looking logs. >> I'm for the latter. We can remove the check once DPDK stops printing >> confusing logs. >> >> Kevin, what do you think? >> > > I could live with either, but I agree that 2. is cleaner from a user > view. Also, the OVS logs will stay consistent when the DPDK log changes > are integrated, so the workaround is only internal to the code and not > seen by the user. Ack, I implemented and tested option 2 earlier today. I'll post it soon. Thanks, Maxime >>> >>> Thanks, >>> Maxime >>> >>>> >>>>> >>>>>> >>>>>>>> >>>>>>>>> + 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 275bcbec0..2cb503c37 100644 >>>>>>>>> --- a/vswitchd/vswitch.xml >>>>>>>>> +++ b/vswitchd/vswitch.xml >>>>>>>>> @@ -3540,6 +3540,19 @@ 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 >>>>>>>> >>>>>>>> Same for the capital V. >>>>>>> >>>>>>> OK, I am fine with 'vhost-user'. >>>>>>> >>>>>>>> >>>>>>>>> + is supported. >>>>>>>>> + Only supported by dpdkvhostuserclient interfaces. >>>>>>>>> + </p> >>>>>>>>> + <p> >>>>>>>>> + Default value is 1. >>>>>>>>> + </p> >>>>>>>>> + </column> >>>>>>>>> + >>>>>>>>> <column name="options" key="tx-retries-max" >>>>>>>>> type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> >>>>>>>>> <p> >>>>>>>> >>>>>>> >>>>>>> Thanks for the review, >>>>>>> Maxime >>>>>>> >>>>>> >>>>> >>>> >>> >> >
diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst index 3c02738cf..20e8eb245 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 number 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 449c660a7..1de0b47d1 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 @@ -554,6 +560,9 @@ struct netdev_dpdk { /* Socket ID detected when vHost device is brought up */ int requested_socket_id; + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */ + uint8_t vhost_max_queue_pairs; + /* Denotes whether vHost port is client/server mode */ uint64_t vhost_driver_flags; @@ -1606,6 +1615,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); } @@ -2488,6 +2499,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)) { @@ -2495,6 +2507,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); } } @@ -2511,6 +2532,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; @@ -6397,6 +6419,15 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev) goto unlock; } + VLOG_INFO("Setting max queue pairs, only effective with VDUSE backends"); + 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 275bcbec0..2cb503c37 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -3540,6 +3540,19 @@ 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 dpdkvhostuserclient interfaces. + </p> + <p> + Default value is 1. + </p> + </column> + <column name="options" key="tx-retries-max" type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'> <p>