On Thu, Dec 10, 2020 at 06:06:26PM -0300, Martin Pieuchot wrote: > On 10/12/20(Thu) 21:40, Alexandre Ratchov wrote: > > On Thu, Dec 10, 2020 at 05:27:16PM +0100, Marcus Glocker 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. > > > > On SP kernels, interrupts are still working while the CPU is spinning > > in kernel mode (as long as current IPL permits it). That's why audio > > works better. > > > > > 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. > > > > > > > Would asmc(4) work if we sleep for very long (tsleep_nsec() has no > > upper bound)? Spinning during 50ms in kernel mode doesn't look right, > > so using tsleep() looks as a step forward as long as sleeping doesn't > > break asmc(4).
I did test this quickly, but it makes the asmc_wait() entirely fail. > Another way to look at the issue is: Can this delay be reduced? > > Considering that asmc_wait() is called twice per asmc_write() and once > per amsc_read() that means it is called at least 14 times in asmc_command() > and up to 20 times when reading fan informations, > > If any of the operations times out the whole command is restarted, > possibly twice. > > So it could be interesting to know if a particular query fail and is > restarted? In this case, is it fixable and/or why do we need to > restart the command 3 times? > > Or is the loop in asmc_wait() doing too big steps? Or too smalls which > would imply that some commands fail? > > Improving/fixing the delays might be easier than re-structuring the > driver and could fix both issues raised in this thread: holding the > KERNEL_LOCK() for too long and delaying other sensors. After doing some deeper analyzes in to asmc_wait() I agree to that. Something seems to go fundamental wrong there. In every asmc_update() execution, I can see asmc_wait() timeout 9 times, always on the ASMC_ACCEPT check. That means out of ~48ms to execute asmc_update(), we spend 45ms waiting for timeouts! When this can be fixed, I guess we should be fine with the remaining ~3ms to execute asmc_update(). I'll have a look.
