On Tue. 28 Jun 2022 at 04:37, Rhett Aultman <rhett.aultman@xxxxxxxxxxx> wrote: > On Sun, 26 Jun 2022, Vincent MAILHOL wrote: > > On Thu. 23 Jun 2022 at 03:13, Rhett Aultman <rhett.aultman@xxxxxxxxxxx> 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
