From: vbabka@suse.cz (Vlastimil Babka)
To: linux-snps-arc@lists.infradead.org
Subject: [PATCH] mm: slub: Ensure that slab_unlock() is atomic
Date: Tue, 8 Mar 2016 16:32:29 +0100 [thread overview]
Message-ID: <56DEF08D.607@suse.cz> (raw)
In-Reply-To: <1457447457-25878-1-git-send-email-vgupta@synopsys.com>
On 03/08/2016 03:30 PM, Vineet Gupta wrote:
> We observed livelocks on ARC SMP setup when running hackbench with SLUB.
> This hardware configuration lacks atomic instructions (LLOCK/SCOND) thus
> kernel resorts to a central @smp_bitops_lock to protect any R-M-W ops
> suh as test_and_set_bit()
Sounds like this architecture should then redefine __clear_bit_unlock
and perhaps other non-atomic __X_bit() variants to be atomic, and not
defer this requirement to places that use the API?
> The spinlock itself is implemented using Atomic [EX]change instruction
> which is always available.
>
> The race happened when both cores tried to slab_lock() the same page.
>
> c1 c0
> ----------- -----------
> slab_lock
> slab_lock
> slab_unlock
> Not observing the unlock
>
> This in turn happened because slab_unlock() doesn't serialize properly
> (doesn't use atomic clear) with a concurrent running
> slab_lock()->test_and_set_bit()
>
> Cc: Christoph Lameter <cl at linux.com>
> Cc: Pekka Enberg <penberg at kernel.org>
> Cc: David Rientjes <rientjes at google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim at lge.com>
> Cc: Andrew Morton <akpm at linux-foundation.org>
> Cc: Noam Camus <noamc at ezchip.com>
> Cc: <stable at vger.kernel.org>
> Cc: <linux-mm at kvack.org>
> Cc: <linux-kernel at vger.kernel.org>
> Cc: <linux-snps-arc at lists.infradead.org>
> Signed-off-by: Vineet Gupta <vgupta at synopsys.com>
> ---
> mm/slub.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index d8fbd4a6ed59..b7d345a508dc 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -345,7 +345,7 @@ static __always_inline void slab_lock(struct page *page)
> static __always_inline void slab_unlock(struct page *page)
> {
> VM_BUG_ON_PAGE(PageTail(page), page);
> - __bit_spin_unlock(PG_locked, &page->flags);
> + bit_spin_unlock(PG_locked, &page->flags);
> }
>
> static inline void set_page_slub_counters(struct page *page, unsigned long counters_new)
>
WARNING: multiple messages have this Message-ID (diff)
From: Vlastimil Babka <vbabka@suse.cz>
To: Vineet Gupta <Vineet.Gupta1@synopsys.com>, linux-mm@kvack.org
Cc: Christoph Lameter <cl@linux.com>,
Pekka Enberg <penberg@kernel.org>,
David Rientjes <rientjes@google.com>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Andrew Morton <akpm@linux-foundation.org>,
Noam Camus <noamc@ezchip.com>,
stable@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-snps-arc@lists.infradead.org
Subject: Re: [PATCH] mm: slub: Ensure that slab_unlock() is atomic
Date: Tue, 8 Mar 2016 16:32:29 +0100 [thread overview]
Message-ID: <56DEF08D.607@suse.cz> (raw)
In-Reply-To: <1457447457-25878-1-git-send-email-vgupta@synopsys.com>
On 03/08/2016 03:30 PM, Vineet Gupta wrote:
> We observed livelocks on ARC SMP setup when running hackbench with SLUB.
> This hardware configuration lacks atomic instructions (LLOCK/SCOND) thus
> kernel resorts to a central @smp_bitops_lock to protect any R-M-W ops
> suh as test_and_set_bit()
Sounds like this architecture should then redefine __clear_bit_unlock
and perhaps other non-atomic __X_bit() variants to be atomic, and not
defer this requirement to places that use the API?
> The spinlock itself is implemented using Atomic [EX]change instruction
> which is always available.
>
> The race happened when both cores tried to slab_lock() the same page.
>
> c1 c0
> ----------- -----------
> slab_lock
> slab_lock
> slab_unlock
> Not observing the unlock
>
> This in turn happened because slab_unlock() doesn't serialize properly
> (doesn't use atomic clear) with a concurrent running
> slab_lock()->test_and_set_bit()
>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Noam Camus <noamc@ezchip.com>
> Cc: <stable@vger.kernel.org>
> Cc: <linux-mm@kvack.org>
> Cc: <linux-kernel@vger.kernel.org>
> Cc: <linux-snps-arc@lists.infradead.org>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
> mm/slub.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index d8fbd4a6ed59..b7d345a508dc 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -345,7 +345,7 @@ static __always_inline void slab_lock(struct page *page)
> static __always_inline void slab_unlock(struct page *page)
> {
> VM_BUG_ON_PAGE(PageTail(page), page);
> - __bit_spin_unlock(PG_locked, &page->flags);
> + bit_spin_unlock(PG_locked, &page->flags);
> }
>
> static inline void set_page_slub_counters(struct page *page, unsigned long counters_new)
>
--
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>
WARNING: multiple messages have this Message-ID (diff)
From: Vlastimil Babka <vbabka@suse.cz>
To: Vineet Gupta <Vineet.Gupta1@synopsys.com>, linux-mm@kvack.org
Cc: Christoph Lameter <cl@linux.com>,
Pekka Enberg <penberg@kernel.org>,
David Rientjes <rientjes@google.com>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Andrew Morton <akpm@linux-foundation.org>,
Noam Camus <noamc@ezchip.com>,
stable@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-snps-arc@lists.infradead.org
Subject: Re: [PATCH] mm: slub: Ensure that slab_unlock() is atomic
Date: Tue, 8 Mar 2016 16:32:29 +0100 [thread overview]
Message-ID: <56DEF08D.607@suse.cz> (raw)
In-Reply-To: <1457447457-25878-1-git-send-email-vgupta@synopsys.com>
On 03/08/2016 03:30 PM, Vineet Gupta wrote:
> We observed livelocks on ARC SMP setup when running hackbench with SLUB.
> This hardware configuration lacks atomic instructions (LLOCK/SCOND) thus
> kernel resorts to a central @smp_bitops_lock to protect any R-M-W ops
> suh as test_and_set_bit()
Sounds like this architecture should then redefine __clear_bit_unlock
and perhaps other non-atomic __X_bit() variants to be atomic, and not
defer this requirement to places that use the API?
> The spinlock itself is implemented using Atomic [EX]change instruction
> which is always available.
>
> The race happened when both cores tried to slab_lock() the same page.
>
> c1 c0
> ----------- -----------
> slab_lock
> slab_lock
> slab_unlock
> Not observing the unlock
>
> This in turn happened because slab_unlock() doesn't serialize properly
> (doesn't use atomic clear) with a concurrent running
> slab_lock()->test_and_set_bit()
>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Noam Camus <noamc@ezchip.com>
> Cc: <stable@vger.kernel.org>
> Cc: <linux-mm@kvack.org>
> Cc: <linux-kernel@vger.kernel.org>
> Cc: <linux-snps-arc@lists.infradead.org>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
> mm/slub.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index d8fbd4a6ed59..b7d345a508dc 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -345,7 +345,7 @@ static __always_inline void slab_lock(struct page *page)
> static __always_inline void slab_unlock(struct page *page)
> {
> VM_BUG_ON_PAGE(PageTail(page), page);
> - __bit_spin_unlock(PG_locked, &page->flags);
> + bit_spin_unlock(PG_locked, &page->flags);
> }
>
> static inline void set_page_slub_counters(struct page *page, unsigned long counters_new)
>
next prev parent reply other threads:[~2016-03-08 15:32 UTC|newest]
Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-08 14:30 [PATCH] mm: slub: Ensure that slab_unlock() is atomic Vineet Gupta
2016-03-08 14:30 ` Vineet Gupta
2016-03-08 14:30 ` Vineet Gupta
2016-03-08 14:30 ` Vineet Gupta
2016-03-08 15:00 ` Christoph Lameter
2016-03-08 15:00 ` Christoph Lameter
2016-03-08 15:00 ` Christoph Lameter
2016-03-08 15:46 ` Vineet Gupta
2016-03-08 15:46 ` Vineet Gupta
2016-03-08 15:46 ` Vineet Gupta
2016-03-08 20:40 ` Christoph Lameter
2016-03-08 20:40 ` Christoph Lameter
2016-03-08 20:40 ` Christoph Lameter
2016-03-09 6:43 ` Vineet Gupta
2016-03-09 6:43 ` Vineet Gupta
2016-03-09 6:43 ` Vineet Gupta
2016-03-09 6:43 ` Vineet Gupta
2016-03-09 6:43 ` Vineet Gupta
2016-03-09 10:13 ` Peter Zijlstra
2016-03-09 10:13 ` Peter Zijlstra
2016-03-09 10:13 ` Peter Zijlstra
2016-03-09 10:13 ` Peter Zijlstra
2016-03-09 10:31 ` Peter Zijlstra
2016-03-09 10:31 ` Peter Zijlstra
2016-03-09 10:31 ` Peter Zijlstra
2016-03-09 11:12 ` Vineet Gupta
2016-03-09 11:12 ` Vineet Gupta
2016-03-09 11:12 ` Vineet Gupta
2016-03-09 11:12 ` Vineet Gupta
2016-03-09 11:12 ` Vineet Gupta
2016-03-09 11:00 ` Vineet Gupta
2016-03-09 11:00 ` Vineet Gupta
2016-03-09 11:00 ` Vineet Gupta
2016-03-09 11:00 ` Vineet Gupta
2016-03-09 11:00 ` Vineet Gupta
2016-03-09 11:40 ` Peter Zijlstra
2016-03-09 11:40 ` Peter Zijlstra
2016-03-09 11:40 ` Peter Zijlstra
2016-03-09 11:40 ` Peter Zijlstra
2016-03-09 11:53 ` Vineet Gupta
2016-03-09 11:53 ` Vineet Gupta
2016-03-09 11:53 ` Vineet Gupta
2016-03-09 11:53 ` Vineet Gupta
2016-03-09 11:53 ` Vineet Gupta
2016-03-09 12:22 ` Peter Zijlstra
2016-03-09 12:22 ` Peter Zijlstra
2016-03-09 12:22 ` Peter Zijlstra
2016-03-14 8:05 ` Vineet Gupta
2016-03-14 8:05 ` Vineet Gupta
2016-03-14 8:05 ` Vineet Gupta
2016-03-14 8:05 ` Vineet Gupta
2016-03-14 8:05 ` Vineet Gupta
2016-03-21 11:16 ` [tip:locking/urgent] bitops: Do not default to __clear_bit() for __clear_bit_unlock() tip-bot for Peter Zijlstra
2016-03-09 13:22 ` [PATCH] mm: slub: Ensure that slab_unlock() is atomic Vineet Gupta
2016-03-09 13:22 ` Vineet Gupta
2016-03-09 13:22 ` Vineet Gupta
2016-03-09 13:22 ` Vineet Gupta
2016-03-09 13:22 ` Vineet Gupta
2016-03-09 14:51 ` Peter Zijlstra
2016-03-09 14:51 ` Peter Zijlstra
2016-03-09 14:51 ` Peter Zijlstra
2016-03-10 5:51 ` Vineet Gupta
2016-03-10 5:51 ` Vineet Gupta
2016-03-10 5:51 ` Vineet Gupta
2016-03-10 5:51 ` Vineet Gupta
2016-03-10 5:51 ` Vineet Gupta
2016-03-10 9:10 ` Peter Zijlstra
2016-03-10 9:10 ` Peter Zijlstra
2016-03-10 9:10 ` Peter Zijlstra
2016-03-10 9:10 ` Peter Zijlstra
2016-03-08 15:32 ` Vlastimil Babka [this message]
2016-03-08 15:32 ` Vlastimil Babka
2016-03-08 15:32 ` Vlastimil Babka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56DEF08D.607@suse.cz \
--to=vbabka@suse.cz \
--cc=linux-snps-arc@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.