vhost: fix misleading log when setting max queue num

Message ID 20250109143130.3696613-1-maxime.coquelin@redhat.com (mailing list archive)
State Accepted
Delegated to: Maxime Coquelin
Headers
Series vhost: fix misleading log when setting max queue num |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-amd64-testing warning Testing issues

Commit Message

Maxime Coquelin Jan. 9, 2025, 2:31 p.m. UTC
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(-)
  

Comments

Ilya Maximets Jan. 9, 2025, 4:35 p.m. UTC | #1
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:
  
Chenbo Xia Jan. 10, 2025, 9:41 a.m. UTC | #2
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
>
  
Kevin Traynor Jan. 10, 2025, 9:56 a.m. UTC | #3
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:
  
Maxime Coquelin Jan. 17, 2025, 9:28 a.m. UTC | #4
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
  

Patch

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: