On 12/18/24 16:04, Maxime Coquelin wrote:
>
>
> On 12/18/24 14:44, Ilya Maximets wrote:
>> On 12/18/24 14:09, Maxime Coquelin wrote:
>>>
>>>
>>> On 12/18/24 12:22, Kevin Traynor wrote:
>>>> On 17/12/2024 10:24, Maxime Coquelin wrote:
>>>>> Hi Kevin,
>>>>>
>>>>> On 12/6/24 21:26, Kevin Traynor wrote:
>>>>>> On 06/12/2024 15:58, Maxime Coquelin wrote:
>>>>>>> This patch uses the new rte_vhost_driver_set_max_queue_num
>>>>>>> API to set the maximum number of queue pairs supported by
>>>>>>> the Vhost-user port.
>>>>>>>
>>>>>>> This is required for VDUSE which needs to specify the
>>>>>>> maximum number of queue pairs at creation time. Without it
>>>>>>> 128 queue pairs metadata would be allocated.
>>>>>>>
>>>>>>> To configure it, a new 'vhost-max-queue-pairs' option is
>>>>>>> introduced.
>>>>>>>
>>>>>>> Signed-off-by: Maxime Coquelin <[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?
>
> Ok, what about moving it here (your reply can wait 2025 ;)):
>
> union {
> struct {
> int requested_mtu; /* 704 4 */
> int requested_n_txq; /* 708 4 */
> int user_n_rxq; /* 712 4 */
> int requested_n_rxq; /* 716 4 */
> int requested_rxq_size; /* 720 4 */
> int requested_txq_size; /* 724 4 */
> int rxq_size; /* 728 4 */
> int txq_size; /* 732 4 */
> int requested_socket_id; /* 736 4 */
> uint8_t vhost_max_queue_pairs; /* 740 1 */
>
> /* XXX 3 bytes hole, try to pack */
>
> uint64_t vhost_driver_flags; /* 744 8 */
> struct rte_eth_fc_conf fc_conf; /* 752 20 */
>
> /* XXX last struct has 2 bytes of padding */
>
This location looks fine to me.
Best regards, Ilya Maximets.
>> /me goes back to PTO and Christmass preparations. :)
>
> Enjoy your PTO,
> Maxime
>
>>>
>>>>
>>>> thanks,
>>>> Kevin.
>>>>
>>>>>>
>>>>>>> /* Flags for virtio features recovery mechanism. */
>>>>>>> uint8_t virtio_features_state;
>>>>>>>
>>>>>>> @@ -1609,6 +1618,8 @@ vhost_common_construct(struct netdev *netdev)
>>>>>>>
>>>>>>> atomic_init(&dev->vhost_tx_retries_max, VHOST_ENQ_RETRY_DEF);
>>>>>>>
>>>>>>> + dev->vhost_max_queue_pairs = VHOST_MAX_QUEUE_PAIRS_DEF;
>>>>>>> +
>>>>>>> return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>>>>>>> DPDK_DEV_VHOST, socket_id);
>>>>>>> }
>>>>>>> @@ -2491,6 +2502,7 @@ netdev_dpdk_vhost_client_set_config(struct netdev
>>>>>>> *netdev,
>>>>>>> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>>>>>>> const char *path;
>>>>>>> int max_tx_retries, cur_max_tx_retries;
>>>>>>> + uint32_t max_queue_pairs;
>>>>>>>
>>>>>>> ovs_mutex_lock(&dev->mutex);
>>>>>>> if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)) {
>>>>>>> @@ -2498,6 +2510,15 @@ netdev_dpdk_vhost_client_set_config(struct
>>>>>>> netdev *netdev,
>>>>>>> if (!nullable_string_is_equal(path, dev->vhost_id)) {
>>>>>>> free(dev->vhost_id);
>>>>>>> dev->vhost_id = nullable_xstrdup(path);
>>>>>>> +
>>>>>>> + max_queue_pairs = smap_get_int(args,
>>>>>>> "vhost-max-queue-pairs",
>>>>>>> + VHOST_MAX_QUEUE_PAIRS_DEF);
>>>>>>> + if (max_queue_pairs < VHOST_MAX_QUEUE_PAIRS_MIN
>>>>>>> + || max_queue_pairs > VHOST_MAX_QUEUE_PAIRS_MAX) {
>>>>>>> + max_queue_pairs = VHOST_MAX_QUEUE_PAIRS_DEF;
>>>>>>> + }
>>>>>>> + dev->vhost_max_queue_pairs = max_queue_pairs;
>>>>>>> +
>>>>>>> netdev_request_reconfigure(netdev);
>>>>>>> }
>>>>>>> }
>>>>>>> @@ -2514,6 +2535,7 @@ netdev_dpdk_vhost_client_set_config(struct netdev
>>>>>>> *netdev,
>>>>>>> VLOG_INFO("Max Tx retries for vhost device '%s' set to %d",
>>>>>>> netdev_get_name(netdev), max_tx_retries);
>>>>>>> }
>>>>>>> +
>>>>>>> ovs_mutex_unlock(&dev->mutex);
>>>>>>>
>>>>>>> return 0;
>>>>>>> @@ -6400,6 +6422,14 @@ netdev_dpdk_vhost_client_reconfigure(struct
>>>>>>> netdev *netdev)
>>>>>>> goto unlock;
>>>>>>> }
>>>>>>>
>>>>>>> + err = rte_vhost_driver_set_max_queue_num(dev->vhost_id,
>>>>>>> +
>>>>>>> dev->vhost_max_queue_pairs);
>>>>>>
>>>>>> The log below is printed:
>>>>>>
>>>>>> 2024-12-06T18:28:51Z|00100|dpdk|INFO|VHOST_CONFIG: (/tmp/vhost0) Setting
>>>>>> max queue pairs to 1
>>>>>>
>>>>>> It's kind of true, but it could be confusing when using vhost-user
>>>>>> backend. Maybe we should add an OVS info log before or after as a
>>>>>> reminder that the max queue pairs setting is only valid for VDUSE
>>>>>> backend.
>>>>>
>>>>> Yes, that's unfortunate that I added a log in
>>>>> rte_vhost_driver_set_max_queue_num(), but only at DEBUG level:
>>>>>
>>>>> + if (!vsocket->is_vduse) {
>>>>> + VHOST_CONFIG_LOG(path, DEBUG,
>>>>> + "Keeping %u max queue pairs for
>>>>> Vhost-user backend",
>>>>> + VHOST_MAX_QUEUE_PAIRS);
>>>>> + goto unlock_exit;
>>>>> + }
>>>>>
>>>>> I can indeed a log before calling the API to avoid the confusion:
>>>>>
>>>>> "Setting max queue pairs, only effective with VDUSE backends"
>>>>>
>>>>>>
>>>>>>> + if (err) {
>>>>>>> + VLOG_ERR("rte_vhost_driver_set_max_queue_num failed for "
>>>>>>> + "vhost-user client port: %s\n", dev->up.name);
>>>>>>> + goto unlock;
>>>>>>> + }
>>>>>>> +
>>>>>>> err = rte_vhost_driver_start(dev->vhost_id);
>>>>>>> if (err) {
>>>>>>> VLOG_ERR("rte_vhost_driver_start failed for vhost user "
>>>>>>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>>>>>>> index 36cb4e495..0b5c5dcd6 100644
>>>>>>> --- a/vswitchd/vswitch.xml
>>>>>>> +++ b/vswitchd/vswitch.xml
>>>>>>> @@ -3520,6 +3520,16 @@ ovs-vsctl add-port br0 p0 -- set Interface p0
>>>>>>> type=patch options:peer=p1 \
>>>>>>> </p>
>>>>>>> </column>
>>>>>>>
>>>>>>> + <column name="options" key="vhost-max-queue-pairs"
>>>>>>> + type='{"type": "integer", "minInteger" : 1,
>>>>>>> "maxInteger": 128}'>
>>>>>
>>>>> I already mention the max value is 128, so uint8_t will be enough for
>>>>> vhost_max_queue_pairs field.
>>>>>
>>>>>>> + <p>
>>>>>>> + The value specifies the maximum number of queue pairs
>>>>>>> supported by
>>>>>>> + a vHost device. This is ignored for Vhost-user backends,
>>>>>>> only VDUSE
>>>>>>> + is supported.
>>>>>>> + Only supported by dpdkvhsotuserclient interfaces.
>>>>>>
>>>>>> typo dpdkvhostuserclient
>>>>>
>>>>> good catch, will fix.
>>>>>
>>>>>>
>>>>>>> + </p>
>>>>>>
>>>>>> It would be good to state the default here (like tx-retries-max below)
>>>>>
>>>>> Indeed, will add in nex revision.
>>>>>
>>>>>>
>>>>>>> + </column>
>>>>>>> +
>>>>>>> <column name="options" key="tx-retries-max"
>>>>>>> type='{"type": "integer", "minInteger": 0,
>>>>>>> "maxInteger": 32}'>
>>>>>>> <p>
>>>>>>
>>>>>
>>>>> Thanks for the review,
>>>>> Maxime
>>>>>
>>>>
>>>
>>> _______________________________________________
>>> dev mailing list
>>> [email protected]
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
