[dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer

[dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer

[Stephen Hemminger]

Since the driver is just using netlink to receive acknowledgements
for command requests, having a large (32K) buffer is unnecessary.
Allocating large buffers on stack will break in application is
using smaller per-thread stacks.

It looks like original code intended to use a smaller buffer
for the read than the socket, so keep that set of defines.

Fixes: ccdcba53a3f4 ("net/mlx5: use Netlink to add/remove MAC addresses")
Cc: nelio.laranjeiro@6wind.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/common/mlx5/mlx5_nl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_nl.c b/drivers/common/mlx5/mlx5_nl.c
index d9f0d4129e35..847e78dbcea6 100644
--- a/drivers/common/mlx5/mlx5_nl.c
+++ b/drivers/common/mlx5/mlx5_nl.c
@@ -28,7 +28,7 @@
 
 
 /* Size of the buffer to receive kernel messages */
-#define MLX5_NL_BUF_SIZE (32 * 1024)
+#define MLX5_NL_BUF_SIZE 4096
 /* Send buffer size for the Netlink socket */
 #define MLX5_SEND_BUF_SIZE 32768
 /* Receive buffer size for the Netlink socket */
@@ -330,7 +330,7 @@ mlx5_nl_recv(int nlsk_fd, uint32_t sn, int (*cb)(struct nlmsghdr *, void *arg),
 	     void *arg)
 {
 	struct sockaddr_nl sa;
-	char buf[MLX5_RECV_BUF_SIZE];
+	char buf[MLX5_NL_BUF_SIZE];
 	struct iovec iov = {
 		.iov_base = buf,
 		.iov_len = sizeof(buf),
-- 
2.20.1

Re: [dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer

[Slava Ovsiienko]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Stephen Hemminger
> Sent: Thursday, April 23, 2020 1:30
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Nélio Laranjeiro
> <nelio.laranjeiro@6wind.com>
> Subject: [dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer
> 
> Since the driver is just using netlink to receive acknowledgements for
> command requests, having a large (32K) buffer is unnecessary.

Not for acknowledgements only, it is also used to get the dumps for the MAC
addresses and Infiniband device ports, as we know dump operations might
merge replies into one large message, smaller buffer might prevent the
dump receiving. What do you think about the buffer allocation  on the device
initialization and storing that somewhere in the device structure?

With best regards,
Slava

> Allocating large buffers on stack will break in application is using smaller per-
> thread stacks.
> 
> It looks like original code intended to use a smaller buffer for the read than
> the socket, so keep that set of defines.
> 
> Fixes: ccdcba53a3f4 ("net/mlx5: use Netlink to add/remove MAC addresses")
> Cc: nelio.laranjeiro@6wind.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/common/mlx5/mlx5_nl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_nl.c
> b/drivers/common/mlx5/mlx5_nl.c index d9f0d4129e35..847e78dbcea6
> 100644
> --- a/drivers/common/mlx5/mlx5_nl.c
> +++ b/drivers/common/mlx5/mlx5_nl.c
> @@ -28,7 +28,7 @@
> 
> 
>  /* Size of the buffer to receive kernel messages */ -#define
> MLX5_NL_BUF_SIZE (32 * 1024)
> +#define MLX5_NL_BUF_SIZE 4096
>  /* Send buffer size for the Netlink socket */  #define MLX5_SEND_BUF_SIZE
> 32768
>  /* Receive buffer size for the Netlink socket */ @@ -330,7 +330,7 @@
> mlx5_nl_recv(int nlsk_fd, uint32_t sn, int (*cb)(struct nlmsghdr *, void *arg),
>  	     void *arg)
>  {
>  	struct sockaddr_nl sa;
> -	char buf[MLX5_RECV_BUF_SIZE];
> +	char buf[MLX5_NL_BUF_SIZE];
>  	struct iovec iov = {
>  		.iov_base = buf,
>  		.iov_len = sizeof(buf),
> --
> 2.20.1

[dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack

[Viacheslav Ovsiienko]

The buffer size to receive netlink reply messages is relatively
large (32K), and it is allocated on the stack and it might
break in application is using smaller per-thread stacks.
This patch allocates temporary buffer from heap.

Fixes: ccdcba53a3f4 ("net/mlx5: use Netlink to add/remove MAC addresses")
Cc: nelio.laranjeiro@6wind.com
Cc: stable@dpdk.org

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

---
v2: allocation from heap, instead of reducing buffer size
v1: http://patches.dpdk.org/patch/69158/
---
 drivers/common/mlx5/mlx5_nl.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_nl.c b/drivers/common/mlx5/mlx5_nl.c
index 65efcd3..1a1033a 100644
--- a/drivers/common/mlx5/mlx5_nl.c
+++ b/drivers/common/mlx5/mlx5_nl.c
@@ -330,10 +330,10 @@ struct mlx5_nl_ifindex_data {
 	     void *arg)
 {
 	struct sockaddr_nl sa;
-	char buf[MLX5_RECV_BUF_SIZE];
+	void *buf = malloc(MLX5_RECV_BUF_SIZE);
 	struct iovec iov = {
 		.iov_base = buf,
-		.iov_len = sizeof(buf),
+		.iov_len = MLX5_RECV_BUF_SIZE,
 	};
 	struct msghdr msg = {
 		.msg_name = &sa,
@@ -345,6 +345,10 @@ struct mlx5_nl_ifindex_data {
 	int multipart = 0;
 	int ret = 0;
 
+	if (!buf) {
+		rte_errno = ENOMEM;
+		return -rte_errno;
+	}
 	do {
 		struct nlmsghdr *nh;
 		int recv_bytes = 0;
@@ -353,7 +357,8 @@ struct mlx5_nl_ifindex_data {
 			recv_bytes = recvmsg(nlsk_fd, &msg, 0);
 			if (recv_bytes == -1) {
 				rte_errno = errno;
-				return -rte_errno;
+				ret = -rte_errno;
+				goto exit;
 			}
 			nh = (struct nlmsghdr *)buf;
 		} while (nh->nlmsg_seq != sn);
@@ -365,24 +370,30 @@ struct mlx5_nl_ifindex_data {
 
 				if (err_data->error < 0) {
 					rte_errno = -err_data->error;
-					return -rte_errno;
+					ret = -rte_errno;
+					goto exit;
 				}
 				/* Ack message. */
-				return 0;
+				ret = 0;
+				goto exit;
 			}
 			/* Multi-part msgs and their trailing DONE message. */
 			if (nh->nlmsg_flags & NLM_F_MULTI) {
-				if (nh->nlmsg_type == NLMSG_DONE)
-					return 0;
+				if (nh->nlmsg_type == NLMSG_DONE) {
+					ret =  0;
+					goto exit;
+				}
 				multipart = 1;
 			}
 			if (cb) {
 				ret = cb(nh, arg);
 				if (ret < 0)
-					return ret;
+					goto exit;
 			}
 		}
 	} while (multipart);
+exit:
+	free(buf);
 	return ret;
 }
 
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack

[Matan Azrad]

From: Viacheslav Ovsiienko
> The buffer size to receive netlink reply messages is relatively large (32K), and
> it is allocated on the stack and it might break in application is using smaller
> per-thread stacks.
> This patch allocates temporary buffer from heap.
> 
> Fixes: ccdcba53a3f4 ("net/mlx5: use Netlink to add/remove MAC addresses")
> Cc: nelio.laranjeiro@6wind.com
> Cc: stable@dpdk.org
> 
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>

Re: [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Sent: Thursday, May 14, 2020 10:11 AM
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; stephen@networkplumber.org; Nélio Laranjeiro
> <nelio.laranjeiro@6wind.com>; stable@dpdk.org
> Subject: [PATCH v2] common/mlx5: fix netlink buffer allocation from stack
> 
> The buffer size to receive netlink reply messages is relatively
> large (32K), and it is allocated on the stack and it might
> break in application is using smaller per-thread stacks.
> This patch allocates temporary buffer from heap.
> 
> Fixes: ccdcba53a3f4 ("net/mlx5: use Netlink to add/remove MAC addresses")
> Cc: nelio.laranjeiro@6wind.com
> Cc: stable@dpdk.org
> 
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> ---
> v2: allocation from heap, instead of reducing buffer size
> v1:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> es.dpdk.org%2Fpatch%2F69158%2F&amp;data=02%7C01%7Crasland%40mell
> anox.com%7C1e38e272cede42d2cfc008d7f7d60e00%7Ca652971c7d2e4d9ba6
> a4d149256f461b%7C0%7C0%7C637250371034928200&amp;sdata=cZ1hFsnSZz
> 5oMj8zOLbB6vmcthY1FnPZWa%2FgoNDibmg%3D&amp;reserved=0
> ---
>  drivers/common/mlx5/mlx5_nl.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_nl.c
> b/drivers/common/mlx5/mlx5_nl.c
> index 65efcd3..1a1033a 100644
> --- a/drivers/common/mlx5/mlx5_nl.c
> +++ b/drivers/common/mlx5/mlx5_nl.c
> @@ -330,10 +330,10 @@ struct mlx5_nl_ifindex_data {
>  	     void *arg)
>  {
>  	struct sockaddr_nl sa;
> -	char buf[MLX5_RECV_BUF_SIZE];
> +	void *buf = malloc(MLX5_RECV_BUF_SIZE);
>  	struct iovec iov = {
>  		.iov_base = buf,
> -		.iov_len = sizeof(buf),
> +		.iov_len = MLX5_RECV_BUF_SIZE,
>  	};
>  	struct msghdr msg = {
>  		.msg_name = &sa,
> @@ -345,6 +345,10 @@ struct mlx5_nl_ifindex_data {
>  	int multipart = 0;
>  	int ret = 0;
> 
> +	if (!buf) {
> +		rte_errno = ENOMEM;
> +		return -rte_errno;
> +	}
>  	do {
>  		struct nlmsghdr *nh;
>  		int recv_bytes = 0;
> @@ -353,7 +357,8 @@ struct mlx5_nl_ifindex_data {
>  			recv_bytes = recvmsg(nlsk_fd, &msg, 0);
>  			if (recv_bytes == -1) {
>  				rte_errno = errno;
> -				return -rte_errno;
> +				ret = -rte_errno;
> +				goto exit;
>  			}
>  			nh = (struct nlmsghdr *)buf;
>  		} while (nh->nlmsg_seq != sn);
> @@ -365,24 +370,30 @@ struct mlx5_nl_ifindex_data {
> 
>  				if (err_data->error < 0) {
>  					rte_errno = -err_data->error;
> -					return -rte_errno;
> +					ret = -rte_errno;
> +					goto exit;
>  				}
>  				/* Ack message. */
> -				return 0;
> +				ret = 0;
> +				goto exit;
>  			}
>  			/* Multi-part msgs and their trailing DONE message.
> */
>  			if (nh->nlmsg_flags & NLM_F_MULTI) {
> -				if (nh->nlmsg_type == NLMSG_DONE)
> -					return 0;
> +				if (nh->nlmsg_type == NLMSG_DONE) {
> +					ret =  0;
> +					goto exit;
> +				}
>  				multipart = 1;
>  			}
>  			if (cb) {
>  				ret = cb(nh, arg);
>  				if (ret < 0)
> -					return ret;
> +					goto exit;
>  			}
>  		}
>  	} while (multipart);
> +exit:
> +	free(buf);
>  	return ret;
>  }
> 
> --
> 1.8.3.1


Patch applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh