* khugepaged doesn't want to freeze
@ 2011-11-08 8:33 Jiri Slaby
2011-11-08 15:29 ` Andrea Arcangeli
0 siblings, 1 reply; 20+ messages in thread
From: Jiri Slaby @ 2011-11-08 8:33 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-pm, LKML, Jiri Slaby, linux-mm, Andrea Arcangeli,
Andrew Morton
Hi,
yesterday my machine refused to suspend several times. It was always due
to khugepaged refusing to freeze. It's with 3.1.0-next-20111025.
In the end, after several tries, it finally did. But it's indeed
bothering me.
PM: Syncing filesystems ... done.
PM: Preparing system for mem sleep
Freezing user space processes ... (elapsed 0.01 seconds) done.
Freezing remaining freezable tasks ...
Freezing of tasks failed after 20.01 seconds (1 tasks refusing to
freeze, wq_busy=0):
khugepaged S 0000000000000001 0 634 2 0x00800000
ffff8801c11cbc90 0000000000000046 0000000000000003 0000000000000000
ffff8801c16f73e0 ffff8801c11cbfd8 ffff8801c11cbfd8 ffff8801c11cbfd8
ffffffff81a0d020 ffff8801c16f73e0 ffff8801c11cbd08 0000000100000001
Call Trace:
[<ffffffff8161e3aa>] schedule+0x3a/0x50
[<ffffffff8161e83e>] schedule_timeout+0x14e/0x220
[<ffffffff81079710>] ? init_timer_deferrable_key+0x20/0x20
[<ffffffff8161e969>] schedule_timeout_interruptible+0x19/0x20
[<ffffffff8110eda0>] khugepaged_alloc_hugepage+0xc0/0xf0
[<ffffffff8108a0d0>] ? add_wait_queue+0x60/0x60
[<ffffffff8110f455>] khugepaged+0x85/0x1280
[<ffffffff8108a0d0>] ? add_wait_queue+0x60/0x60
[<ffffffff8110f3d0>] ? collect_mm_slot+0xa0/0xa0
[<ffffffff81089937>] kthread+0x87/0x90
[<ffffffff81621834>] kernel_thread_helper+0x4/0x10
[<ffffffff810898b0>] ? kthread_worker_fn+0x1a0/0x1a0
[<ffffffff81621830>] ? gs_change+0xb/0xb
Restarting tasks ... done.
thanks,
--
js
suse labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: khugepaged doesn't want to freeze
2011-11-08 8:33 khugepaged doesn't want to freeze Jiri Slaby
@ 2011-11-08 15:29 ` Andrea Arcangeli
2011-11-08 15:29 ` [PATCH] thp: reduce khugepaged freezing latency Andrea Arcangeli
0 siblings, 1 reply; 20+ messages in thread
From: Andrea Arcangeli @ 2011-11-08 15:29 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-pm, linux-kernel, Jiri Slaby, linux-mm, Andrew Morton
[PATCH] thp: reduce khugepaged freezing latency
Beware patch untested but I suspect the problem was the lack of
wakeup during long schedule_timeout. And a missing try_to_freeze in
case alloc_hugepage repeatedly fails in the CONFIG_NUMA=n case.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] thp: reduce khugepaged freezing latency
2011-11-08 15:29 ` Andrea Arcangeli
@ 2011-11-08 15:29 ` Andrea Arcangeli
2011-11-08 20:01 ` Srivatsa S. Bhat
0 siblings, 1 reply; 20+ messages in thread
From: Andrea Arcangeli @ 2011-11-08 15:29 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-pm, linux-kernel, Jiri Slaby, linux-mm, Andrew Morton
Lack of set_freezable_with_signal() prevented khugepaged to be waken
up (and prevented to sleep again) across the
schedule_timeout_interruptible() calls after freezing() becomes
true. The tight loop in khugepaged_alloc_hugepage() also missed one
try_to_freeze() call in case alloc_hugepage() would repeatedly fail in
turn preventing the loop to break and to reach the try_to_freeze() in
the khugepaged main loop.
khugepaged would still freeze just fine by trying again the next
minute but it's better if it freezes immediately.
Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
mm/huge_memory.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 4298aba..67311d1 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2277,6 +2277,7 @@ static struct page *khugepaged_alloc_hugepage(void)
if (!hpage) {
count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
khugepaged_alloc_sleep();
+ try_to_freeze();
} else
count_vm_event(THP_COLLAPSE_ALLOC);
} while (unlikely(!hpage) &&
@@ -2331,7 +2332,7 @@ static int khugepaged(void *none)
{
struct mm_slot *mm_slot;
- set_freezable();
+ set_freezable_with_signal();
set_user_nice(current, 19);
/* serialize with start_khugepaged() */
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-08 15:29 ` [PATCH] thp: reduce khugepaged freezing latency Andrea Arcangeli
@ 2011-11-08 20:01 ` Srivatsa S. Bhat
2011-11-09 0:01 ` Andrea Arcangeli
2011-11-09 12:45 ` Srivatsa S. Bhat
0 siblings, 2 replies; 20+ messages in thread
From: Srivatsa S. Bhat @ 2011-11-08 20:01 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Rafael J. Wysocki, linux-pm, linux-kernel, Jiri Slaby, linux-mm,
Andrew Morton
On 11/08/2011 08:59 PM, Andrea Arcangeli wrote:
> Lack of set_freezable_with_signal() prevented khugepaged to be waken
> up (and prevented to sleep again) across the
> schedule_timeout_interruptible() calls after freezing() becomes
> true. The tight loop in khugepaged_alloc_hugepage() also missed one
> try_to_freeze() call in case alloc_hugepage() would repeatedly fail in
> turn preventing the loop to break and to reach the try_to_freeze() in
> the khugepaged main loop.
>
> khugepaged would still freeze just fine by trying again the next
> minute but it's better if it freezes immediately.
>
> Reported-by: Jiri Slaby <jslaby@suse.cz>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
> mm/huge_memory.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 4298aba..67311d1 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2277,6 +2277,7 @@ static struct page *khugepaged_alloc_hugepage(void)
> if (!hpage) {
> count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
> khugepaged_alloc_sleep();
> + try_to_freeze();
> } else
> count_vm_event(THP_COLLAPSE_ALLOC);
> } while (unlikely(!hpage) &&
> @@ -2331,7 +2332,7 @@ static int khugepaged(void *none)
> {
> struct mm_slot *mm_slot;
>
> - set_freezable();
> + set_freezable_with_signal();
> set_user_nice(current, 19);
>
> /* serialize with start_khugepaged() */
>
Why do we need to use both set_freezable_with_signal() and an additional
try_to_freeze()? Won't just using either one of them be good enough?
Or am I missing something here?
Thanks,
Srivatsa S. Bhat
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-08 20:01 ` Srivatsa S. Bhat
@ 2011-11-09 0:01 ` Andrea Arcangeli
2011-11-09 9:03 ` Srivatsa S. Bhat
2011-11-09 12:45 ` Srivatsa S. Bhat
1 sibling, 1 reply; 20+ messages in thread
From: Andrea Arcangeli @ 2011-11-09 0:01 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Rafael J. Wysocki, linux-pm, linux-kernel, Jiri Slaby, linux-mm,
Andrew Morton
On Wed, Nov 09, 2011 at 01:31:07AM +0530, Srivatsa S. Bhat wrote:
> On 11/08/2011 08:59 PM, Andrea Arcangeli wrote:
> > Lack of set_freezable_with_signal() prevented khugepaged to be waken
> > up (and prevented to sleep again) across the
> > schedule_timeout_interruptible() calls after freezing() becomes
> > true. The tight loop in khugepaged_alloc_hugepage() also missed one
> > try_to_freeze() call in case alloc_hugepage() would repeatedly fail in
> > turn preventing the loop to break and to reach the try_to_freeze() in
> > the khugepaged main loop.
> >
> > khugepaged would still freeze just fine by trying again the next
> > minute but it's better if it freezes immediately.
> >
> > Reported-by: Jiri Slaby <jslaby@suse.cz>
> > Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> > ---
> > mm/huge_memory.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 4298aba..67311d1 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2277,6 +2277,7 @@ static struct page *khugepaged_alloc_hugepage(void)
> > if (!hpage) {
> > count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
> > khugepaged_alloc_sleep();
> > + try_to_freeze();
> > } else
> > count_vm_event(THP_COLLAPSE_ALLOC);
> > } while (unlikely(!hpage) &&
> > @@ -2331,7 +2332,7 @@ static int khugepaged(void *none)
> > {
> > struct mm_slot *mm_slot;
> >
> > - set_freezable();
> > + set_freezable_with_signal();
> > set_user_nice(current, 19);
> >
> > /* serialize with start_khugepaged() */
> >
>
> Why do we need to use both set_freezable_with_signal() and an additional
> try_to_freeze()? Won't just using either one of them be good enough?
> Or am I missing something here?
set_freezable_with_signal() makes khugepaged quit and not re-enter the
sleep, try_to_freeze is needed to get the task from freezing to
frozen, otherwise it'll loop without getting frozen.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 0:01 ` Andrea Arcangeli
@ 2011-11-09 9:03 ` Srivatsa S. Bhat
0 siblings, 0 replies; 20+ messages in thread
From: Srivatsa S. Bhat @ 2011-11-09 9:03 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Rafael J. Wysocki, linux-pm, linux-kernel, Jiri Slaby, linux-mm,
Andrew Morton
On 11/09/2011 05:31 AM, Andrea Arcangeli wrote:
> On Wed, Nov 09, 2011 at 01:31:07AM +0530, Srivatsa S. Bhat wrote:
>> On 11/08/2011 08:59 PM, Andrea Arcangeli wrote:
>>> Lack of set_freezable_with_signal() prevented khugepaged to be waken
>>> up (and prevented to sleep again) across the
>>> schedule_timeout_interruptible() calls after freezing() becomes
>>> true. The tight loop in khugepaged_alloc_hugepage() also missed one
>>> try_to_freeze() call in case alloc_hugepage() would repeatedly fail in
>>> turn preventing the loop to break and to reach the try_to_freeze() in
>>> the khugepaged main loop.
>>>
>>> khugepaged would still freeze just fine by trying again the next
>>> minute but it's better if it freezes immediately.
>>>
>>> Reported-by: Jiri Slaby <jslaby@suse.cz>
>>> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
>>> ---
>>> mm/huge_memory.c | 3 ++-
>>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 4298aba..67311d1 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -2277,6 +2277,7 @@ static struct page *khugepaged_alloc_hugepage(void)
>>> if (!hpage) {
>>> count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
>>> khugepaged_alloc_sleep();
>>> + try_to_freeze();
>>> } else
>>> count_vm_event(THP_COLLAPSE_ALLOC);
>>> } while (unlikely(!hpage) &&
>>> @@ -2331,7 +2332,7 @@ static int khugepaged(void *none)
>>> {
>>> struct mm_slot *mm_slot;
>>>
>>> - set_freezable();
>>> + set_freezable_with_signal();
>>> set_user_nice(current, 19);
>>>
>>> /* serialize with start_khugepaged() */
>>>
>>
>> Why do we need to use both set_freezable_with_signal() and an additional
>> try_to_freeze()? Won't just using either one of them be good enough?
>> Or am I missing something here?
>
> set_freezable_with_signal() makes khugepaged quit and not re-enter the
> sleep, try_to_freeze is needed to get the task from freezing to
> frozen, otherwise it'll loop without getting frozen.
>
Sorry, I still don't get it. Correct me if I am wrong, but my understanding
is this:
There are 2 ways to freeze a freezable kernel thread (one which has unset
the PF_NOFREEZE flag by calling set_freezable()):
set TIF_FREEZE flag and,
a) send a signal if PF_FREEZER_NOSIG is unset for that kernel thread (due
to the call to set_freezable_with_signal()). Then, try_to_freeze() will
get called in the signal handler.
b) otherwise, just wake up the kernel thread and hope that the kernel thread
itself will call try_to_freeze() sometime soon.
Now coming to your patch,
Case 1: You use set_freezable_with_signal() instead of set_freezable():
In this case, since the kernel thread doesn't block signals for
freezing, it will get a signal (with TIF_FREEZE set) and the signal
handler will call try_to_freeze(). So, no need for additional
try_to_freeze() here.
Case 2: You add the extra try_to_freeze():
In this case, the freezer will wake up the kernel thread, which in
turn will now execute the newly added try_to_freeze() and will get
frozen successfully. So, no need for set_freezable_with_signal() here.
Rafael, am I right?
Thanks,
Srivatsa S. Bhat
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-08 20:01 ` Srivatsa S. Bhat
2011-11-09 0:01 ` Andrea Arcangeli
@ 2011-11-09 12:45 ` Srivatsa S. Bhat
2011-11-09 15:53 ` Tejun Heo
1 sibling, 1 reply; 20+ messages in thread
From: Srivatsa S. Bhat @ 2011-11-09 12:45 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Rafael J. Wysocki, linux-pm, linux-kernel, Jiri Slaby, linux-mm,
Andrew Morton, Tejun Heo
On 11/09/2011 01:31 AM, Srivatsa S. Bhat wrote:
> On 11/08/2011 08:59 PM, Andrea Arcangeli wrote:
>> Lack of set_freezable_with_signal() prevented khugepaged to be waken
>> up (and prevented to sleep again) across the
>> schedule_timeout_interruptible() calls after freezing() becomes
>> true. The tight loop in khugepaged_alloc_hugepage() also missed one
>> try_to_freeze() call in case alloc_hugepage() would repeatedly fail in
>> turn preventing the loop to break and to reach the try_to_freeze() in
>> the khugepaged main loop.
>>
>> khugepaged would still freeze just fine by trying again the next
>> minute but it's better if it freezes immediately.
>>
>> Reported-by: Jiri Slaby <jslaby@suse.cz>
>> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
>> ---
>> mm/huge_memory.c | 3 ++-
>> 1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 4298aba..67311d1 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -2277,6 +2277,7 @@ static struct page *khugepaged_alloc_hugepage(void)
>> if (!hpage) {
>> count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
>> khugepaged_alloc_sleep();
>> + try_to_freeze();
>> } else
>> count_vm_event(THP_COLLAPSE_ALLOC);
>> } while (unlikely(!hpage) &&
>> @@ -2331,7 +2332,7 @@ static int khugepaged(void *none)
>> {
>> struct mm_slot *mm_slot;
>>
>> - set_freezable();
>> + set_freezable_with_signal();
>> set_user_nice(current, 19);
>>
>> /* serialize with start_khugepaged() */
>>
>
> Why do we need to use both set_freezable_with_signal() and an additional
> try_to_freeze()? Won't just using either one of them be good enough?
> Or am I missing something here?
>
Moreover, please note that Tejun Heo has sent patches to remove
set_freezable_with_signal() altogether (he has even cleaned up the last
existing user: usb_storage). So it wouldn't be good to start using it again,
if it can be avoided.
http://thread.gmane.org/gmane.linux.kernel/1209416
Thanks,
Srivatsa S. Bhat
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 12:45 ` Srivatsa S. Bhat
@ 2011-11-09 15:53 ` Tejun Heo
2011-11-09 16:20 ` Srivatsa S. Bhat
2011-11-09 16:52 ` Andrea Arcangeli
0 siblings, 2 replies; 20+ messages in thread
From: Tejun Heo @ 2011-11-09 15:53 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: Andrea Arcangeli, Rafael J. Wysocki, linux-pm, linux-kernel,
Jiri Slaby, linux-mm, Andrew Morton
Hello, Andrea, Srivatsa.
On Wed, Nov 09, 2011 at 06:15:38PM +0530, Srivatsa S. Bhat wrote:
> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >> index 4298aba..67311d1 100644
> >> --- a/mm/huge_memory.c
> >> +++ b/mm/huge_memory.c
> >> @@ -2277,6 +2277,7 @@ static struct page *khugepaged_alloc_hugepage(void)
> >> if (!hpage) {
> >> count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
> >> khugepaged_alloc_sleep();
> >> + try_to_freeze();
> >> } else
> >> count_vm_event(THP_COLLAPSE_ALLOC);
> >> } while (unlikely(!hpage) &&
> >> @@ -2331,7 +2332,7 @@ static int khugepaged(void *none)
> >> {
> >> struct mm_slot *mm_slot;
> >>
> >> - set_freezable();
> >> + set_freezable_with_signal();
> >> set_user_nice(current, 19);
Oooh, please don't do that. It's already gone in the pm tree. It
would be best if wait_event_freezable_timeout() can be used
(ie. wakeup condition should be set somewhere) but, if not, something
like the following sould work.
static void khugepaged_alloc_sleep(void)
{
DEFINE_WAIT(wait);
add_wait_queue(&khugepaged_wait, &wait);
try_to_freeze();
schedule_timeout_interruptible(
msecs_to_jiffies(
khugepaged_alloc_sleep_millisecs));
try_to_freeze();
remove_wait_queue(&khugepaged_wait, &wait);
}
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 15:53 ` Tejun Heo
@ 2011-11-09 16:20 ` Srivatsa S. Bhat
2011-11-09 16:52 ` Andrea Arcangeli
1 sibling, 0 replies; 20+ messages in thread
From: Srivatsa S. Bhat @ 2011-11-09 16:20 UTC (permalink / raw)
To: Tejun Heo
Cc: Andrea Arcangeli, Rafael J. Wysocki, linux-pm, linux-kernel,
Jiri Slaby, linux-mm, Andrew Morton
On 11/09/2011 09:23 PM, Tejun Heo wrote:
> Hello, Andrea, Srivatsa.
>
> On Wed, Nov 09, 2011 at 06:15:38PM +0530, Srivatsa S. Bhat wrote:
>>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>>> index 4298aba..67311d1 100644
>>>> --- a/mm/huge_memory.c
>>>> +++ b/mm/huge_memory.c
>>>> @@ -2277,6 +2277,7 @@ static struct page *khugepaged_alloc_hugepage(void)
>>>> if (!hpage) {
>>>> count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
>>>> khugepaged_alloc_sleep();
>>>> + try_to_freeze();
>>>> } else
>>>> count_vm_event(THP_COLLAPSE_ALLOC);
>>>> } while (unlikely(!hpage) &&
>>>> @@ -2331,7 +2332,7 @@ static int khugepaged(void *none)
>>>> {
>>>> struct mm_slot *mm_slot;
>>>>
>>>> - set_freezable();
>>>> + set_freezable_with_signal();
>>>> set_user_nice(current, 19);
>
> Oooh, please don't do that. It's already gone in the pm tree. It
> would be best if wait_event_freezable_timeout() can be used
> (ie. wakeup condition should be set somewhere) but, if not, something
> like the following sould work.
>
> static void khugepaged_alloc_sleep(void)
> {
> DEFINE_WAIT(wait);
> add_wait_queue(&khugepaged_wait, &wait);
> try_to_freeze();
> schedule_timeout_interruptible(
> msecs_to_jiffies(
> khugepaged_alloc_sleep_millisecs));
> try_to_freeze();
> remove_wait_queue(&khugepaged_wait, &wait);
> }
>
Right, Andrea this is what I meant. First of all we don't need both
set_freezable_with_signal() _and_ an additional try_to_freeze().
And since we are anyway doing away with set_freezable_with_signal(),
we can just use try_to_freeze() as Tejun suggested above.
Thanks,
Srivatsa S. Bhat
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 15:53 ` Tejun Heo
2011-11-09 16:20 ` Srivatsa S. Bhat
@ 2011-11-09 16:52 ` Andrea Arcangeli
2011-11-09 16:59 ` Tejun Heo
1 sibling, 1 reply; 20+ messages in thread
From: Andrea Arcangeli @ 2011-11-09 16:52 UTC (permalink / raw)
To: Tejun Heo
Cc: Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm, linux-kernel,
Jiri Slaby, linux-mm, Andrew Morton
On Wed, Nov 09, 2011 at 07:53:42AM -0800, Tejun Heo wrote:
> Hello, Andrea, Srivatsa.
>
> On Wed, Nov 09, 2011 at 06:15:38PM +0530, Srivatsa S. Bhat wrote:
> > >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > >> index 4298aba..67311d1 100644
> > >> --- a/mm/huge_memory.c
> > >> +++ b/mm/huge_memory.c
> > >> @@ -2277,6 +2277,7 @@ static struct page *khugepaged_alloc_hugepage(void)
> > >> if (!hpage) {
> > >> count_vm_event(THP_COLLAPSE_ALLOC_FAILED);
> > >> khugepaged_alloc_sleep();
> > >> + try_to_freeze();
> > >> } else
> > >> count_vm_event(THP_COLLAPSE_ALLOC);
> > >> } while (unlikely(!hpage) &&
> > >> @@ -2331,7 +2332,7 @@ static int khugepaged(void *none)
> > >> {
> > >> struct mm_slot *mm_slot;
> > >>
> > >> - set_freezable();
> > >> + set_freezable_with_signal();
> > >> set_user_nice(current, 19);
>
> Oooh, please don't do that. It's already gone in the pm tree. It
> would be best if wait_event_freezable_timeout() can be used
> (ie. wakeup condition should be set somewhere) but, if not, something
> like the following sould work.
>
> static void khugepaged_alloc_sleep(void)
> {
> DEFINE_WAIT(wait);
> add_wait_queue(&khugepaged_wait, &wait);
> try_to_freeze();
XXXXX
> schedule_timeout_interruptible(
> msecs_to_jiffies(
> khugepaged_alloc_sleep_millisecs));
> try_to_freeze();
> remove_wait_queue(&khugepaged_wait, &wait);
> }
I thought about that but isn't there a race condition if TIF_FREEZE is
set just in the point I marked above? I thought the
set_freezable_with_signal by forcing the task runnable would fix it.
How exactly wait_event_freezable_timeout() would avoid the same race
as above? I mean the freezer won't have visibility on the
khugepaged_wait waitqueue head so it surely cannot wake it up. And if
the freezing() check happens before TIF_FREEZE get set but before
schedule() is called, we're still screwed even if I use
wait_event_freezable_timeout()... Or is the signal_pending check
fixing that? But without set_freezable_with_signal() we don't set
TIF_SIGPENDING... so it's not immediately care how this whole logic is
race free. If you use stop_machine that could avoid the races though,
but it doesn't look like the freezer uses that.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 16:52 ` Andrea Arcangeli
@ 2011-11-09 16:59 ` Tejun Heo
2011-11-09 17:02 ` Tejun Heo
2011-11-09 17:06 ` Tejun Heo
0 siblings, 2 replies; 20+ messages in thread
From: Tejun Heo @ 2011-11-09 16:59 UTC (permalink / raw)
To: Andrea Arcangeli, Oleg Nesterov
Cc: Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm, linux-kernel,
Jiri Slaby, linux-mm, Andrew Morton
Hello,
On Wed, Nov 09, 2011 at 05:52:01PM +0100, Andrea Arcangeli wrote:
> > schedule_timeout_interruptible(
> > msecs_to_jiffies(
> > khugepaged_alloc_sleep_millisecs));
> > try_to_freeze();
> > remove_wait_queue(&khugepaged_wait, &wait);
> > }
>
> I thought about that but isn't there a race condition if TIF_FREEZE is
> set just in the point I marked above? I thought the
> set_freezable_with_signal by forcing the task runnable would fix it.
>
> How exactly wait_event_freezable_timeout() would avoid the same race
> as above? I mean the freezer won't have visibility on the
> khugepaged_wait waitqueue head so it surely cannot wake it up. And if
> the freezing() check happens before TIF_FREEZE get set but before
> schedule() is called, we're still screwed even if I use
> wait_event_freezable_timeout()... Or is the signal_pending check
> fixing that? But without set_freezable_with_signal() we don't set
> TIF_SIGPENDING... so it's not immediately care how this whole logic is
> race free. If you use stop_machine that could avoid the races though,
> but it doesn't look like the freezer uses that.
Freezer depends on the usual "set_current_state(INTERRUPTIBLE); check
freezing; schedule(); check freezing" construct and sends
INTERRUPTIBLE wake up after setting freezing state. The
synchronization hasn't been completely clear but recently been cleaned
up, so as long as freezing condition is tested after INTERRUPTIBLE is
set before going to sleep, the event won't go missing.
Maybe we need a helper here, which would be named horribly -
schedule_timeout_interruptible_freezable(). (cc'ing Oleg) Oleg, maybe
we need schedule_timeout(@sleep_type) too?
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 16:59 ` Tejun Heo
@ 2011-11-09 17:02 ` Tejun Heo
2011-11-09 17:29 ` Andrea Arcangeli
2011-11-09 17:06 ` Tejun Heo
1 sibling, 1 reply; 20+ messages in thread
From: Tejun Heo @ 2011-11-09 17:02 UTC (permalink / raw)
To: Andrea Arcangeli, Oleg Nesterov
Cc: Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm, linux-kernel,
Jiri Slaby, linux-mm, Andrew Morton
On Wed, Nov 09, 2011 at 08:59:25AM -0800, Tejun Heo wrote:
> Freezer depends on the usual "set_current_state(INTERRUPTIBLE); check
> freezing; schedule(); check freezing" construct and sends
> INTERRUPTIBLE wake up after setting freezing state. The
> synchronization hasn't been completely clear but recently been cleaned
> up, so as long as freezing condition is tested after INTERRUPTIBLE is
> set before going to sleep, the event won't go missing.
Just in case, it's scheduled for the next merge window but TIF_FREEZE
is gone now. There is freezing() helper which tests all pending
freezing conditions and the freezer guarantees there's mb between
assertion of freezing() and sending interruptible wakeups to target
tasks.
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 16:59 ` Tejun Heo
2011-11-09 17:02 ` Tejun Heo
@ 2011-11-09 17:06 ` Tejun Heo
2011-11-09 17:33 ` Andrea Arcangeli
1 sibling, 1 reply; 20+ messages in thread
From: Tejun Heo @ 2011-11-09 17:06 UTC (permalink / raw)
To: Andrea Arcangeli, Oleg Nesterov
Cc: Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm, linux-kernel,
Jiri Slaby, linux-mm, Andrew Morton
Hello,
On Wed, Nov 09, 2011 at 08:59:25AM -0800, Tejun Heo wrote:
> Freezer depends on the usual "set_current_state(INTERRUPTIBLE); check
> freezing; schedule(); check freezing" construct and sends
> INTERRUPTIBLE wake up after setting freezing state. The
> synchronization hasn't been completely clear but recently been cleaned
> up, so as long as freezing condition is tested after INTERRUPTIBLE is
> set before going to sleep, the event won't go missing.
>
> Maybe we need a helper here, which would be named horribly -
> schedule_timeout_interruptible_freezable(). (cc'ing Oleg) Oleg, maybe
> we need schedule_timeout(@sleep_type) too?
Ah, crap, still waking up. Sorry about that. So, yes, there's a race
condition above. You need to set TASK_INTERRUPTIBLE before testing
freezing and use schedule_timeout() instead of
schedule_timeout_interruptible(). Was getting confused with
prepare_to_wait(). That said, why not use prepare_to_wait() instead?
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 17:02 ` Tejun Heo
@ 2011-11-09 17:29 ` Andrea Arcangeli
2011-11-09 18:09 ` Tejun Heo
0 siblings, 1 reply; 20+ messages in thread
From: Andrea Arcangeli @ 2011-11-09 17:29 UTC (permalink / raw)
To: Tejun Heo
Cc: Oleg Nesterov, Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm,
linux-kernel, Jiri Slaby, linux-mm, Andrew Morton
On Wed, Nov 09, 2011 at 09:02:48AM -0800, Tejun Heo wrote:
> On Wed, Nov 09, 2011 at 08:59:25AM -0800, Tejun Heo wrote:
> > Freezer depends on the usual "set_current_state(INTERRUPTIBLE); check
> > freezing; schedule(); check freezing" construct and sends
> > INTERRUPTIBLE wake up after setting freezing state. The
> > synchronization hasn't been completely clear but recently been cleaned
> > up, so as long as freezing condition is tested after INTERRUPTIBLE is
> > set before going to sleep, the event won't go missing.
>
> Just in case, it's scheduled for the next merge window but TIF_FREEZE
> is gone now. There is freezing() helper which tests all pending
> freezing conditions and the freezer guarantees there's mb between
> assertion of freezing() and sending interruptible wakeups to target
> tasks.
My point is if what happens is:
freezer CPU khugepaged
------
assert freezing
wake_up(interruptible)
__set_current_state(interruptible)
schedule()
are we still hanging then? And I think it's silly to use
wait_event_freezable_timeout if I don't have any waitqueue to wait
on. Or are the pending changes hooking into the scheduler internally
similarly to what the set_freezable_with_signal would have done? That
would also kind of solve it. Removing set_freezable_with_signal sounds
like the scheduler internal knowledge will go too, so I guess what's
really missing is a schedule_timeout_freezable().
I think the below should solve the race but not having read your
pending changes that make the TIF_FREEZE go away you may want to see
if this is still relevant.
Still there is a try_to_freeze we need to add somewhere in the tight
loop, with this patch is automatically executed by
schedule_timeout_freezable (the same way it would be run by
wait_event_freezable_timeout if I had a waitqueue to deal with).
Also note the smp_mb setting the current state interruptible and
before calling freezing(). That should work fine if on the writer side
asserts freezing, memory barrier and than wakeup. Memory barrier is
only needed if the assert of the freezing could move into the wakeup
critical section (wakeup will likely take a spinlock with acquire
semantics so a write before taking the spinlock could mix inside the
critical section of the wakeup).
===
From: Andrea Arcangeli <aarcange@redhat.com>
Subject: [PATCH] thp: reduce khugepaged freezing latency
Lack of schedule_timeout_freezable() prevented khugepaged to be waken
up across the schedule_timeout_interruptible() if freezing() becomes
true.
khugepaged would still freeze just fine by trying again the next
minute but it's better if it freezes immediately.
Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
include/linux/sched.h | 1 +
kernel/timer.c | 10 ++++++++++
mm/huge_memory.c | 4 ++--
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 68daf4f..cfe07ef 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -356,6 +356,7 @@ extern int in_sched_functions(unsigned long addr);
#define MAX_SCHEDULE_TIMEOUT LONG_MAX
extern signed long schedule_timeout(signed long timeout);
extern signed long schedule_timeout_interruptible(signed long timeout);
+extern signed long schedule_timeout_freezable(signed long timeout);
extern signed long schedule_timeout_killable(signed long timeout);
extern signed long schedule_timeout_uninterruptible(signed long timeout);
asmlinkage void schedule(void);
diff --git a/kernel/timer.c b/kernel/timer.c
index dbaa624..06a7322 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -40,6 +40,7 @@
#include <linux/irq_work.h>
#include <linux/sched.h>
#include <linux/slab.h>
+#include <linux/freezer.h>
#include <asm/uaccess.h>
#include <asm/unistd.h>
@@ -1493,6 +1494,15 @@ signed long __sched schedule_timeout_interruptible(signed long timeout)
}
EXPORT_SYMBOL(schedule_timeout_interruptible);
+signed long __sched schedule_timeout_freezable(signed long timeout)
+{
+ do
+ set_current_state(TASK_INTERRUPTIBLE);
+ while (try_to_freeze());
+ return schedule_timeout(timeout);
+}
+EXPORT_SYMBOL(schedule_timeout_freezable);
+
signed long __sched schedule_timeout_killable(signed long timeout)
{
__set_current_state(TASK_KILLABLE);
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 4298aba..63d4f63 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2261,7 +2261,7 @@ static void khugepaged_alloc_sleep(void)
{
DEFINE_WAIT(wait);
add_wait_queue(&khugepaged_wait, &wait);
- schedule_timeout_interruptible(
+ schedule_timeout_freezable(
msecs_to_jiffies(
khugepaged_alloc_sleep_millisecs));
remove_wait_queue(&khugepaged_wait, &wait);
@@ -2317,7 +2317,7 @@ static void khugepaged_loop(void)
if (!khugepaged_scan_sleep_millisecs)
continue;
add_wait_queue(&khugepaged_wait, &wait);
- schedule_timeout_interruptible(
+ schedule_timeout_freezable(
msecs_to_jiffies(
khugepaged_scan_sleep_millisecs));
remove_wait_queue(&khugepaged_wait, &wait);
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 17:06 ` Tejun Heo
@ 2011-11-09 17:33 ` Andrea Arcangeli
0 siblings, 0 replies; 20+ messages in thread
From: Andrea Arcangeli @ 2011-11-09 17:33 UTC (permalink / raw)
To: Tejun Heo
Cc: Oleg Nesterov, Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm,
linux-kernel, Jiri Slaby, linux-mm, Andrew Morton
On Wed, Nov 09, 2011 at 09:06:57AM -0800, Tejun Heo wrote:
> Ah, crap, still waking up. Sorry about that. So, yes, there's a race
> condition above. You need to set TASK_INTERRUPTIBLE before testing
> freezing and use schedule_timeout() instead of
Yep that's the race I was thinking about. I see the wakeup in the
no-signal case avoids the race in wait_even_freezable_timeout so that
is ok but it'd race if I were just to add try_to_freeze before calling
schedule_timeout_interruptible.
> schedule_timeout_interruptible(). Was getting confused with
> prepare_to_wait(). That said, why not use prepare_to_wait() instead?
Because I don't need to wait on a waitqueue there. A THP failure
occurred, that caused some CPU overload and it's usually happening at
time of heavy VM stress, so I don't want to retry and cause more CPU
load from khugepaged until after some time even if more wakeups come
by. khugepaged is a very low cost background op, so it shouldn't cause
unnecessary CPU usage at times of VM pressure, waiting a better time
later is better.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 17:29 ` Andrea Arcangeli
@ 2011-11-09 18:09 ` Tejun Heo
2011-11-09 18:19 ` Andrea Arcangeli
0 siblings, 1 reply; 20+ messages in thread
From: Tejun Heo @ 2011-11-09 18:09 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Oleg Nesterov, Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm,
linux-kernel, Jiri Slaby, linux-mm, Andrew Morton
Hello, Anrea.
On Wed, Nov 09, 2011 at 06:29:42PM +0100, Andrea Arcangeli wrote:
> My point is if what happens is:
>
> freezer CPU khugepaged
> ------
> assert freezing
> wake_up(interruptible)
> __set_current_state(interruptible)
> schedule()
>
> are we still hanging then?
Yeap, you're right. I was thinking INTERRUPTILBE was being set before
try_to_freeze().
> And I think it's silly to use wait_event_freezable_timeout if I
> don't have any waitqueue to wait on.
I'm confused. You're doing add_wait_queue() before
schedule_timeout_interruptible(). prepare_to_wait() is essentially
add_wait_queue() + set_current_state(). What am I missing? ie. why
not do the following?
prepare_to_wait(INTERRUPTIBLE);
try_to_freeze();
schedule_timeout();
try_to_freeze();
finish_wait();
or even simpler,
wait_event_freezable_timeout(wq, false, timeout);
In terms of overhead, there is no appreciable difference from
add_wait_queue();
schedule_timeout_interruptible();
remove_wait_queue()
Or is the logic there scheduled to change?
> +signed long __sched schedule_timeout_freezable(signed long timeout)
> +{
> + do
> + set_current_state(TASK_INTERRUPTIBLE);
> + while (try_to_freeze());
> + return schedule_timeout(timeout);
> +}
> +EXPORT_SYMBOL(schedule_timeout_freezable);
Hmmm... I don't know. I really hope all freezable tasks stick to
higher level interface. It's way too easy to get things wrong and eat
either freezing or actual wakeup condition.
Thank you.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 18:09 ` Tejun Heo
@ 2011-11-09 18:19 ` Andrea Arcangeli
2011-11-09 18:34 ` Tejun Heo
0 siblings, 1 reply; 20+ messages in thread
From: Andrea Arcangeli @ 2011-11-09 18:19 UTC (permalink / raw)
To: Tejun Heo
Cc: Oleg Nesterov, Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm,
linux-kernel, Jiri Slaby, linux-mm, Andrew Morton
On Wed, Nov 09, 2011 at 10:09:00AM -0800, Tejun Heo wrote:
> I'm confused. You're doing add_wait_queue() before
> schedule_timeout_interruptible(). prepare_to_wait() is essentially
> add_wait_queue() + set_current_state(). What am I missing? ie. why
> not do the following?
Ah the reason of the waitqueue is the sysfs store, to get out of there
if somebody decreases the wait time from 1min to 10sec or
similar. It's not really needed for other things, in theory it could
be a separate waitqueue just for sysfs but probably not worth it.
> prepare_to_wait(INTERRUPTIBLE);
> try_to_freeze();
> schedule_timeout();
> try_to_freeze();
> finish_wait();
>
> or even simpler,
>
> wait_event_freezable_timeout(wq, false, timeout);
>
> In terms of overhead, there is no appreciable difference from
>
> add_wait_queue();
> schedule_timeout_interruptible();
> remove_wait_queue()
>
> Or is the logic there scheduled to change?
I have no "event" to wait other than the wakeup itself, this in the
end is the only reason it isn't already using
wait_event_freezable_timeout. Of course I can pass "false" as the
event.
> Hmmm... I don't know. I really hope all freezable tasks stick to
> higher level interface. It's way too easy to get things wrong and eat
> either freezing or actual wakeup condition.
Well you've just to tell me if I have to pass "false" and if
add_wait_queue+schedule_timeout_interruptible is obsoleted. If it's
not obsoleted the patch I posted should already be ok. It also will be
useful if others need to wait for a long time (> the freezer max wait)
without a waitqueue which I don't think is necessarily impossible. It
wasn't the case here just because I need to promptly react to the
sysfs writes (or setting the wait time to 1 day would then require 1
day before sysfs new value becomes meaningful, well unless somebody
doess killall khugepaged.. :)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 18:19 ` Andrea Arcangeli
@ 2011-11-09 18:34 ` Tejun Heo
2011-11-09 19:40 ` Andrea Arcangeli
0 siblings, 1 reply; 20+ messages in thread
From: Tejun Heo @ 2011-11-09 18:34 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Oleg Nesterov, Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm,
linux-kernel, Jiri Slaby, linux-mm, Andrew Morton
Hello, Andrea.
On Wed, Nov 09, 2011 at 07:19:25PM +0100, Andrea Arcangeli wrote:
> On Wed, Nov 09, 2011 at 10:09:00AM -0800, Tejun Heo wrote:
> > I'm confused. You're doing add_wait_queue() before
> > schedule_timeout_interruptible(). prepare_to_wait() is essentially
> > add_wait_queue() + set_current_state(). What am I missing? ie. why
> > not do the following?
>
> Ah the reason of the waitqueue is the sysfs store, to get out of there
> if somebody decreases the wait time from 1min to 10sec or
> similar. It's not really needed for other things, in theory it could
> be a separate waitqueue just for sysfs but probably not worth it.
Oh I see.
> I have no "event" to wait other than the wakeup itself, this in the
> end is the only reason it isn't already using
> wait_event_freezable_timeout. Of course I can pass "false" as the
> event.
I think, for this specific case, wait_event_freezable_timeout() w/
false is the simplest thing to do.
> > Hmmm... I don't know. I really hope all freezable tasks stick to
> > higher level interface. It's way too easy to get things wrong and eat
> > either freezing or actual wakeup condition.
>
> Well you've just to tell me if I have to pass "false" and if
> add_wait_queue+schedule_timeout_interruptible is obsoleted. If it's
> not obsoleted the patch I posted should already be ok. It also will be
> useful if others need to wait for a long time (> the freezer max wait)
> without a waitqueue which I don't think is necessarily impossible. It
> wasn't the case here just because I need to promptly react to the
> sysfs writes (or setting the wait time to 1 day would then require 1
> day before sysfs new value becomes meaningful, well unless somebody
> doess killall khugepaged.. :)
I agree that there can be use cases where freezable interruptible
sleep is useful. Thanks to the the inherently racy nature of
schedule_interruptible_timeout() w.r.t. non-persistent interruptible
wakeups (ie. everything other than signal), race conditions introduced
by try_to_freeze() should be okay
The biggest problem I have with schedule_timeout_freezable() is that
it doesn't advertise that it's racy - ie. it doesn't have sleep
condition in the function name. Its wait counterpart
wait_event_freezable() isn't racy thanks to the explicit wait
condition and doesn't have such problem.
Maybe my concern is just paraonia and people wouldn't assume it's
schedule_timeout() with magic freezer support. Or we can name it
schedule_timeout_interruptible_freezable() (urgh........). I don't
know. My instinct tells me to strongly recommend use of
wait_event_freezable_timeout() and run away. :)
Thanks.
--
tejun
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 18:34 ` Tejun Heo
@ 2011-11-09 19:40 ` Andrea Arcangeli
2011-11-11 12:20 ` Jiri Slaby
0 siblings, 1 reply; 20+ messages in thread
From: Andrea Arcangeli @ 2011-11-09 19:40 UTC (permalink / raw)
To: Tejun Heo
Cc: Oleg Nesterov, Srivatsa S. Bhat, Rafael J. Wysocki, linux-pm,
linux-kernel, Jiri Slaby, linux-mm, Andrew Morton
On Wed, Nov 09, 2011 at 10:34:47AM -0800, Tejun Heo wrote:
> know. My instinct tells me to strongly recommend use of
> wait_event_freezable_timeout() and run away. :)
Passing false wasn't so appealing to me but ok. Jiri can you test this
with some suspend? (beware builds but untested)
===
From: Andrea Arcangeli <aarcange@redhat.com>
Subject: thp: reduce khugepaged freezing latency
Use wait_event_freezable_timeout() instead of
schedule_timeout_interruptible() to avoid missing freezer wakeups. A
try_to_freeze() would have been needed in the
khugepaged_alloc_hugepage tight loop too in case of the allocation
failing repeatedly, and wait_event_freezable_timeout will provide it
too.
khugepaged would still freeze just fine by trying again the next
minute but it's better if it freezes immediately.
Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
mm/huge_memory.c | 14 ++++----------
1 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 4298aba..fd925d0 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2259,12 +2259,9 @@ static void khugepaged_do_scan(struct page **hpage)
static void khugepaged_alloc_sleep(void)
{
- DEFINE_WAIT(wait);
- add_wait_queue(&khugepaged_wait, &wait);
- schedule_timeout_interruptible(
- msecs_to_jiffies(
- khugepaged_alloc_sleep_millisecs));
- remove_wait_queue(&khugepaged_wait, &wait);
+ wait_event_freezable_timeout(khugepaged_wait, false,
+ msecs_to_jiffies(
+ khugepaged_alloc_sleep_millisecs));
}
#ifndef CONFIG_NUMA
@@ -2313,14 +2310,11 @@ static void khugepaged_loop(void)
if (unlikely(kthread_should_stop()))
break;
if (khugepaged_has_work()) {
- DEFINE_WAIT(wait);
if (!khugepaged_scan_sleep_millisecs)
continue;
- add_wait_queue(&khugepaged_wait, &wait);
- schedule_timeout_interruptible(
+ wait_event_freezable_timeout(khugepaged_wait, false,
msecs_to_jiffies(
khugepaged_scan_sleep_millisecs));
- remove_wait_queue(&khugepaged_wait, &wait);
} else if (khugepaged_enabled())
wait_event_freezable(khugepaged_wait,
khugepaged_wait_event());
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] thp: reduce khugepaged freezing latency
2011-11-09 19:40 ` Andrea Arcangeli
@ 2011-11-11 12:20 ` Jiri Slaby
0 siblings, 0 replies; 20+ messages in thread
From: Jiri Slaby @ 2011-11-11 12:20 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Tejun Heo, Oleg Nesterov, Srivatsa S. Bhat, Rafael J. Wysocki,
linux-pm, linux-kernel, linux-mm, Andrew Morton
On 11/09/2011 08:40 PM, Andrea Arcangeli wrote:
> On Wed, Nov 09, 2011 at 10:34:47AM -0800, Tejun Heo wrote:
>> know. My instinct tells me to strongly recommend use of
>> wait_event_freezable_timeout() and run away. :)
>
> Passing false wasn't so appealing to me but ok. Jiri can you test this
> with some suspend? (beware builds but untested)
>
> ===
> From: Andrea Arcangeli <aarcange@redhat.com>
> Subject: thp: reduce khugepaged freezing latency
>
> Use wait_event_freezable_timeout() instead of
> schedule_timeout_interruptible() to avoid missing freezer wakeups. A
> try_to_freeze() would have been needed in the
> khugepaged_alloc_hugepage tight loop too in case of the allocation
> failing repeatedly, and wait_event_freezable_timeout will provide it
> too.
>
> khugepaged would still freeze just fine by trying again the next
> minute but it's better if it freezes immediately.
>
> Reported-by: Jiri Slaby <jslaby@suse.cz>
Tested-by: Jiri Slaby <jslaby@suse.cz>
> Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
> ---
> mm/huge_memory.c | 14 ++++----------
> 1 files changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 4298aba..fd925d0 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2259,12 +2259,9 @@ static void khugepaged_do_scan(struct page **hpage)
>
> static void khugepaged_alloc_sleep(void)
> {
> - DEFINE_WAIT(wait);
> - add_wait_queue(&khugepaged_wait, &wait);
> - schedule_timeout_interruptible(
> - msecs_to_jiffies(
> - khugepaged_alloc_sleep_millisecs));
> - remove_wait_queue(&khugepaged_wait, &wait);
> + wait_event_freezable_timeout(khugepaged_wait, false,
> + msecs_to_jiffies(
> + khugepaged_alloc_sleep_millisecs));
> }
>
> #ifndef CONFIG_NUMA
> @@ -2313,14 +2310,11 @@ static void khugepaged_loop(void)
> if (unlikely(kthread_should_stop()))
> break;
> if (khugepaged_has_work()) {
> - DEFINE_WAIT(wait);
> if (!khugepaged_scan_sleep_millisecs)
> continue;
> - add_wait_queue(&khugepaged_wait, &wait);
> - schedule_timeout_interruptible(
> + wait_event_freezable_timeout(khugepaged_wait, false,
> msecs_to_jiffies(
> khugepaged_scan_sleep_millisecs));
> - remove_wait_queue(&khugepaged_wait, &wait);
> } else if (khugepaged_enabled())
> wait_event_freezable(khugepaged_wait,
> khugepaged_wait_event());
thanks,
--
js
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2011-11-11 12:20 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-08 8:33 khugepaged doesn't want to freeze Jiri Slaby
2011-11-08 15:29 ` Andrea Arcangeli
2011-11-08 15:29 ` [PATCH] thp: reduce khugepaged freezing latency Andrea Arcangeli
2011-11-08 20:01 ` Srivatsa S. Bhat
2011-11-09 0:01 ` Andrea Arcangeli
2011-11-09 9:03 ` Srivatsa S. Bhat
2011-11-09 12:45 ` Srivatsa S. Bhat
2011-11-09 15:53 ` Tejun Heo
2011-11-09 16:20 ` Srivatsa S. Bhat
2011-11-09 16:52 ` Andrea Arcangeli
2011-11-09 16:59 ` Tejun Heo
2011-11-09 17:02 ` Tejun Heo
2011-11-09 17:29 ` Andrea Arcangeli
2011-11-09 18:09 ` Tejun Heo
2011-11-09 18:19 ` Andrea Arcangeli
2011-11-09 18:34 ` Tejun Heo
2011-11-09 19:40 ` Andrea Arcangeli
2011-11-11 12:20 ` Jiri Slaby
2011-11-09 17:06 ` Tejun Heo
2011-11-09 17:33 ` Andrea Arcangeli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).