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

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

On Tue. 28 Jun 2022 at 04:37, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> On Sun, 26 Jun 2022, Vincent MAILHOL wrote:
> > On Thu. 23 Jun 2022 at 03:13, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> > > On Thu, 23 Jun 2022, Vincent MAILHOL wrote:
> > > > On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
> > > > 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.
> >
> > I can throw a few hints which might be helpful.
> >
> > First, you should obviously prepare two versions of the gs_usb driver:
> > one using usb_alloc_coherent() (the current one), the other using
> > kmalloc() and compare the two.
> >
> > Right now, I can think of two relevant benchmarks: transmission
> > latency and CPU load.
> >
> > For the transmission latency, I posted one on my tools:
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dcan_20220626075317.746535-2D1-2Dmailhol.vincent-40wanadoo.fr_T_-23u&d=DwIFaQ&c=5cz3ZESzsFPW6Kn30oD8Yg&r=yZeJccB4JMhCRfLQXCMV_s56v3-BAi0tMrD3qzCwGTk&m=E5qqM5zYANpQqfZ0c8AHYrd-lkJZsS6-u-Jj2iTfHIjLle6JxCMRuTlmC_3bH8oA&s=sqvGqOvbtLqlZGMC-9q6gY1nF3203MT7gJIIqbKEXUM&e=
> >
> > For the CPU load, I suggest to put the bus on full load, for example using:
> > | cangen -g0 -p1 can0
> > (you might also want to play with other parameters such as the length using -L)
> > Then use an existing tool to get the CPU load figures. I don't know
> > for sure which tool is a good one to benchmark CPU usage in kernel
> > land so you will have to research that part. If anyone has a
> > suggestion…
> >
> > > 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.
> >
> > Yes, I looked at the log, the very first CAN USB driver is ems_usb and
> > was using DMA memory from the beginning. From that point on, nearly
> > all the drivers copied the trend (the only exception I am aware of is
> > peak_usb).
> >
> > I agree that the scope is wider than the gs_can (thus my proposal to
> > fix it at API level).
>
> (removed the USB mailing list since this is CAN driver related
> specifically)
>
> I appreciate these pointers and I can look into making the time for this.
> As I mentioned, I do have a gs_usb device (a Canable using the Candlelight
> firmware) which can help shed some light on this question.  I do
> understand the ideas being expressed in these pointers.  I do want to
> bring up some practical matters around it.
>
> First, it seems there's a pretty strong set of permutations to consider,
> given that this memory allocation scheme is common to so many drivers.  I
> only have a gs_usb device (a Canable using its CandleLight firmware).  I
> also cannot rule out the possibility that the underlying hardware of the
> host matters here.  For example, I discovered this leak in the first place
> because I work with a specific ARM platform where it's easy to exhaust the
> DMA memory.
>
> Secondly, this sort of benchmarking work will require lab setup time and
> my locating adequate free time in which to do it.  This isn't exactly
> labor covered under the original mandate from my employer, so I'm going to
> have to figure out how to work it in.

There is no rush. If this is interesting for you, go ahead, but I
won’t blame you if you prefer to give up for lack of time or
motivation.

> In light of this, while I remain committed to helping work the problem, I
> can't help but wonder if it's worth it to consider my original patch in a
> new light?

Yes, it makes sense to take your initial patch. I will reiterate that
I do not like the way it is done but you are fixing a memory leak and
delaying the fix furthermore is not good.

I am curious to see the benchmark results but at the same time, I do
not want to force anyone to do it.

If Marc agrees, I think we should just take your initial patch as is.
And later we can reconsider those two options:
  * apply the URB_FREE_COHERENT flag if the flag gets accepted (not
sure anymore that would be the case).
  * change from DMA memory to normal kmalloc()ed memory depending on
the benchmark result

Personally, I will try to push a bit more for the inclusion of the
URB_FREE_COHERENT flag.

> The code is less elegant than it otherwise could be, but it's
> consistent with practices found in the other drivers and it does resolve
> the original issue of leaking DMA memory.  I'd hate to see a long-standing
> issue continue to languish because I struggle to find adequate time to
> devote to the benchmarking needed to reach a decision about the USB API
> changes we've proposed.


Yours sincerely,
Vincent Mailhol