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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Thu, 23 Jun 2022, Vincent MAILHOL wrote:

> On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> > > On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> > > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > > > From: Vincent MAILHOL
> > > > > > Sent: 21 June 2022 16:56
> > > > > >
> > > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > > > > allocated length and urb::actual_length was what was actually being
> > > > > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > > > > a bit more and understand the real purpose of
> > > > > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > > > > which situation this can be different from the allocated length.
> > > > > > >
> > > > > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > > > > that was actually sent or actually received.
> > > > > > >
> > > > > > > Neither of these values has to be the same as the size of the buffer --
> > > > > > > but they better not be bigger!
> > > > > >
> > > > > > Thanks. Now things are a bit clearer.
> > > > > > I guess that for the outcoming URB what I proposed made no sense. For
> > > > > > incoming URB, I guess that most of the drivers want to set
> > > > > > urb::transfer_buffer once for all with the allocated size and never
> > > > > > touch it again.
> > > > > >
> > > > > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > > > > sense to keep it but with an additional check to trigger a dmesg
> > > > > > warning if this is used on an outcoming endpoint and with additional
> > > > > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > > > > the allocated size?
> > > > >
> > > > > IIRC urb are pretty big.
> > > >
> > > > What exactly do you mean by "pretty big" here?  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.  All USB bottlenecks
> > > > that I know of are either in the hardware layer, or in the protocol
> > > > layer itself (i.e. usb-storage protocol).
> > > >
> > > > > 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.
> > >
> > > Well, I wouldn't have proposed the patch if it only applied to a
> > > single driver. If we add a urb::allocated_transfer_size as suggested
> > > by David, I believe that the majority of the drivers using DMA memory
> > > will be able to rely on that URB_FREE_COHERENT flag for the garbage
> > > collection.
> > >
> > > The caveat, as you pointed before, is that the developper still needs
> > > to be aware of the limitations of DMA and that it should not be freed
> > > in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> > > functions that would lead to urb_destroy().
> > >
> > > > 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.
> > >
> > > The CAN protocol, in its latest revision, allows for transfer speed up
> > > to ~5Mbits. For low performance CPUs, this starts to be a significant
> > > load. Also, the CAN PDU being small (0 to 64 bytes), many small
> > > transfers occur.
> >
> > And is the memcpy the actual issue here?  Even tiny cpus can do large
> > and small memcopy very very very fast.
> >
> > > Unfortunately I did not do any benchmark myself so I won't be able to
> > > back my explanation with numbers.
> >
> > That might be the simplest solution here :)
>
> Yes, this would give a clear answer whether or not DMA was needed in
> the first place. But I do not own that gs_usb device to do the
> benchmark myself (and to be honest I do not have time to dedicate for
> this at the moment, maybe I will do it later on some other devices).
>
> Has anyone from the linux-can mailing list ever done such a benchmark?
> Else, is there anyone who would like to volunteer?

I have access to a couple of gs_usb devices but I am afraid I have no
experience performing this sort of benchmarking and also would have to
squeeze it in as a weekend project or something similar.  That said, if
someone's willing to help step me through it, I can see if it's feasible
for me to do.

That said, the gs_usb driver is mostly following along a very well
established pattern for writing USB CAN devices.  Both the pattern
followed that created the memory leak, as well as the pattern I followed
to resolve the memory leak, were also seen in the esd2 USB CAN driver as
well, and likely others are following suit.  So, I don't know that we'd
need to keep it specific to gs_usb to gain good information here.

Best,
Rhett



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux