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.

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?  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.

Best,
Rhett