[RFC][PATCH] Kernel thread signal handling.

[RFC][PATCH] Kernel thread signal handling.

[David Woodhouse]

1. We need disallow_signal() to complement allow_signal().

2. We need a function which does dequeue_signal() _with_ locking.

3. We might need a better name for #2. Change if you care.

4. We need allow_signal() to actually allow signals other than
   SIGKILL. Currently they get either converted to SIGKILL or
   silently dropped, according to whether your kernel thread
   happens to have sa_handler set for the signal in question.

   It would be nicer to fix this in the signal delivery code
   itself if (!tsk->mm) rather than by faking a handler in
   allow_signal(). I'm not touching the signal delivery code
   with a bargepole though. Hopefully the proposed change to
   allow_signal() will provoke someone else into doing so.

===== include/linux/sched.h 1.171 vs edited =====
--- 1.171/include/linux/sched.h	Thu Oct  9 23:13:53 2003
+++ edited/include/linux/sched.h	Mon Oct 13 10:11:07 2003
@@ -576,6 +576,19 @@
 extern void flush_signals(struct task_struct *);
 extern void flush_signal_handlers(struct task_struct *, int force_default);
 extern int dequeue_signal(struct task_struct *tsk, sigset_t *mask, siginfo_t *info);
+
+static inline int dequeue_signal_lock(struct task_struct *tsk, sigset_t *mask, siginfo_t *info)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&tsk->sighand->siglock, flags);
+	ret = dequeue_signal(tsk, mask, info);
+	spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
+
+	return ret;
+}	
+
 extern void block_all_signals(int (*notifier)(void *priv), void *priv,
 			      sigset_t *mask);
 extern void unblock_all_signals(void);
@@ -673,6 +686,7 @@
 extern void reparent_to_init(void);
 extern void daemonize(const char *, ...);
 extern int allow_signal(int);
+extern int disallow_signal(int);
 extern task_t *child_reaper;
 
 extern int do_execve(char *, char __user * __user *, char __user * __user *, struct pt_regs *);
===== kernel/exit.c 1.116 vs edited =====
--- 1.116/kernel/exit.c	Thu Oct  9 23:13:54 2003
+++ edited/kernel/exit.c	Mon Oct 13 11:19:04 2003
@@ -273,12 +273,33 @@
 
 	spin_lock_irq(&current->sighand->siglock);
 	sigdelset(&current->blocked, sig);
+	if (!current->mm) {
+		/* Kernel threads handle their own signals.
+		   Let the signal code know it'll be handled, so
+		   that they don't get converted to SIGKILL or
+		   just silently dropped */
+		current->sighand->action[(sig)-1].sa.sa_handler = (void *)2;
+	}
 	recalc_sigpending();
 	spin_unlock_irq(&current->sighand->siglock);
 	return 0;
 }
 
 EXPORT_SYMBOL(allow_signal);
+
+int disallow_signal(int sig)
+{
+	if (sig < 1 || sig > _NSIG)
+		return -EINVAL;
+
+	spin_lock_irq(&current->sighand->siglock);
+	sigaddset(&current->blocked, sig);
+	recalc_sigpending();
+	spin_unlock_irq(&current->sighand->siglock);
+	return 0;
+}
+
+EXPORT_SYMBOL(disallow_signal);
 
 /*
  *	Put all the gunge required to become a kernel thread without
===== fs/jffs2/background.c 1.20 vs edited =====
--- 1.20/fs/jffs2/background.c	Sat Oct 11 15:47:54 2003
+++ edited/fs/jffs2/background.c	Mon Oct 13 11:17:54 2003
@@ -19,6 +19,7 @@
 #include <linux/completion.h>
 #include <linux/sched.h>
 #include <linux/unistd.h>
+#include <linux/suspend.h>
 #include "nodelist.h"
 
 
@@ -75,6 +76,9 @@
 	struct jffs2_sb_info *c = _c;
 
 	daemonize("jffs2_gcd_mtd%d", c->mtd->index);
+	allow_signal(SIGKILL);
+	allow_signal(SIGSTOP);
+	allow_signal(SIGCONT);
 
 	c->gc_task = current;
 	up(&c->gc_thread_start);
@@ -82,10 +86,7 @@
 	set_user_nice(current, 10);
 
 	for (;;) {
-		spin_lock_irq(&current_sig_lock);
-		siginitsetinv (&current->blocked, sigmask(SIGHUP) | sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGCONT));
-		recalc_sigpending();
-		spin_unlock_irq(&current_sig_lock);
+		allow_signal(SIGHUP);
 
 		if (!thread_should_wake(c)) {
 			set_current_state (TASK_INTERRUPTIBLE);
@@ -97,6 +98,13 @@
 			schedule();
 		}
 
+		if (current->flags & PF_FREEZE) {
+			refrigerator(0);
+			/* refrigerator() should recalc sigpending for us
+			   but doesn't. No matter - allow_signal() will. */
+			continue;
+		}
+
 		cond_resched();
 
 		/* Put_super will send a SIGKILL and then wait on the sem. 
@@ -105,9 +113,7 @@
 			siginfo_t info;
 			unsigned long signr;
 
-			spin_lock_irq(&current_sig_lock);
-			signr = dequeue_signal(current, &current->blocked, &info);
-			spin_unlock_irq(&current_sig_lock);
+			signr = dequeue_signal_lock(current, &current->blocked, &info);
 
 			switch(signr) {
 			case SIGSTOP:
@@ -132,10 +138,7 @@
 			}
 		}
 		/* We don't want SIGHUP to interrupt us. STOP and KILL are OK though. */
-		spin_lock_irq(&current_sig_lock);
-		siginitsetinv (&current->blocked, sigmask(SIGKILL) | sigmask(SIGSTOP) | sigmask(SIGCONT));
-		recalc_sigpending();
-		spin_unlock_irq(&current_sig_lock);
+		disallow_signal(SIGHUP);
 
 		D1(printk(KERN_DEBUG "jffs2_garbage_collect_thread(): pass\n"));
 		if (jffs2_garbage_collect_pass(c) == -ENOSPC) {


-- 
dwmw2

Re: [RFC][PATCH] Kernel thread signal handling.

[Andrew Morton]

David Woodhouse <dwmw2@infradead.org> wrote:
>
>  1. We need disallow_signal() to complement allow_signal().
> 
>  2. We need a function which does dequeue_signal() _with_ locking.
> 
>  3. We might need a better name for #2. Change if you care.
> 
>  4. We need allow_signal() to actually allow signals other than
>     SIGKILL. Currently they get either converted to SIGKILL or
>     silently dropped, according to whether your kernel thread
>     happens to have sa_handler set for the signal in question.
> 
>     It would be nicer to fix this in the signal delivery code
>     itself if (!tsk->mm) rather than by faking a handler in
>     allow_signal(). I'm not touching the signal delivery code
>     with a bargepole though. Hopefully the proposed change to
>     allow_signal() will provoke someone else into doing so.

Sigh.  Using signals to communicate with kernel threads is evil.  It keeps
on breaking and each site does it differently and we've had plenty of bugs
due to this practice.

Signals are for userspace and the signal developers shouldn't have to worry
about weird in-kernel abuse and we have other simpler, more reliable
mechanisms available in-kernel and even more such ranting you get the
point.

Is there no way in which jffs2 can be weaned off this obnoxious habit?

Re: [RFC][PATCH] Kernel thread signal handling.

[Russell King]

On Mon, Oct 13, 2003 at 04:02:19AM -0700, Andrew Morton wrote:
> Signals are for userspace and the signal developers shouldn't have to worry
> about weird in-kernel abuse and we have other simpler, more reliable
> mechanisms available in-kernel and even more such ranting you get the
> point.

Even so, how does the current signal code react to the case where
a userspace process installs (eg) a SIGINT handler immediately
before entering a region which it needs to clean up.  Meanwhile
a SIGINT is sent on some other CPU, discovers that there wasn't a
SIGINT handler when it looked, and queues a SIGKILL instead.
(this case was one which dwmw2 mentioned.)

I'm not certain if this can happen (the signal code is far to hairly
to work through the possibilities.)  However, we used to decide if
the signal was fatal to the process far later in the signal handling
(when delivering it to the user space process in the context of that
process) rather than in some other random context which may be on a
different CPU.

> Is there no way in which jffs2 can be weaned off this obnoxious habit?

jffs2 is using signal handlers as a method of communicating from user
space to kernel space.  Maybe it should create some sysfs files.
However, since there aren't any existing sysfs entities for jffs2 to
attach these files to, this wouldn't seem to be reasonable.

Maybe jffs2 needs /proc/fs/jffs2/<jffs2_instance>/foo but we all know
peoples feelings on procfs.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: [RFC][PATCH] Kernel thread signal handling.

[David Woodhouse]

On Mon, 2003-10-13 at 04:02 -0700, Andrew Morton wrote:
> Sigh.  Using signals to communicate with kernel threads is evil.  It keeps
> on breaking and each site does it differently and we've had plenty of bugs
> due to this practice.

The point in cleaning up allow_signal() et al. is that it gets simple
and it stops breaking. Not that I recall having signal problems with the
JFFS2 garbage collection thread other than this one, mind you.

> Is there no way in which jffs2 can be weaned off this obnoxious habit?

We have a kernel thread which performs garbage collection on our
log-structured file system, to make space ahead of time for writes to
happen. It's purely an optimisation -- we also perform garbage
collection just-in-time in the context of a process which wants to
actually _write_, if there's no free space but some could be made.

This garbage collection involves reading, writing and erasing the flash.
It takes CPU time and power. Sometimes userspace wants it to stop
happening in the background; sometimes userspace wants it to resume
again.

So userspace sends SIGSTOP, SIGCONT and SIGKILL to the garbage
collection thread and all of them have the expected effect. 

Since we handle these signals anyway, the normal wakeup of the GC thread
when the amount of free space changes is also done by SIGHUP, which
userspace can also send to trigger a single pass.

I don't any the benefit in changing this practice.

-- 
dwmw2

Re: [RFC][PATCH] Kernel thread signal handling.

[David Woodhouse]

On Mon, 2003-10-13 at 12:08 +0100, Russell King wrote:
> jffs2 is using signal handlers as a method of communicating from user
> space to kernel space.  Maybe it should create some sysfs files.
> However, since there aren't any existing sysfs entities for jffs2 to
> attach these files to, this wouldn't seem to be reasonable.

To clarify -- the JFFS2 garbage collect thread is using SIGSTOP, SIGCONT
and SIGKILL from userspace to mean exactly what SIGSTOP, SIGCONT and
SIGKILL always mean in userspace.

Since it already has signal handling code, we also use SIGHUP for
wakeup, and userspace can send that too to trigger a single GC pass.

-- 
dwmw2

Re: [RFC][PATCH] Kernel thread signal handling.

[Andrew Morton]

David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Mon, 2003-10-13 at 04:02 -0700, Andrew Morton wrote:
> > Sigh.  Using signals to communicate with kernel threads is evil.  It keeps
> > on breaking and each site does it differently and we've had plenty of bugs
> > due to this practice.
> 
> The point in cleaning up allow_signal() et al. is that it gets simple
> and it stops breaking.

It will encourage kernel developers to use signals-to-kernel threads more,
and we don't *need* this capability.

People think "I need to send a message to a kernel thread" and then,
immediately, "ah-hah!  I'll use a signal!"

> ...
> This garbage collection involves reading, writing and erasing the flash.
> It takes CPU time and power. Sometimes userspace wants it to stop
> happening in the background; sometimes userspace wants it to resume
> again.
> 
> So userspace sends SIGSTOP, SIGCONT and SIGKILL to the garbage
> collection thread and all of them have the expected effect. 

Sounds like the GC should have been performed by a userspace process in the
first place?

How does userspace identify the JFFS2 process to which to send the signal?

> I don't any the benefit in changing this practice.

Well I know I'm going to lose this one, but at least I get to bitch about
stuff.

sysfs would be appropriate, as would a sysctl handler.  An ioctl might even
work, although it gets a visit from the ioctl police and sometimes it is
hard to obtain an fd on the appropriate filesystem.  If the call rate is
low, `mount -o remount,...' can be used to send a message to a filesystem.

Re: [RFC][PATCH] Kernel thread signal handling.

[David Woodhouse]

On Mon, 2003-10-13 at 04:40 -0700, Andrew Morton wrote:
> People think "I need to send a message to a kernel thread" and then,
> immediately, "ah-hah!  I'll use a signal!"

I've seen relatively little of this. Most of the problems I've been
aware of have been kernel threads _not_ handling signals (or handling
only SIGKILL) and going into endless loops of bouncing straight back out
of schedule().

That problem is almost unrelated -- it happens because driver writes
want to sleep in TASK_INTERRUPTIBLE state rather than
TASK_UNINTERRUPTIBLE. The fix for that is to have a per-task 
'uninterruptible_count' along much the same lines as preempt_count,
where each function which is unable to handle an -EINTR return
increments the count before calling down to another function which may
have done that. But that's a 2.7 thing and mostly not related to this
particular bug.

> Sounds like the GC should have been performed by a userspace process in the
> first place?

Well, it would have to actually be done in kernel space but I suppose
there could be an ioctl or syscall or something which causes a call to
jffs2_garbage_collect_pass() to happen in the context of the caller, and
the variables which are used to decide when to wake up could be exposed
to userspace via sysfs, and the userspace daemon itself could register
with the JFFS2 code so that it gets woken when those variables change...
or maybe I could poll() on the sysfs file which contains them I
suppose... 

Er, no :)

> How does userspace identify the JFFS2 process to which to send the
> signal?

	daemonize("jffs2_gcd_mtd%d", c->mtd->index);

> > I don't any the benefit in changing this practice.
> 
> Well I know I'm going to lose this one, but at least I get to bitch about
> stuff.

Fair enough :)

Bitching accomplished; now can we fix the bug?

-- 
dwmw2

Re: [RFC][PATCH] Kernel thread signal handling.

[Andrew Morton]

David Woodhouse <dwmw2@infradead.org> wrote:
>
> > How does userspace identify the JFFS2 process to which to send the
> > signal?
> 
> 	daemonize("jffs2_gcd_mtd%d", c->mtd->index);

And then what?  Parse the output of ps(1)?  Use pidof(8)?

> Bitching accomplished; now can we fix the bug?

Insufficient contrition detected :)

Why cannot a procfs or sysfs control be used?

Re: [RFC][PATCH] Kernel thread signal handling.

[David Woodhouse]

On Mon, 2003-10-13 at 05:11 -0700, Andrew Morton wrote:
> > 	daemonize("jffs2_gcd_mtd%d", c->mtd->index);
> 
> And then what?  Parse the output of ps(1)?  Use pidof(8)?

Those work. 

> Insufficient contrition detected :)

Hehe.

> Why cannot a procfs or sysfs control be used?

Mostly because I think that idea sucks, and partly because I think your
ire is misdirected -- it shouldn't be directed at kernel code which
handles signals, but rather at kernel code which sleeps in state
TASK_INTERRUPTIBLE but _doesn't_ handle signals.

On the other hand, if you ever find people actually trying to pass
_data_ to kernel threads with sys_rt_sigqueueinfo(), I'll be right
behind you with my own baseball bat waiting to bash the pieces you leave
behind :)

-- 
dwmw2

Re: [RFC][PATCH] Kernel thread signal handling.

[Linus Torvalds]

On Mon, 13 Oct 2003, Andrew Morton wrote:
> 
> Sigh.  Using signals to communicate with kernel threads is evil.  It keeps
> on breaking and each site does it differently and we've had plenty of bugs
> due to this practice.

Yeah, it's not wonderfully pretty, but at least this patch makes bugs 
_less_ likely, by having more of the common stuff abstracted out. And 
clearly allow_signal() was broken before, since it didn't allow anything 
but deadly signals, despite the fact that it was supposed to be generic.

		Linus