* [PATCH] mm: prevent mmap_cache race in find_vma() @ 2013-04-04 18:35 ` Hugh Dickins 0 siblings, 0 replies; 31+ messages in thread From: Hugh Dickins @ 2013-04-04 18:35 UTC (permalink / raw) To: Linus Torvalds Cc: Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Paul E. McKenney, Ian Lance Taylor, linux-mm, linux-kernel From: Jan Stancek <jstancek@redhat.com> find_vma() can be called by multiple threads with read lock held on mm->mmap_sem and any of them can update mm->mmap_cache. Prevent compiler from re-fetching mm->mmap_cache, because other readers could update it in the meantime: thread 1 thread 2 | find_vma() | find_vma() struct vm_area_struct *vma = NULL; | vma = mm->mmap_cache; | if (!(vma && vma->vm_end > addr | && vma->vm_start <= addr)) { | | mm->mmap_cache = vma; return vma; | ^^ compiler may optimize this | local variable out and re-read | mm->mmap_cache | This issue can be reproduced with gcc-4.8.0-1 on s390x by running mallocstress testcase from LTP, which triggers: kernel BUG at mm/rmap.c:1088! Call Trace: ([<000003d100c57000>] 0x3d100c57000) [<000000000023a1c0>] do_wp_page+0x2fc/0xa88 [<000000000023baae>] handle_pte_fault+0x41a/0xac8 [<000000000023d832>] handle_mm_fault+0x17a/0x268 [<000000000060507a>] do_protection_exception+0x1e2/0x394 [<0000000000603a04>] pgm_check_handler+0x138/0x13c [<000003fffcf1f07a>] 0x3fffcf1f07a Last Breaking-Event-Address: [<000000000024755e>] page_add_new_anon_rmap+0xc2/0x168 Thanks to Jakub Jelinek for his insight on gcc and helping to track this down. Signed-off-by: Jan Stancek <jstancek@redhat.com> Acked-by: David Rientjes <rientjes@google.com> Signed-off-by: Hugh Dickins <hughd@google.com> Cc: stable@vger.kernel.org --- This is Jan's valuable patch, posted a couple of days ago, which then triggered "some discussion" of ACCESS_ONCE() on linux-mm. I've marked it for stable: should go back years, but 3.5 changed the indentation. mm/mmap.c | 2 +- mm/nommu.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mm/mmap.c b/mm/mmap.c index 6466699..0db0de1 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1940,7 +1940,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr) /* Check the cache first. */ /* (Cache hit rate is typically around 35%.) */ - vma = mm->mmap_cache; + vma = ACCESS_ONCE(mm->mmap_cache); if (!(vma && vma->vm_end > addr && vma->vm_start <= addr)) { struct rb_node *rb_node; diff --git a/mm/nommu.c b/mm/nommu.c index e193280..2f3ea74 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -821,7 +821,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr) struct vm_area_struct *vma; /* check the cache first */ - vma = mm->mmap_cache; + vma = ACCESS_ONCE(mm->mmap_cache); if (vma && vma->vm_start <= addr && vma->vm_end > addr) return vma; -- 1.7.1 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH] mm: prevent mmap_cache race in find_vma() @ 2013-04-04 18:35 ` Hugh Dickins 0 siblings, 0 replies; 31+ messages in thread From: Hugh Dickins @ 2013-04-04 18:35 UTC (permalink / raw) To: Linus Torvalds Cc: Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Paul E. McKenney, Ian Lance Taylor, linux-mm, linux-kernel From: Jan Stancek <jstancek@redhat.com> find_vma() can be called by multiple threads with read lock held on mm->mmap_sem and any of them can update mm->mmap_cache. Prevent compiler from re-fetching mm->mmap_cache, because other readers could update it in the meantime: thread 1 thread 2 | find_vma() | find_vma() struct vm_area_struct *vma = NULL; | vma = mm->mmap_cache; | if (!(vma && vma->vm_end > addr | && vma->vm_start <= addr)) { | | mm->mmap_cache = vma; return vma; | ^^ compiler may optimize this | local variable out and re-read | mm->mmap_cache | This issue can be reproduced with gcc-4.8.0-1 on s390x by running mallocstress testcase from LTP, which triggers: kernel BUG at mm/rmap.c:1088! Call Trace: ([<000003d100c57000>] 0x3d100c57000) [<000000000023a1c0>] do_wp_page+0x2fc/0xa88 [<000000000023baae>] handle_pte_fault+0x41a/0xac8 [<000000000023d832>] handle_mm_fault+0x17a/0x268 [<000000000060507a>] do_protection_exception+0x1e2/0x394 [<0000000000603a04>] pgm_check_handler+0x138/0x13c [<000003fffcf1f07a>] 0x3fffcf1f07a Last Breaking-Event-Address: [<000000000024755e>] page_add_new_anon_rmap+0xc2/0x168 Thanks to Jakub Jelinek for his insight on gcc and helping to track this down. Signed-off-by: Jan Stancek <jstancek@redhat.com> Acked-by: David Rientjes <rientjes@google.com> Signed-off-by: Hugh Dickins <hughd@google.com> Cc: stable@vger.kernel.org --- This is Jan's valuable patch, posted a couple of days ago, which then triggered "some discussion" of ACCESS_ONCE() on linux-mm. I've marked it for stable: should go back years, but 3.5 changed the indentation. mm/mmap.c | 2 +- mm/nommu.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mm/mmap.c b/mm/mmap.c index 6466699..0db0de1 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1940,7 +1940,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr) /* Check the cache first. */ /* (Cache hit rate is typically around 35%.) */ - vma = mm->mmap_cache; + vma = ACCESS_ONCE(mm->mmap_cache); if (!(vma && vma->vm_end > addr && vma->vm_start <= addr)) { struct rb_node *rb_node; diff --git a/mm/nommu.c b/mm/nommu.c index e193280..2f3ea74 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -821,7 +821,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr) struct vm_area_struct *vma; /* check the cache first */ - vma = mm->mmap_cache; + vma = ACCESS_ONCE(mm->mmap_cache); if (vma && vma->vm_start <= addr && vma->vm_end > addr) return vma; -- 1.7.1 -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-04 18:35 ` Hugh Dickins @ 2013-04-04 18:48 ` Linus Torvalds -1 siblings, 0 replies; 31+ messages in thread From: Linus Torvalds @ 2013-04-04 18:48 UTC (permalink / raw) To: Hugh Dickins Cc: Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Paul E. McKenney, Ian Lance Taylor, linux-mm, Linux Kernel Mailing List On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote: > > find_vma() can be called by multiple threads with read lock > held on mm->mmap_sem and any of them can update mm->mmap_cache. > Prevent compiler from re-fetching mm->mmap_cache, because other > readers could update it in the meantime: Ack. I do wonder if we should mark the unlocked update too some way (also in find_vma()), although it's probably not a problem in practice since there's no way the compiler can reasonably really do anything odd with it. We *could* make that an ACCESS_ONCE() write too just to highlight the fact that it's an unlocked write to this optimistic data structure. Anyway, applied. Linus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() @ 2013-04-04 18:48 ` Linus Torvalds 0 siblings, 0 replies; 31+ messages in thread From: Linus Torvalds @ 2013-04-04 18:48 UTC (permalink / raw) To: Hugh Dickins Cc: Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Paul E. McKenney, Ian Lance Taylor, linux-mm, Linux Kernel Mailing List On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote: > > find_vma() can be called by multiple threads with read lock > held on mm->mmap_sem and any of them can update mm->mmap_cache. > Prevent compiler from re-fetching mm->mmap_cache, because other > readers could update it in the meantime: Ack. I do wonder if we should mark the unlocked update too some way (also in find_vma()), although it's probably not a problem in practice since there's no way the compiler can reasonably really do anything odd with it. We *could* make that an ACCESS_ONCE() write too just to highlight the fact that it's an unlocked write to this optimistic data structure. Anyway, applied. Linus -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-04 18:48 ` Linus Torvalds @ 2013-04-04 19:01 ` Hugh Dickins -1 siblings, 0 replies; 31+ messages in thread From: Hugh Dickins @ 2013-04-04 19:01 UTC (permalink / raw) To: Linus Torvalds Cc: Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Paul E. McKenney, Ian Lance Taylor, linux-mm, Linux Kernel Mailing List On Thu, 4 Apr 2013, Linus Torvalds wrote: > On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote: > > > > find_vma() can be called by multiple threads with read lock > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > Prevent compiler from re-fetching mm->mmap_cache, because other > > readers could update it in the meantime: > > Ack. I do wonder if we should mark the unlocked update too some way > (also in find_vma()), although it's probably not a problem in practice > since there's no way the compiler can reasonably really do anything > odd with it. We *could* make that an ACCESS_ONCE() write too just to > highlight the fact that it's an unlocked write to this optimistic data > structure. Hah, you beat me to it. I wanted to get Jan's patch in first, seeing as it actually fixes his observed issue; and it is very nice to have such a good description of one of those, when ACCESS_ONCE() is usually just an insurance policy. But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage (popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and sound/firewire, but few places else). When Paul reminded us of it yesterday, I came to wonder if actually every use of ACCESS_ONCE in the read form should strictly be matched by ACCESS_ONCE whenever modifying the location. My uneducated guess is that strictly it ought to, in the sense of insurance policy; but that (apart from that strange split writing issue which came up a couple of months ago) in practice our compilers have not "advanced" to the point of making this an issue yet. > > Anyway, applied. Thanks, Hugh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() @ 2013-04-04 19:01 ` Hugh Dickins 0 siblings, 0 replies; 31+ messages in thread From: Hugh Dickins @ 2013-04-04 19:01 UTC (permalink / raw) To: Linus Torvalds Cc: Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Paul E. McKenney, Ian Lance Taylor, linux-mm, Linux Kernel Mailing List On Thu, 4 Apr 2013, Linus Torvalds wrote: > On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote: > > > > find_vma() can be called by multiple threads with read lock > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > Prevent compiler from re-fetching mm->mmap_cache, because other > > readers could update it in the meantime: > > Ack. I do wonder if we should mark the unlocked update too some way > (also in find_vma()), although it's probably not a problem in practice > since there's no way the compiler can reasonably really do anything > odd with it. We *could* make that an ACCESS_ONCE() write too just to > highlight the fact that it's an unlocked write to this optimistic data > structure. Hah, you beat me to it. I wanted to get Jan's patch in first, seeing as it actually fixes his observed issue; and it is very nice to have such a good description of one of those, when ACCESS_ONCE() is usually just an insurance policy. But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage (popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and sound/firewire, but few places else). When Paul reminded us of it yesterday, I came to wonder if actually every use of ACCESS_ONCE in the read form should strictly be matched by ACCESS_ONCE whenever modifying the location. My uneducated guess is that strictly it ought to, in the sense of insurance policy; but that (apart from that strange split writing issue which came up a couple of months ago) in practice our compilers have not "advanced" to the point of making this an issue yet. > > Anyway, applied. Thanks, Hugh -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-04 19:01 ` Hugh Dickins @ 2013-04-04 19:10 ` Linus Torvalds -1 siblings, 0 replies; 31+ messages in thread From: Linus Torvalds @ 2013-04-04 19:10 UTC (permalink / raw) To: Hugh Dickins Cc: Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Paul E. McKenney, Ian Lance Taylor, linux-mm, Linux Kernel Mailing List On Thu, Apr 4, 2013 at 12:01 PM, Hugh Dickins <hughd@google.com> wrote: > > When Paul reminded us of it yesterday, I came to wonder if actually > every use of ACCESS_ONCE in the read form should strictly be matched > by ACCESS_ONCE whenever modifying the location. > > My uneducated guess is that strictly it ought to, in the sense of > insurance policy; but that (apart from that strange split writing > issue which came up a couple of months ago) in practice our compilers > have not "advanced" to the point of making this an issue yet. I don't see how a compiler could reasonably really ever do anything different, but I do think the ACCESS_ONCE() modification version might be a good thing just as a "documentation". This is a good example of this issue, exactly because we have a mix of both speculative cases (the find_vma() lookup and modification) together with strictly exclusive locked accesses to the same field (the ones that invalidate the cache under the write lock). So documenting that the write in find_vma() is this kind of "optimistic unlocked access" is actually a potentially interesting piece of information for programmers, completely independently of whether the compiler will then treat it really differently or not. Of course, a plain comment would do the same, but would be less greppable. And despite the verbiage here, I don't really have a very strong opinion on this. I'm going to let it go, and if somebody sends me a patch with a good explanation in the next merge window, I'll probably apply it. Linus ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() @ 2013-04-04 19:10 ` Linus Torvalds 0 siblings, 0 replies; 31+ messages in thread From: Linus Torvalds @ 2013-04-04 19:10 UTC (permalink / raw) To: Hugh Dickins Cc: Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Paul E. McKenney, Ian Lance Taylor, linux-mm, Linux Kernel Mailing List On Thu, Apr 4, 2013 at 12:01 PM, Hugh Dickins <hughd@google.com> wrote: > > When Paul reminded us of it yesterday, I came to wonder if actually > every use of ACCESS_ONCE in the read form should strictly be matched > by ACCESS_ONCE whenever modifying the location. > > My uneducated guess is that strictly it ought to, in the sense of > insurance policy; but that (apart from that strange split writing > issue which came up a couple of months ago) in practice our compilers > have not "advanced" to the point of making this an issue yet. I don't see how a compiler could reasonably really ever do anything different, but I do think the ACCESS_ONCE() modification version might be a good thing just as a "documentation". This is a good example of this issue, exactly because we have a mix of both speculative cases (the find_vma() lookup and modification) together with strictly exclusive locked accesses to the same field (the ones that invalidate the cache under the write lock). So documenting that the write in find_vma() is this kind of "optimistic unlocked access" is actually a potentially interesting piece of information for programmers, completely independently of whether the compiler will then treat it really differently or not. Of course, a plain comment would do the same, but would be less greppable. And despite the verbiage here, I don't really have a very strong opinion on this. I'm going to let it go, and if somebody sends me a patch with a good explanation in the next merge window, I'll probably apply it. Linus -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-04 19:01 ` Hugh Dickins @ 2013-04-04 22:30 ` Paul E. McKenney -1 siblings, 0 replies; 31+ messages in thread From: Paul E. McKenney @ 2013-04-04 22:30 UTC (permalink / raw) To: Hugh Dickins Cc: Linus Torvalds, Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Ian Lance Taylor, linux-mm, Linux Kernel Mailing List On Thu, Apr 04, 2013 at 12:01:52PM -0700, Hugh Dickins wrote: > On Thu, 4 Apr 2013, Linus Torvalds wrote: > > On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote: > > > > > > find_vma() can be called by multiple threads with read lock > > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > > Prevent compiler from re-fetching mm->mmap_cache, because other > > > readers could update it in the meantime: > > > > Ack. I do wonder if we should mark the unlocked update too some way > > (also in find_vma()), although it's probably not a problem in practice > > since there's no way the compiler can reasonably really do anything > > odd with it. We *could* make that an ACCESS_ONCE() write too just to > > highlight the fact that it's an unlocked write to this optimistic data > > structure. > > Hah, you beat me to it. > > I wanted to get Jan's patch in first, seeing as it actually fixes his > observed issue; and it is very nice to have such a good description of > one of those, when ACCESS_ONCE() is usually just an insurance policy. > > But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage > (popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and > sound/firewire, but few places else). > > When Paul reminded us of it yesterday, I came to wonder if actually > every use of ACCESS_ONCE in the read form should strictly be matched > by ACCESS_ONCE whenever modifying the location. >From a hygiene/insurance/documentation point of view, I agree. Of course, it is OK to use things like cmpxchg() in place of ACCESS_ONCE(). The possible exceptions that come to mind are (1) if the access in question is done holding a lock that excludes all other accesses to that location, (2) if the access in question happens during initialization before any other CPU has access to that location, and (3) if the access in question happens during cleanup after all other CPUs have lost access to that location. Any others? /me goes to look to see if the RCU code follows this good advice... Thanx, Paul > My uneducated guess is that strictly it ought to, in the sense of > insurance policy; but that (apart from that strange split writing > issue which came up a couple of months ago) in practice our compilers > have not "advanced" to the point of making this an issue yet. > > > > > Anyway, applied. > > Thanks, > Hugh > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() @ 2013-04-04 22:30 ` Paul E. McKenney 0 siblings, 0 replies; 31+ messages in thread From: Paul E. McKenney @ 2013-04-04 22:30 UTC (permalink / raw) To: Hugh Dickins Cc: Linus Torvalds, Andrew Morton, Jan Stancek, Jakub Jelinek, David Rientjes, Johannes Weiner, Ian Lance Taylor, linux-mm, Linux Kernel Mailing List On Thu, Apr 04, 2013 at 12:01:52PM -0700, Hugh Dickins wrote: > On Thu, 4 Apr 2013, Linus Torvalds wrote: > > On Thu, Apr 4, 2013 at 11:35 AM, Hugh Dickins <hughd@google.com> wrote: > > > > > > find_vma() can be called by multiple threads with read lock > > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > > Prevent compiler from re-fetching mm->mmap_cache, because other > > > readers could update it in the meantime: > > > > Ack. I do wonder if we should mark the unlocked update too some way > > (also in find_vma()), although it's probably not a problem in practice > > since there's no way the compiler can reasonably really do anything > > odd with it. We *could* make that an ACCESS_ONCE() write too just to > > highlight the fact that it's an unlocked write to this optimistic data > > structure. > > Hah, you beat me to it. > > I wanted to get Jan's patch in first, seeing as it actually fixes his > observed issue; and it is very nice to have such a good description of > one of those, when ACCESS_ONCE() is usually just an insurance policy. > > But then I was researching the much rarer "ACCESS_ONCE(x) = y" usage > (popular in drivers/net/wireless/ath/ath9k and kernel/rcutree* and > sound/firewire, but few places else). > > When Paul reminded us of it yesterday, I came to wonder if actually > every use of ACCESS_ONCE in the read form should strictly be matched > by ACCESS_ONCE whenever modifying the location. >From a hygiene/insurance/documentation point of view, I agree. Of course, it is OK to use things like cmpxchg() in place of ACCESS_ONCE(). The possible exceptions that come to mind are (1) if the access in question is done holding a lock that excludes all other accesses to that location, (2) if the access in question happens during initialization before any other CPU has access to that location, and (3) if the access in question happens during cleanup after all other CPUs have lost access to that location. Any others? /me goes to look to see if the RCU code follows this good advice... Thanx, Paul > My uneducated guess is that strictly it ought to, in the sense of > insurance policy; but that (apart from that strange split writing > issue which came up a couple of months ago) in practice our compilers > have not "advanced" to the point of making this an issue yet. > > > > > Anyway, applied. > > Thanks, > Hugh > -- 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] 31+ messages in thread
* [PATCH] mm: prevent mmap_cache race in find_vma() @ 2013-04-02 21:59 Jan Stancek 2013-04-02 22:33 ` David Rientjes 0 siblings, 1 reply; 31+ messages in thread From: Jan Stancek @ 2013-04-02 21:59 UTC (permalink / raw) To: linux-mm find_vma() can be called by multiple threads with read lock held on mm->mmap_sem and any of them can update mm->mmap_cache. Prevent compiler from re-fetching mm->mmap_cache, because other readers could update it in the meantime: thread 1 thread 2 | find_vma() | find_vma() struct vm_area_struct *vma = NULL; | vma = mm->mmap_cache; | if (!(vma && vma->vm_end > addr | && vma->vm_start <= addr)) { | | mm->mmap_cache = vma; return vma; | ^^ compiler may optimize this | local variable out and re-read | mm->mmap_cache | This issue can be reproduced with gcc-4.8.0-1 on s390x by running mallocstress testcase from LTP, which triggers: kernel BUG at mm/rmap.c:1088! Call Trace: ([<000003d100c57000>] 0x3d100c57000) [<000000000023a1c0>] do_wp_page+0x2fc/0xa88 [<000000000023baae>] handle_pte_fault+0x41a/0xac8 [<000000000023d832>] handle_mm_fault+0x17a/0x268 [<000000000060507a>] do_protection_exception+0x1e2/0x394 [<0000000000603a04>] pgm_check_handler+0x138/0x13c [<000003fffcf1f07a>] 0x3fffcf1f07a Last Breaking-Event-Address: [<000000000024755e>] page_add_new_anon_rmap+0xc2/0x168 Thanks to Jakub Jelinek for his insight on gcc and helping to track this down. Signed-off-by: Jan Stancek <jstancek@redhat.com> --- mm/mmap.c | 2 +- mm/nommu.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mm/mmap.c b/mm/mmap.c index 6466699..0db0de1 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1940,7 +1940,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr) /* Check the cache first. */ /* (Cache hit rate is typically around 35%.) */ - vma = mm->mmap_cache; + vma = ACCESS_ONCE(mm->mmap_cache); if (!(vma && vma->vm_end > addr && vma->vm_start <= addr)) { struct rb_node *rb_node; diff --git a/mm/nommu.c b/mm/nommu.c index e193280..2f3ea74 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -821,7 +821,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr) struct vm_area_struct *vma; /* check the cache first */ - vma = mm->mmap_cache; + vma = ACCESS_ONCE(mm->mmap_cache); if (vma && vma->vm_start <= addr && vma->vm_end > addr) return vma; -- 1.7.1 -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-02 21:59 Jan Stancek @ 2013-04-02 22:33 ` David Rientjes 2013-04-02 23:09 ` Hugh Dickins 2013-04-03 9:37 ` Jakub Jelinek 0 siblings, 2 replies; 31+ messages in thread From: David Rientjes @ 2013-04-02 22:33 UTC (permalink / raw) To: Jan Stancek; +Cc: linux-mm On Tue, 2 Apr 2013, Jan Stancek wrote: > find_vma() can be called by multiple threads with read lock > held on mm->mmap_sem and any of them can update mm->mmap_cache. > Prevent compiler from re-fetching mm->mmap_cache, because other > readers could update it in the meantime: > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch mm->mmap_cache whatsoever; there is nothing that prevents this either in the C standard. You'll be relying solely on gcc's implementation of how it dereferences volatile-qualified pointers. -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-02 22:33 ` David Rientjes @ 2013-04-02 23:09 ` Hugh Dickins 2013-04-02 23:55 ` David Rientjes 2013-04-03 9:37 ` Jakub Jelinek 1 sibling, 1 reply; 31+ messages in thread From: Hugh Dickins @ 2013-04-02 23:09 UTC (permalink / raw) To: David Rientjes; +Cc: Jan Stancek, Paul McKenney, Ian Lance Taylor, linux-mm On Tue, 2 Apr 2013, David Rientjes wrote: > On Tue, 2 Apr 2013, Jan Stancek wrote: > > > find_vma() can be called by multiple threads with read lock > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > Prevent compiler from re-fetching mm->mmap_cache, because other > > readers could update it in the meantime: > > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch > mm->mmap_cache whatsoever; there is nothing that prevents this either in > the C standard. You'll be relying solely on gcc's implementation of how > it dereferences volatile-qualified pointers. Jan is using ACCESS_ONCE() as it should be used, for its intended purpose. If the kernel's implementation of ACCESS_ONCE() is deficient, then we should fix that, not discourage its use. Hugh > > > > thread 1 thread 2 > > | > > find_vma() | find_vma() > > struct vm_area_struct *vma = NULL; | > > vma = mm->mmap_cache; | > > if (!(vma && vma->vm_end > addr | > > && vma->vm_start <= addr)) { | > > | mm->mmap_cache = vma; > > return vma; | > > ^^ compiler may optimize this | > > local variable out and re-read | > > mm->mmap_cache | > > > > This issue can be reproduced with gcc-4.8.0-1 on s390x by running > > mallocstress testcase from LTP, which triggers: > > kernel BUG at mm/rmap.c:1088! > > Call Trace: > > ([<000003d100c57000>] 0x3d100c57000) > > [<000000000023a1c0>] do_wp_page+0x2fc/0xa88 > > [<000000000023baae>] handle_pte_fault+0x41a/0xac8 > > [<000000000023d832>] handle_mm_fault+0x17a/0x268 > > [<000000000060507a>] do_protection_exception+0x1e2/0x394 > > [<0000000000603a04>] pgm_check_handler+0x138/0x13c > > [<000003fffcf1f07a>] 0x3fffcf1f07a > > Last Breaking-Event-Address: > > [<000000000024755e>] page_add_new_anon_rmap+0xc2/0x168 > > > > Thanks to Jakub Jelinek for his insight on gcc and helping to > > track this down. > > > > Signed-off-by: Jan Stancek <jstancek@redhat.com> > > --- > > mm/mmap.c | 2 +- > > mm/nommu.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index 6466699..0db0de1 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -1940,7 +1940,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr) > > > > /* Check the cache first. */ > > /* (Cache hit rate is typically around 35%.) */ > > - vma = mm->mmap_cache; > > + vma = ACCESS_ONCE(mm->mmap_cache); > > if (!(vma && vma->vm_end > addr && vma->vm_start <= addr)) { > > struct rb_node *rb_node; > > > > diff --git a/mm/nommu.c b/mm/nommu.c > > index e193280..2f3ea74 100644 > > --- a/mm/nommu.c > > +++ b/mm/nommu.c > > @@ -821,7 +821,7 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr) > > struct vm_area_struct *vma; > > > > /* check the cache first */ > > - vma = mm->mmap_cache; > > + vma = ACCESS_ONCE(mm->mmap_cache); > > if (vma && vma->vm_start <= addr && vma->vm_end > addr) > > return vma; > > > > -- > > 1.7.1 -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-02 23:09 ` Hugh Dickins @ 2013-04-02 23:55 ` David Rientjes 2013-04-03 3:19 ` Paul E. McKenney 2013-04-03 4:14 ` Johannes Weiner 0 siblings, 2 replies; 31+ messages in thread From: David Rientjes @ 2013-04-02 23:55 UTC (permalink / raw) To: Hugh Dickins; +Cc: Jan Stancek, Paul McKenney, Ian Lance Taylor, linux-mm On Tue, 2 Apr 2013, Hugh Dickins wrote: > > > find_vma() can be called by multiple threads with read lock > > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > > Prevent compiler from re-fetching mm->mmap_cache, because other > > > readers could update it in the meantime: > > > > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch > > mm->mmap_cache whatsoever; there is nothing that prevents this either in > > the C standard. You'll be relying solely on gcc's implementation of how > > it dereferences volatile-qualified pointers. > > Jan is using ACCESS_ONCE() as it should be used, for its intended > purpose. If the kernel's implementation of ACCESS_ONCE() is deficient, > then we should fix that, not discourage its use. > My comment is about the changelog, quoted above, saying "prevent compiler from re-fetching mm->mmap_cache..." ACCESS_ONCE(), as implemented, does not prevent the compiler from re-fetching anything. It is entirely plausible that in gcc's current implementation that this guarantee is made, but it is not prevented by the language standard and I think the changelog should be reworded for anybody who reads it in the future. There is a dependency here on gcc's implementation, it's a meaningful distinction. I never discouraged its use since for gcc's current implementation it appears to work as desired and without gcc extensions there is no way to make such a guarantee by the standard. In fact, I acked a patch from Eric Dumazet that fixes a NULL pointer dereference by using ACCESS_ONCE() with gcc in slub. -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-02 23:55 ` David Rientjes @ 2013-04-03 3:19 ` Paul E. McKenney 2013-04-03 4:21 ` David Rientjes 2013-04-03 4:14 ` Johannes Weiner 1 sibling, 1 reply; 31+ messages in thread From: Paul E. McKenney @ 2013-04-03 3:19 UTC (permalink / raw) To: David Rientjes; +Cc: Hugh Dickins, Jan Stancek, Ian Lance Taylor, linux-mm On Tue, Apr 02, 2013 at 04:55:45PM -0700, David Rientjes wrote: > On Tue, 2 Apr 2013, Hugh Dickins wrote: > > > > > find_vma() can be called by multiple threads with read lock > > > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > > > Prevent compiler from re-fetching mm->mmap_cache, because other > > > > readers could update it in the meantime: > > > > > > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch > > > mm->mmap_cache whatsoever; there is nothing that prevents this either in > > > the C standard. You'll be relying solely on gcc's implementation of how > > > it dereferences volatile-qualified pointers. > > > > Jan is using ACCESS_ONCE() as it should be used, for its intended > > purpose. If the kernel's implementation of ACCESS_ONCE() is deficient, > > then we should fix that, not discourage its use. > > > > My comment is about the changelog, quoted above, saying "prevent compiler > from re-fetching mm->mmap_cache..." ACCESS_ONCE(), as implemented, does > not prevent the compiler from re-fetching anything. It is entirely > plausible that in gcc's current implementation that this guarantee is > made, but it is not prevented by the language standard and I think the > changelog should be reworded for anybody who reads it in the future. > There is a dependency here on gcc's implementation, it's a meaningful > distinction. > > I never discouraged its use since for gcc's current implementation it > appears to work as desired and without gcc extensions there is no way to > make such a guarantee by the standard. In fact, I acked a patch from Eric > Dumazet that fixes a NULL pointer dereference by using ACCESS_ONCE() with > gcc in slub. This LWN comment from user "nix" is helpful here: https://lwn.net/Articles/509731/ In particular: ... volatile's meaning as 'minimize optimizations applied to things manipulating anything of volatile type, do not duplicate, elide, move, fold, spindle or mutilate' is of long standing. So although I agree that the standard does not say as much as one might like about volatile, ACCESS_ONCE()'s use of volatile should be expected to work in a wide range of C compilers. ACCESS_ONCE()'s use of typeof() might not be quite so generally applicable, but a fair range of C compilers do seem to support typeof() as well as ACCESS_ONCE()'s use of volatile. Thanx, Paul -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 3:19 ` Paul E. McKenney @ 2013-04-03 4:21 ` David Rientjes 2013-04-03 16:38 ` Paul E. McKenney 0 siblings, 1 reply; 31+ messages in thread From: David Rientjes @ 2013-04-03 4:21 UTC (permalink / raw) To: Paul E. McKenney; +Cc: Hugh Dickins, Jan Stancek, Ian Lance Taylor, linux-mm On Tue, 2 Apr 2013, Paul E. McKenney wrote: > So although I agree that the standard does not say as much as one might > like about volatile, ACCESS_ONCE()'s use of volatile should be expected > to work in a wide range of C compilers. ACCESS_ONCE()'s use of typeof() > might not be quite so generally applicable, but a fair range of C > compilers do seem to support typeof() as well as ACCESS_ONCE()'s use > of volatile. > Agreed and I have nothing against code that uses it in that manner based on the implementations of those compilers. The _only_ thing I've said in this thread is that ACCESS_ONCE() does not "prevent the compiler from re-fetching." The only thing that is going to prevent the compiler from doing anything is the standard and, as you eluded, it's legal for a compiler to compile code such as vma = ACCESS_ONCE(mm->mmap_cache); if (vma && vma->vm_start <= addr && vma->vm_end > addr) return vma; to be equivalent as if it had been written if (mm->mmap_cache && mm->mmap_cache->vm_start <= addr && mm->mmap_cache->vm_end > addr) return mm->mmap_cache; and still be a conforming implementation. We know gcc doesn't do that, so nobody is arguing the code in this patch as being incorrect. In fact, to remove any question about it: Acked-by: David Rientjes <rientjes@google.com> However, as originally stated, I would prefer that the changelog be reworded so nobody believes ACCESS_ONCE() prevents the compiler from re-fetching anything. -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 4:21 ` David Rientjes @ 2013-04-03 16:38 ` Paul E. McKenney 0 siblings, 0 replies; 31+ messages in thread From: Paul E. McKenney @ 2013-04-03 16:38 UTC (permalink / raw) To: David Rientjes; +Cc: Hugh Dickins, Jan Stancek, Ian Lance Taylor, linux-mm On Tue, Apr 02, 2013 at 09:21:49PM -0700, David Rientjes wrote: > On Tue, 2 Apr 2013, Paul E. McKenney wrote: > > > So although I agree that the standard does not say as much as one might > > like about volatile, ACCESS_ONCE()'s use of volatile should be expected > > to work in a wide range of C compilers. ACCESS_ONCE()'s use of typeof() > > might not be quite so generally applicable, but a fair range of C > > compilers do seem to support typeof() as well as ACCESS_ONCE()'s use > > of volatile. > > > > Agreed and I have nothing against code that uses it in that manner based > on the implementations of those compilers. The _only_ thing I've said in > this thread is that ACCESS_ONCE() does not "prevent the compiler from > re-fetching." The only thing that is going to prevent the compiler from > doing anything is the standard and, as you eluded, it's legal for a > compiler to compile code such as > > vma = ACCESS_ONCE(mm->mmap_cache); > if (vma && vma->vm_start <= addr && vma->vm_end > addr) > return vma; > > to be equivalent as if it had been written > > if (mm->mmap_cache && mm->mmap_cache->vm_start <= addr && > mm->mmap_cache->vm_end > addr) > return mm->mmap_cache; > > and still be a conforming implementation. We know gcc doesn't do that, so > nobody is arguing the code in this patch as being incorrect. In fact, to > remove any question about it: > > Acked-by: David Rientjes <rientjes@google.com> Thank you! > However, as originally stated, I would prefer that the changelog be > reworded so nobody believes ACCESS_ONCE() prevents the compiler from > re-fetching anything. If you were to instead say: However, as originally stated, I would prefer that the changelog be reworded so nobody believes that the C standard guarantees that volatile casts prevent the compiler from re-fetching anything. I might agree with you. But ACCESS_ONCE() really is defined to prevent the compiler from refetching anything. If a new version of gcc appears for which volatile casts does not protect against refetching, then we will change either (1) gcc or (2) the implementation of ACCESS_ONCE(). Whatever is needed to provide the guarantee against refetching. The Linux kernel absolutely needs -something- that provides this guarantee. Thanx, Paul -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-02 23:55 ` David Rientjes 2013-04-03 3:19 ` Paul E. McKenney @ 2013-04-03 4:14 ` Johannes Weiner 2013-04-03 4:25 ` David Rientjes 1 sibling, 1 reply; 31+ messages in thread From: Johannes Weiner @ 2013-04-03 4:14 UTC (permalink / raw) To: David Rientjes Cc: Hugh Dickins, Jan Stancek, Paul McKenney, Ian Lance Taylor, linux-mm On Tue, Apr 02, 2013 at 04:55:45PM -0700, David Rientjes wrote: > On Tue, 2 Apr 2013, Hugh Dickins wrote: > > > > > find_vma() can be called by multiple threads with read lock > > > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > > > Prevent compiler from re-fetching mm->mmap_cache, because other > > > > readers could update it in the meantime: > > > > > > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch > > > mm->mmap_cache whatsoever; there is nothing that prevents this either in > > > the C standard. You'll be relying solely on gcc's implementation of how > > > it dereferences volatile-qualified pointers. > > > > Jan is using ACCESS_ONCE() as it should be used, for its intended > > purpose. If the kernel's implementation of ACCESS_ONCE() is deficient, > > then we should fix that, not discourage its use. > > > > My comment is about the changelog, quoted above, saying "prevent compiler > from re-fetching mm->mmap_cache..." ACCESS_ONCE(), as implemented, does > not prevent the compiler from re-fetching anything. It is entirely > plausible that in gcc's current implementation that this guarantee is > made, but it is not prevented by the language standard and I think the > changelog should be reworded for anybody who reads it in the future. > There is a dependency here on gcc's implementation, it's a meaningful > distinction. The definition of ACCESS_ONCE() relies on gcc's current implementation, the users of ACCESS_ONCE() only rely on ACCESS_ONCE() being defined. Should it ever break you have to either fix it at the implementation level or remove/replace the abstraction in its entirety, how does the individual callsite matter in this 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/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 4:14 ` Johannes Weiner @ 2013-04-03 4:25 ` David Rientjes 2013-04-03 4:58 ` Johannes Weiner 0 siblings, 1 reply; 31+ messages in thread From: David Rientjes @ 2013-04-03 4:25 UTC (permalink / raw) To: Johannes Weiner Cc: Hugh Dickins, Jan Stancek, Paul McKenney, Ian Lance Taylor, linux-mm On Wed, 3 Apr 2013, Johannes Weiner wrote: > The definition of ACCESS_ONCE() relies on gcc's current > implementation, the users of ACCESS_ONCE() only rely on ACCESS_ONCE() > being defined. > > Should it ever break you have to either fix it at the implementation > level or remove/replace the abstraction in its entirety, how does the > individual callsite matter in this case? > As stated, it doesn't. I made the comment "for what it's worth" that ACCESS_ONCE() doesn't do anything to "prevent the compiler from re-fetching" as the changelog insists it does. I'd much rather it refer to gcc's implementation, which we're counting on here, to avoid any confusion since I know a couple people have thought that ACCESS_ONCE() forces the compiler to load memory onto the stack and that belief is completely and utterly wrong. -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 4:25 ` David Rientjes @ 2013-04-03 4:58 ` Johannes Weiner 2013-04-03 5:13 ` David Rientjes 2013-04-03 13:45 ` Ian Lance Taylor 0 siblings, 2 replies; 31+ messages in thread From: Johannes Weiner @ 2013-04-03 4:58 UTC (permalink / raw) To: David Rientjes Cc: Hugh Dickins, Jan Stancek, Paul McKenney, Ian Lance Taylor, linux-mm On Tue, Apr 02, 2013 at 09:25:40PM -0700, David Rientjes wrote: > On Wed, 3 Apr 2013, Johannes Weiner wrote: > > > The definition of ACCESS_ONCE() relies on gcc's current > > implementation, the users of ACCESS_ONCE() only rely on ACCESS_ONCE() > > being defined. > > > > Should it ever break you have to either fix it at the implementation > > level or remove/replace the abstraction in its entirety, how does the > > individual callsite matter in this case? > > > > As stated, it doesn't. I made the comment "for what it's worth" that > ACCESS_ONCE() doesn't do anything to "prevent the compiler from > re-fetching" as the changelog insists it does. That's exactly what it does: /* * Prevent the compiler from merging or refetching accesses. This is the guarantee ACCESS_ONCE() gives, users should absolutely be allowed to rely on this literal definition. The underlying gcc implementation does not matter one bit. That's the whole point of abstraction! > I'd much rather it refer to gcc's implementation, which we're > counting on here, No, we really don't. There is no "(*(volatile typeof(x)*)&(x))" anywhere in this patch. > to avoid any confusion since I know a couple people have thought > that ACCESS_ONCE() forces the compiler to load memory onto the stack > and that belief is completely and utterly wrong. Maybe "forces the compiler to load memory onto the stack" should be removed from the documentation of ACCESS_ONCE() then? -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 4:58 ` Johannes Weiner @ 2013-04-03 5:13 ` David Rientjes 2013-04-03 13:45 ` Ian Lance Taylor 1 sibling, 0 replies; 31+ messages in thread From: David Rientjes @ 2013-04-03 5:13 UTC (permalink / raw) To: Johannes Weiner Cc: Hugh Dickins, Jan Stancek, Paul McKenney, Ian Lance Taylor, linux-mm On Wed, 3 Apr 2013, Johannes Weiner wrote: > > As stated, it doesn't. I made the comment "for what it's worth" that > > ACCESS_ONCE() doesn't do anything to "prevent the compiler from > > re-fetching" as the changelog insists it does. > > That's exactly what it does: > > /* > * Prevent the compiler from merging or refetching accesses. > > This is the guarantee ACCESS_ONCE() gives, users should absolutely be > allowed to rely on this literal definition. The underlying gcc > implementation does not matter one bit. That's the whole point of > abstraction! > The C99 and earlier C standards do not provide any way of "preventing the compiler from refetching accesses," and in fact C99 leaves an access to a volatile qualified object as implementation defined. (If you disagree, then specify what exactly about ACCESS_ONCE() prevents the compiler from doing so.) I agree that comment is confusing unless you specify that gcc's implementation provides that guarantee and I would tend to agree with Paul's assessment that the wide majority (all?) of compilers do the same. I would hesitate to say positively that gcc will continue to implement anything in the future other than what the standard specifies, though. But I do agree that the comment is confusing. -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 4:58 ` Johannes Weiner 2013-04-03 5:13 ` David Rientjes @ 2013-04-03 13:45 ` Ian Lance Taylor 2013-04-03 14:33 ` Johannes Weiner 2013-04-03 16:33 ` Paul E. McKenney 1 sibling, 2 replies; 31+ messages in thread From: Ian Lance Taylor @ 2013-04-03 13:45 UTC (permalink / raw) To: Johannes Weiner Cc: David Rientjes, Hugh Dickins, Jan Stancek, Paul McKenney, linux-mm On Tue, Apr 2, 2013 at 9:58 PM, Johannes Weiner <hannes@cmpxchg.org> wrote: > On Tue, Apr 02, 2013 at 09:25:40PM -0700, David Rientjes wrote: > >> As stated, it doesn't. I made the comment "for what it's worth" that >> ACCESS_ONCE() doesn't do anything to "prevent the compiler from >> re-fetching" as the changelog insists it does. > > That's exactly what it does: > > /* > * Prevent the compiler from merging or refetching accesses. > > This is the guarantee ACCESS_ONCE() gives, users should absolutely be > allowed to rely on this literal definition. The underlying gcc > implementation does not matter one bit. That's the whole point of > abstraction! If the definition of ACCESS_ONCE is indeed #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) then its behaviour is compiler-specific. The C language standard only describes how access to volatile-qualified objects behave. In this case x is (presumably) not a volatile-qualifed object. The standard never defines the behaviour of volatile-qualified pointers. That might seem like an oversight, but it is not: using a non-volatile-qualified pointer to access a volatile-qualified object is undefined behaviour. In short, casting a pointer to a non-volatile-qualified object to a volatile-qualified pointer has no specific meaning in C. It's true that most compilers will behave as you wish, but there is no guarantee. If using a sufficiently recent version of GCC, you can get the behaviour that I think you want by using __atomic_load(&x, __ATOMIC_RELAXED) Ian -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 13:45 ` Ian Lance Taylor @ 2013-04-03 14:33 ` Johannes Weiner 2013-04-03 23:59 ` David Rientjes 2013-04-03 16:33 ` Paul E. McKenney 1 sibling, 1 reply; 31+ messages in thread From: Johannes Weiner @ 2013-04-03 14:33 UTC (permalink / raw) To: Ian Lance Taylor Cc: David Rientjes, Hugh Dickins, Jan Stancek, Paul McKenney, linux-mm On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote: > On Tue, Apr 2, 2013 at 9:58 PM, Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Tue, Apr 02, 2013 at 09:25:40PM -0700, David Rientjes wrote: > > > >> As stated, it doesn't. I made the comment "for what it's worth" that > >> ACCESS_ONCE() doesn't do anything to "prevent the compiler from > >> re-fetching" as the changelog insists it does. > > > > That's exactly what it does: > > > > /* > > * Prevent the compiler from merging or refetching accesses. > > > > This is the guarantee ACCESS_ONCE() gives, users should absolutely be > > allowed to rely on this literal definition. The underlying gcc > > implementation does not matter one bit. That's the whole point of > > abstraction! > > If the definition of ACCESS_ONCE is indeed > > #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) > > then its behaviour is compiler-specific. Who cares about the implementation, we are discussing a user here. ACCESS_ONCE() isolates a problem so that the users don't have to think about it, that's the whole point of abstraction. ACCESS_ONCE() is an opaque building block that says it prevents the compiler from merging and refetching accesses. That's all we care about right now. It may rely on compiler-specific behavior to achieve this, and its implementation may change if the underlying compilers change, but this will not affect the promise that it makes and so this is off-topic. This patch uses "ACCESS_ONCE()" and not "<compiler-specific tricks>". > The C language standard only describes how access to > volatile-qualified objects behave. In this case x is (presumably) not > a volatile-qualifed object. The standard never defines the behaviour > of volatile-qualified pointers. That might seem like an oversight, > but it is not: using a non-volatile-qualified pointer to access a > volatile-qualified object is undefined behaviour. > > In short, casting a pointer to a non-volatile-qualified object to a > volatile-qualified pointer has no specific meaning in C. It's true > that most compilers will behave as you wish, but there is no > guarantee. I am operating under the assumption that people compile their kernels with a subset of "most compilers" and not the C standard. [ Actually, I just tried to imagine how you would compile the kernel using the C standard instead of a compiler and that may have popped a blood vessel in my eye. ] > If using a sufficiently recent version of GCC, you can get the > behaviour that I think you want by using > __atomic_load(&x, __ATOMIC_RELAXED) This is good to know but the implementation details of ACCESS_ONCE() are irrelevant here. -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 14:33 ` Johannes Weiner @ 2013-04-03 23:59 ` David Rientjes 0 siblings, 0 replies; 31+ messages in thread From: David Rientjes @ 2013-04-03 23:59 UTC (permalink / raw) To: Johannes Weiner Cc: Ian Lance Taylor, Hugh Dickins, Jan Stancek, Paul McKenney, linux-mm On Wed, 3 Apr 2013, Johannes Weiner wrote: > Who cares about the implementation, we are discussing a user here. > ACCESS_ONCE() isolates a problem so that the users don't have to think > about it, that's the whole point of abstraction. ACCESS_ONCE() is an > opaque building block that says it prevents the compiler from merging > and refetching accesses. That's all we care about right now. > The discussion is about the implementation of ACCESS_ONCE(). Nobody, thus far, has said anything about this specific patch other than me when I acked it. I didn't start another thread off this patch because the changelog is relying on the comment above ACCESS_ONCE() to say that this "prevents the compiler from refetching." Some have been confused by this comment and accept it at face value that it's really preventing the compiler from doing something; in reality, it's "using gcc's current implementation to prevent refetching." It's an important distinction for anyone who comes away from the comment believing that volatile-qualified pointer dereferences are forbidden to be refetched by the compiler. Others have said that I've somehow discouraged its use because its somehow an invalid way of preventing gcc from refetching (when I've acked patches that do this exact thing in slub!). I have no idea how anyone could parse anything I've said about discouraging its use. I simply noted that the changelog could be reworded to be clearer since we're not relying on any standard here but rather a compiler's current implementation. It was never intended to be a long lengthy thread, but the comment I made is correct. I've worked with Paul to make that clearer in the comment so that people aren't confused in the future. -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 13:45 ` Ian Lance Taylor 2013-04-03 14:33 ` Johannes Weiner @ 2013-04-03 16:33 ` Paul E. McKenney 2013-04-03 16:41 ` Paul E. McKenney 2013-04-03 17:47 ` Ian Lance Taylor 1 sibling, 2 replies; 31+ messages in thread From: Paul E. McKenney @ 2013-04-03 16:33 UTC (permalink / raw) To: Ian Lance Taylor Cc: Johannes Weiner, David Rientjes, Hugh Dickins, Jan Stancek, linux-mm On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote: > On Tue, Apr 2, 2013 at 9:58 PM, Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Tue, Apr 02, 2013 at 09:25:40PM -0700, David Rientjes wrote: > > > >> As stated, it doesn't. I made the comment "for what it's worth" that > >> ACCESS_ONCE() doesn't do anything to "prevent the compiler from > >> re-fetching" as the changelog insists it does. > > > > That's exactly what it does: > > > > /* > > * Prevent the compiler from merging or refetching accesses. > > > > This is the guarantee ACCESS_ONCE() gives, users should absolutely be > > allowed to rely on this literal definition. The underlying gcc > > implementation does not matter one bit. That's the whole point of > > abstraction! > > If the definition of ACCESS_ONCE is indeed > > #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x)) > > then its behaviour is compiler-specific. That is the implementation of ACCESS_ONCE(). As Johannes noted, in the unlikely event that this implementation ever fails to provide the semantics required of ACCESS_ONCE(), something will be changed. This has already happened at least once. A recent version of gcc allowed volatile stores of certain constants to be split, but gcc was changed to avoid this behavior, while of course preserving this optimization for non-volatile stores. If we later need to change the ACCESS_ONCE() macro, we will make that change. > The C language standard only describes how access to > volatile-qualified objects behave. In this case x is (presumably) not > a volatile-qualifed object. The standard never defines the behaviour > of volatile-qualified pointers. That might seem like an oversight, > but it is not: using a non-volatile-qualified pointer to access a > volatile-qualified object is undefined behaviour. > > In short, casting a pointer to a non-volatile-qualified object to a > volatile-qualified pointer has no specific meaning in C. It's true > that most compilers will behave as you wish, but there is no > guarantee. But we are not using a non-volatile-qualified pointer to access a volatile-qualified object. We are doing the opposite. I therefore don't understand the relevance of your comment about undefined behavior. > If using a sufficiently recent version of GCC, you can get the > behaviour that I think you want by using > __atomic_load(&x, __ATOMIC_RELAXED) If this maps to the memory_order_relaxed token defined in earlier versions of the C11 standard, then this absolutely does -not-, repeat -not-, work for ACCESS_ONCE(). The relaxed load instead guarantees is that the load will be atomic with respect to other atomic stores to that same variable, in other words, it will prevent "load tearing" and "store tearing". I also believe that it prevents reloading, in other words, preventing this: tmp = __atomic_load(&x, __ATOMIC_RELAXED); do_something_with(tmp); do_something_else_with(tmp); from being optimized into something like this: do_something_with(__atomic_load(&x, __ATOMIC_RELAXED)); do_something_else_with(__atomic_load(&x, __ATOMIC_RELAXED)); It says nothing about combining nearby loads from that same variable. As I understand it, the compiler would be within its rights to do the reverse optimization from this: do_something_with(__atomic_load(&x, __ATOMIC_RELAXED)); do_something_else_with(__atomic_load(&x, __ATOMIC_RELAXED)); into this: tmp = __atomic_load(&x, __ATOMIC_RELAXED); do_something_with(tmp); do_something_else_with(tmp); It is only permitted to do finite combining, so that it is prohibited from turning this: while (__atomic_load(&x, __ATOMIC_RELAXED) != 0) do_some_other_thing(); into this: tmp = __atomic_load(&x, __ATOMIC_RELAXED); while (tmp) do_some_other_thing(); and thus into this: tmp = __atomic_load(&x, __ATOMIC_RELAXED); for (;;) do_some_other_thing(); But it would be within its rights to unroll the original loop into something like this: while (__atomic_load(&x, __ATOMIC_RELAXED) != 0) { do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); do_some_other_thing(); } This could of course destroy the response-time characteristics of the resulting program, so we absolutely must have a way to prevent the compiler from doing this. One way to prevent it from doing this is in fact a volatile cast: while (__atomic_load((volatile typeof(x) *)&x, __ATOMIC_RELAXED) != 0) do_some_other_thing(); The last time I went through this with the C/C++ standards committee members, they agreed with my interpretation. Perhaps the standard has been changed to allow volatile to be dispensed with, but I have not seen any such change. So, if you believe differently, please show me the wording in the standard that supports your view. Thanx, Paul -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 16:33 ` Paul E. McKenney @ 2013-04-03 16:41 ` Paul E. McKenney 2013-04-03 17:47 ` Ian Lance Taylor 1 sibling, 0 replies; 31+ messages in thread From: Paul E. McKenney @ 2013-04-03 16:41 UTC (permalink / raw) To: Ian Lance Taylor Cc: Johannes Weiner, David Rientjes, Hugh Dickins, Jan Stancek, linux-mm On Wed, Apr 03, 2013 at 09:33:48AM -0700, Paul E. McKenney wrote: > On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote: [ . . . ] > > If using a sufficiently recent version of GCC, you can get the > > behaviour that I think you want by using > > __atomic_load(&x, __ATOMIC_RELAXED) > > If this maps to the memory_order_relaxed token defined in earlier versions > of the C11 standard, then this absolutely does -not-, repeat -not-, work > for ACCESS_ONCE(). The relaxed load instead guarantees is that the load > will be atomic with respect to other atomic stores to that same variable, > in other words, it will prevent "load tearing" and "store tearing". I > also believe that it prevents reloading ... In addition, even if the semantics of relaxed loads now guarantee against load combining, note that ACCESS_ONCE() is also used to prevent combining and splitting of stores, for example: ACCESS_ONCE(p) = give_me_a_pointer(); Thanx, Paul -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 16:33 ` Paul E. McKenney 2013-04-03 16:41 ` Paul E. McKenney @ 2013-04-03 17:47 ` Ian Lance Taylor 2013-04-03 22:11 ` Paul E. McKenney 1 sibling, 1 reply; 31+ messages in thread From: Ian Lance Taylor @ 2013-04-03 17:47 UTC (permalink / raw) To: Paul McKenney Cc: Johannes Weiner, David Rientjes, Hugh Dickins, Jan Stancek, linux-mm On Wed, Apr 3, 2013 at 9:33 AM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote: > >> The C language standard only describes how access to >> volatile-qualified objects behave. In this case x is (presumably) not >> a volatile-qualifed object. The standard never defines the behaviour >> of volatile-qualified pointers. That might seem like an oversight, >> but it is not: using a non-volatile-qualified pointer to access a >> volatile-qualified object is undefined behaviour. >> >> In short, casting a pointer to a non-volatile-qualified object to a >> volatile-qualified pointer has no specific meaning in C. It's true >> that most compilers will behave as you wish, but there is no >> guarantee. > > But we are not using a non-volatile-qualified pointer to access a > volatile-qualified object. We are doing the opposite. I therefore > don't understand the relevance of your comment about undefined behavior. That was just a digression to explain why the standard does not need to define the behaviour of volatile-qualified pointers. >> If using a sufficiently recent version of GCC, you can get the >> behaviour that I think you want by using >> __atomic_load(&x, __ATOMIC_RELAXED) > > If this maps to the memory_order_relaxed token defined in earlier versions > of the C11 standard, then this absolutely does -not-, repeat -not-, work > for ACCESS_ONCE(). Yes, I'm sorry, you are right. It will work in practice today but you're quite right that there is no reason to think that it will work in principle. This need suggests that GCC needs a new builtin function to implement the functionality that you want. Would you consider opening a request for that at http://gcc.gnu.org/bugzilla/ ? Ian -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 17:47 ` Ian Lance Taylor @ 2013-04-03 22:11 ` Paul E. McKenney 2013-04-03 22:28 ` Ian Lance Taylor 0 siblings, 1 reply; 31+ messages in thread From: Paul E. McKenney @ 2013-04-03 22:11 UTC (permalink / raw) To: Ian Lance Taylor Cc: Johannes Weiner, David Rientjes, Hugh Dickins, Jan Stancek, linux-mm On Wed, Apr 03, 2013 at 10:47:28AM -0700, Ian Lance Taylor wrote: > On Wed, Apr 3, 2013 at 9:33 AM, Paul E. McKenney > <paulmck@linux.vnet.ibm.com> wrote: > > On Wed, Apr 03, 2013 at 06:45:51AM -0700, Ian Lance Taylor wrote: > > > >> The C language standard only describes how access to > >> volatile-qualified objects behave. In this case x is (presumably) not > >> a volatile-qualifed object. The standard never defines the behaviour > >> of volatile-qualified pointers. That might seem like an oversight, > >> but it is not: using a non-volatile-qualified pointer to access a > >> volatile-qualified object is undefined behaviour. > >> > >> In short, casting a pointer to a non-volatile-qualified object to a > >> volatile-qualified pointer has no specific meaning in C. It's true > >> that most compilers will behave as you wish, but there is no > >> guarantee. > > > > But we are not using a non-volatile-qualified pointer to access a > > volatile-qualified object. We are doing the opposite. I therefore > > don't understand the relevance of your comment about undefined behavior. > > That was just a digression to explain why the standard does not need > to define the behaviour of volatile-qualified pointers. > > > >> If using a sufficiently recent version of GCC, you can get the > >> behaviour that I think you want by using > >> __atomic_load(&x, __ATOMIC_RELAXED) > > > > If this maps to the memory_order_relaxed token defined in earlier versions > > of the C11 standard, then this absolutely does -not-, repeat -not-, work > > for ACCESS_ONCE(). > > Yes, I'm sorry, you are right. It will work in practice today but > you're quite right that there is no reason to think that it will work > in principle. > > This need suggests that GCC needs a new builtin function to implement > the functionality that you want. Would you consider opening a request > for that at http://gcc.gnu.org/bugzilla/ ? How about a request for gcc to formally honor the current uses of volatile? Thanx, Paul -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 22:11 ` Paul E. McKenney @ 2013-04-03 22:28 ` Ian Lance Taylor 2013-04-12 18:05 ` Paul E. McKenney 0 siblings, 1 reply; 31+ messages in thread From: Ian Lance Taylor @ 2013-04-03 22:28 UTC (permalink / raw) To: Paul McKenney Cc: Johannes Weiner, David Rientjes, Hugh Dickins, Jan Stancek, linux-mm On Wed, Apr 3, 2013 at 3:11 PM, Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote: > > How about a request for gcc to formally honor the current uses of volatile? Seems harder to define, but, sure, if it can be made to work. Ian -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-03 22:28 ` Ian Lance Taylor @ 2013-04-12 18:05 ` Paul E. McKenney 0 siblings, 0 replies; 31+ messages in thread From: Paul E. McKenney @ 2013-04-12 18:05 UTC (permalink / raw) To: Ian Lance Taylor Cc: Johannes Weiner, David Rientjes, Hugh Dickins, Jan Stancek, linux-mm On Wed, Apr 03, 2013 at 03:28:35PM -0700, Ian Lance Taylor wrote: > On Wed, Apr 3, 2013 at 3:11 PM, Paul E. McKenney > <paulmck@linux.vnet.ibm.com> wrote: > > > > How about a request for gcc to formally honor the current uses of volatile? > > Seems harder to define, but, sure, if it can be made to work. Actually, I am instead preparing to take this up with the standards committee. Even those who hate volatile (of which there are many) have an interest in a good codification of the defacto definition of volatile. After all, without a definition how can they hope to replace it? ;-) And the C committee cares deeply about backwards compatibility, so getting a good definition will help there as well. Thanx, Paul -- 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] 31+ messages in thread
* Re: [PATCH] mm: prevent mmap_cache race in find_vma() 2013-04-02 22:33 ` David Rientjes 2013-04-02 23:09 ` Hugh Dickins @ 2013-04-03 9:37 ` Jakub Jelinek 1 sibling, 0 replies; 31+ messages in thread From: Jakub Jelinek @ 2013-04-03 9:37 UTC (permalink / raw) To: David Rientjes; +Cc: Jan Stancek, linux-mm On Tue, Apr 02, 2013 at 03:33:58PM -0700, David Rientjes wrote: > On Tue, 2 Apr 2013, Jan Stancek wrote: > > > find_vma() can be called by multiple threads with read lock > > held on mm->mmap_sem and any of them can update mm->mmap_cache. > > Prevent compiler from re-fetching mm->mmap_cache, because other > > readers could update it in the meantime: > > > > FWIW, ACCESS_ONCE() does not guarantee that the compiler will not refetch > mm->mmap_cache whatsoever; there is nothing that prevents this either in > the C standard. You'll be relying solely on gcc's implementation of how > it dereferences volatile-qualified pointers. FYI, the volatile access can be also unnecessarily pessimizing, other projects like glibc use a friendlier alternative to the kernel's ACCESS_ONCE. In glibc it is: # define atomic_forced_read(x) \ ({ __typeof (x) __x; __asm ("" : "=r" (__x) : "0" (x)); __x; }) (could as well use "=g" instead). This isn't volatile, so it can be scheduled freely but if you store the result of it in a local variable and use only that local variable everywhere in the function (and don't modify it), there is a guarantee that you'll always see the same value. The above should work fine with any GCC versions that can be used to compile kernel. Or of course, starting with GCC 4.7 you have also the alternative to use # define ATOMIC_ONCE(x) __atomic_load_n (&x, __ATOMIC_RELAXED) Jakub -- 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] 31+ messages in thread
end of thread, other threads:[~2013-04-12 18:07 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-04-04 18:35 [PATCH] mm: prevent mmap_cache race in find_vma() Hugh Dickins 2013-04-04 18:35 ` Hugh Dickins 2013-04-04 18:48 ` Linus Torvalds 2013-04-04 18:48 ` Linus Torvalds 2013-04-04 19:01 ` Hugh Dickins 2013-04-04 19:01 ` Hugh Dickins 2013-04-04 19:10 ` Linus Torvalds 2013-04-04 19:10 ` Linus Torvalds 2013-04-04 22:30 ` Paul E. McKenney 2013-04-04 22:30 ` Paul E. McKenney -- strict thread matches above, loose matches on Subject: below -- 2013-04-02 21:59 Jan Stancek 2013-04-02 22:33 ` David Rientjes 2013-04-02 23:09 ` Hugh Dickins 2013-04-02 23:55 ` David Rientjes 2013-04-03 3:19 ` Paul E. McKenney 2013-04-03 4:21 ` David Rientjes 2013-04-03 16:38 ` Paul E. McKenney 2013-04-03 4:14 ` Johannes Weiner 2013-04-03 4:25 ` David Rientjes 2013-04-03 4:58 ` Johannes Weiner 2013-04-03 5:13 ` David Rientjes 2013-04-03 13:45 ` Ian Lance Taylor 2013-04-03 14:33 ` Johannes Weiner 2013-04-03 23:59 ` David Rientjes 2013-04-03 16:33 ` Paul E. McKenney 2013-04-03 16:41 ` Paul E. McKenney 2013-04-03 17:47 ` Ian Lance Taylor 2013-04-03 22:11 ` Paul E. McKenney 2013-04-03 22:28 ` Ian Lance Taylor 2013-04-12 18:05 ` Paul E. McKenney 2013-04-03 9:37 ` Jakub Jelinek
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.