diff for duplicates of <20130403001401.GC16026@blaptop> diff --git a/a/1.txt b/N1/1.txt index 6c80c28..253c683 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -44,3 +44,102 @@ That's one everybody are really missing. Here it goes! ==================== 8< ===================== + +>From fb0b9f3df698547bfb70f81d85e0d1e00f19e1fc Mon Sep 17 00:00:00 2001 +From: Minchan Kim <minchan@kernel.org> +Date: Wed, 3 Apr 2013 08:53:27 +0900 +Subject: [PATCH] THP: fix comment about memory barrier + +Now, memory barrier in __do_huge_pmd_anonymous_page doesn't work. +Because lru_cache_add_lru uses pagevec so it could miss spinlock +easily so above rule was broken so user might see inconsistent data. + +I was not first person who pointed out the problem. Mel and Peter +pointed out a few months ago and Peter pointed out further that +even spin_lock/unlock can't make sure it. +http://marc.info/?t=134333512700004 + + In particular: + + *A = a; + LOCK + UNLOCK + *B = b; + + may occur as: + + LOCK, STORE *B, STORE *A, UNLOCK + +At last, Hugh pointed out that even we don't need memory barrier +in there because __SetPageUpdate already have done it from +Nick's [1] explicitly. + +So this patch fixes comment on THP and adds same comment for +do_anonymous_page, too because everybody except Hugh was missing +that. It means we needs COMMENT about that. + +[1] 0ed361dec "mm: fix PageUptodate data race" + +Cc: Mel Gorman <mgorman@suse.de> +Cc: Andrea Arcangeli <aarcange@redhat.com> +Cc: Hugh Dickins <hughd@google.com> +Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> +Cc: David Rientjes <rientjes@google.com> +Cc: Peter Zijlstra <a.p.zijlstra@chello.nl> +Signed-off-by: Minchan Kim <minchan@kernel.org> +--- + mm/huge_memory.c | 11 +++++------ + mm/memory.c | 5 +++++ + 2 files changed, 10 insertions(+), 6 deletions(-) + +diff --git a/mm/huge_memory.c b/mm/huge_memory.c +index e2f7f5aa..f2f17ff 100644 +--- a/mm/huge_memory.c ++++ b/mm/huge_memory.c +@@ -713,6 +713,11 @@ static int __do_huge_pmd_anonymous_page(struct mm_struct *mm, + return VM_FAULT_OOM; + + clear_huge_page(page, haddr, HPAGE_PMD_NR); ++ /* ++ * The memory barrier inside __SetPageUptodate makes sure that ++ * clear_huge_page writes become visible after the set_pmd_at() ++ * write. ++ */ + __SetPageUptodate(page); + + spin_lock(&mm->page_table_lock); +@@ -724,12 +729,6 @@ static int __do_huge_pmd_anonymous_page(struct mm_struct *mm, + } else { + pmd_t entry; + entry = mk_huge_pmd(page, vma); +- /* +- * The spinlocking to take the lru_lock inside +- * page_add_new_anon_rmap() acts as a full memory +- * barrier to be sure clear_huge_page writes become +- * visible after the set_pmd_at() write. +- */ + page_add_new_anon_rmap(page, vma, haddr); + set_pmd_at(mm, haddr, pmd, entry); + pgtable_trans_huge_deposit(mm, pgtable); +diff --git a/mm/memory.c b/mm/memory.c +index 494526a..d0da51e 100644 +--- a/mm/memory.c ++++ b/mm/memory.c +@@ -3196,6 +3196,11 @@ static int do_anonymous_page(struct mm_struct *mm, struct vm_area_struct *vma, + page = alloc_zeroed_user_highpage_movable(vma, address); + if (!page) + goto oom; ++ /* ++ * The memory barrier inside __SetPageUptodate makes sure that ++ * preceeding stores to the page contents become visible after ++ * the set_pte_at() write. ++ */ + __SetPageUptodate(page); + + if (mem_cgroup_newpage_charge(page, mm, GFP_KERNEL)) +-- +1.8.2 + +-- +Kind regards, +Minchan Kim diff --git a/a/content_digest b/N1/content_digest index b471a7b..6830ce9 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -61,6 +61,105 @@ "\n" "Here it goes!\n" "\n" - ==================== 8< ===================== + "==================== 8< =====================\n" + "\n" + ">From fb0b9f3df698547bfb70f81d85e0d1e00f19e1fc Mon Sep 17 00:00:00 2001\n" + "From: Minchan Kim <minchan@kernel.org>\n" + "Date: Wed, 3 Apr 2013 08:53:27 +0900\n" + "Subject: [PATCH] THP: fix comment about memory barrier\n" + "\n" + "Now, memory barrier in __do_huge_pmd_anonymous_page doesn't work.\n" + "Because lru_cache_add_lru uses pagevec so it could miss spinlock\n" + "easily so above rule was broken so user might see inconsistent data.\n" + "\n" + "I was not first person who pointed out the problem. Mel and Peter\n" + "pointed out a few months ago and Peter pointed out further that\n" + "even spin_lock/unlock can't make sure it.\n" + "http://marc.info/?t=134333512700004\n" + "\n" + "\tIn particular:\n" + "\n" + " \t*A = a;\n" + " \tLOCK\n" + " \tUNLOCK\n" + " \t*B = b;\n" + "\n" + "\tmay occur as:\n" + "\n" + " \tLOCK, STORE *B, STORE *A, UNLOCK\n" + "\n" + "At last, Hugh pointed out that even we don't need memory barrier\n" + "in there because __SetPageUpdate already have done it from\n" + "Nick's [1] explicitly.\n" + "\n" + "So this patch fixes comment on THP and adds same comment for\n" + "do_anonymous_page, too because everybody except Hugh was missing\n" + "that. It means we needs COMMENT about that.\n" + "\n" + "[1] 0ed361dec \"mm: fix PageUptodate data race\"\n" + "\n" + "Cc: Mel Gorman <mgorman@suse.de>\n" + "Cc: Andrea Arcangeli <aarcange@redhat.com>\n" + "Cc: Hugh Dickins <hughd@google.com>\n" + "Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>\n" + "Cc: David Rientjes <rientjes@google.com>\n" + "Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>\n" + "Signed-off-by: Minchan Kim <minchan@kernel.org>\n" + "---\n" + " mm/huge_memory.c | 11 +++++------\n" + " mm/memory.c | 5 +++++\n" + " 2 files changed, 10 insertions(+), 6 deletions(-)\n" + "\n" + "diff --git a/mm/huge_memory.c b/mm/huge_memory.c\n" + "index e2f7f5aa..f2f17ff 100644\n" + "--- a/mm/huge_memory.c\n" + "+++ b/mm/huge_memory.c\n" + "@@ -713,6 +713,11 @@ static int __do_huge_pmd_anonymous_page(struct mm_struct *mm,\n" + " \t\treturn VM_FAULT_OOM;\n" + " \n" + " \tclear_huge_page(page, haddr, HPAGE_PMD_NR);\n" + "+\t/*\n" + "+\t * The memory barrier inside __SetPageUptodate makes sure that\n" + "+\t * clear_huge_page writes become visible after the set_pmd_at()\n" + "+\t * write.\n" + "+\t */\n" + " \t__SetPageUptodate(page);\n" + " \n" + " \tspin_lock(&mm->page_table_lock);\n" + "@@ -724,12 +729,6 @@ static int __do_huge_pmd_anonymous_page(struct mm_struct *mm,\n" + " \t} else {\n" + " \t\tpmd_t entry;\n" + " \t\tentry = mk_huge_pmd(page, vma);\n" + "-\t\t/*\n" + "-\t\t * The spinlocking to take the lru_lock inside\n" + "-\t\t * page_add_new_anon_rmap() acts as a full memory\n" + "-\t\t * barrier to be sure clear_huge_page writes become\n" + "-\t\t * visible after the set_pmd_at() write.\n" + "-\t\t */\n" + " \t\tpage_add_new_anon_rmap(page, vma, haddr);\n" + " \t\tset_pmd_at(mm, haddr, pmd, entry);\n" + " \t\tpgtable_trans_huge_deposit(mm, pgtable);\n" + "diff --git a/mm/memory.c b/mm/memory.c\n" + "index 494526a..d0da51e 100644\n" + "--- a/mm/memory.c\n" + "+++ b/mm/memory.c\n" + "@@ -3196,6 +3196,11 @@ static int do_anonymous_page(struct mm_struct *mm, struct vm_area_struct *vma,\n" + " \tpage = alloc_zeroed_user_highpage_movable(vma, address);\n" + " \tif (!page)\n" + " \t\tgoto oom;\n" + "+\t/*\n" + "+\t * The memory barrier inside __SetPageUptodate makes sure that\n" + "+\t * preceeding stores to the page contents become visible after\n" + "+\t * the set_pte_at() write.\n" + "+\t */\n" + " \t__SetPageUptodate(page);\n" + " \n" + " \tif (mem_cgroup_newpage_charge(page, mm, GFP_KERNEL))\n" + "-- \n" + "1.8.2\n" + "\n" + "-- \n" + "Kind regards,\n" + Minchan Kim -bbccda9add9abf2c796ed65beb3a3c6cbfe07d9fa5e5a44057b6f5b56c8631c0 +fa413f316eee037bf7401bc8a7662886d37bc1fafbe46166c53d3d9f4a1e76ee
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.