On 12/18/24 14:09, Maxime Coquelin wrote:
>
>
> On 12/18/24 12:22, Kevin Traynor wrote:
>> On 17/12/2024 10:24, Maxime Coquelin wrote:
>>> Hi Kevin,
>>>
>>> On 12/6/24 21:26, Kevin Traynor wrote:
>>>> On 06/12/2024 15:58, Maxime Coquelin wrote:
>>>>> This patch uses the new rte_vhost_driver_set_max_queue_num
>>>>> API to set the maximum number of queue pairs supported by
>>>>> the Vhost-user port.
>>>>>
>>>>> This is required for VDUSE which needs to specify the
>>>>> maximum number of queue pairs at creation time. Without it
>>>>> 128 queue pairs metadata would be allocated.
>>>>>
>>>>> To configure it, a new 'vhost-max-queue-pairs' option is
>>>>> introduced.
>>>>>
>>>>> Signed-off-by: Maxime Coquelin <[email protected]>
>>>>> Acked-by: Eelco Chaudron <[email protected]>
>>>>> ---
>>>>>
>>>>> Note: depends on DPDK v24.11.
>>>>>
>>>>> Changes in v4:
>>>>> ==============
>>>>> - Change to vhost-queue-pairs-max to vhost-max-queue-pairs (Eelco)
>>>>> - Remove mention to DPDK version in documentation (Eelco)
>>>>> - Add missing parameter description in vswitch.xml (Eelco)
>>>>> - Define min and max values for the new option (Maxime)
>>>>> Changes in v3:
>>>>> ==============
>>>>> - Introduce a new option to set the number of max queue pairs (Kevin)
>>>>> - Add documentation for new option
>>>>>
>>>>> Changes in v2:
>>>>> ==============
>>>>> - Address checkpatch warnings.
>>>>> ---
>>>>
>>>> Hi Maxime,
>>>>
>>>> Testing with vhost-user backend with DPDK 24.11 worked well - in that it
>>>> called the API with the right number and didn't break anything :-)
>>>>
>>>> A few comments on the code/docs below.
>>>>
>>>> fyi - I initially tested on OVS main branch with 23.11 and I saw a loop
>>>> between the destroy_connection() callback triggering
>>>> netdev_dpdk_vhost_client_reconfigure(), repeat, etc when i started a VM
>>>> with 4 queues. So OVS having DPDK 24.11 support will need to be a
>>>> dependency I think (even aside from the experimental API issue).
>>>>
>>>
>>> Yes, I confirm 24.11 is also a functional dependency, as v23.11.2 miss
>>> one fix that will be in upcoming v23.11.3:
>>>
>>
>> As the 24.11 update patch is under review, we can just keep your
>> patchset for the main branch and make sure we apply in the correct order
>> (all going well - fingers crossed).
>>
>>>
>>> commit bc578c07f03ec3943fab88a5d156f28b98e1e652
>>> Author: Maxime Coquelin <[email protected]>
>>> Date: Thu Oct 3 10:11:10 2024 +0200
>>>
>>> vhost: restrict set max queue pair API to VDUSE
>>>
>>> [ upstream commit e1808999d36bb2e136a649f4651f36030aa468f1 ]
>>>
>>> In order to avoid breaking Vhost-user live-migration, we want the
>>> rte_vhost_driver_set_max_queue_num API to only be effective with
>>> VDUSE.
>>>
>>> Furthermore, this API is only really needed for VDUSE where the
>>> device number of queues is defined by the backend. For Vhost-user,
>>> this is defined by the frontend (e.g. QEMU), so the advantages of
>>> restricting more the maximum number of queue pairs is limited to
>>> a small memory gain (a handful of pointers).
>>>
>>> Fixes: 4aa1f88ac13d ("vhost: add API to set max queue pairs")
>>>
>>> Reported-by: Yu Jiang <[email protected]>
>>> Signed-off-by: Maxime Coquelin <[email protected]>
>>> Acked-by: David Marchand <[email protected]>
>>>
>>>
>>>> thanks,
>>>> Kevin.
>>>>
>>>>> Documentation/topics/dpdk/vhost-user.rst | 15 ++++++++++++
>>>>> lib/netdev-dpdk.c | 30 ++++++++++++++++++++++++
>>>>> vswitchd/vswitch.xml | 10 ++++++++
>>>>> 3 files changed, 55 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/topics/dpdk/vhost-user.rst
>>>>> b/Documentation/topics/dpdk/vhost-user.rst
>>>>> index 7bba08ac2..656f7f69f 100644
>>>>> --- a/Documentation/topics/dpdk/vhost-user.rst
>>>>> +++ b/Documentation/topics/dpdk/vhost-user.rst
>>>>> @@ -375,6 +375,21 @@ Tx retries max can be set for vhost-user-client
>>>>> ports::
>>>>>
>>>>> Configurable vhost tx retries are not supported with vhost-user
>>>>> ports.
>>>>>
>>>>> +vhost-user-client max queue pairs config
>>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>> +
>>>>> +For vhost-user-client interfaces using the VDUSE backend, the maximum
>>>>> umber of
>>>>
>>>> typo number
>>>>
>>>>> +queue pairs the Virtio device will support can be set at port creation
>>>>> time. If
>>>>> +not set, the default value is 1 queue pair. This value is ignored for
>>>>> +Vhost-user backends.
>>>>> +
>>>>> +Maximum number of queue pairs can be set for vhost-user-client-ports::
>>>>> +
>>>>> + $ ovs-vsctl add-port br0 vduse0 \
>>>>> + -- set Interface vduse0 type=dpdkvhostuserclient \
>>>>> + options:vhost-server-path=/dev/vduse/vduse0 \
>>>>> + options:vhost-max-queue-pairs=4
>>>>> +
>>>>> .. _dpdk-testpmd:
>>>>>
>>>>> DPDK in the Guest
>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>>>> index dc52a2b56..a8b605113 100644
>>>>> --- a/lib/netdev-dpdk.c
>>>>> +++ b/lib/netdev-dpdk.c
>>>>> @@ -19,6 +19,7 @@
>>>>>
>>>>> #include <errno.h>
>>>>> #include <signal.h>
>>>>> +#include <stdint.h>
>>>>> #include <stdlib.h>
>>>>> #include <string.h>
>>>>> #include <unistd.h>
>>>>> @@ -153,6 +154,11 @@ typedef uint16_t dpdk_port_t;
>>>>> /* Legacy default value for vhost tx retries. */
>>>>> #define VHOST_ENQ_RETRY_DEF 8
>>>>>
>>>>> +/* VDUSE-only, ignore for Vhost-user */
>>>>> +#define VHOST_MAX_QUEUE_PAIRS_MIN 1
>>>>> +#define VHOST_MAX_QUEUE_PAIRS_DEF VHOST_MAX_QUEUE_PAIRS_MIN
>>>>> +#define VHOST_MAX_QUEUE_PAIRS_MAX 128
>>>>> +
>>>>> #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>>>>>
>>>>> /* List of required flags advertised by the hardware that will be used
>>>>> @@ -497,6 +503,9 @@ struct netdev_dpdk {
>>>>>
>>>>> atomic_uint8_t vhost_tx_retries_max;
>>>>>
>>>>> + /* Ignored by DPDK for Vhost-user backends, only for VDUSE */
>>>>> + uint32_t vhost_max_queue_pairs;
>>>>> +
>>>>
>>>> I noticed this added a cacheline - perhaps we could use something
>>>> smaller and squash it in ?
>>>>
>>>> /* --- cacheline 1 boundary (64 bytes) --- */
>>>> union {
>>>> OVS_CACHE_LINE_MARKER cacheline1; /* 64 1 */
>>>> struct {
>>>> struct ovs_mutex mutex; /* 64 48 */
>>>> struct dpdk_mp * dpdk_mp; /* 112 8 */
>>>> ovsrcu_index vid; /* 120 4 */
>>>> _Bool vhost_reconfigured; /* 124 1 */
>>>> atomic_uint8_t vhost_tx_retries_max; /* 125 1 */
>>>>
>>>> /* XXX 2 bytes hole, try to pack */
>>>>
>>>> /* --- cacheline 2 boundary (128 bytes) --- */
>>>> uint32_t vhost_max_queue_pairs; /* 128 4 */
>>>> uint8_t virtio_features_state; /* 132 1 */
>>>> }; /* 64 72 */
>>>> uint8_t pad55[128]; /* 64 128 */
>>>> }; /* 64 128 */
>>>> /* --- cacheline 3 boundary (192 bytes) --- */
>>>
>>>
>>> Good catch, David already mentioned it to me off-list.
>>> DPDK Vhost library only supports up to 128 queue pairs, so I think we
>>> could use uint8_t type and so it would fit into the 1 byte hole.
>>>
>>> Do you agree with this suggestion?
>>>
>>
>> Yes, sounds good. It should be fine but you may want to confirm with
>> pahole as sometimes the comments can get stale.
>
> Looks good with pahole:
>
> /* --- cacheline 1 boundary (64 bytes) --- */
> union {
> OVS_CACHE_LINE_MARKER cacheline1; /* 64 1 */
> struct {
> struct ovs_mutex mutex; /* 64 48 */
> struct dpdk_mp * dpdk_mp; /* 112 8 */
> ovsrcu_index vid; /* 120 4 */
> _Bool vhost_reconfigured; /* 124 1 */
> atomic_uint8_t vhost_tx_retries_max; /* 125
> 1 */
> uint8_t vhost_max_queue_pairs; /* 126 1 */
> uint8_t virtio_features_state; /* 127 1 */
> }; /* 64 64 */
> uint8_t pad55[64]; /* 64 64 */
> }; /* 64 64 */
> /* --- cacheline 2 boundary (128 bytes) --- */
>
Why does it need to be in the first cache line? It's not on the hot path,
right?
/me goes back to PTO and Christmass preparations. :)
>
>>
>> thanks,
>> Kevin.
>>
>>>>
>>>>> /* Flags for virtio features recovery mechanism. */
>>>>> uint8_t virtio_features_state;
>>>>>
>>>>> @@ -1609,6 +1618,8 @@ vhost_common_construct(struct netdev *netdev)
>>>>>
>>>>> atomic_init(&dev->vhost_tx_retries_max, VHOST_ENQ_RETRY_DEF);
>>>>>
>>>>> + dev->vhost_max_queue_pairs = VHOST_MAX_QUEUE_PAIRS_DEF;
>>>>> +
>>>>> return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>>>> DPDK_DEV_VHOST, socket_id);
>>>>> }
>>>>> @@ -2491,6 +2502,7 @@ netdev_dpdk_vhost_client_set_config(struct netdev
>>>>> *netdev,
>>>>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>>>> const char *path;
>>>>> int max_tx_retries, cur_max_tx_retries;
>>>>> + uint32_t max_queue_pairs;
>>>>>
>>>>> ovs_mutex_lock(&dev->mutex);
>>>>> if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
>>>>> @@ -2498,6 +2510,15 @@ netdev_dpdk_vhost_client_set_config(struct netdev
>>>>> *netdev,
>>>>> if (!nullable_string_is_equal(path, dev->vhost_id)) {
>>>>> free(dev->vhost_id);
>>>>> dev->vhost_id = nullable_xstrdup(path);
>>>>> +
>>>>> + max_queue_pairs = smap_get_int(args, "vhost-max-queue-pairs",
>>>>> + VHOST_MAX_QUEUE_PAIRS_DEF);
>>>>> + if (max_queue_pairs < VHOST_MAX_QUEUE_PAIRS_MIN
>>>>> + || max_queue_pairs > VHOST_MAX_QUEUE_PAIRS_MAX) {
>>>>> + max_queue_pairs = VHOST_MAX_QUEUE_PAIRS_DEF;
>>>>> + }
>>>>> + dev->vhost_max_queue_pairs = max_queue_pairs;
>>>>> +
>>>>> netdev_request_reconfigure(netdev);
>>>>> }
>>>>> }
>>>>> @@ -2514,6 +2535,7 @@ netdev_dpdk_vhost_client_set_config(struct netdev
>>>>> *netdev,
>>>>> VLOG_INFO("Max Tx retries for vhost device '%s' set to %d",
>>>>> netdev_get_name(netdev), max_tx_retries);
>>>>> }
>>>>> +
>>>>> ovs_mutex_unlock(&dev->mutex);
>>>>>
>>>>> return 0;
>>>>> @@ -6400,6 +6422,14 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev
>>>>> *netdev)
>>>>> goto unlock;
>>>>> }
>>>>>
>>>>> + err = rte_vhost_driver_set_max_queue_num(dev->vhost_id,
>>>>> +
>>>>> dev->vhost_max_queue_pairs);
>>>>
>>>> The log below is printed:
>>>>
>>>> 2024-12-06T18:28:51Z|00100|dpdk|INFO|VHOST_CONFIG: (/tmp/vhost0) Setting
>>>> max queue pairs to 1
>>>>
>>>> It's kind of true, but it could be confusing when using vhost-user
>>>> backend. Maybe we should add an OVS info log before or after as a
>>>> reminder that the max queue pairs setting is only valid for VDUSE backend.
>>>
>>> Yes, that's unfortunate that I added a log in
>>> rte_vhost_driver_set_max_queue_num(), but only at DEBUG level:
>>>
>>> + if (!vsocket->is_vduse) {
>>> + VHOST_CONFIG_LOG(path, DEBUG,
>>> + "Keeping %u max queue pairs for
>>> Vhost-user backend",
>>> + VHOST_MAX_QUEUE_PAIRS);
>>> + goto unlock_exit;
>>> + }
>>>
>>> I can indeed a log before calling the API to avoid the confusion:
>>>
>>> "Setting max queue pairs, only effective with VDUSE backends"
>>>
>>>>
>>>>> + if (err) {
>>>>> + VLOG_ERR("rte_vhost_driver_set_max_queue_num failed for "
>>>>> + "vhost-user client port: %s\n", dev->up.name);
>>>>> + goto unlock;
>>>>> + }
>>>>> +
>>>>> err = rte_vhost_driver_start(dev->vhost_id);
>>>>> if (err) {
>>>>> VLOG_ERR("rte_vhost_driver_start failed for vhost user "
>>>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>>>>> index 36cb4e495..0b5c5dcd6 100644
>>>>> --- a/vswitchd/vswitch.xml
>>>>> +++ b/vswitchd/vswitch.xml
>>>>> @@ -3520,6 +3520,16 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
>>>>> type=patch options:peer=p1 \
>>>>> </p>
>>>>> </column>
>>>>>
>>>>> + <column name="options" key="vhost-max-queue-pairs"
>>>>> + type='{"type": "integer", "minInteger" : 1, "maxInteger":
>>>>> 128}'>
>>>
>>> I already mention the max value is 128, so uint8_t will be enough for
>>> vhost_max_queue_pairs field.
>>>
>>>>> + <p>
>>>>> + The value specifies the maximum number of queue pairs
>>>>> supported by
>>>>> + a vHost device. This is ignored for Vhost-user backends, only
>>>>> VDUSE
>>>>> + is supported.
>>>>> + Only supported by dpdkvhsotuserclient interfaces.
>>>>
>>>> typo dpdkvhostuserclient
>>>
>>> good catch, will fix.
>>>
>>>>
>>>>> + </p>
>>>>
>>>> It would be good to state the default here (like tx-retries-max below)
>>>
>>> Indeed, will add in nex revision.
>>>
>>>>
>>>>> + </column>
>>>>> +
>>>>> <column name="options" key="tx-retries-max"
>>>>> type='{"type": "integer", "minInteger": 0, "maxInteger":
>>>>> 32}'>
>>>>> <p>
>>>>
>>>
>>> Thanks for the review,
>>> Maxime
>>>
>>
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
