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