[PATCH] Quieten Freezer if !CONFIG_PM_DEBUG

[PATCH] Quieten Freezer if !CONFIG_PM_DEBUG

[Nigel Cunningham]

The freezer currently prints an '=' for every process that is frozen.
This is pretty pointless, as the equals sign says nothing about which
process is frozen, and makes logs look messier (especially if there were
a large number of processes running). All we really need to know is that
we started trying to freeze processes and what processes (if any) failed
to freeze, or that we succeeded.

Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 29be608..b0edfc6 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -40,7 +40,6 @@ void refrigerator(void)
 	long save;
 	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
-	printk("=");
 
 	frozen_process(current);
 	spin_lock_irq(&current->sighand->siglock);

Re: [PATCH] Quieten Freezer if !CONFIG_PM_DEBUG

[Pavel Machek]

Hi!

> The freezer currently prints an '=' for every process that is frozen.
> This is pretty pointless, as the equals sign says nothing about which
> process is frozen, and makes logs look messier (especially if there were
> a large number of processes running). All we really need to know is that
> we started trying to freeze processes and what processes (if any) failed
> to freeze, or that we succeeded.

> Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

ACK.
								Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] Quieten freezer if !CONFIG_PM_DEBUG.

[Nigel Cunningham]

The freezing of processes is currently very noisy. This patch makes the
noise dependant upon CONFIG_PM_DEBUG.

Prepared against current git.
    
Signed-off-by: Nigel Cunningham <nigel@suspend2.net>

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 29be608..6829612 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -15,6 +15,12 @@ #include <linux/module.h>
 #include <linux/syscalls.h>
 #include <linux/freezer.h>
 
+#ifdef CONFIG_PM_DEBUG
+#define freezer_message(msg, a...) do { printk(msg, ##a); } while(0)
+#else
+#define freezer_message(msg, a...) do { } while(0)
+#endif
+
 /* 
  * Timeout for stopping processes
  */
@@ -40,7 +46,7 @@ void refrigerator(void)
 	long save;
 	save = current->state;
 	pr_debug("%s entered refrigerator\n", current->comm);
-	printk("=");
+	freezer_message("=");
 
 	frozen_process(current);
 	spin_lock_irq(&current->sighand->siglock);
@@ -87,7 +93,7 @@ int freeze_processes(void)
 	unsigned long start_time;
 	struct task_struct *g, *p;
 
-	printk( "Stopping tasks: " );
+	freezer_message( "Stopping tasks: " );
 	start_time = jiffies;
 	user_frozen = 0;
 	do {
@@ -135,7 +141,7 @@ int freeze_processes(void)
 	 * but it cleans up leftover PF_FREEZE requests.
 	 */
 	if (todo) {
-		printk( "\n" );
+		freezer_message( "\n" );
 		printk(KERN_ERR " stopping tasks timed out "
 			"after %d seconds (%d tasks remaining):\n",
 			TIMEOUT / HZ, todo);
@@ -149,7 +155,7 @@ int freeze_processes(void)
 		return todo;
 	}
 
-	printk( "|\n" );
+	freezer_message( "|\n" );
 	BUG_ON(in_atomic());
 	return 0;
 }
@@ -158,18 +164,18 @@ void thaw_processes(void)
 {
 	struct task_struct *g, *p;
 
-	printk( "Restarting tasks..." );
+	freezer_message( "Restarting tasks..." );
 	read_lock(&tasklist_lock);
 	do_each_thread(g, p) {
 		if (!freezeable(p))
 			continue;
 		if (!thaw_process(p))
-			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
+			freezer_message(KERN_INFO " Strange, %s not stopped\n", p->comm );
 	} while_each_thread(g, p);
 
 	read_unlock(&tasklist_lock);
 	schedule();
-	printk( " done\n" );
+	freezer_message( " done\n" );
 }
 
 EXPORT_SYMBOL(refrigerator);

Re: [PATCH] Quieten freezer if !CONFIG_PM_DEBUG.

[Rafael J. Wysocki]

On Saturday, 21 October 2006 14:22, Nigel Cunningham wrote:
> The freezing of processes is currently very noisy. This patch makes the
> noise dependant upon CONFIG_PM_DEBUG.

Well, I don't think it's _that_ noisy.  It only printks one character per
frozen task.

In fact I think it at least should print "Freezing processes" and "done"
messages.
 
> Prepared against current git.
>     
> Signed-off-by: Nigel Cunningham <nigel@suspend2.net>
> 
> diff --git a/kernel/power/process.c b/kernel/power/process.c
> index 29be608..6829612 100644
> --- a/kernel/power/process.c
> +++ b/kernel/power/process.c
> @@ -15,6 +15,12 @@ #include <linux/module.h>
>  #include <linux/syscalls.h>
>  #include <linux/freezer.h>
>  
> +#ifdef CONFIG_PM_DEBUG
> +#define freezer_message(msg, a...) do { printk(msg, ##a); } while(0)
> +#else
> +#define freezer_message(msg, a...) do { } while(0)
> +#endif
> +
>  /* 
>   * Timeout for stopping processes
>   */
> @@ -40,7 +46,7 @@ void refrigerator(void)
>  	long save;
>  	save = current->state;
>  	pr_debug("%s entered refrigerator\n", current->comm);
> -	printk("=");
> +	freezer_message("=");

I'd prefer to treat the pr_debug thing in a similar way, like
freezer_debug_message("%s entered refrigerator\n" ...).

Or better yet, I'd leave just one message like

freezer_print_task(current->comm);

instead of the two that will be defined to either printk the entire
"%s entered refrigerator\n", ... message or printk the "=" character or do
nothing, depending on a predefined verbosity level.

>  
>  	frozen_process(current);
>  	spin_lock_irq(&current->sighand->siglock);
> @@ -87,7 +93,7 @@ int freeze_processes(void)
>  	unsigned long start_time;
>  	struct task_struct *g, *p;
>  
> -	printk( "Stopping tasks: " );
> +	freezer_message( "Stopping tasks: " );

I wouldn't change this.

>  	start_time = jiffies;
>  	user_frozen = 0;
>  	do {
> @@ -135,7 +141,7 @@ int freeze_processes(void)
>  	 * but it cleans up leftover PF_FREEZE requests.
>  	 */
>  	if (todo) {
> -		printk( "\n" );
> +		freezer_message( "\n" );

Ditto.

>  		printk(KERN_ERR " stopping tasks timed out "
>  			"after %d seconds (%d tasks remaining):\n",
>  			TIMEOUT / HZ, todo);
> @@ -149,7 +155,7 @@ int freeze_processes(void)
>  		return todo;
>  	}
>  
> -	printk( "|\n" );
> +	freezer_message( "|\n" );

I'd call it freezer_print_finish(); and define to printk the "|\n" or do
nothing like for freezer_print_task().

>  	BUG_ON(in_atomic());
>  	return 0;
>  }
> @@ -158,18 +164,18 @@ void thaw_processes(void)
>  {
>  	struct task_struct *g, *p;
>  
> -	printk( "Restarting tasks..." );

I wouldn't change this.

> +	freezer_message( "Restarting tasks..." );
>  	read_lock(&tasklist_lock);
>  	do_each_thread(g, p) {
>  		if (!freezeable(p))
>  			continue;
>  		if (!thaw_process(p))
> -			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
> +			freezer_message(KERN_INFO " Strange, %s not stopped\n", p->comm );
>  	} while_each_thread(g, p);
>  
>  	read_unlock(&tasklist_lock);
>  	schedule();
> -	printk( " done\n" );
> +	freezer_message( " done\n" );

Ditto.

>  }
>  
>  EXPORT_SYMBOL(refrigerator);
> 
> 
> -

Greetings,
Rafael


-- 
You never change things by fighting the existing reality.
		R. Buckminster Fuller

Re: [PATCH] Quieten freezer if !CONFIG_PM_DEBUG.

[Nigel Cunningham]

Hi.

On Sat, 2006-10-21 at 16:01 +0200, Rafael J. Wysocki wrote:
> On Saturday, 21 October 2006 14:22, Nigel Cunningham wrote:
> > The freezing of processes is currently very noisy. This patch makes the
> > noise dependant upon CONFIG_PM_DEBUG.
> 
> Well, I don't think it's _that_ noisy.  It only printks one character per
> frozen task.

Yeah, but it should just work. The only reason for it to printk should
be for information or if there's a possibility of deadlocking or failure
that isn't handled. Doing printks here is akin to doing a printk for
every page you write in the image, and I'm reasonably sure you're not
about to start doing that.

> In fact I think it at least should print "Freezing processes" and "done"
> messages.

That sounds better.
 
> > Prepared against current git.
> >     
> > Signed-off-by: Nigel Cunningham <nigel@suspend2.net>
> > 
> > diff --git a/kernel/power/process.c b/kernel/power/process.c
> > index 29be608..6829612 100644
> > --- a/kernel/power/process.c
> > +++ b/kernel/power/process.c
> > @@ -15,6 +15,12 @@ #include <linux/module.h>
> >  #include <linux/syscalls.h>
> >  #include <linux/freezer.h>
> >  
> > +#ifdef CONFIG_PM_DEBUG
> > +#define freezer_message(msg, a...) do { printk(msg, ##a); } while(0)
> > +#else
> > +#define freezer_message(msg, a...) do { } while(0)
> > +#endif
> > +
> >  /* 
> >   * Timeout for stopping processes
> >   */
> > @@ -40,7 +46,7 @@ void refrigerator(void)
> >  	long save;
> >  	save = current->state;
> >  	pr_debug("%s entered refrigerator\n", current->comm);
> > -	printk("=");
> > +	freezer_message("=");
> 
> I'd prefer to treat the pr_debug thing in a similar way, like
> freezer_debug_message("%s entered refrigerator\n" ...).
> 
> Or better yet, I'd leave just one message like
> 
> freezer_print_task(current->comm);
> 
> instead of the two that will be defined to either printk the entire
> "%s entered refrigerator\n", ... message or printk the "=" character or do
> nothing, depending on a predefined verbosity level.

Ah, yeah. Didn't notice the pr_debug thing. Will look at it.

> >  
> >  	frozen_process(current);
> >  	spin_lock_irq(&current->sighand->siglock);
> > @@ -87,7 +93,7 @@ int freeze_processes(void)
> >  	unsigned long start_time;
> >  	struct task_struct *g, *p;
> >  
> > -	printk( "Stopping tasks: " );
> > +	freezer_message( "Stopping tasks: " );
> 
> I wouldn't change this.
> 
> >  	start_time = jiffies;
> >  	user_frozen = 0;
> >  	do {
> > @@ -135,7 +141,7 @@ int freeze_processes(void)
> >  	 * but it cleans up leftover PF_FREEZE requests.
> >  	 */
> >  	if (todo) {
> > -		printk( "\n" );
> > +		freezer_message( "\n" );
> 
> Ditto.
> 
> >  		printk(KERN_ERR " stopping tasks timed out "
> >  			"after %d seconds (%d tasks remaining):\n",
> >  			TIMEOUT / HZ, todo);
> > @@ -149,7 +155,7 @@ int freeze_processes(void)
> >  		return todo;
> >  	}
> >  
> > -	printk( "|\n" );
> > +	freezer_message( "|\n" );
> 
> I'd call it freezer_print_finish(); and define to printk the "|\n" or do
> nothing like for freezer_print_task().
> 
> >  	BUG_ON(in_atomic());
> >  	return 0;
> >  }
> > @@ -158,18 +164,18 @@ void thaw_processes(void)
> >  {
> >  	struct task_struct *g, *p;
> >  
> > -	printk( "Restarting tasks..." );
> 
> I wouldn't change this.
> 
> > +	freezer_message( "Restarting tasks..." );
> >  	read_lock(&tasklist_lock);
> >  	do_each_thread(g, p) {
> >  		if (!freezeable(p))
> >  			continue;
> >  		if (!thaw_process(p))
> > -			printk(KERN_INFO " Strange, %s not stopped\n", p->comm );
> > +			freezer_message(KERN_INFO " Strange, %s not stopped\n", p->comm );
> >  	} while_each_thread(g, p);
> >  
> >  	read_unlock(&tasklist_lock);
> >  	schedule();
> > -	printk( " done\n" );
> > +	freezer_message( " done\n" );
> 
> Ditto.
> 
> >  }
> >  
> >  EXPORT_SYMBOL(refrigerator);

Will reconsider and resend tomorrow.

Regards,

Nigel