> From: "Theo de Raadt" <[email protected]> > Date: Thu, 10 Dec 2020 10:13:52 -0700 > > Marcus Glocker <[email protected]> wrote: > > > Hi All, > > > > I recently started to play around with uvideo(4) and uaudio(4) on my > > amd64 iMacs. There I quickly noticed regular freezes when streaming > > USB video or audio. On some of those machines it was very frequent, > > like every few seconds the video or audio stream did freeze for ~1s, > > then resume, while the rest of the system did continue to operate fine. > > > > First I found that when running the machine with an SP kernel, the issue > > disappears. Secondly some debugging hours, and quite some e-mail > > exchanges with mpi@ later, I found that the freeze is getting triggered > > by the asmc(4) driver, specifically by the sensor_task_register() > > update function. My first intention was to change > > sensor_task_register() to call taskq_create() with the TASKQ_MPSAFE > > flag for a test, to remove the kernel lock, which also resolved the > > freezing with an MP kernel. [1] > > > > In the end I found that the asmc(4) sensor update code is calling a > > busy loop in asmc_wait(), where the delay call is spending ~50ms in > > average. Doing that during the KERNEL_LOCK() is resulting in > > noticeable USB ISOC transfer delays. Obviously replacing the delay(9) > > with tsleep_nsec(9) in asmc(4) did fix the issue as well. [2] > > > > I'm not sure if just applying diff [2] to the driver is the right > > approach finally or if we need to take a more generic path to address > > this problem. Any feedback, help, comments appreciated. > > I've discussed with Marcus that there is a better way to solve these > drivers. > > They should install their own timeout or thread, which collects the > information. Then, when the sensor thread comes around it can instantly > copy the information.
Well, we deliberately chose not to do that since that would lead to a gazillion of kernel threads if you have lots of sensor drivers. Sensor drivers that are "good citizens" should probably continue to use the sensor thread. Butfor things like asmc(4) that handle a largish bundle of sensors, a separate thread would be fine. And I suspect that thread could run unlocked. > Of course, there should be no delay() in the information collector. > > In an ideal world, delay() whould only be used in low-IPL or > bootstrapping code. It depends a bit. Small delays (of the order of a few microseconds) are probably fine as the overhead of setting up a sleep will be large. Longer delays should be done using sleeps. The problem we have right now is that the granularity of sleeps is so high. Which drives us to using delay(9) in cases where we need to sleep for hundreds of microseconds.
