* 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 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: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
* 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: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
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).