RE: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

From: David Laight <David.Laight@ACULAB.COM>
To: 'Greg Kroah-Hartman' <gregkh@linuxfoundation.org>
Cc: 'Vincent MAILHOL' <mailhol.vincent@wanadoo.fr>,
	Alan Stern <stern@rowland.harvard.edu>,
	Marc Kleine-Budde <mkl@pengutronix.de>,
	"Rhett Aultman" <rhett.aultman@samsara.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	linux-can <linux-can@vger.kernel.org>,
	"Oliver Neukum" <oneukum@suse.com>
Subject: RE: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT
Date: Wed, 22 Jun 2022 10:03:07 +0000	[thread overview]
Message-ID: <186561fa929f459985ad2986936988e5@AcuMS.aculab.com> (raw)
In-Reply-To: <YrLjsAOlsizMc/1c@kroah.com>

From: Greg Kroah-Hartman
> Sent: 22 June 2022 10:41
...
> > IIRC urb are pretty big.
> 
> What exactly do you mean by "pretty big" here?

Maybe 100-200 bytes?

> And what is wrong with
> that, I have never seen any issues with the current size of that
> structure in any benchmark or performance results.

Nothing really, the cost of allocating a sub-page structure
is pretty much independent of its size.
What I really meant is it isn't (say) 32 bytes where adding
another 4 could be a significant increase.

> All USB bottlenecks
> that I know of are either in the hardware layer, or in the protocol
> layer itself (i.e. usb-storage protocol).

There is a bufferbloat issue for usbnet on XHCI.
It would be better to fill the ring with (probably) 4k buffers
and have something sort out the USB frames from the buffers
and then get the network driver to use (IIRC) build_skb
to generate the skb from the data fragments.

I was only using 100M last time I was testing that and
didn't get performance issues.
Just problems with the USB connections 'bouncing', that
project got canned for other reasons ...

But at higher speeds and high network use it might all
be a problem.

> 
> > You'd be unlucky if adding an extra field to hold the allocated
> > size would ever need more memory.
> > So it might just be worth saving the allocated size.
> 
> Maybe, yes, then we could transition to allocating the urb and buffer at
> the same time like we do partially for iso streams in an urb.  But that
> still might be overkill for just this one driver.  I'm curious as to why
> a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> its buffers in the first place.

Yes, being able to do short transfers from buffer space in the urb
would save all the issues about having to allocate an extra
buffer to avoid DMA from stack.

Indeed for XHCI there is a bit that allows 8 bytes of data to
replace the pointer in the ring structure itself.
I don't remember the driver every doing that.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)