rtsx_usb_ms: Use msleep_interruptible() in polling loop
PATCH RESEND 1/1 rtsx_usb_ms: Use msleep_interruptible() in polling loop
This patch has already been posted to LKML by Ben Hutchings ~6 months ago, but AFAIK no further action were performed. However, this patch really fixes weird loadavg with RTS5129 card reader, so I would wonder if this could be merged. AFAIK, it has been applied to some distros' kernels, e.g., Ubuntu.
Original Ben's message goes below.
rtsx_usb_ms creates a task that mostly sleeps, but tasks in uninterruptible sleep still contribute to the load average (for bug-compatibility with Unix). A load average of ~1 on a system that should be idle is somewhat alarming.
Change the sleep to be interruptible, but still ignore signals.
A better fix might be to replace this loop with a delayed work item.
References: https://bugs.debian.org/765717
--- drivers/memstick/host/rtsx_usb_ms.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/memstick/host/rtsx_usb_ms.c +2 -1
@@ -706,7 +706,8 @@ poll_again:if (host->eject)break ; -msleep (1000 ); +if (msleep_interruptible (1000 )) +flush_signals (current); }complete (&host->detect_ms_exit);
Signature · 1 line
2.8.2
Quoted 8 lines · On Mon, 02 May 2016 23:17:41 +0300 Oleksandr Natalenko…
This patch has already been posted to LKML by Ben Hutchings ~6 months ago, but AFAIK no further action were performed. However, this patch really fixes weird loadavg with RTS5129 card reader, so I would wonder if this could be merged. AFAIK, it has been applied to some distros' kernels, e.g., Ubuntu.
Original Ben's message goes below.
Quoted 9 lines
rtsx_usb_mscreates a task that mostly sleeps, but tasks in uninterruptible sleep still contribute to the load average (for bug-compatibility with Unix). A load average of ~1 on a system that should be idle is somewhat alarming.Change the sleep to be interruptible, but still ignore signals.
A better fix might be to replace this loop with a delayed work item.
hm.
drivers/memstick/host/rtsx_usb_ms.c poll_again: · line 713
Quoted 13 lines
index 1105db2..645dede 100644
drivers/memstick/host/rtsx_usb_ms.c +2
-1@@ -706,7 +706,8 @@ poll_again:if (host->eject)break ; -msleep (1000 ); +if (msleep_interruptible (1000 )) +flush_signals (current); }complete (&host->detect_ms_exit);
flush_signals() is a bit scary. If this was a userspace task and it had (say) SIGINT pending then it would be very rude for a device driver to rub that out.
But this isn't a userspace task - it's a kthread. So I don't *think* it can get any signals anyway?
And looking at Oleg's 9e7c8f8c62c1e1cda203b it appears that flush_signals() is for flushing signals of a userspace task, not a kthread? Despite the comment "Flush all pending signals for this kthread".
Confused. It's been a while since I looked at this stuff and people have mucked with it. Oleg, can you please sort me out?
Quoted 6 lines · Andrew Morton
Quoted 3 lines · On Mon, 02 May 2016 23:17:41 +0300 Oleksandr Natalenko…
rtsx_usb_mscreates a task that mostly sleeps, but tasks in uninterruptible sleep still contribute to the load average (for bug-compatibility with Unix).
We have TASK_NOLOAD/TASK_IDLE, you can just use schedule_timeout_idle(HZ).
but msleep_interruptible(1000) is fine too.
Quoted 14 lines
drivers/memstick/host/rtsx_usb_ms.c poll_again: · line 713
Quoted 12 lines
drivers/memstick/host/rtsx_usb_ms.c +2
-1@@ -706,7 +706,8 @@ poll_again:if (host->eject)break ; -msleep (1000 ); +if (msleep_interruptible (1000 )) +flush_signals (current); }complete (&host->detect_ms_exit);
flush_signals()is a bit scary.
...
Quoted 2 lines
But this isn't a userspace task - it's a kthread. So I don't *think* it can get any signals anyway?
Agreed, it is not needed and only adds some confusion, so I think rtsx_usb_ms-use-msleep_interruptible-in-polling-loop.patch should be updated.
A kernel thread ignores all signals unless it does allow_signal(), so you can safely remove flush_signals().
Oleg.
Quoted 10 lines · Oleg Nesterov
Quoted 8 lines · Andrew Morton
Quoted 4 lines · On Mon, 02 May 2016 23:17:41 +0300 Oleksandr Natalenko wrote:
rtsx_usb_mscreates a task that mostly sleeps, but tasks in uninterruptible sleep still contribute to the load average (for bug-compatibility with Unix).We have TASK_NOLOAD/TASK_IDLE, you can just use
schedule_timeout_idle(HZ).
[...]
I wasn't aware of that function... ah, that would be because it's new in 4.6. It seems much clearer to use that than to use msleep_interruptible() and ignore the result.
Ben.
Signature · 2 lines
Ben Hutchings In a hierarchy, every employee tends to rise to his level of incompetence.