RFC: [PATCH 1/3] usb: cdc_ncm: patch for VMware

This patch limits the Rx URB size to 16kB if the driver is compiled for a
VMware environment. As of workstation 9, there are some major performance
problems if the Rx URB size exceeds that limit.

This patch applies to longterm kernel version 3.4.35.

Signed-Off-By: Loic Domaigne <loic.domaigne@jambit.com> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, 2013-03-08 at 22:03 +0100, Loic Domaigne wrote:
> This patch limits the Rx URB size to 16kB if the driver is compiled for a
> VMware environment. As of workstation 9, there are some major performance
> problems if the Rx URB size exceeds that limit.
> 
> This patch applies to longterm kernel version 3.4.35.
> 
> Signed-Off-By: Loic Domaigne <loic.domaigne@jambit.com> 
> 
> --- linux-3.4.35/drivers/net/usb/cdc_ncm.c.orig	2013-03-05 09:36:02.858740489 +0100
> +++ linux-3.4.35/drivers/net/usb/cdc_ncm.c	2013-03-05 10:20:18.092588668 +0100
> @@ -80,6 +80,23 @@
>  #define	CDC_NCM_TIMER_PENDING_CNT		2
>  #define CDC_NCM_TIMER_INTERVAL			(400UL * NSEC_PER_USEC)
>  
> +/* maximum Rx URB size */
> +/*
> + * in the original Linux driver, the rx urb size can be up to
> + * CDC_NCM_NTB_MAX_SIZE_RX.
> + *
> + * Under VMware (as of wks9), URB size greater than 16kB is a problem,
> + * so simply adjust this define when the driver is compiled for a VMware
> + * environment.
> + *
> + */
> +#ifdef VMWARE_BUG
> +#warning "Compiling for VMware"
> +#define CDC_NCM_MAX_RX_URB_SIZE     16384
> +#else
> +#define CDC_NCM_MAX_RX_URB_SIZE     CDC_NCM_NTB_MAX_SIZE_RX
> +#endif

I can't see how that is going to get past any sort of review.  Either
there's some other way of detecting that the CPU is the VMWare emulated
one or you're stuck with the bug until VMWare fixes it.

Dan

> +
>  /* The following macro defines the minimum header space */
>  #define	CDC_NCM_MIN_HDR_SIZE \
>  	(sizeof(struct usb_cdc_ncm_nth16) + sizeof(struct usb_cdc_ncm_ndp16) + \
> @@ -589,6 +606,9 @@ advance:
>  		ctx->out_ep->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK);
>  	dev->status = ctx->status_ep;
>  	dev->rx_urb_size = ctx->rx_max;
> +	if (dev->rx_urb_size > CDC_NCM_MAX_RX_URB_SIZE)
> +		dev->rx_urb_size = CDC_NCM_MAX_RX_URB_SIZE;
> +	pr_debug("dev->rx_urb_size = %zu", dev->rx_urb_size);
>  
>  	/*
>  	 * We should get an event when network connection is "connected" or
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Mar 08, 2013 at 04:28:59PM -0600, Dan Williams wrote:
> On Fri, 2013-03-08 at 22:03 +0100, Loic Domaigne wrote:
> >  
> > +/* maximum Rx URB size */
> > +/*
> > + * in the original Linux driver, the rx urb size can be up to
> > + * CDC_NCM_NTB_MAX_SIZE_RX.
> > + *
> > + * Under VMware (as of wks9), URB size greater than 16kB is a problem,
> > + * so simply adjust this define when the driver is compiled for a VMware
> > + * environment.
> > + *
> > + */
> > +#ifdef VMWARE_BUG
> > +#warning "Compiling for VMware"
> > +#define CDC_NCM_MAX_RX_URB_SIZE     16384
> > +#else
> > +#define CDC_NCM_MAX_RX_URB_SIZE     CDC_NCM_NTB_MAX_SIZE_RX
> > +#endif
> 
> I can't see how that is going to get past any sort of review.  Either
> there's some other way of detecting that the CPU is the VMWare emulated
> one or you're stuck with the bug until VMWare fixes it.

Yeah, I know.

The kludge consists to (re)compile the kernel module on the VMWare guest with
the VMWARE_BUG compiler flag set. 

We have a helper script for that task, but it's distros specific. We can 
detect automatically a VMWare emulated CPU in some cases, but not always.
As a result, we end up sometimes asking the user.

I am aware that it's not suitable as a generic solution. But waiting a fix
from VMWare might not be practical for you either.

Any better ideas?

Loic.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sun, 2013-03-10 at 22:12 +0100, Loic Domaigne wrote:
> On Fri, Mar 08, 2013 at 04:28:59PM -0600, Dan Williams wrote:
> > On Fri, 2013-03-08 at 22:03 +0100, Loic Domaigne wrote:
> > >  
> > > +/* maximum Rx URB size */
> > > +/*
> > > + * in the original Linux driver, the rx urb size can be up to
> > > + * CDC_NCM_NTB_MAX_SIZE_RX.
> > > + *
> > > + * Under VMware (as of wks9), URB size greater than 16kB is a problem,
> > > + * so simply adjust this define when the driver is compiled for a VMware
> > > + * environment.
> > > + *
> > > + */
> > > +#ifdef VMWARE_BUG
> > > +#warning "Compiling for VMware"
> > > +#define CDC_NCM_MAX_RX_URB_SIZE     16384
> > > +#else
> > > +#define CDC_NCM_MAX_RX_URB_SIZE     CDC_NCM_NTB_MAX_SIZE_RX
> > > +#endif
> > 
> > I can't see how that is going to get past any sort of review.  Either
> > there's some other way of detecting that the CPU is the VMWare emulated
> > one or you're stuck with the bug until VMWare fixes it.
> 
> Yeah, I know.
> 
> The kludge consists to (re)compile the kernel module on the VMWare guest with
> the VMWARE_BUG compiler flag set. 
> 
> We have a helper script for that task, but it's distros specific. We can 
> detect automatically a VMWare emulated CPU in some cases, but not always.
> As a result, we end up sometimes asking the user.
> 
> I am aware that it's not suitable as a generic solution. But waiting a fix
> from VMWare might not be practical for you either.
> 
> Any better ideas?

Example from drivers/misc/vmw_balloon.c:

#include <asm/hypervisor.h>
...
	/*
	 * Check if we are running on VMware's hypervisor and bail out
	 * if we are not.
	 */
	if (x86_hyper != &x86_hyper_vmware)
		return -ENODEV;

Obviously for a non-x86-specific driver this needs to be conditional on
#ifdef CONFIG_X86.

Ben.