[PATCH net 0/3] netvsc fix buffer size issues

[PATCH net 0/3] netvsc fix buffer size issues

[Stephen Hemminger]

The changes to allow setting buffer size can cause issues
on older versions of Windows Server which have smaller limits.
And the actual maximum value for WS2016 is 31M not 16M.

This is a resend of patchset that didn't make it to
netdev correctly.

Haiyang Zhang (3):
  hv_netvsc: Correct the max receive buffer size
  hv_netvsc: Limit the receive buffer size for legacy hosts
  hv_netvsc: Fix the default receive buffer size

 drivers/net/hyperv/hyperv_net.h | 6 ++++--
 drivers/net/hyperv/netvsc.c     | 5 +++++
 drivers/net/hyperv/netvsc_drv.c | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.11.0

[PATCH net 1/3] hv_netvsc: Correct the max receive buffer size

[Stephen Hemminger]

From: Haiyang Zhang <haiyangz@microsoft.com>

It should be 31 MB on recent host versions.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/hyperv_net.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 3d940c67ea94..373455f216ce 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -637,9 +637,11 @@ struct nvsp_message {
 #define NETVSC_MTU 65535
 #define NETVSC_MTU_MIN ETH_MIN_MTU
 
-#define NETVSC_RECEIVE_BUFFER_SIZE		(1024*1024*16)	/* 16MB */
-#define NETVSC_RECEIVE_BUFFER_SIZE_LEGACY	(1024*1024*15)  /* 15MB */
+/* Max buffer sizes allowed by a host */
+#define NETVSC_RECEIVE_BUFFER_SIZE		(1024 * 1024 * 31) /* 31MB */
+#define NETVSC_RECEIVE_BUFFER_SIZE_LEGACY	(1024 * 1024 * 15) /* 15MB */
 #define NETVSC_SEND_BUFFER_SIZE			(1024 * 1024 * 15)   /* 15MB */
+
 #define NETVSC_INVALID_INDEX			-1
 
 #define NETVSC_SEND_SECTION_SIZE		6144
-- 
2.11.0

Re: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size

[Dan Carpenter]

On Thu, Dec 07, 2017 at 04:10:53PM -0800, Stephen Hemminger wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> It should be 31 MB on recent host versions.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>

This is very vague.  What does "recent" mean in this context?  There are
also some unrelated white space changes here which make the patch harder
to read.

This patch kind of makes the bug fixed by patch 2 even worse because
before the receive buffer was capped at around 16MB and now we can set
the receive buffer to 31MB.  It might make sense to fold the two
patches together.

Is patch 2 a memory corruption bug?  The changelog doesn't really say
what the user visible effects of the bug are.  Basically if you make the
buffer too small then it's a performance issue but if you make it too
large what happens?  It's not clear to me.

regards,
dan carpenter

Re: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Fri, 8 Dec 2017 13:33:25 +0300

> On Thu, Dec 07, 2017 at 04:10:53PM -0800, Stephen Hemminger wrote:
>> From: Haiyang Zhang <haiyangz@microsoft.com>
>> 
>> It should be 31 MB on recent host versions.
>> 
>> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> 
> This is very vague.  What does "recent" mean in this context?  There are
> also some unrelated white space changes here which make the patch harder
> to read.
> 
> This patch kind of makes the bug fixed by patch 2 even worse because
> before the receive buffer was capped at around 16MB and now we can set
> the receive buffer to 31MB.  It might make sense to fold the two
> patches together.
> 
> Is patch 2 a memory corruption bug?  The changelog doesn't really say
> what the user visible effects of the bug are.  Basically if you make the
> buffer too small then it's a performance issue but if you make it too
> large what happens?  It's not clear to me.

Agreed with Dan, we definitely need more verbose and detailed commit
log messages for this series.

RE: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size

[Haiyang Zhang]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, December 8, 2017 5:33 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; devel@linuxdriverproject.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size
> 
> On Thu, Dec 07, 2017 at 04:10:53PM -0800, Stephen Hemminger wrote:
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > It should be 31 MB on recent host versions.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> 
> This is very vague.  What does "recent" mean in this context?  There are also
> some unrelated white space changes here which make the patch harder to
> read.
> 
> This patch kind of makes the bug fixed by patch 2 even worse because
> before the receive buffer was capped at around 16MB and now we can set
> the receive buffer to 31MB.  It might make sense to fold the two patches
> together.
> 
> Is patch 2 a memory corruption bug?  The changelog doesn't really say what
> the user visible effects of the bug are.  Basically if you make the buffer too
> small then it's a performance issue but if you make it too large what happens?
> It's not clear to me.

For NVSP v2, and earlier host, the limit is 15MB. Later hosts, the limit is 31MB.
Setting beyond it will be denied by the host, resulting the vNIC doesn't come up.

I will merge this one together with the patch 2.

Thanks,
- Haiyang

[PATCH net 2/3] hv_netvsc: Limit the receive buffer size for legacy hosts

[Stephen Hemminger]

From: Haiyang Zhang <haiyangz@microsoft.com>

Legacy hosts only allow 15 MB receive buffer, and we know the
NVSP version only after negotiation. So, we put the limit in
netvsc_init_buf().

Fixes: 5023a6db73196 ("netvsc: increase default receive buffer size")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index e4bcd202a56a..e5d16a8cf0d6 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -268,6 +268,11 @@ static int netvsc_init_buf(struct hv_device *device,
 	buf_size = device_info->recv_sections * device_info->recv_section_size;
 	buf_size = roundup(buf_size, PAGE_SIZE);
 
+	/* Legacy hosts only allow smaller receive buffer */
+	if (net_device->nvsp_version <= NVSP_PROTOCOL_VERSION_2)
+		buf_size = min_t(unsigned int, buf_size,
+				 NETVSC_RECEIVE_BUFFER_SIZE_LEGACY);
+
 	net_device->recv_buf = vzalloc(buf_size);
 	if (!net_device->recv_buf) {
 		netdev_err(ndev,
-- 
2.11.0

[PATCH net 3/3] hv_netvsc: Fix the default receive buffer size

[Stephen Hemminger]

From: Haiyang Zhang <haiyangz@microsoft.com>

The intended size is 16 MB, and the default slot size is 1728.
So, NETVSC_DEFAULT_RX should be 16*1024*1024 / 1728 = 9709.

Fixes: 5023a6db73196 ("netvsc: increase default receive buffer size")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index dc70de674ca9..edfcde5d3621 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -50,7 +50,7 @@
 #define NETVSC_MIN_TX_SECTIONS	10
 #define NETVSC_DEFAULT_TX	192	/* ~1M */
 #define NETVSC_MIN_RX_SECTIONS	10	/* ~64K */
-#define NETVSC_DEFAULT_RX	10485   /* Max ~16M */
+#define NETVSC_DEFAULT_RX	9709    /* ~16M */
 
 #define LINKCHANGE_INT (2 * HZ)
 #define VF_TAKEOVER_INT (HZ / 10)
-- 
2.11.0

Re: [PATCH net 3/3] hv_netvsc: Fix the default receive buffer size

[Dan Carpenter]

On Thu, Dec 07, 2017 at 04:10:55PM -0800, Stephen Hemminger wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> The intended size is 16 MB, and the default slot size is 1728.
> So, NETVSC_DEFAULT_RX should be 16*1024*1024 / 1728 = 9709.
> 
> Fixes: 5023a6db73196 ("netvsc: increase default receive buffer size")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  drivers/net/hyperv/netvsc_drv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index dc70de674ca9..edfcde5d3621 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -50,7 +50,7 @@
>  #define NETVSC_MIN_TX_SECTIONS	10
>  #define NETVSC_DEFAULT_TX	192	/* ~1M */
>  #define NETVSC_MIN_RX_SECTIONS	10	/* ~64K */
> -#define NETVSC_DEFAULT_RX	10485   /* Max ~16M */
> +#define NETVSC_DEFAULT_RX	9709    /* ~16M */

How does this bug look like to the user?  Memory corruption?

It's weird to me reviewing this code that the default sizes are
stored in netvsc_drv.c and the max sizes are stored in hyperv_net.h.
Could we move these to hyperv_net.h?  We could write it like:
#define NETVSC_DEFAULT_RX ((16 * 1024 * 1024) / NETVSC_RECV_SECTION_SIZE)

16MB is sort of a weird default because it's larger than the 15MB
allowed for legacy versions, but it's smaller than the 32MB you'd want
for the current versions.

regards,
dan carpenter

RE: [PATCH net 3/3] hv_netvsc: Fix the default receive buffer size

[Haiyang Zhang]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, December 8, 2017 5:45 AM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; devel@linuxdriverproject.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH net 3/3] hv_netvsc: Fix the default receive buffer size
> 
> On Thu, Dec 07, 2017 at 04:10:55PM -0800, Stephen Hemminger wrote:
> > From: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > The intended size is 16 MB, and the default slot size is 1728.
> > So, NETVSC_DEFAULT_RX should be 16*1024*1024 / 1728 = 9709.
> >
> > Fixes: 5023a6db73196 ("netvsc: increase default receive buffer size")
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > ---
> >  drivers/net/hyperv/netvsc_drv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/hyperv/netvsc_drv.c
> > b/drivers/net/hyperv/netvsc_drv.c index dc70de674ca9..edfcde5d3621
> > 100644
> > --- a/drivers/net/hyperv/netvsc_drv.c
> > +++ b/drivers/net/hyperv/netvsc_drv.c
> > @@ -50,7 +50,7 @@
> >  #define NETVSC_MIN_TX_SECTIONS	10
> >  #define NETVSC_DEFAULT_TX	192	/* ~1M */
> >  #define NETVSC_MIN_RX_SECTIONS	10	/* ~64K */
> > -#define NETVSC_DEFAULT_RX	10485   /* Max ~16M */
> > +#define NETVSC_DEFAULT_RX	9709    /* ~16M */
> 
> How does this bug look like to the user?  Memory corruption?
> 
> It's weird to me reviewing this code that the default sizes are stored in
> netvsc_drv.c and the max sizes are stored in hyperv_net.h.
> Could we move these to hyperv_net.h?  We could write it like:
> #define NETVSC_DEFAULT_RX ((16 * 1024 * 1024) /
> NETVSC_RECV_SECTION_SIZE)
> 
> 16MB is sort of a weird default because it's larger than the 15MB allowed for
> legacy versions, but it's smaller than the 32MB you'd want for the current
> versions.

As a tradeoff between traffic burst and latency, we set the default to be 16MB.
And this default is reduced automatically for legacy hosts based on the NVSP
version in patch 2. 

I will move the change to hyperv_net.h

Thanks,
- Haiyang