windfarm got signal

windfarm got signal

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 627 bytes --]

Hey,

after cpu hotplug I decided to write some fake suspend routines for
ppc64 that always fail to see what all the drivers would say... The
first thing I saw was during the phase where all threads are stopped,
that windfarm got a signal!

Shortly after that, the fans were revved up fully but I guess that's
expected if the wf control loop exits.

So now I'm trying to see *why* it got a signal there. Any ideas? Is that
expected with pm and windfarm just does the wrong thing there by taking
the signal as a reason to exit the control thread?

[code in question is windfarm_core.c:wf_thread_func]

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

windfarm got signal

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

Hey,

after cpu hotplug I decided to write some fake suspend routines for
ppc64 that always fail to see what all the drivers would say... The
first thing I saw was during the phase where all threads are stopped,
that windfarm got a signal!

Shortly after that, the fans were revved up fully but I guess that's
expected if the wf control loop exits.

So now I'm trying to see *why* it got a signal there. Any ideas? Is that
expected with pm and windfarm just does the wrong thing there by taking
the signal as a reason to exit the control thread?

[code in question is windfarm_core.c:wf_thread_func]

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: windfarm got signal

[Benjamin Herrenschmidt]

On Wed, 2006-06-21 at 01:51 +0200, Johannes Berg wrote:
> Hey,
> 
> after cpu hotplug I decided to write some fake suspend routines for
> ppc64 that always fail to see what all the drivers would say... The
> first thing I saw was during the phase where all threads are stopped,
> that windfarm got a signal!
> 
> Shortly after that, the fans were revved up fully but I guess that's
> expected if the wf control loop exits.
> 
> So now I'm trying to see *why* it got a signal there. Any ideas? Is that
> expected with pm and windfarm just does the wrong thing there by taking
> the signal as a reason to exit the control thread?
> 
> [code in question is windfarm_core.c:wf_thread_func]

I think it's the way the freezer works ... it sends a pseudo signal to
all kernel threads who are then supposed to do something like test for
PF_FREEZE or something like that.

Ben.

Re: windfarm got signal

[Rafael J. Wysocki]

On Thursday 22 June 2006 08:02, Benjamin Herrenschmidt wrote:
> On Wed, 2006-06-21 at 01:51 +0200, Johannes Berg wrote:
> > Hey,
> > 
> > after cpu hotplug I decided to write some fake suspend routines for
> > ppc64 that always fail to see what all the drivers would say... The
> > first thing I saw was during the phase where all threads are stopped,
> > that windfarm got a signal!
> > 
> > Shortly after that, the fans were revved up fully but I guess that's
> > expected if the wf control loop exits.
> > 
> > So now I'm trying to see *why* it got a signal there. Any ideas? Is that
> > expected with pm and windfarm just does the wrong thing there by taking
> > the signal as a reason to exit the control thread?
> > 
> > [code in question is windfarm_core.c:wf_thread_func]
> 
> I think it's the way the freezer works ... it sends a pseudo signal to
> all kernel threads who are then supposed to do something like test for
> PF_FREEZE or something like that.

Yes.  More precisely, they are supposed to use try_to_freeze().

Please see Documentation/power/kernel_threads.txt.

Greetings,
Rafael

Re: [linux-pm] windfarm got signal

[Rafael J. Wysocki]

On Thursday 22 June 2006 08:02, Benjamin Herrenschmidt wrote:
> On Wed, 2006-06-21 at 01:51 +0200, Johannes Berg wrote:
> > Hey,
> > 
> > after cpu hotplug I decided to write some fake suspend routines for
> > ppc64 that always fail to see what all the drivers would say... The
> > first thing I saw was during the phase where all threads are stopped,
> > that windfarm got a signal!
> > 
> > Shortly after that, the fans were revved up fully but I guess that's
> > expected if the wf control loop exits.
> > 
> > So now I'm trying to see *why* it got a signal there. Any ideas? Is that
> > expected with pm and windfarm just does the wrong thing there by taking
> > the signal as a reason to exit the control thread?
> > 
> > [code in question is windfarm_core.c:wf_thread_func]
> 
> I think it's the way the freezer works ... it sends a pseudo signal to
> all kernel threads who are then supposed to do something like test for
> PF_FREEZE or something like that.

Yes.  More precisely, they are supposed to use try_to_freeze().

Please see Documentation/power/kernel_threads.txt.

Greetings,
Rafael

Re: [linux-pm] windfarm got signal

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 496 bytes --]

On Thu, 2006-06-22 at 13:03 +0200, Rafael J. Wysocki wrote:
> > I think it's the way the freezer works ... it sends a pseudo signal to
> > all kernel threads who are then supposed to do something like test for
> > PF_FREEZE or something like that.
> 
> Yes.  More precisely, they are supposed to use try_to_freeze().
> 
> Please see Documentation/power/kernel_threads.txt.

Thanks, I'll look and submit a patch. It does try_to_freeze() but also
checks for pending signals.

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

[-- Attachment #2: Type: text/plain, Size: 146 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [linux-pm] windfarm got signal

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

On Thu, 2006-06-22 at 13:03 +0200, Rafael J. Wysocki wrote:
> > I think it's the way the freezer works ... it sends a pseudo signal to
> > all kernel threads who are then supposed to do something like test for
> > PF_FREEZE or something like that.
> 
> Yes.  More precisely, they are supposed to use try_to_freeze().
> 
> Please see Documentation/power/kernel_threads.txt.

Thanks, I'll look and submit a patch. It does try_to_freeze() but also
checks for pending signals.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: [linux-pm] windfarm got signal

[Johannes Berg]

[-- Attachment #1.1: Type: text/plain, Size: 457 bytes --]

On Thu, 2006-06-22 at 13:13 +0200, Johannes Berg wrote:

> Thanks, I'll look and submit a patch. It does try_to_freeze() but also
> checks for pending signals.

Ah. The code is just in the wrong order:
while (!kthread_should_stop()) {
  try_to_freeze();
  ...
  schedule_timeout_interruptible(...);
  if (signal_pending())
    break;
}

Would it be correct to just move the try_to_freeze() before the
signal_pending() statement?

johannes

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

[-- Attachment #2: Type: text/plain, Size: 146 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [linux-pm] windfarm got signal

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

On Thu, 2006-06-22 at 13:13 +0200, Johannes Berg wrote:

> Thanks, I'll look and submit a patch. It does try_to_freeze() but also
> checks for pending signals.

Ah. The code is just in the wrong order:
while (!kthread_should_stop()) {
  try_to_freeze();
  ...
  schedule_timeout_interruptible(...);
  if (signal_pending())
    break;
}

Would it be correct to just move the try_to_freeze() before the
signal_pending() statement?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 793 bytes --]

Re: windfarm got signal

[Nigel Cunningham]

[-- Attachment #1.1: Type: text/plain, Size: 1028 bytes --]

Hi.

On Thursday 22 June 2006 21:34, Johannes Berg wrote:
> On Thu, 2006-06-22 at 13:13 +0200, Johannes Berg wrote:
> > Thanks, I'll look and submit a patch. It does try_to_freeze() but also
> > checks for pending signals.
>
> Ah. The code is just in the wrong order:
> while (!kthread_should_stop()) {
>   try_to_freeze();
>   ...
>   schedule_timeout_interruptible(...);
>   if (signal_pending())
>     break;
> }
>
> Would it be correct to just move the try_to_freeze() before the
> signal_pending() statement?

Hmm. Will kthread_should_stop() be set if it should really exit? If so, 
perhaps you can just remove the signal_pending() check. Otherwise, you'd want 
to change the signal_pending() test to something like the "if 
(signal_pending() && !try_to_freeze())" to avoid any possibility of a race. 
(In this case, you should be able to remove the try_to_freeze() from above).

Regards,

Nigel
-- 
Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [linux-pm] windfarm got signal

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

Hi.

On Thursday 22 June 2006 21:34, Johannes Berg wrote:
> On Thu, 2006-06-22 at 13:13 +0200, Johannes Berg wrote:
> > Thanks, I'll look and submit a patch. It does try_to_freeze() but also
> > checks for pending signals.
>
> Ah. The code is just in the wrong order:
> while (!kthread_should_stop()) {
>   try_to_freeze();
>   ...
>   schedule_timeout_interruptible(...);
>   if (signal_pending())
>     break;
> }
>
> Would it be correct to just move the try_to_freeze() before the
> signal_pending() statement?

Hmm. Will kthread_should_stop() be set if it should really exit? If so, 
perhaps you can just remove the signal_pending() check. Otherwise, you'd want 
to change the signal_pending() test to something like the "if 
(signal_pending() && !try_to_freeze())" to avoid any possibility of a race. 
(In this case, you should be able to remove the try_to_freeze() from above).

Regards,

Nigel
-- 
Nigel, Michelle and Alisdair Cunningham
5 Mitchell Street
Cobden 3266
Victoria, Australia

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [linux-pm] windfarm got signal

[Benjamin Herrenschmidt]

On Thu, 2006-06-22 at 13:34 +0200, Johannes Berg wrote:
> On Thu, 2006-06-22 at 13:13 +0200, Johannes Berg wrote:
> 
> > Thanks, I'll look and submit a patch. It does try_to_freeze() but also
> > checks for pending signals.
> 
> Ah. The code is just in the wrong order:

The code should be

while (!kthread_should_stop()) {
  try_to_freeze();
  ...
  schedule_timeout_interruptible(...);
}

That is, I think we just don't care about the signal stuff.

Care to do a new patch ?

Cheers,
Ben.

[PATCH] fix windfarm core thread wrt. signal handling

[Johannes Berg]

This patch removes the signal_pending() from the windfarm core thread
loop as it isn't necessary and messes up when there actually *is* a
signal pending because we should enter the freezer.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
index ab3faa7..c05a838 100644
--- a/drivers/macintosh/windfarm_core.c
+++ b/drivers/macintosh/windfarm_core.c
@@ -116,12 +116,6 @@ static int wf_thread_func(void *data)
 		delay = next - jiffies;
 		if (delay <= HZ)
 			schedule_timeout_interruptible(delay);
-
-		/* there should be no signal, but oh well */
-		if (signal_pending(current)) {
-			printk(KERN_WARNING "windfarm: thread got sigl !\n");
-			break;
-		}
 	}
 
 	DBG("wf: thread stopped\n");

Re: [PATCH] fix windfarm core thread wrt. signal handling

[Benjamin Herrenschmidt]

On Tue, 2006-06-27 at 20:17 +0200, Johannes Berg wrote:
> This patch removes the signal_pending() from the windfarm core thread
> loop as it isn't necessary and messes up when there actually *is* a
> signal pending because we should enter the freezer.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
> index ab3faa7..c05a838 100644
> --- a/drivers/macintosh/windfarm_core.c
> +++ b/drivers/macintosh/windfarm_core.c
> @@ -116,12 +116,6 @@ static int wf_thread_func(void *data)
>  		delay = next - jiffies;
>  		if (delay <= HZ)
>  			schedule_timeout_interruptible(delay);
> -
> -		/* there should be no signal, but oh well */
> -		if (signal_pending(current)) {
> -			printk(KERN_WARNING "windfarm: thread got sigl !\n");
> -			break;
> -		}
>  	}
>  
>  	DBG("wf: thread stopped\n");
> 

Re: [PATCH] fix windfarm core thread wrt. signal handling

[Benjamin Herrenschmidt]

On Tue, 2006-06-27 at 20:17 +0200, Johannes Berg wrote:
> This patch removes the signal_pending() from the windfarm core thread
> loop as it isn't necessary and messes up when there actually *is* a
> signal pending because we should enter the freezer.
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> diff --git a/drivers/macintosh/windfarm_core.c b/drivers/macintosh/windfarm_core.c
> index ab3faa7..c05a838 100644
> --- a/drivers/macintosh/windfarm_core.c
> +++ b/drivers/macintosh/windfarm_core.c
> @@ -116,12 +116,6 @@ static int wf_thread_func(void *data)
>  		delay = next - jiffies;
>  		if (delay <= HZ)
>  			schedule_timeout_interruptible(delay);
> -
> -		/* there should be no signal, but oh well */
> -		if (signal_pending(current)) {
> -			printk(KERN_WARNING "windfarm: thread got sigl !\n");
> -			break;
> -		}
>  	}
>  
>  	DBG("wf: thread stopped\n");
> 

Re: windfarm got signal

[Pavel Machek]

Hi!

> after cpu hotplug I decided to write some fake suspend routines for
> ppc64 that always fail to see what all the drivers would say... The
> first thing I saw was during the phase where all threads are stopped,
> that windfarm got a signal!
> 
> Shortly after that, the fans were revved up fully but I guess that's
> expected if the wf control loop exits.
> 
> So now I'm trying to see *why* it got a signal there. Any ideas? Is that
> expected with pm and windfarm just does the wrong thing there by taking
> the signal as a reason to exit the control thread?
> 
> [code in question is windfarm_core.c:wf_thread_func]

See kernel/power/process.c

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [linux-pm] windfarm got signal

[Pavel Machek]

Hi!

> after cpu hotplug I decided to write some fake suspend routines for
> ppc64 that always fail to see what all the drivers would say... The
> first thing I saw was during the phase where all threads are stopped,
> that windfarm got a signal!
> 
> Shortly after that, the fans were revved up fully but I guess that's
> expected if the wf control loop exits.
> 
> So now I'm trying to see *why* it got a signal there. Any ideas? Is that
> expected with pm and windfarm just does the wrong thing there by taking
> the signal as a reason to exit the control thread?
> 
> [code in question is windfarm_core.c:wf_thread_func]

See kernel/power/process.c

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

[PATCH] windfarm: proper try_to_freeze / signal_pending handling

[Johannes Berg]

This seems to work for me:
---
This patch makes windfarm handle signal_pending()/try_to_freeze()
better.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>

--- linux-2.6-git.orig/drivers/macintosh/windfarm_core.c	2006-06-22 16:41:18.182172154 +0200
+++ linux-2.6-git/drivers/macintosh/windfarm_core.c	2006-06-22 16:42:49.045354378 +0200
@@ -93,8 +93,6 @@ static int wf_thread_func(void *data)
 	DBG("wf: thread started\n");
 
 	while(!kthread_should_stop()) {
-		try_to_freeze();
-
 		if (time_after_eq(jiffies, next)) {
 			wf_notify(WF_EVENT_TICK, NULL);
 			if (wf_overtemp) {
@@ -117,8 +115,8 @@ static int wf_thread_func(void *data)
 		if (delay <= HZ)
 			schedule_timeout_interruptible(delay);
 
-		/* there should be no signal, but oh well */
-		if (signal_pending(current)) {
+		/* there should be no non-suspend signal, but oh well */
+		if (signal_pending(current) && !try_to_freeze()) {
 			printk(KERN_WARNING "windfarm: thread got sigl !\n");
 			break;
 		}