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 
> 
> 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 
> Signed-off-by: Stephen Hemminger 
> ---
>  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_TX192 /* ~1M */
>  #define NETVSC_MIN_RX_SECTIONS   10  /* ~64K */
> -#define NETVSC_DEFAULT_RX10485   /* Max ~16M */
> +#define NETVSC_DEFAULT_RX9709/* ~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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Haiyang Zhang]

> -Original Message-
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Friday, December 8, 2017 5:45 AM
> To: Stephen Hemminger 
> Cc: KY Srinivasan ; Haiyang Zhang
> ; Stephen Hemminger
> ; de...@linuxdriverproject.org;
> net...@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 
> >
> > 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 
> > Signed-off-by: Stephen Hemminger 
> > ---
> >  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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel