JFFS2 swsusp / signal cleanup.

JFFS2 swsusp / signal cleanup.

[David Woodhouse]

How's this look?

===== fs/jffs2/background.c 1.19 vs edited =====
--- 1.19/fs/jffs2/background.c	Wed May 28 16:01:05 2003
+++ edited/fs/jffs2/background.c	Sat Oct  4 12:23:52 2003
@@ -19,6 +19,7 @@
 #include <linux/completion.h>
 #include <linux/sched.h>
 #include <linux/unistd.h>
+#include <linux/suspend.h>
 #include "nodelist.h"
 
 
@@ -89,10 +90,10 @@
 	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);
+		allow_signal(SIGKILL);
+		allow_signal(SIGSTOP);
+		allow_signal(SIGCONT);
 
 		if (!thread_should_wake(c)) {
 			set_current_state (TASK_INTERRUPTIBLE);
@@ -104,6 +105,13 @@
 			schedule();
 		}
 
+		if (current->flags & PF_FREEZE) {
+			refrigerator(0);	/* When this loses its stupid flush_signals() 
+						   and starts just calling recalc_sigpending(),
+						   you can remove the following: */
+			recalc_sigpending();
+		}
+
 		cond_resched();
 
 		/* Put_super will send a SIGKILL and then wait on the sem. 
@@ -112,9 +120,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:
@@ -138,10 +144,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"));
 		jffs2_garbage_collect_pass(c);
===== include/linux/sched.h 1.168 vs edited =====
--- 1.168/include/linux/sched.h	Thu Oct  2 08:12:14 2003
+++ edited/include/linux/sched.h	Sat Oct  4 12:23:15 2003
@@ -577,6 +577,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);
@@ -674,6 +687,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.113 vs edited =====
--- 1.113/kernel/exit.c	Sun Sep 21 22:49:52 2003
+++ edited/kernel/exit.c	Sat Oct  4 12:11:54 2003
@@ -278,7 +278,20 @@
 	return 0;
 }
 
+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(allow_signal);
+EXPORT_SYMBOL(disallow_signal);
 
 /*
  *	Put all the gunge required to become a kernel thread without

-- 
dwmw2

Re: JFFS2 swsusp / signal cleanup.

[Pavel Machek]

Hi!

> How's this look?

Is flush_signals() really so stupid to do? Goal was to make
modifications to code as simple as possible, and as most pieces do not
expect to be interrupted, pretending signal never happened seems like
good idea...
								Pavel

> @@ -104,6 +105,13 @@
>  			schedule();
>  		}
>  
> +		if (current->flags & PF_FREEZE) {
> +			refrigerator(0);	/* When this loses its stupid flush_signals() 
> +						   and starts just calling recalc_sigpending(),
> +						   you can remove the following: */
> +			recalc_sigpending();
> +		}
> +
>  		cond_resched();
>  
>  		/* Put_super will send a SIGKILL and then wait on the sem. 

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: JFFS2 swsusp / signal cleanup.

[Russell King]

On Sun, Oct 05, 2003 at 06:11:55PM +0200, Pavel Machek wrote:
> Is flush_signals() really so stupid to do? Goal was to make
> modifications to code as simple as possible, and as most pieces do not
> expect to be interrupted, pretending signal never happened seems like
> good idea...

What if that signal was necessary for the operation of the thread, and
dropping it would cause a problem?

Since you're effectively using signal handling to cause a false pending
signal indication, surely the correct cleanup is to re-calculate the
pending signal indication.  That way, we won't be throwing away signals.

I'm also wondering if there could be a problem with (ab)using TASK_STOPPED
here - could a stopped task be woken prematurely and thereby sent spinning
in refrigerator() by a non-stopped process sending a SIGCONT at just the
right time?

Maybe we want a TASK_FROZEN state to describe the "frozen, may not be woken
by anything except thawing" state?

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
      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: JFFS2 swsusp / signal cleanup.

[Pavel Machek]

Hi!

> > Is flush_signals() really so stupid to do? Goal was to make
> > modifications to code as simple as possible, and as most pieces do not
> > expect to be interrupted, pretending signal never happened seems like
> > good idea...
> 
> What if that signal was necessary for the operation of the thread, and
> dropping it would cause a problem?
> 
> Since you're effectively using signal handling to cause a false pending
> signal indication, surely the correct cleanup is to re-calculate the
> pending signal indication.  That way, we won't be throwing away
> signals.

Should I do recalc_sigpending() instead of flush_signals(current)?

> I'm also wondering if there could be a problem with (ab)using TASK_STOPPED
> here - could a stopped task be woken prematurely and thereby sent spinning
> in refrigerator() by a non-stopped process sending a SIGCONT at just the
> right time?
> 
> Maybe we want a TASK_FROZEN state to describe the "frozen, may not be woken
> by anything except thawing" state?

That would certainly be cleaner, but I think it would require
modifications all over the kernel...

That SIGCONT race... while() in refrigerator() should catch
that. Maybe we spin, but we go back to sleep pretty soon.

							Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: JFFS2 swsusp / signal cleanup.

[David Woodhouse]

On Sun, 2003-10-05 at 21:13 +0200, Pavel Machek wrote:
> Should I do recalc_sigpending() instead of flush_signals(current)?

Yes. You can do that unconditionally, too -- no need to do it depending
on an argument from the caller.

-- 
dwmw2

[pm] Re: JFFS2 swsusp / signal cleanup.

[Pavel Machek]

Hi!

> > Should I do recalc_sigpending() instead of flush_signals(current)?
> 
> Yes. You can do that unconditionally, too -- no need to do it depending
> on an argument from the caller.

Yes, and it actually works that way. Good.

[Patrick, this patch is probably good idea, flush_signals has
potential to loose some signal].

[flag argument can be killed, but I guess its bad idea to do it now].

								Pavel

--- tmp/linux/kernel/power/process.c	2003-08-27 12:00:53.000000000 +0200
+++ linux/kernel/power/process.c	2003-10-05 21:15:21.000000000 +0200
@@ -49,10 +49,7 @@
 	pr_debug("%s entered refrigerator\n", current->comm);
 	printk("=");
 	current->flags &= ~PF_FREEZE;
-	if (flag)
-		flush_signals(current); /* We have signaled a kernel thread, which isn't normal behaviour
-					   and that may lead to 100%CPU sucking because those threads
-					   just don't manage signals. */
+	recalc_sigpending(); /* We sent fake signal, clean it up */
 	current->flags |= PF_FROZEN;
 	while (current->flags & PF_FROZEN)
 		schedule();

-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [pm] Re: JFFS2 swsusp / signal cleanup.

[David Woodhouse]

Do you need locking for recalc_sigpending()?

-- 
dwmw2

Re: [pm] Re: JFFS2 swsusp / signal cleanup.

[Pavel Machek]

Hi!

> Do you need locking for recalc_sigpending()?

Hmm, seems so; btw your patch seems to miss it too:

> @@ -104,6 +105,13 @@
>                       schedule();
>               }
>
> +             if (current->flags & PF_FREEZE) {
> +                     refrigerator(0);        /* When this loses its
> stupid flush_signals()
> +                                                and starts just
> calling recalc_sigpending(),
> +                                                you can remove the
> following: */
> +                     recalc_sigpending();
> +             }
> +
>               cond_resched();
>
>               /* Put_super will send a SIGKILL and then wait on the
> sem.

I'll test it and send updated patch.
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]