DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer
@ 2020-04-22 22:30 Stephen Hemminger
  2020-04-28  9:34 ` Slava Ovsiienko
  2020-05-14  7:11 ` [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack Viacheslav Ovsiienko
  0 siblings, 2 replies; 5+ messages in thread
From: Stephen Hemminger @ 2020-04-22 22:30 UTC (permalink / raw)
  To: dev; +Cc: Stephen Hemminger, nelio.laranjeiro

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer
  2020-04-22 22:30 [dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer Stephen Hemminger
@ 2020-04-28  9:34 ` Slava Ovsiienko
  2020-05-14  7:11 ` [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack Viacheslav Ovsiienko
  1 sibling, 0 replies; 5+ messages in thread
From: Slava Ovsiienko @ 2020-04-28  9:34 UTC (permalink / raw)
  To: Stephen Hemminger, dev; +Cc: Nélio Laranjeiro

> -----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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack
  2020-04-22 22:30 [dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer Stephen Hemminger
  2020-04-28  9:34 ` Slava Ovsiienko
@ 2020-05-14  7:11 ` Viacheslav Ovsiienko
  2020-05-17 12:02   ` Matan Azrad
  2020-05-17 12:51   ` Raslan Darawsheh
  1 sibling, 2 replies; 5+ messages in thread
From: Viacheslav Ovsiienko @ 2020-05-14  7:11 UTC (permalink / raw)
  To: dev; +Cc: matan, rasland, stephen, nelio.laranjeiro, stable

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack
  2020-05-14  7:11 ` [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack Viacheslav Ovsiienko
@ 2020-05-17 12:02   ` Matan Azrad
  2020-05-17 12:51   ` Raslan Darawsheh
  1 sibling, 0 replies; 5+ messages in thread
From: Matan Azrad @ 2020-05-17 12:02 UTC (permalink / raw)
  To: Slava Ovsiienko, dev
  Cc: Raslan Darawsheh, stephen, Nélio Laranjeiro, stable



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>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack
  2020-05-14  7:11 ` [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack Viacheslav Ovsiienko
  2020-05-17 12:02   ` Matan Azrad
@ 2020-05-17 12:51   ` Raslan Darawsheh
  1 sibling, 0 replies; 5+ messages in thread
From: Raslan Darawsheh @ 2020-05-17 12:51 UTC (permalink / raw)
  To: Slava Ovsiienko, dev; +Cc: Matan Azrad, stephen, Nélio Laranjeiro, stable

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-05-17 12:51 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-22 22:30 [dpdk-dev] [PATCH] common/mlx5: reduce size of netlink buffer Stephen Hemminger
2020-04-28  9:34 ` Slava Ovsiienko
2020-05-14  7:11 ` [dpdk-dev] [PATCH v2] common/mlx5: fix netlink buffer allocation from stack Viacheslav Ovsiienko
2020-05-17 12:02   ` Matan Azrad
2020-05-17 12:51   ` Raslan Darawsheh

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git