netvsc fix buffer size issues
Subsystems (3)
cover netvsc fix buffer size issues
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(-)
Signature · 1 line
2.11.0
PATCH 1/3 hv_netvsc: Correct the max receive buffer size
From: Haiyang Zhang <[email protected]>
It should be 31 MB on recent host versions.
--- drivers/net/hyperv/hyperv_net.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/net/hyperv/hyperv_net.h +4 -2
@@ -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
Signature · 1 line
2.11.0
Re: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size
Quoted 6 lines · Stephen Hemminger
From: Haiyang Zhang <[email protected]>
It should be 31 MB on recent host versions.
Signed-off-by Haiyang Zhang <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>
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
David Miller
maintainerRe: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size
From: Dan Carpenter <[email protected]>
Date: Fri, 8 Dec 2017 13:33:25 +0300
Quoted 17 lines · Stephen Hemminger
Quoted 6 lines
From: Haiyang Zhang <[email protected]>
It should be 31 MB on recent host versions.
Signed-off-by Haiyang Zhang <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>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.
Quoted 4 lines
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.
Haiyang Zhang
maintainerRE: [PATCH net 1/3] hv_netvsc: Correct the max receive buffer size
In reply to Dan CarpenterQuoted 29 lines
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Friday, December 8, 2017 5:33 AM
To: Stephen Hemminger <[email protected]>
Cc: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH net 1/3]hv_netvsc: Correct the max receive buffer sizeQuoted 6 lines · Stephen Hemminger
From: Haiyang Zhang <[email protected]>
It should be 31 MB on recent host versions.
Signed-off-by Haiyang Zhang <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>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.
Quoted 4 lines
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 2/3 hv_netvsc: Limit the receive buffer size for legacy hosts
In reply to Stephen HemmingerFrom: Haiyang Zhang <[email protected]>
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().
--- drivers/net/hyperv/netvsc.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/net/hyperv/netvsc.c +5 -0
@@ -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,
Signature · 1 line
2.11.0
PATCH 3/3 hv_netvsc: Fix the default receive buffer size
In reply to Stephen HemmingerFrom: Haiyang Zhang <[email protected]>
The intended size is 16 MB, and the default slot size is 1728. So, NETVSC_DEFAULT_RX should be 16*1024*1024 / 1728 = 9709.
--- drivers/net/hyperv/netvsc_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/hyperv/netvsc_drv.c +1 -1
@@ -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)
Signature · 1 line
2.11.0
Re: [PATCH net 3/3] hv_netvsc: Fix the default receive buffer size
Quoted 17 lines · Stephen Hemminger
From: Haiyang Zhang <[email protected]>
The intended size is 16 MB, and the default slot size is 1728. So,
NETVSC_DEFAULT_RXshould be 16*1024*1024 / 1728 = 9709.Fixes 5023a6db73196 ("netvsc: increase default receive buffer size")Signed-off-by Haiyang Zhang <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>--- drivers/net/hyperv/netvsc_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/net/hyperv/netvsc_drv.c +0
-0
drivers/net/hyperv/netvsc_drv.c line 53
Quoted 5 lines
drivers/net/hyperv/netvsc_drv.c +1
-1@@ -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
Haiyang Zhang
maintainerRE: [PATCH net 3/3] hv_netvsc: Fix the default receive buffer size
Quoted 45 lines
-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Friday, December 8, 2017 5:45 AM
To: Stephen Hemminger <[email protected]>
Cc: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; [email protected]; [email protected]
Subject: Re: [PATCH net 3/3]hv_netvsc: Fix the default receive buffer sizeQuoted 18 lines · Stephen Hemminger
From: Haiyang Zhang <[email protected]>
The intended size is 16 MB, and the default slot size is 1728. So,
NETVSC_DEFAULT_RXshould be 16*1024*1024 / 1728 = 9709.Fixes 5023a6db73196 ("netvsc: increase default receive buffer size")Signed-off-by Haiyang Zhang <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>--- drivers/net/hyperv/netvsc_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)(quoted diff) +0
-0
b/drivers/net/hyperv/netvsc_drv.cindexdc70de674ca9..edfcde5d3621100644drivers/net/hyperv/netvsc_drv.c line 53
Quoted 5 lines
drivers/net/hyperv/netvsc_drv.c +1
-1@@ -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.cand the max sizes are stored inhyperv_net.h. Could we move these tohyperv_net.h? We could write it like:
#defineNETVSC_DEFAULT_RX((16 * 1024 * 1024) /NETVSC_RECV_SECTION_SIZE)
Quoted 3 lines
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