Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1392499 > unrolled thread

[PATCH RESEND] rtsx_usb_ms: Use msleep_interruptible() in polling loop

Back to article view | Back to linux.kernel

Contents

  [PATCH RESEND] rtsx_usb_ms: Use msleep_interruptible() in polling  loop Oleksandr Natalenko <oleksandr@natalenko.name> - 2016-05-02 22:30 +0200
    Re: [PATCH RESEND] rtsx_usb_ms: Use msleep_interruptible() in  polling loop Andrew Morton <akpm@linux-foundation.org> - 2016-05-02 23:00 +0200
      Re: [PATCH RESEND] rtsx_usb_ms: Use msleep_interruptible() in  polling loop Oleg Nesterov <oleg@redhat.com> - 2016-05-03 15:50 +0200
        Re: [PATCH RESEND] rtsx_usb_ms: Use msleep_interruptible() in  polling loop Ben Hutchings <ben@decadent.org.uk> - 2016-05-03 17:30 +0200

#1392499 — [PATCH RESEND] 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
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
  drivers/memstick/host/rtsx_usb_ms.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/memstick/host/rtsx_usb_ms.c 
b/drivers/memstick/host/rtsx_usb_ms.c
index 1105db2..645dede 100644
--- a/drivers/memstick/host/rtsx_usb_ms.c
+++ b/drivers/memstick/host/rtsx_usb_ms.c
@@ -706,7 +706,8 @@ poll_again:
  		if (host->eject)
  			break;

-		msleep(1000);
+		if (msleep_interruptible(1000))
+			flush_signals(current);
  	}

  	complete(&host->detect_ms_exit);
-- 
2.8.2

[toc] | [next] | [standalone]

#1392510 — Re: [PATCH RESEND] rtsx_usb_ms: Use msleep_interruptible() in polling loop

On Mon, 02 May 2016 23:17:41 +0300 Oleksandr Natalenko <oleksandr@natalenko.name> wrote:

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

hm.

> index 1105db2..645dede 100644
> --- a/drivers/memstick/host/rtsx_usb_ms.c
> +++ b/drivers/memstick/host/rtsx_usb_ms.c
> @@ -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?

[toc] | [prev] | [next] | [standalone]

#1393426 — Re: [PATCH RESEND] rtsx_usb_ms: Use msleep_interruptible() in polling loop

On 05/02, Andrew Morton wrote:
>
> On Mon, 02 May 2016 23:17:41 +0300 Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>
> > 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).

We have TASK_NOLOAD/TASK_IDLE, you can just use schedule_timeout_idle(HZ).

but msleep_interruptible(1000) is fine too.

> > --- a/drivers/memstick/host/rtsx_usb_ms.c
> > +++ b/drivers/memstick/host/rtsx_usb_ms.c
> > @@ -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.
...
> 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.

[toc] | [prev] | [next] | [standalone]

#1393521 — Re: [PATCH RESEND] rtsx_usb_ms: Use msleep_interruptible() in polling loop

[Multipart message — attachments visible in raw view] — view raw

On Tue, 2016-05-03 at 15:40 +0200, Oleg Nesterov wrote:
> On 05/02, Andrew Morton wrote:
> > 
> > 
> > On Mon, 02 May 2016 23:17:41 +0300 Oleksandr Natalenko  wrote:
> > 
> > > 
> > > 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).
> 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.

-- 
Ben Hutchings
In a hierarchy, every employee tends to rise to his level of incompetence.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web