Fix race in set_max_huge_pages for multiple updaters of nr_huge_pages
Subsystems (2)
PATCH 1/1 Fix race in set_max_huge_pages for multiple updaters of nr_huge_pages
If there are multiple updaters to /proc/sys/vm/nr_hugepages simultaneously it is possible for the nr_huge_pages variable to become incorrect. There is no locking in the set_max_huge_pages function around alloc_fresh_huge_page which is able to update nr_huge_pages. Two callers to alloc_fresh_huge_page could race against each other as could a call to alloc_fresh_huge_page and a call to update_and_free_page. This patch just expands the area covered by the hugetlb_lock to cover the call into alloc_fresh_huge_page. I'm not sure how we could say that a sysctl section is performance critical where more specific locking would be needed.
My reproducer was to run a couple copies of the following script simultaneously
while [ true ]; do
echo 1000 > /proc/sys/vm/nr_hugepages
echo 500 > /proc/sys/vm/nr_hugepages
echo 750 > /proc/sys/vm/nr_hugepages
echo 100 > /proc/sys/vm/nr_hugepages
echo 0 > /proc/sys/vm/nr_hugepages
done
and then watch /proc/meminfo and eventually you will see things like
HugePages_Total: 100 HugePages_Free: 109
After applying the patch all seemed well.
----
hugetlb.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)
mm/hugetlb.c renamed from mm/hugetlb.c.old +7 -3
@@ -22,6 +22,7 @@static struct list_head hugepage_freelists[MAX_NUMNODES ];static unsigned int nr_huge_pages_node[MAX_NUMNODES ];static unsigned int free_huge_pages_node[MAX_NUMNODES ]; +/* This lock protects updates to hugepage_freelists, nr_huge_pages, and free_huge_pages */ static DEFINE_SPINLOCK (hugetlb_lock);static void enqueue_huge_page (struct page *page)
@@ -172,10 +173,13 @@static unsigned long set_max_huge_pages (unsigned long count) {while (count > nr_huge_pages) { -struct page *page =alloc_fresh_huge_page (); -if (!page) -return nr_huge_pages; +struct page *page;spin_lock (&hugetlb_lock); + page =alloc_fresh_huge_page (); +if (!page) { +spin_unlock (&hugetlb_lock); +return nr_huge_pages; + }enqueue_huge_page (page);spin_unlock (&hugetlb_lock); }
Quoted 53 lines · Eric Paris
If there are multiple updaters to
/proc/sys/vm/nr_hugepagessimultaneously it is possible for thenr_huge_pagesvariable to become incorrect. There is no locking in theset_max_huge_pagesfunction aroundalloc_fresh_huge_pagewhich is able to updatenr_huge_pages. Two callers toalloc_fresh_huge_pagecould race against each other as could a call toalloc_fresh_huge_pageand a call toupdate_and_free_page. This patch just expands the area covered by thehugetlb_lockto cover the call intoalloc_fresh_huge_page. I'm not sure how we could say that a sysctl section is performance critical where more specific locking would be needed.My reproducer was to run a couple copies of the following script simultaneously
while [ true ]; do
echo 1000 >/proc/sys/vm/nr_hugepages
echo 500 >/proc/sys/vm/nr_hugepages
echo 750 >/proc/sys/vm/nr_hugepages
echo 100 >/proc/sys/vm/nr_hugepages
echo 0 >/proc/sys/vm/nr_hugepages
doneand then watch
/proc/meminfoand eventually you will see things likeHugePages_Total: 100 HugePages_Free: 109After applying the patch all seemed well.
Signed-off-by Eric Paris <[email protected]>----
hugetlb.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)mm/hugetlb.c renamed from mm/hugetlb.c.old +1
-3@@ -22,6 +22,7 @@static struct list_head hugepage_freelists[MAX_NUMNODES ];static unsigned int nr_huge_pages_node[MAX_NUMNODES ];static unsigned int free_huge_pages_node[MAX_NUMNODES ]; +/* This lock protects updates to hugepage_freelists, nr_huge_pages, and free_huge_pages */ static DEFINE_SPINLOCK (hugetlb_lock);static void enqueue_huge_page (struct page *page)@@ -172,10 +173,13 @@static unsigned long set_max_huge_pages (unsigned long count) {while (count > nr_huge_pages) { -struct page *page =alloc_fresh_huge_page (); -if (!page) -return nr_huge_pages;
mm/hugetlb.c line 185
Quoted 10 lines
mm/hugetlb.c renamed from mm/hugetlb.c.old +6
-0+struct page *page;spin_lock (&hugetlb_lock); + page =alloc_fresh_huge_page (); +if (!page) { +spin_unlock (&hugetlb_lock); +return nr_huge_pages; + }enqueue_huge_page (page);spin_unlock (&hugetlb_lock); }
Nope, alloc_fresh_huge_page() does a GFP_HIGHUSER allocation, which can sleep and may not be called inside spinlock. You would have seen a spew of might_sleep() warnings if this was tested with the appropriate kernel debugging options.
How about this?
mm/hugetlb.c renamed from mm/hugetlb.c~hugetlb-fix-race-in-set_max_huge_pages-for-multiple-updaters-of-nr_huge_pages +6 -0
@@ -22,6 +22,10 @@ unsigned long max_huge_pages;static struct list_head hugepage_freelists[MAX_NUMNODES ];static unsigned int nr_huge_pages_node[MAX_NUMNODES ];static unsigned int free_huge_pages_node[MAX_NUMNODES ]; + +/* +* Protects updates to hugepage_freelists, nr_huge_pages, and free_huge_pages +*/ static DEFINE_SPINLOCK (hugetlb_lock);static void enqueue_huge_page (struct page *page)
@@ -61,8 +65,10 @@ static struct page *alloc_fresh_huge_pag HUGETLB_PAGE_ORDER); nid = (nid +1 ) %num_online_nodes ();if (page) { +spin_lock (&hugetlb_lock); nr_huge_pages++; nr_huge_pages_node[page_to_nid (page)]++; +spin_unlock (&hugetlb_lock); }return page; }
_
William Lee Irwin III
Acked-byQuoted 5 lines · Andrew Morton
Nope,
alloc_fresh_huge_page()does aGFP_HIGHUSERallocation, which can sleep and may not be called inside spinlock. You would have seen a spew ofmight_sleep()warnings if this was tested with the appropriate kernel debugging options.
How about this?
Looks good. My reply got stuck behind a queue of other things to do since it needed a correction.
-- wli