Skip to contentSkip to conversation

Remaining straight forward kthread API conversions...

PATCH New Eric W. Biederman 30 participants 218 messages Apr 19, 2007 – May 17, 2007 Page 2 of 2

Subsystems (22)
lore
Share & tools
lore
#212 lore raw

PATCH 1/1 md: Remove broken SIGKILL support

In reply to Eric W. Biederman

From: Eric W. Biederman <[email protected]>

Currently md_thread calls allow_signal so it can receive a SIGKILL but then does nothing with it except flush the sigkill so that it not can use an interruptible sleep.

This whole dance is silly so remove the unnecessary and broken signal handling logic.

---
 drivers/md/md.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)
drivers/md/md.c +0 -6
index 1299c23..dfd0cb9 100644
@@ -4542,17 +4542,11 @@ static int md_thread(void * arg)
 	 */
 
 	current->flags |= PF_NOFREEZE;
-	allow_signal(SIGKILL);
 	while (!kthread_should_stop()) {
 
 		/* We need to wait INTERRUPTIBLE so that
 		 * we don't add to the load-average.
-		 * That means we need to be sure no signals are
-		 * pending
 		 */
-		if (signal_pending(current))
-			flush_signals(current);
-
 		wait_event_interruptible_timeout
 			(thread->wqueue,
 			 test_bit(THREAD_WAKEUP, &thread->flags)
Signature · 1 line
1.5.0.g53756
#213 lore raw

Re: [PATCH] md: Remove broken SIGKILL support

Quoted 8 lines · On Thursday April 19, [email protected] wrote:

From: Eric W. Biederman <[email protected]>

Currently md_thread calls allow_signal so it can receive a SIGKILL but then does nothing with it except flush the sigkill so that it not can use an interruptible sleep.

This whole dance is silly so remove the unnecessary and broken signal handling logic.

(sorry of the delay in replying)

You missed some related code which should help you see that it is - maybe - not completely 'silly' (though I confess it might be slightly broken).
In md_check_recovery:

	if (signal_pending(current)) {
		if (mddev->pers->sync_request) {
			printk(KERN_INFO "md: %s in immediate safe mode\n",
			       mdname(mddev));
			mddev->safemode = 2;
		}
		flush_signals(current);
	}

The idea is that alt-sysrq-K will send SIGKILL to all processes including the md support threads, which will cause them to enter "immediate safe mode" so that the metadata will be marked clean immediately at every opportunity. That way you can use alt-sysrq:
sync,unmount,kill,reboot
and be fairly sure that you md array will be shut down cleanly.

I'd be just as happy to link this into Unmount (aka do_emergency_remount), but that doesn't seem at all straight forward, and in any case should be done before the current code is ripped out.

While we do have a reboot_notifier which tries to stop all arrays, I've never been comfortable with that. A reboot really should just reboot...

What I would REALLY like is for the block device to know whether it is open read-only or read-write. Then I could mark it clean when it becomes read-only as would happen when do_emergency_remount remounts it read-only.

I might see how hard that would be...

NeilBrown

#214 lore raw

Re: [PATCH] md: Remove broken SIGKILL support

Quoted 17 lines · Neil Brown
Quoted 8 lines · On Thursday April 19, [email protected] wrote:

From: Eric W. Biederman <[email protected]>

Currently md_thread calls allow_signal so it can receive a SIGKILL but then does nothing with it except flush the sigkill so that it not can use an interruptible sleep.

This whole dance is silly so remove the unnecessary and broken signal handling logic.

(sorry of the delay in replying)

You missed some related code which should help you see that it is - maybe - not completely 'silly' (though I confess it might be slightly broken).
In md_check_recovery:

Quoted 8 lines
	if (signal_pending(current)) {
		if (mddev->pers->sync_request) {
			printk(KERN_INFO "md: %s in immediate safe mode\n",
			       mdname(mddev));
			mddev->safemode = 2;
		}
		flush_signals(current);
	}

Thanks.

Quoted 15 lines

The idea is that alt-sysrq-K will send SIGKILL to all processes including the md support threads, which will cause them to enter "immediate safe mode" so that the metadata will be marked clean immediately at every opportunity. That way you can use alt-sysrq:
sync,unmount,kill,reboot
and be fairly sure that you md array will be shut down cleanly.

I'd be just as happy to link this into Unmount (aka do_emergency_remount), but that doesn't seem at all straight forward, and in any case should be done before the current code is ripped out.

While we do have a reboot_notifier which tries to stop all arrays, I've never been comfortable with that. A reboot really should just reboot...

Quoted 6 lines

What I would REALLY like is for the block device to know whether it is open read-only or read-write. Then I could mark it clean when it becomes read-only as would happen when do_emergency_remount remounts it read-only.

I might see how hard that would be...

My goal to get signals to kernel threads out of the user space interface especially for non-privileged processes, so everything that we do with kernel threads can just be an unimportant implementation detail to user space.

Eric

#215 lore raw

PATCH 1/1 nfs4state reclaimer: Remove unnecessary allow_signal

In reply to Eric W. Biederman

From: Eric W. Biederman <[email protected]>

Cc Neil Brown <[email protected]>
Cc Trond Myklebust <[email protected]>
---
 fs/nfs/nfs4state.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
fs/nfs/nfs4state.c +0 -2
index 5fffbdf..d16393f 100644
@@ -775,8 +775,6 @@ static int reclaimer(void *ptr)
 	struct rpc_cred *cred;
 	int status = 0;
 
-	allow_signal(SIGKILL);
-
 	/* Ensure exclusive access to NFSv4 state */
 	lock_kernel();
 	down_write(&clp->cl_sem);
Signature · 1 line
1.5.0.g53756
#216 lore raw

Re: [PATCH] nfs4state reclaimer: Remove unnecessary allow_signal

Quoted 14 lines · Eric W. Biederman

From: Eric W. Biederman <[email protected]>

Cc Neil Brown <[email protected]>
Cc Trond Myklebust <[email protected]>
---
 fs/nfs/nfs4state.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
fs/nfs/nfs4state.c +0 -0
index 5fffbdf..d16393f 100644

fs/nfs/nfs4state.c static int reclaimer(void *ptr) · line 780

Quoted 8 lines
fs/nfs/nfs4state.c +0 -2
@@ -775,8 +775,6 @@ static int reclaimer(void *ptr)
 	struct rpc_cred *cred;
 	int status = 0;
 
-	allow_signal(SIGKILL);
-
 	/* Ensure exclusive access to NFSv4 state */
 	lock_kernel();
 	down_write(&clp->cl_sem);

Ditto...

#217 lore raw
In reply to Eric W. Biederman

Looks like you were missing at least the pcie hotplug driver. Another one of the horrible thread abuses in drivers/pci/hotpug.

- full conversion to kthread infrastructure
- use wake_up_process to wake the thread up

Like most pci hotplug drivers it still uses very race non-atomic variable assignment to communicated with the thread, but that's something the maintainers should look into.

drivers/pci/hotplug/pciehp_ctrl.c +21 -33
Index: linux-2.6/drivers/pci/hotplug/pciehp_ctrl.c
===================================================================
@@ -32,14 +32,13 @@
 #include <linux/types.h>
 #include <linux/smp_lock.h>
 #include <linux/pci.h>
+#include <linux/kthread.h>
 #include "../pci.h"
 #include "pciehp.h"
 
 static void interrupt_event_handler(struct controller *ctrl);
 
-static struct semaphore event_semaphore;	/* mutex for process loop (up if something to process) */
-static struct semaphore event_exit;		/* guard ensure thread has exited before calling it quits */
-static int event_finished;
+static struct task_struct *pciehpd_event_thread;
 static unsigned long pushbutton_pending;	/* = 0 */
 static unsigned long surprise_rm_pending;	/* = 0 */
 
@@ -93,8 +92,9 @@ u8 pciehp_handle_attention_button(u8 hp_
 		info("Button ignore on Slot(%s)\n", slot_name(p_slot));
 	}
 
+	/* signal event thread that new event is posted */
 	if (rc)
-		up(&event_semaphore);	/* signal event thread that new event is posted */
+		wake_up_process(pciehpd_event_thread);
 
 	return 0;
 
@@ -135,8 +135,9 @@ u8 pciehp_handle_switch_change(u8 hp_slo
 		taskInfo->event_type = INT_SWITCH_CLOSE;
 	}
 
+	/* signal event thread that new event is posted */
 	if (rc)
-		up(&event_semaphore);	/* signal event thread that new event is posted */
+		wake_up_process(pciehpd_event_thread);
 
 	return rc;
 }
@@ -178,8 +179,9 @@ u8 pciehp_handle_presence_change(u8 hp_s
 		taskInfo->event_type = INT_PRESENCE_OFF;
 	}
 
+	/* signal event thread that new event is posted */
 	if (rc)
-		up(&event_semaphore);	/* signal event thread that new event is posted */
+		wake_up_process(pciehpd_event_thread);
 
 	return rc;
 }
@@ -217,8 +219,10 @@ u8 pciehp_handle_power_fault(u8 hp_slot,
 		taskInfo->event_type = INT_POWER_FAULT;
 		info("power fault bit %x set\n", hp_slot);
 	}
+
+	/* signal event thread that new event is posted */
 	if (rc)
-		up(&event_semaphore);	/* signal event thread that new event is posted */
+		wake_up_process(pciehpd_event_thread);
 
 	return rc;
 }
@@ -362,7 +366,7 @@ static void pushbutton_helper_thread(uns
 {
 	pushbutton_pending = data;
 
-	up(&event_semaphore);
+	wake_up_process(pciehpd_event_thread);
 }
 
 /**
@@ -452,19 +456,14 @@ static void pciehp_surprise_rm_thread(un
 
 
 /* this is the main worker thread */
-static int event_thread(void* data)
+static int event_thread(void *data)
 {
 	struct controller *ctrl;
-	lock_kernel();
-	daemonize("pciehpd_event");
-
-	unlock_kernel();
 
 	while (1) {
-		dbg("!!!!event_thread sleeping\n");
-		down_interruptible (&event_semaphore);
-		dbg("event_thread woken finished = %d\n", event_finished);
-		if (event_finished || signal_pending(current))
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
+		if (kthread_should_stop())
 			break;
 		/* Do stuff here */
 		if (pushbutton_pending)
@@ -476,24 +475,15 @@ static int event_thread(void* data)
 				interrupt_event_handler(ctrl);
 	}
 	dbg("event_thread signals exit\n");
-	up(&event_exit);
 	return 0;
 }
 
 int pciehp_event_start_thread(void)
 {
-	int pid;
-
-	/* initialize our semaphores */
-	init_MUTEX_LOCKED(&event_exit);
-	event_finished=0;
-
-	init_MUTEX_LOCKED(&event_semaphore);
-	pid = kernel_thread(event_thread, NULL, 0);
-
-	if (pid < 0) {
+	pciehpd_event_thread = kthread_run(event_thread, NULL, "pciehpd_event");
+	if (IS_ERR(pciehpd_event_thread)) {
 		err ("Can't start up our event thread\n");
-		return -1;
+		return PTR_ERR(pciehpd_event_thread);
 	}
 	return 0;
 }
@@ -501,9 +491,7 @@ int pciehp_event_start_thread(void)
 
 void pciehp_event_stop_thread(void)
 {
-	event_finished = 1;
-	up(&event_semaphore);
-	down(&event_exit);
+	kthread_stop(pciehpd_event_thread);
 }
 
 
@@ -624,7 +612,7 @@ static void interrupt_event_handler(stru
 						dbg("Surprise Removal\n");
 						if (p_slot) {
 							surprise_rm_pending = (unsigned long) p_slot;
-							up(&event_semaphore);
+							wake_up_process(pciehpd_event_thread);
 							update_slot_info(p_slot);
 						}
 					}