* [PATCH] thp: do not adjust zone water marks if khugepaged is not started @ 2015-03-27 11:39 Kirill A. Shutemov 2015-03-27 21:47 ` Andrew Morton 0 siblings, 1 reply; 6+ messages in thread From: Kirill A. Shutemov @ 2015-03-27 11:39 UTC (permalink / raw) To: Andrew Morton, David Rientjes Cc: Andrea Arcangeli, linux-mm, Kirill A. Shutemov set_recommended_min_free_kbytes() adjusts zone water marks to be suitable for khugepaged. We avoid doing this if khugepaged is disabled, but don't catch the case when khugepaged is failed to start. Let's address this by checking khugepaged_thread instead of khugepaged_enabled() in set_recommended_min_free_kbytes(). It's NULL if the kernel thread is stopped or failed to start. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> --- mm/huge_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mm/huge_memory.c b/mm/huge_memory.c index a1594b18bc1b..370a3bbc960d 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -110,7 +110,8 @@ static int set_recommended_min_free_kbytes(void) int nr_zones = 0; unsigned long recommended_min; - if (!khugepaged_enabled()) + /* khugepaged thread has stopped to failed to start */ + if (!khugepaged_thread) return 0; for_each_populated_zone(zone) -- 2.1.4 -- 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/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] thp: do not adjust zone water marks if khugepaged is not started 2015-03-27 11:39 [PATCH] thp: do not adjust zone water marks if khugepaged is not started Kirill A. Shutemov @ 2015-03-27 21:47 ` Andrew Morton 2015-03-27 22:00 ` Andrew Morton 2015-03-27 22:00 ` Kirill A. Shutemov 0 siblings, 2 replies; 6+ messages in thread From: Andrew Morton @ 2015-03-27 21:47 UTC (permalink / raw) To: Kirill A. Shutemov; +Cc: David Rientjes, Andrea Arcangeli, linux-mm On Fri, 27 Mar 2015 13:39:38 +0200 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote: > set_recommended_min_free_kbytes() adjusts zone water marks to be suitable > for khugepaged. We avoid doing this if khugepaged is disabled, but don't > catch the case when khugepaged is failed to start. > > Let's address this by checking khugepaged_thread instead of > khugepaged_enabled() in set_recommended_min_free_kbytes(). > It's NULL if the kernel thread is stopped or failed to start. > hm, why didn't khugepaged start up? Is this a theoretical by-code-inspection thing or has the problem been observed in real life? > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -110,7 +110,8 @@ static int set_recommended_min_free_kbytes(void) > int nr_zones = 0; > unsigned long recommended_min; > > - if (!khugepaged_enabled()) > + /* khugepaged thread has stopped to failed to start */ > + if (!khugepaged_thread) > return 0; > > for_each_populated_zone(zone) Fair enough, but take a look at start_khugepaged(): : static int start_khugepaged(void) : { : int err = 0; : if (khugepaged_enabled()) { : if (!khugepaged_thread) : khugepaged_thread = kthread_run(khugepaged, NULL, : "khugepaged"); : if (unlikely(IS_ERR(khugepaged_thread))) { : pr_err("khugepaged: kthread_run(khugepaged) failed\n"); : err = PTR_ERR(khugepaged_thread); : khugepaged_thread = NULL; -->> stop here : } : : if (!list_empty(&khugepaged_scan.mm_head)) : wake_up_interruptible(&khugepaged_wait); : : set_recommended_min_free_kbytes(); : } else if (khugepaged_thread) { : kthread_stop(khugepaged_thread); : khugepaged_thread = NULL; : } : : return err; : } -- 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/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thp: do not adjust zone water marks if khugepaged is not started 2015-03-27 21:47 ` Andrew Morton @ 2015-03-27 22:00 ` Andrew Morton 2015-03-27 22:08 ` Kirill A. Shutemov 2015-04-02 12:08 ` Kirill A. Shutemov 2015-03-27 22:00 ` Kirill A. Shutemov 1 sibling, 2 replies; 6+ messages in thread From: Andrew Morton @ 2015-03-27 22:00 UTC (permalink / raw) To: Kirill A. Shutemov, David Rientjes, Andrea Arcangeli, linux-mm On Fri, 27 Mar 2015 14:47:08 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > Fair enough, but take a look at start_khugepaged(): > > : static int start_khugepaged(void) > : { > : int err = 0; > : if (khugepaged_enabled()) { > : if (!khugepaged_thread) > : khugepaged_thread = kthread_run(khugepaged, NULL, > : "khugepaged"); > : if (unlikely(IS_ERR(khugepaged_thread))) { > : pr_err("khugepaged: kthread_run(khugepaged) failed\n"); > : err = PTR_ERR(khugepaged_thread); > : khugepaged_thread = NULL; > > -->> stop here > > : } > : > : if (!list_empty(&khugepaged_scan.mm_head)) > : wake_up_interruptible(&khugepaged_wait); > : > : set_recommended_min_free_kbytes(); > : } else if (khugepaged_thread) { > : kthread_stop(khugepaged_thread); > : khugepaged_thread = NULL; > : } > : > : return err; > : } Looking more closely... This code seems a bit screwy. - why is set_recommended_min_free_kbytes() a late_initcall? We've already done that within subsys_initcall->hugepage_init->set_recommended_min_free_kbytes() - there isn't much point in running start_khugepaged() if we've just set transparent_hugepage_flags to zero. - start_khugepaged() is misnamed. So something like this? --- a/mm/huge_memory.c~a +++ a/mm/huge_memory.c @@ -110,10 +110,6 @@ static int set_recommended_min_free_kbyt int nr_zones = 0; unsigned long recommended_min; - /* khugepaged thread has stopped to failed to start */ - if (!khugepaged_thread) - return 0; - for_each_populated_zone(zone) nr_zones++; @@ -145,9 +141,8 @@ static int set_recommended_min_free_kbyt setup_per_zone_wmarks(); return 0; } -late_initcall(set_recommended_min_free_kbytes); -static int start_khugepaged(void) +static int start_stop_khugepaged(void) { int err = 0; if (khugepaged_enabled()) { @@ -158,6 +153,7 @@ static int start_khugepaged(void) pr_err("khugepaged: kthread_run(khugepaged) failed\n"); err = PTR_ERR(khugepaged_thread); khugepaged_thread = NULL; + goto fail; } if (!list_empty(&khugepaged_scan.mm_head)) @@ -168,7 +164,7 @@ static int start_khugepaged(void) kthread_stop(khugepaged_thread); khugepaged_thread = NULL; } - +fail: return err; } @@ -302,7 +298,7 @@ static ssize_t enabled_store(struct kobj int err; mutex_lock(&khugepaged_mutex); - err = start_khugepaged(); + err = start_stop_khugepaged(); mutex_unlock(&khugepaged_mutex); if (err) @@ -651,12 +647,13 @@ static int __init hugepage_init(void) * where the extra memory used could hurt more than TLB overhead * is likely to save. The admin can still enable it through /sys. */ - if (totalram_pages < (512 << (20 - PAGE_SHIFT))) + if (totalram_pages < (512 << (20 - PAGE_SHIFT))) { transparent_hugepage_flags = 0; - - err = start_khugepaged(); - if (err) - goto err_khugepaged; + } else { + err = start_stop_khugepaged(); + if (err) + goto err_khugepaged; + } return 0; err_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/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thp: do not adjust zone water marks if khugepaged is not started 2015-03-27 22:00 ` Andrew Morton @ 2015-03-27 22:08 ` Kirill A. Shutemov 2015-04-02 12:08 ` Kirill A. Shutemov 1 sibling, 0 replies; 6+ messages in thread From: Kirill A. Shutemov @ 2015-03-27 22:08 UTC (permalink / raw) To: Andrew Morton Cc: Kirill A. Shutemov, David Rientjes, Andrea Arcangeli, linux-mm On Fri, Mar 27, 2015 at 03:00:26PM -0700, Andrew Morton wrote: > On Fri, 27 Mar 2015 14:47:08 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > > > Fair enough, but take a look at start_khugepaged(): > > > > : static int start_khugepaged(void) > > : { > > : int err = 0; > > : if (khugepaged_enabled()) { > > : if (!khugepaged_thread) > > : khugepaged_thread = kthread_run(khugepaged, NULL, > > : "khugepaged"); > > : if (unlikely(IS_ERR(khugepaged_thread))) { > > : pr_err("khugepaged: kthread_run(khugepaged) failed\n"); > > : err = PTR_ERR(khugepaged_thread); > > : khugepaged_thread = NULL; > > > > -->> stop here > > > > : } > > : > > : if (!list_empty(&khugepaged_scan.mm_head)) > > : wake_up_interruptible(&khugepaged_wait); > > : > > : set_recommended_min_free_kbytes(); > > : } else if (khugepaged_thread) { > > : kthread_stop(khugepaged_thread); > > : khugepaged_thread = NULL; > > : } > > : > > : return err; > > : } > > Looking more closely... This code seems a bit screwy. > > - why is set_recommended_min_free_kbytes() a late_initcall? We've > already done that within > subsys_initcall->hugepage_init->set_recommended_min_free_kbytes() > > - there isn't much point in running start_khugepaged() if we've just > set transparent_hugepage_flags to zero. > > - start_khugepaged() is misnamed. > > So something like this? Yeah, looks good to me. > --- a/mm/huge_memory.c~a > +++ a/mm/huge_memory.c > @@ -110,10 +110,6 @@ static int set_recommended_min_free_kbyt > int nr_zones = 0; > unsigned long recommended_min; > > - /* khugepaged thread has stopped to failed to start */ > - if (!khugepaged_thread) > - return 0; > - > for_each_populated_zone(zone) > nr_zones++; > > @@ -145,9 +141,8 @@ static int set_recommended_min_free_kbyt > setup_per_zone_wmarks(); > return 0; > } > -late_initcall(set_recommended_min_free_kbytes); > > -static int start_khugepaged(void) > +static int start_stop_khugepaged(void) > { > int err = 0; > if (khugepaged_enabled()) { > @@ -158,6 +153,7 @@ static int start_khugepaged(void) > pr_err("khugepaged: kthread_run(khugepaged) failed\n"); > err = PTR_ERR(khugepaged_thread); > khugepaged_thread = NULL; > + goto fail; > } > > if (!list_empty(&khugepaged_scan.mm_head)) > @@ -168,7 +164,7 @@ static int start_khugepaged(void) > kthread_stop(khugepaged_thread); > khugepaged_thread = NULL; > } > - > +fail: > return err; > } > > @@ -302,7 +298,7 @@ static ssize_t enabled_store(struct kobj > int err; > > mutex_lock(&khugepaged_mutex); > - err = start_khugepaged(); > + err = start_stop_khugepaged(); > mutex_unlock(&khugepaged_mutex); > > if (err) > @@ -651,12 +647,13 @@ static int __init hugepage_init(void) > * where the extra memory used could hurt more than TLB overhead > * is likely to save. The admin can still enable it through /sys. > */ > - if (totalram_pages < (512 << (20 - PAGE_SHIFT))) > + if (totalram_pages < (512 << (20 - PAGE_SHIFT))) { > transparent_hugepage_flags = 0; > - > - err = start_khugepaged(); > - if (err) > - goto err_khugepaged; > + } else { > + err = start_stop_khugepaged(); > + if (err) > + goto err_khugepaged; > + } > > return 0; > err_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/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- Kirill A. Shutemov -- 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/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thp: do not adjust zone water marks if khugepaged is not started 2015-03-27 22:00 ` Andrew Morton 2015-03-27 22:08 ` Kirill A. Shutemov @ 2015-04-02 12:08 ` Kirill A. Shutemov 1 sibling, 0 replies; 6+ messages in thread From: Kirill A. Shutemov @ 2015-04-02 12:08 UTC (permalink / raw) To: Andrew Morton Cc: Kirill A. Shutemov, David Rientjes, Andrea Arcangeli, linux-mm On Fri, Mar 27, 2015 at 03:00:26PM -0700, Andrew Morton wrote: > On Fri, 27 Mar 2015 14:47:08 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > > > Fair enough, but take a look at start_khugepaged(): > > > > : static int start_khugepaged(void) > > : { > > : int err = 0; > > : if (khugepaged_enabled()) { > > : if (!khugepaged_thread) > > : khugepaged_thread = kthread_run(khugepaged, NULL, > > : "khugepaged"); > > : if (unlikely(IS_ERR(khugepaged_thread))) { > > : pr_err("khugepaged: kthread_run(khugepaged) failed\n"); > > : err = PTR_ERR(khugepaged_thread); > > : khugepaged_thread = NULL; > > > > -->> stop here > > > > : } > > : > > : if (!list_empty(&khugepaged_scan.mm_head)) > > : wake_up_interruptible(&khugepaged_wait); > > : > > : set_recommended_min_free_kbytes(); > > : } else if (khugepaged_thread) { > > : kthread_stop(khugepaged_thread); > > : khugepaged_thread = NULL; > > : } > > : > > : return err; > > : } > > Looking more closely... This code seems a bit screwy. > > - why is set_recommended_min_free_kbytes() a late_initcall? We've > already done that within > subsys_initcall->hugepage_init->set_recommended_min_free_kbytes() > > - there isn't much point in running start_khugepaged() if we've just > set transparent_hugepage_flags to zero. > > - start_khugepaged() is misnamed. > > So something like this? Looks like you didn't apply this. Here's proper patch: ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thp: do not adjust zone water marks if khugepaged is not started 2015-03-27 21:47 ` Andrew Morton 2015-03-27 22:00 ` Andrew Morton @ 2015-03-27 22:00 ` Kirill A. Shutemov 1 sibling, 0 replies; 6+ messages in thread From: Kirill A. Shutemov @ 2015-03-27 22:00 UTC (permalink / raw) To: Andrew Morton, Andrea Arcangeli Cc: Kirill A. Shutemov, David Rientjes, linux-mm On Fri, Mar 27, 2015 at 02:47:08PM -0700, Andrew Morton wrote: > On Fri, 27 Mar 2015 13:39:38 +0200 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote: > > > set_recommended_min_free_kbytes() adjusts zone water marks to be suitable > > for khugepaged. We avoid doing this if khugepaged is disabled, but don't > > catch the case when khugepaged is failed to start. > > > > Let's address this by checking khugepaged_thread instead of > > khugepaged_enabled() in set_recommended_min_free_kbytes(). > > It's NULL if the kernel thread is stopped or failed to start. > > > > hm, why didn't khugepaged start up? Is this a theoretical > by-code-inspection thing or has the problem been observed in real life? David mentioned this scenario in comment to my previous patch. > > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -110,7 +110,8 @@ static int set_recommended_min_free_kbytes(void) > > int nr_zones = 0; > > unsigned long recommended_min; > > > > - if (!khugepaged_enabled()) > > + /* khugepaged thread has stopped to failed to start */ > > + if (!khugepaged_thread) > > return 0; > > > > for_each_populated_zone(zone) > > Fair enough, but take a look at start_khugepaged(): > > : static int start_khugepaged(void) > : { > : int err = 0; > : if (khugepaged_enabled()) { > : if (!khugepaged_thread) > : khugepaged_thread = kthread_run(khugepaged, NULL, > : "khugepaged"); > : if (unlikely(IS_ERR(khugepaged_thread))) { > : pr_err("khugepaged: kthread_run(khugepaged) failed\n"); > : err = PTR_ERR(khugepaged_thread); > : khugepaged_thread = NULL; > > -->> stop here Right, but set_recommended_min_free_kbytes() is also registered to late_initcall() and will get called anyway. It's not obvious why would we need it registered there. Call from start_khugepaged() should be enough. Andrea? > : } > : > : if (!list_empty(&khugepaged_scan.mm_head)) > : wake_up_interruptible(&khugepaged_wait); > : > : set_recommended_min_free_kbytes(); > : } else if (khugepaged_thread) { > : kthread_stop(khugepaged_thread); > : khugepaged_thread = NULL; > : } > : > : return err; > : } > > -- > 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/ . > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> -- Kirill A. Shutemov -- 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/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-04-02 12:08 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-27 11:39 [PATCH] thp: do not adjust zone water marks if khugepaged is not started Kirill A. Shutemov 2015-03-27 21:47 ` Andrew Morton 2015-03-27 22:00 ` Andrew Morton 2015-03-27 22:08 ` Kirill A. Shutemov 2015-04-02 12:08 ` Kirill A. Shutemov 2015-03-27 22:00 ` Kirill A. Shutemov
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).