* Re: [PATCH] vhost: fix misleading log when setting max queue num
2025-01-09 14:31 [PATCH] vhost: fix misleading log when setting max queue num Maxime Coquelin
@ 2025-01-09 16:35 ` Ilya Maximets
2025-01-10 9:41 ` Chenbo Xia
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Ilya Maximets @ 2025-01-09 16:35 UTC (permalink / raw)
To: Maxime Coquelin, dev, chenbox, david.marchand, ktraynor
Cc: i.maximets, stable
On 1/9/25 15:31, Maxime Coquelin wrote:
> rte_vhost_driver_set_max_queue_num API returns early when
> called for a Vhost-user device, as this API is intended to
> limit the maximum number of queue pairs supported by VDUSE
> devices. However, a log mentioning the maximim number of
> queue pairs is being set is emitted unconditionally, which
> may confuse the end user.
>
> This patch moves this log after the backend type is
> checked, so that it is only called with VDUSE backends.
> The check on the requested value is also moved at the same
> place.
>
> Fixes: e1808999d36b ("vhost: restrict set max queue pair API to VDUSE")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
Thanks, Maxime! This makes the logs way less confusing.
Reviewed-by: Ilya Maximets <i.maximets@ovn.org>
> lib/vhost/socket.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index d29d15494c..07247907b0 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -844,13 +844,6 @@ rte_vhost_driver_set_max_queue_num(const char *path, uint32_t max_queue_pairs)
> struct vhost_user_socket *vsocket;
> int ret = 0;
>
> - VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", max_queue_pairs);
> -
> - if (max_queue_pairs > VHOST_MAX_QUEUE_PAIRS) {
> - VHOST_CONFIG_LOG(path, ERR, "Library only supports up to %u queue pairs",
> - VHOST_MAX_QUEUE_PAIRS);
> - return -1;
> - }
>
> pthread_mutex_lock(&vhost_user.mutex);
> vsocket = find_vhost_user_socket(path);
> @@ -872,6 +865,15 @@ rte_vhost_driver_set_max_queue_num(const char *path, uint32_t max_queue_pairs)
> goto unlock_exit;
> }
>
> + VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", max_queue_pairs);
> +
> + if (max_queue_pairs > VHOST_MAX_QUEUE_PAIRS) {
> + VHOST_CONFIG_LOG(path, ERR, "Library only supports up to %u queue pairs",
> + VHOST_MAX_QUEUE_PAIRS);
> + ret = -1;
> + goto unlock_exit;
> + }
> +
> vsocket->max_queue_pairs = max_queue_pairs;
>
> unlock_exit:
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] vhost: fix misleading log when setting max queue num
2025-01-09 14:31 [PATCH] vhost: fix misleading log when setting max queue num Maxime Coquelin
2025-01-09 16:35 ` Ilya Maximets
@ 2025-01-10 9:41 ` Chenbo Xia
2025-01-10 9:56 ` Kevin Traynor
2025-01-17 9:28 ` Maxime Coquelin
3 siblings, 0 replies; 5+ messages in thread
From: Chenbo Xia @ 2025-01-10 9:41 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, david.marchand, ktraynor, i.maximets, stable
Hi Maxime,
> On Jan 9, 2025, at 22:31, Maxime Coquelin <maxime.coquelin@redhat.com> wrote:
>
> External email: Use caution opening links or attachments
>
>
> rte_vhost_driver_set_max_queue_num API returns early when
> called for a Vhost-user device, as this API is intended to
> limit the maximum number of queue pairs supported by VDUSE
> devices. However, a log mentioning the maximim number of
maximum
With above fixed:
Reviewed-by: Chenbo Xia <chenbox@nvidia.com>
> queue pairs is being set is emitted unconditionally, which
> may confuse the end user.
>
> This patch moves this log after the backend type is
> checked, so that it is only called with VDUSE backends.
> The check on the requested value is also moved at the same
> place.
>
> Fixes: e1808999d36b ("vhost: restrict set max queue pair API to VDUSE")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/vhost/socket.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index d29d15494c..07247907b0 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -844,13 +844,6 @@ rte_vhost_driver_set_max_queue_num(const char *path, uint32_t max_queue_pairs)
> struct vhost_user_socket *vsocket;
> int ret = 0;
>
> - VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", max_queue_pairs);
> -
> - if (max_queue_pairs > VHOST_MAX_QUEUE_PAIRS) {
> - VHOST_CONFIG_LOG(path, ERR, "Library only supports up to %u queue pairs",
> - VHOST_MAX_QUEUE_PAIRS);
> - return -1;
> - }
>
> pthread_mutex_lock(&vhost_user.mutex);
> vsocket = find_vhost_user_socket(path);
> @@ -872,6 +865,15 @@ rte_vhost_driver_set_max_queue_num(const char *path, uint32_t max_queue_pairs)
> goto unlock_exit;
> }
>
> + VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", max_queue_pairs);
> +
> + if (max_queue_pairs > VHOST_MAX_QUEUE_PAIRS) {
> + VHOST_CONFIG_LOG(path, ERR, "Library only supports up to %u queue pairs",
> + VHOST_MAX_QUEUE_PAIRS);
> + ret = -1;
> + goto unlock_exit;
> + }
> +
> vsocket->max_queue_pairs = max_queue_pairs;
>
> unlock_exit:
> --
> 2.47.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] vhost: fix misleading log when setting max queue num
2025-01-09 14:31 [PATCH] vhost: fix misleading log when setting max queue num Maxime Coquelin
2025-01-09 16:35 ` Ilya Maximets
2025-01-10 9:41 ` Chenbo Xia
@ 2025-01-10 9:56 ` Kevin Traynor
2025-01-17 9:28 ` Maxime Coquelin
3 siblings, 0 replies; 5+ messages in thread
From: Kevin Traynor @ 2025-01-10 9:56 UTC (permalink / raw)
To: Maxime Coquelin, dev, chenbox, david.marchand, i.maximets; +Cc: stable
On 09/01/2025 14:31, Maxime Coquelin wrote:
> rte_vhost_driver_set_max_queue_num API returns early when
> called for a Vhost-user device, as this API is intended to
> limit the maximum number of queue pairs supported by VDUSE
> devices. However, a log mentioning the maximim number of
> queue pairs is being set is emitted unconditionally, which
> may confuse the end user.
>
> This patch moves this log after the backend type is
> checked, so that it is only called with VDUSE backends.
> The check on the requested value is also moved at the same
> place.
>
> Fixes: e1808999d36b ("vhost: restrict set max queue pair API to VDUSE")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/vhost/socket.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
Thanks, this will make it easier for vhost-user users. One nit below,
Acked-by: Kevin Traynor <ktraynor@redhat.com>
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index d29d15494c..07247907b0 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -844,13 +844,6 @@ rte_vhost_driver_set_max_queue_num(const char *path, uint32_t max_queue_pairs)
> struct vhost_user_socket *vsocket;
> int ret = 0;
>
> - VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", max_queue_pairs);
> -
> - if (max_queue_pairs > VHOST_MAX_QUEUE_PAIRS) {
> - VHOST_CONFIG_LOG(path, ERR, "Library only supports up to %u queue pairs",
> - VHOST_MAX_QUEUE_PAIRS);
> - return -1;
> - }
>
you can remove the extra blank line here
> pthread_mutex_lock(&vhost_user.mutex);
> vsocket = find_vhost_user_socket(path);
> @@ -872,6 +865,15 @@ rte_vhost_driver_set_max_queue_num(const char *path, uint32_t max_queue_pairs)
> goto unlock_exit;
> }
>
> + VHOST_CONFIG_LOG(path, INFO, "Setting max queue pairs to %u", max_queue_pairs);
> +
> + if (max_queue_pairs > VHOST_MAX_QUEUE_PAIRS) {
> + VHOST_CONFIG_LOG(path, ERR, "Library only supports up to %u queue pairs",
> + VHOST_MAX_QUEUE_PAIRS);
> + ret = -1;
> + goto unlock_exit;
> + }
> +
> vsocket->max_queue_pairs = max_queue_pairs;
>
> unlock_exit:
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] vhost: fix misleading log when setting max queue num
2025-01-09 14:31 [PATCH] vhost: fix misleading log when setting max queue num Maxime Coquelin
` (2 preceding siblings ...)
2025-01-10 9:56 ` Kevin Traynor
@ 2025-01-17 9:28 ` Maxime Coquelin
3 siblings, 0 replies; 5+ messages in thread
From: Maxime Coquelin @ 2025-01-17 9:28 UTC (permalink / raw)
To: dev, chenbox, david.marchand, ktraynor, i.maximets; +Cc: stable
On 1/9/25 3:31 PM, Maxime Coquelin wrote:
> rte_vhost_driver_set_max_queue_num API returns early when
> called for a Vhost-user device, as this API is intended to
> limit the maximum number of queue pairs supported by VDUSE
> devices. However, a log mentioning the maximim number of
> queue pairs is being set is emitted unconditionally, which
> may confuse the end user.
>
> This patch moves this log after the backend type is
> checked, so that it is only called with VDUSE backends.
> The check on the requested value is also moved at the same
> place.
>
> Fixes: e1808999d36b ("vhost: restrict set max queue pair API to VDUSE")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/vhost/socket.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
With suggested changes from Kevin:
Applied to next-virtio/for-next-net
Thanks,
Maxime
^ permalink raw reply [flat|nested] 5+ messages in thread