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 <[email protected]>
>>> Acked-by: Eelco Chaudron <[email protected]>
>>> ---
>>> 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
