Unusually long delay in the kernel

Unusually long delay in the kernel

[Alan Stern]

This code excerpt is taken from the start of the control thread for the 
usb-storage driver in 2.6.14-rc1:


static int usb_stor_control_thread(void * __us)
{
	struct us_data *us = (struct us_data *)__us;
	struct Scsi_Host *host = us_to_host(us);

printk(KERN_INFO "Before thread start\n");
	lock_kernel();

	/*
	 * This thread doesn't need any user-level access,
	 * so get rid of all our resources.
	 */
	daemonize("usb-storage");
	current->flags |= PF_NOFREEZE;
	unlock_kernel();
printk(KERN_INFO "After thread start\n");


The code between the two printk's takes a long time to run.  I don't have 
precise numbers, but it feels like more than 1 second.

(1) Can anyone explain why, or indicate how to speed it up?

(2) Are the {un}lock_kernel calls really needed?

Thanks,

Alan Stern

Re: Unusually long delay in the kernel

[Fawad Lateef]

On 9/16/05, Alan Stern <stern@rowland.harvard.edu> wrote:
> This code excerpt is taken from the start of the control thread for the
> usb-storage driver in 2.6.14-rc1:
> 
> 
> static int usb_stor_control_thread(void * __us)
> {
>         struct us_data *us = (struct us_data *)__us;
>         struct Scsi_Host *host = us_to_host(us);
> 
> printk(KERN_INFO "Before thread start\n");
>         lock_kernel();
> 
>         /*
>          * This thread doesn't need any user-level access,
>          * so get rid of all our resources.
>          */
>         daemonize("usb-storage");
>         current->flags |= PF_NOFREEZE;
>         unlock_kernel();
> printk(KERN_INFO "After thread start\n");
> 
> 
> The code between the two printk's takes a long time to run.  I don't have
> precise numbers, but it feels like more than 1 second.
> 
> (1) Can anyone explain why, or indicate how to speed it up?
> 
> (2) Are the {un}lock_kernel calls really needed?
> 

AFAIR the article on the lwn.net in the driver porting porting to 2.6
kernel mentioned that big kernel locks lock_kernel and unlock_kernel
gone, but as I searched into the kernel's drivers directory for the
kernel_thread functions (drivers creating threads), I found some of
them using lock_kernel and some not .... So I also wants to know are
they really needed ??

By the way I havn't saw/felt any long delay when starting thread in
this way using lock_kernel !!!!


-- 
Fawad Lateef

Re: Unusually long delay in the kernel

[Andrew Morton]

Alan Stern <stern@rowland.harvard.edu> wrote:
>
> This code excerpt is taken from the start of the control thread for the 
>  usb-storage driver in 2.6.14-rc1:
> 
> 
>  static int usb_stor_control_thread(void * __us)
>  {
>  	struct us_data *us = (struct us_data *)__us;
>  	struct Scsi_Host *host = us_to_host(us);
> 
>  printk(KERN_INFO "Before thread start\n");
>  	lock_kernel();
> 
>  	/*
>  	 * This thread doesn't need any user-level access,
>  	 * so get rid of all our resources.
>  	 */
>  	daemonize("usb-storage");
>  	current->flags |= PF_NOFREEZE;
>  	unlock_kernel();
>  printk(KERN_INFO "After thread start\n");
> 
> 
>  The code between the two printk's takes a long time to run.  I don't have 
>  precise numbers, but it feels like more than 1 second.
> 
>  (1) Can anyone explain why, or indicate how to speed it up?

What's it doing at the time?  (kgdb is great for this sort of thing: hit
^C, go for a wander through the thread callchains).

Presumably it's spinning on the bkl.  Is this actually an SMP machine?  If
so, perhaps some other process is holding the bkl for a long time.  Perhaps
a netdevice spending a long time diddling hardware in an ioctl, something
like that.

>  (2) Are the {un}lock_kernel calls really needed?

Definitely not.

That code could be converted to the kthread API btw.

Re: Unusually long delay in the kernel

[Alan Stern]

On Fri, 16 Sep 2005, Andrew Morton wrote:

> Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > This code excerpt is taken from the start of the control thread for the 
> >  usb-storage driver in 2.6.14-rc1:
> > 
> > 
> >  static int usb_stor_control_thread(void * __us)
> >  {
> >  	struct us_data *us = (struct us_data *)__us;
> >  	struct Scsi_Host *host = us_to_host(us);
> > 
> >  printk(KERN_INFO "Before thread start\n");
> >  	lock_kernel();
> > 
> >  	/*
> >  	 * This thread doesn't need any user-level access,
> >  	 * so get rid of all our resources.
> >  	 */
> >  	daemonize("usb-storage");
> >  	current->flags |= PF_NOFREEZE;
> >  	unlock_kernel();
> >  printk(KERN_INFO "After thread start\n");
> > 
> > 
> >  The code between the two printk's takes a long time to run.  I don't have 
> >  precise numbers, but it feels like more than 1 second.
> > 
> >  (1) Can anyone explain why, or indicate how to speed it up?
> 
> What's it doing at the time?  (kgdb is great for this sort of thing: hit
> ^C, go for a wander through the thread callchains).
> 
> Presumably it's spinning on the bkl.  Is this actually an SMP machine?  If
> so, perhaps some other process is holding the bkl for a long time.  Perhaps
> a netdevice spending a long time diddling hardware in an ioctl, something
> like that.

I need to do more precise tests.  Some quick informal tests indicated that 
the lock_kernel call and the daemonize call each took a noticeable time.  
I'll respond later with more details.

> >  (2) Are the {un}lock_kernel calls really needed?
> 
> Definitely not.

Good; I'll take them out.

> That code could be converted to the kthread API btw.

Hmph.  Near as I can tell, the only changes that would involve are:

	Converting the thread creation call from kernel_thread to
	kthread_run.

	Adding another call to wake the thread up once it has been
	created.

	Removing the call to daemonize.

There wouldn't be any need to call kthread_stop -- and in fact it wouldn't 
work, as the thread waits on a semaphore while it is idle (kthread_stop 
can't cope with things like that).

So overall, it appears that converting to the kthread API would add the
overhead of an extra context switch at startup and synchronous
termination, without bringing any advantages at all.


That reminds me, I've got another question.  Once a thread has called
daemonize, or if it was started using kthread_run, all its signals are
blocked.  Is it still possible that through some extraordinary
circumstance the thread could receive a signal, or are we absolutely
guaranteed that no signals will arrive until the thread enables them?  
It's important to know the answer, because normally a thread spends its
idle time waiting on down_interruptible or something similar.  If a signal
managed to get through somehow, the thread would never be able to go back
to sleep unless it explicitly flushed its signals.

(I ask because I once noticed, looking through the signal-processing code, 
that the kernel itself could send an unblockable signal.  Don't recall 
where or when, though.)

Alan Stern

Re: Unusually long delay in the kernel

[Andrew Morton]

Alan Stern <stern@rowland.harvard.edu> wrote:
>
> > Presumably it's spinning on the bkl.  Is this actually an SMP machine?  If
> > so, perhaps some other process is holding the bkl for a long time.  Perhaps
> > a netdevice spending a long time diddling hardware in an ioctl, something
> > like that.
> 
> I need to do more precise tests.  Some quick informal tests indicated that 
> the lock_kernel call and the daemonize call each took a noticeable time.  

Something odd is happening.

> > That code could be converted to the kthread API btw.
> 
> Hmph.  Near as I can tell, the only changes that would involve are:
> 
> 	Converting the thread creation call from kernel_thread to
> 	kthread_run.
> 
> 	Adding another call to wake the thread up once it has been
> 	created.
> 
> 	Removing the call to daemonize.
> 
> There wouldn't be any need to call kthread_stop -- and in fact it wouldn't 
> work, as the thread waits on a semaphore while it is idle (kthread_stop 
> can't cope with things like that).

Well I was assuming that the semaphore would go away as well.  Kernel
threads normally use waitqueues to await more work.

> 
> That reminds me, I've got another question.  Once a thread has called
> daemonize, or if it was started using kthread_run, all its signals are
> blocked.  Is it still possible that through some extraordinary
> circumstance the thread could receive a signal, or are we absolutely
> guaranteed that no signals will arrive until the thread enables them?  

Kernel threads should sleep in state TASK_INTERRUPTIBLE, with all signals
blocked.  Because they don't want to contribute to the load average, and
because they shouldn't use signals.

So if it was possible to deliver a signal to an all-signals-blocked kernel
thread, that kernel thread would go into a busy loop, because all of its
sleep attempts will fall straight through (signal_pending() is true).  So I
think it's safe to assume that nobody ever does force_sig() on a kenrel
thread.

> It's important to know the answer, because normally a thread spends its
> idle time waiting on down_interruptible or something similar.  If a signal
> managed to get through somehow, the thread would never be able to go back
> to sleep unless it explicitly flushed its signals.

Yup.

Re: Unusually long delay in the kernel

[Alan Stern]

On Sat, 17 Sep 2005, Andrew Morton wrote:

> > > That code could be converted to the kthread API btw.
> > 
> > Hmph.  Near as I can tell, the only changes that would involve are:
> > 
> > 	Converting the thread creation call from kernel_thread to
> > 	kthread_run.
> > 
> > 	Adding another call to wake the thread up once it has been
> > 	created.
> > 
> > 	Removing the call to daemonize.
> > 
> > There wouldn't be any need to call kthread_stop -- and in fact it wouldn't 
> > work, as the thread waits on a semaphore while it is idle (kthread_stop 
> > can't cope with things like that).
> 
> Well I was assuming that the semaphore would go away as well.  Kernel
> threads normally use waitqueues to await more work.

Some kernel threads have a producer-consumer relationship with their
clients, and it's important that they wake exactly once each time they are
invoked.  A semaphore is the natural way to manage such a thread, but the
kthread API isn't set up to handle such things.  It's possible to make
this work, by using a manual poor-man's semaphore implementation, but that
seems ridiculous.

Would this patch be acceptable?

Alan Stern



Enhance the kthread API so that it can be used with threads that wait on a 
semaphore when they are idle.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

--- a/include/linux/kthread.h	Mon Sep 12 23:12:09 2005
+++ b/include/linux/kthread.h	Sat Sep 17 23:24:59 2005
@@ -67,7 +67,19 @@
  *
  * Returns the result of threadfn(), or -EINTR if wake_up_process()
  * was never called. */
-int kthread_stop(struct task_struct *k);
+#define kthread_stop(k)		kthread_stop_sem(k, NULL)
+
+/**
+ * kthread_stop_sem: stop a thread created by kthread_create().
+ * @k: thread created by kthread_create().
+ * @s: semaphore that @k waits on while idle.
+ *
+ * Does essentially the same thing as kthread_stop() above, but wakes
+ * @k by calling up(@s).
+ *
+ * Returns the result of threadfn(), or -EINTR if wake_up_process()
+ * was never called. */
+int kthread_stop_sem(struct task_struct *k, struct semaphore *s);
 
 /**
  * kthread_should_stop: should this kthread return now?
--- a/kernel/kthread.c	Mon Sep 12 23:12:09 2005
+++ b/kernel/kthread.c	Sat Sep 17 23:19:08 2005
@@ -163,7 +163,7 @@
 }
 EXPORT_SYMBOL(kthread_bind);
 
-int kthread_stop(struct task_struct *k)
+int kthread_stop_sem(struct task_struct *k, struct semaphore *s)
 {
 	int ret;
 
@@ -178,7 +178,10 @@
 
 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
-	wake_up_process(k);
+	if (s)
+		up(s);
+	else
+		wake_up_process(k);
 	put_task_struct(k);
 
 	/* Once it dies, reset stop ptr, gather result and we're done. */
@@ -189,7 +192,7 @@
 
 	return ret;
 }
-EXPORT_SYMBOL(kthread_stop);
+EXPORT_SYMBOL(kthread_stop_sem);
 
 static __init int helper_init(void)
 {

Re: Unusually long delay in the kernel

[Andrew Morton]

Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Sat, 17 Sep 2005, Andrew Morton wrote:
> 
>  > > > That code could be converted to the kthread API btw.
>  > > 
>  > > Hmph.  Near as I can tell, the only changes that would involve are:
>  > > 
>  > > 	Converting the thread creation call from kernel_thread to
>  > > 	kthread_run.
>  > > 
>  > > 	Adding another call to wake the thread up once it has been
>  > > 	created.
>  > > 
>  > > 	Removing the call to daemonize.
>  > > 
>  > > There wouldn't be any need to call kthread_stop -- and in fact it wouldn't 
>  > > work, as the thread waits on a semaphore while it is idle (kthread_stop 
>  > > can't cope with things like that).
>  > 
>  > Well I was assuming that the semaphore would go away as well.  Kernel
>  > threads normally use waitqueues to await more work.
> 
>  Some kernel threads have a producer-consumer relationship with their
>  clients, and it's important that they wake exactly once each time they are
>  invoked.  A semaphore is the natural way to manage such a thread, but the
>  kthread API isn't set up to handle such things.  It's possible to make
>  this work, by using a manual poor-man's semaphore implementation, but that
>  seems ridiculous.

OK.

>  Would this patch be acceptable?

Well it makes all kthread_stop() callers pass an additional (unused)
argument.  I'd make kthread_stop() and kthread_stop_sem() real C functions,
hide the code sharing within kthread.c.

[Proposed PATCH] Add kthread_stop_sem

[Alan Stern]

On Sun, 18 Sep 2005, Andrew Morton wrote:

> >  Would this patch be acceptable?
> 
> Well it makes all kthread_stop() callers pass an additional (unused)
> argument.  I'd make kthread_stop() and kthread_stop_sem() real C functions,
> hide the code sharing within kthread.c.

This may not be needed anywhere, since James Bottomley has said that the
SCSI error handler thread doesn't need a strict one-invocation <->
one-iteration relation.  I'll post it anyway just in case someone thinks
it may come in handy later.  At the moment the new routine has no callers.

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Enlarge the kthread API by adding kthread_stop_sem, for use in stopping 
threads that spend their idle time waiting on a semaphore.

Index: usb-2.6/include/linux/kthread.h
===================================================================
--- usb-2.6.orig/include/linux/kthread.h
+++ usb-2.6/include/linux/kthread.h
@@ -70,6 +70,18 @@ void kthread_bind(struct task_struct *k,
 int kthread_stop(struct task_struct *k);
 
 /**
+ * kthread_stop_sem: stop a thread created by kthread_create().
+ * @k: thread created by kthread_create().
+ * @s: semaphore that @k waits on while idle.
+ *
+ * Does essentially the same thing as kthread_stop() above, but wakes
+ * @k by calling up(@s).
+ *
+ * Returns the result of threadfn(), or -EINTR if wake_up_process()
+ * was never called. */
+int kthread_stop_sem(struct task_struct *k, struct semaphore *s);
+
+/**
  * kthread_should_stop: should this kthread return now?
  *
  * When someone calls kthread_stop on your kthread, it will be woken
Index: usb-2.6/kernel/kthread.c
===================================================================
--- usb-2.6.orig/kernel/kthread.c
+++ usb-2.6/kernel/kthread.c
@@ -165,6 +165,12 @@ EXPORT_SYMBOL(kthread_bind);
 
 int kthread_stop(struct task_struct *k)
 {
+	return kthread_stop_sem(k, NULL);
+}
+EXPORT_SYMBOL(kthread_stop);
+
+int kthread_stop_sem(struct task_struct *k, struct semaphore *s)
+{
 	int ret;
 
 	down(&kthread_stop_lock);
@@ -178,7 +184,10 @@ int kthread_stop(struct task_struct *k)
 
 	/* Now set kthread_should_stop() to true, and wake it up. */
 	kthread_stop_info.k = k;
-	wake_up_process(k);
+	if (s)
+		up(s);
+	else
+		wake_up_process(k);
 	put_task_struct(k);
 
 	/* Once it dies, reset stop ptr, gather result and we're done. */
@@ -189,7 +198,7 @@ int kthread_stop(struct task_struct *k)
 
 	return ret;
 }
-EXPORT_SYMBOL(kthread_stop);
+EXPORT_SYMBOL(kthread_stop_sem);
 
 static __init int helper_init(void)
 {

Re: Unusually long delay in the kernel

[Alan Stern]

On Sat, 17 Sep 2005, Andrew Morton wrote:

> Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > > Presumably it's spinning on the bkl.  Is this actually an SMP machine?  If
> > > so, perhaps some other process is holding the bkl for a long time.  Perhaps
> > > a netdevice spending a long time diddling hardware in an ioctl, something
> > > like that.
> > 
> > I need to do more precise tests.  Some quick informal tests indicated that 
> > the lock_kernel call and the daemonize call each took a noticeable time.  
> 
> Something odd is happening.

Forget about this.  It turned out to be an unexpected side effect from the 
problems with the SCSI error handler.  Once that was fixed, the delay went 
away.

Alan Stern