Re: [PATCH 01/19] freezer: don't get over-anxious while waiting

Re: [PATCH 01/19] freezer: don't get over-anxious while waiting

[Rafael J. Wysocki]

On Thursday 01 October 2009, Pavel Machek wrote:
> 
> > Freezing isn't exactly the most latency sensitive operation and
> > there's no reason to burn cpu cycles and power waiting for it to
> > complete.  msleep(10) instead of yield().  This should improve
> > reliability of emergency hibernation.
> 
> i don't see how it improves reliability, but its probably ok.
> 
> Well... for hibernation anyway. I can imagine cgroup users where
> freeze is so fast that this matters. rjw cc-ed.		pavel

Thanks.  I'd like to hear from the cgroup freezer people about that.

> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > ---
> >  kernel/power/process.c |   13 +++++++++----
> >  1 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/power/process.c b/kernel/power/process.c
> > index cc2e553..9d26a0a 100644
> > --- a/kernel/power/process.c
> > +++ b/kernel/power/process.c
> > @@ -41,7 +41,7 @@ static int try_to_freeze_tasks(bool sig_only)
> >  	do_gettimeofday(&start);
> >  
> >  	end_time = jiffies + TIMEOUT;
> > -	do {
> > +	while (true) {
> >  		todo = 0;
> >  		read_lock(&tasklist_lock);
> >  		do_each_thread(g, p) {
> > @@ -62,10 +62,15 @@ static int try_to_freeze_tasks(bool sig_only)
> >  				todo++;
> >  		} while_each_thread(g, p);
> >  		read_unlock(&tasklist_lock);
> > -		yield();			/* Yield is okay here */
> > -		if (time_after(jiffies, end_time))
> > +		if (!todo || time_after(jiffies, end_time))
> >  			break;
> > -	} while (todo);
> > +
> > +		/*
> > +		 * We need to retry.  There's no reason to be
> > +		 * over-anxious about it and waste power.
> > +		 */

The comment above looks like it's only meaningful in the context of the patch.
After it's been applied the meaning of the comment won't be so obvious, I'm
afraid.

> > +		msleep(10);
> > +	}
> >  
> >  	do_gettimeofday(&end);
> >  	elapsed_csecs64 = timeval_to_ns(&end) - timeval_to_ns(&start);

Thanks,
Rafael

Re: [PATCH 01/19] freezer: don't get over-anxious while waiting

[Tejun Heo]

Hello, Pavel, Rafael.

Rafael J. Wysocki wrote:
> On Thursday 01 October 2009, Pavel Machek wrote:
>>> Freezing isn't exactly the most latency sensitive operation and
>>> there's no reason to burn cpu cycles and power waiting for it to
>>> complete.  msleep(10) instead of yield().  This should improve
>>> reliability of emergency hibernation.
>> i don't see how it improves reliability, but its probably ok.

It's about battery.  When emergency hibernation kicks in and something
is taking a while to freeze (usually nfs does this for me) burning
power waiting for it to finish is a pretty bad idea.

>> Well... for hibernation anyway. I can imagine cgroup users where
>> freeze is so fast that this matters. rjw cc-ed.		pavel
> 
> Thanks.  I'd like to hear from the cgroup freezer people about that.

Oh... didn't know that.  10ms sleeps really matter there?

-- 
tejun

Re: [PATCH 01/19] freezer: don't get over-anxious while waiting

[Oren Laadan]

Rafael J. Wysocki wrote:
> On Thursday 01 October 2009, Pavel Machek wrote:
>>> Freezing isn't exactly the most latency sensitive operation and
>>> there's no reason to burn cpu cycles and power waiting for it to
>>> complete.  msleep(10) instead of yield().  This should improve
>>> reliability of emergency hibernation.
>> i don't see how it improves reliability, but its probably ok.
>>
>> Well... for hibernation anyway. I can imagine cgroup users where
>> freeze is so fast that this matters. rjw cc-ed.		pavel
> 
> Thanks.  I'd like to hear from the cgroup freezer people about that.
> 

[Adding Matt Helsley to the CC list]

To checkpoint or migrate an application, the cgroup to which it belongs
must be frozen first.

It's a bit down the road, but if one seeks minimum application downtime
during application checkpoint and/or migration, then a (minimum of)
10ms - or multiples of it - may result in a visible/undesired hick-up.

Perhaps avoid it when freezing a cgroup ?  or maybe a way for the user
to control this behavior per cgroup ?

Oren.

>>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>> ---
>>>  kernel/power/process.c |   13 +++++++++----
>>>  1 files changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/power/process.c b/kernel/power/process.c
>>> index cc2e553..9d26a0a 100644
>>> --- a/kernel/power/process.c
>>> +++ b/kernel/power/process.c
>>> @@ -41,7 +41,7 @@ static int try_to_freeze_tasks(bool sig_only)
>>>  	do_gettimeofday(&start);
>>>  
>>>  	end_time = jiffies + TIMEOUT;
>>> -	do {
>>> +	while (true) {
>>>  		todo = 0;
>>>  		read_lock(&tasklist_lock);
>>>  		do_each_thread(g, p) {
>>> @@ -62,10 +62,15 @@ static int try_to_freeze_tasks(bool sig_only)
>>>  				todo++;
>>>  		} while_each_thread(g, p);
>>>  		read_unlock(&tasklist_lock);
>>> -		yield();			/* Yield is okay here */
>>> -		if (time_after(jiffies, end_time))
>>> +		if (!todo || time_after(jiffies, end_time))
>>>  			break;
>>> -	} while (todo);
>>> +
>>> +		/*
>>> +		 * We need to retry.  There's no reason to be
>>> +		 * over-anxious about it and waste power.
>>> +		 */
> 
> The comment above looks like it's only meaningful in the context of the patch.
> After it's been applied the meaning of the comment won't be so obvious, I'm
> afraid.
> 
>>> +		msleep(10);
>>> +	}
>>>  
>>>  	do_gettimeofday(&end);
>>>  	elapsed_csecs64 = timeval_to_ns(&end) - timeval_to_ns(&start);
> 
> Thanks,
> Rafael

Re: [PATCH 01/19] freezer: don't get over-anxious while waiting

[Matt Helsley]

On Fri, Oct 02, 2009 at 03:47:14PM -0400, Oren Laadan wrote:
> 
> 
> Rafael J. Wysocki wrote:
> > On Thursday 01 October 2009, Pavel Machek wrote:
> >>> Freezing isn't exactly the most latency sensitive operation and
> >>> there's no reason to burn cpu cycles and power waiting for it to
> >>> complete.  msleep(10) instead of yield().  This should improve
> >>> reliability of emergency hibernation.
> >> i don't see how it improves reliability, but its probably ok.

>From what little of the patch I can see at this point I agree. 
On a single cpu system the yield gives up the cpu so other tasks
are more likely to make the progress necessary to become freezable.

> >>
> >> Well... for hibernation anyway. I can imagine cgroup users where
> >> freeze is so fast that this matters. rjw cc-ed.		pavel

It doesn't (more below), though I appreciate your keeping us in mind.

> > 
> > Thanks.  I'd like to hear from the cgroup freezer people about that.
> > 
> 
> [Adding Matt Helsley to the CC list]
> 
> To checkpoint or migrate an application, the cgroup to which it belongs
> must be frozen first.
> 
> It's a bit down the road, but if one seeks minimum application downtime
> during application checkpoint and/or migration, then a (minimum of)
> 10ms - or multiples of it - may result in a visible/undesired hick-up.
> 
> Perhaps avoid it when freezing a cgroup ?  or maybe a way for the user
> to control this behavior per cgroup ?

This is already the case.

The cgroup freezer does not use this yield-loop to iterate over all the tasks.
Instead of yield() the cgroup freezer has its own "loop". It changes its
own state to FREEZING and returns to userspace so that userspace can decide
what to do -- sleep? keep trying to freeze? go back to THAWED? etc. 

[ In the future this may change depending on the blocking/non-blocking
flag of the open freezer.state cgroup file handle. ]

Cheers,
	-Matt Helsley

> 
> Oren.
> 
> >>> Signed-off-by: Tejun Heo <tj@kernel.org>
> >>> ---
> >>>  kernel/power/process.c |   13 +++++++++----
> >>>  1 files changed, 9 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/kernel/power/process.c b/kernel/power/process.c
> >>> index cc2e553..9d26a0a 100644
> >>> --- a/kernel/power/process.c
> >>> +++ b/kernel/power/process.c
> >>> @@ -41,7 +41,7 @@ static int try_to_freeze_tasks(bool sig_only)
> >>>  	do_gettimeofday(&start);
> >>>  
> >>>  	end_time = jiffies + TIMEOUT;
> >>> -	do {
> >>> +	while (true) {
> >>>  		todo = 0;
> >>>  		read_lock(&tasklist_lock);
> >>>  		do_each_thread(g, p) {
> >>> @@ -62,10 +62,15 @@ static int try_to_freeze_tasks(bool sig_only)
> >>>  				todo++;
> >>>  		} while_each_thread(g, p);
> >>>  		read_unlock(&tasklist_lock);
> >>> -		yield();			/* Yield is okay here */
> >>> -		if (time_after(jiffies, end_time))
> >>> +		if (!todo || time_after(jiffies, end_time))
> >>>  			break;
> >>> -	} while (todo);
> >>> +
> >>> +		/*
> >>> +		 * We need to retry.  There's no reason to be
> >>> +		 * over-anxious about it and waste power.
> >>> +		 */
> > 
> > The comment above looks like it's only meaningful in the context of the patch.
> > After it's been applied the meaning of the comment won't be so obvious, I'm
> > afraid.
> > 
> >>> +		msleep(10);
> >>> +	}
> >>>  
> >>>  	do_gettimeofday(&end);
> >>>  	elapsed_csecs64 = timeval_to_ns(&end) - timeval_to_ns(&start);
> > 
> > Thanks,
> > Rafael

Re: [PATCH 01/19] freezer: don't get over-anxious while waiting

[Rafael J. Wysocki]

On Friday 02 October 2009, Matt Helsley wrote:
> On Fri, Oct 02, 2009 at 03:47:14PM -0400, Oren Laadan wrote:
> > 
> > 
> > Rafael J. Wysocki wrote:
> > > On Thursday 01 October 2009, Pavel Machek wrote:
> > >>> Freezing isn't exactly the most latency sensitive operation and
> > >>> there's no reason to burn cpu cycles and power waiting for it to
> > >>> complete.  msleep(10) instead of yield().  This should improve
> > >>> reliability of emergency hibernation.
> > >> i don't see how it improves reliability, but its probably ok.
> 
> From what little of the patch I can see at this point I agree. 
> On a single cpu system the yield gives up the cpu so other tasks
> are more likely to make the progress necessary to become freezable.
> 
> > >>
> > >> Well... for hibernation anyway. I can imagine cgroup users where
> > >> freeze is so fast that this matters. rjw cc-ed.		pavel
> 
> It doesn't (more below), though I appreciate your keeping us in mind.
> 
> > > 
> > > Thanks.  I'd like to hear from the cgroup freezer people about that.
> > > 
> > 
> > [Adding Matt Helsley to the CC list]
> > 
> > To checkpoint or migrate an application, the cgroup to which it belongs
> > must be frozen first.
> > 
> > It's a bit down the road, but if one seeks minimum application downtime
> > during application checkpoint and/or migration, then a (minimum of)
> > 10ms - or multiples of it - may result in a visible/undesired hick-up.
> > 
> > Perhaps avoid it when freezing a cgroup ?  or maybe a way for the user
> > to control this behavior per cgroup ?
> 
> This is already the case.
> 
> The cgroup freezer does not use this yield-loop to iterate over all the tasks.
> Instead of yield() the cgroup freezer has its own "loop". It changes its
> own state to FREEZING and returns to userspace so that userspace can decide
> what to do -- sleep? keep trying to freeze? go back to THAWED? etc. 
> 
> [ In the future this may change depending on the blocking/non-blocking
> flag of the open freezer.state cgroup file handle. ]

OK, thanks for the info.

Tejun, do you want me to take the patch or is it more convenient to you to
push it yourself?

Also, care to respond to my previous comment?

Rafael

Re: [PATCH 01/19] freezer: don't get over-anxious while waiting

[Tejun Heo]

Hello, Rafael.

Rafael J. Wysocki wrote:
> Tejun, do you want me to take the patch or is it more convenient to you to
> push it yourself?

Please take the patch.  As long as it's in a stable git tree, it
should be fine for me.

> Also, care to respond to my previous comment?

About the comment?  Dropping it is fine.  Do you want me to resend the
patch w/o the comment?

Thanks.

-- 
tejun

Re: [PATCH 01/19] freezer: don't get over-anxious while waiting

[Rafael J. Wysocki]

On Saturday 03 October 2009, Tejun Heo wrote:
> Hello, Rafael.
> 
> Rafael J. Wysocki wrote:
> > Tejun, do you want me to take the patch or is it more convenient to you to
> > push it yourself?
> 
> Please take the patch.  As long as it's in a stable git tree, it
> should be fine for me.

OK, I'll put it into suspend-2.6/for-linus, but first it'll stay in
suspend-2.6/linux-next for a few days.

> > Also, care to respond to my previous comment?
> 
> About the comment?  Dropping it is fine.  Do you want me to resend the
> patch w/o the comment?

No, thanks. 

Best,
Rafael