Re: SLUB: Avoid atomic operation for slab_unlock
Acked-by: Christoph Lameter <[EMAIL PROTECTED]>
Slub can use the non-atomic version to unlock because other flags will not
get modified with the lock held.
Signed-off-by: Nick Piggin <[EMAIL PROTECTED]>
---
mm/slub.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/slub.c
===
--- linux-2.6.orig/mm/slub.c
+++ linux-2.6/mm/slub.c
@@ -1185,7 +1185,7 @@ static __always_inline void slab_lock(st
static __always_inline void slab_unlock(struct page *page)
{
- bit_spin_unlock(PG_locked, &page->flags);
+ __bit_spin_unlock(PG_locked, &page->flags);
}
static __always_inline int slab_trylock(struct page *page)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: SLUB: Avoid atomic operation for slab_unlock
On Friday 19 October 2007 12:01, Christoph Lameter wrote: > On Fri, 19 Oct 2007, Nick Piggin wrote: > > > Yes that is what I attempted to do with the write barrier. To my > > > knowledge there are no reads that could bleed out and I wanted to avoid > > > a full fence instruction there. > > > > Oh, OK. Bit risky ;) You might be right, but anyway I think it > > should be just as fast with the optimised bit_unlock on most > > architectures. > > How expensive is the fence? An store with release semantics would be safer > and okay for IA64. I'm not sure, I had an idea it was relatively expensive on ia64, but I didn't really test with a good workload (a microbenchmark probably isn't that good because it won't generate too much out of order memory traffic that needs to be fenced). > > Which reminds me, it would be interesting to test the ia64 > > implementation I did. For the non-atomic unlock, I'm actually > > doing an atomic operation there so that it can use the release > > barrier rather than the mf. Maybe it's faster the other way around > > though? Will be useful to test with something that isn't a trivial > > loop, so the slub case would be a good benchmark. > > Lets avoid mf (too expensive) and just use a store with release semantics. OK, that's what I've done at the moment. > Where can I find your patchset? I looked through lkml but did not see it. Infrastructure in -mm, starting at bitops-introduce-lock-ops.patch. bit_spin_lock-use-lock-bitops.patch and ia64-lock-bitops.patch are ones to look at. The rest of the patches I have queued here, apart from the SLUB patch, I guess aren't so interesting to you (they don't do anything fancy like convert to non-atomic unlocks, just switch things like page and buffer locks to use new bitops). - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: SLUB: Avoid atomic operation for slab_unlock
On Fri, 19 Oct 2007, Nick Piggin wrote: > > Yes that is what I attempted to do with the write barrier. To my knowledge > > there are no reads that could bleed out and I wanted to avoid a full fence > > instruction there. > > Oh, OK. Bit risky ;) You might be right, but anyway I think it > should be just as fast with the optimised bit_unlock on most > architectures. How expensive is the fence? An store with release semantics would be safer and okay for IA64. > Which reminds me, it would be interesting to test the ia64 > implementation I did. For the non-atomic unlock, I'm actually > doing an atomic operation there so that it can use the release > barrier rather than the mf. Maybe it's faster the other way around > though? Will be useful to test with something that isn't a trivial > loop, so the slub case would be a good benchmark. Lets avoid mf (too expensive) and just use a store with release semantics. Where can I find your patchset? I looked through lkml but did not see it. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: SLUB: Avoid atomic operation for slab_unlock
On Friday 19 October 2007 11:21, Christoph Lameter wrote: > On Fri, 19 Oct 2007, Nick Piggin wrote: > > Ah, thanks, but can we just use my earlier patch that does the > > proper __bit_spin_unlock which is provided by > > bit_spin_lock-use-lock-bitops.patch > > Ok. > > > This primitive should have a better chance at being correct, and > > also potentially be more optimised for each architecture (it > > only has to provide release consistency). > > Yes that is what I attempted to do with the write barrier. To my knowledge > there are no reads that could bleed out and I wanted to avoid a full fence > instruction there. Oh, OK. Bit risky ;) You might be right, but anyway I think it should be just as fast with the optimised bit_unlock on most architectures. Which reminds me, it would be interesting to test the ia64 implementation I did. For the non-atomic unlock, I'm actually doing an atomic operation there so that it can use the release barrier rather than the mf. Maybe it's faster the other way around though? Will be useful to test with something that isn't a trivial loop, so the slub case would be a good benchmark. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: SLUB: Avoid atomic operation for slab_unlock
On Fri, 19 Oct 2007, Nick Piggin wrote: > Ah, thanks, but can we just use my earlier patch that does the > proper __bit_spin_unlock which is provided by > bit_spin_lock-use-lock-bitops.patch Ok. > This primitive should have a better chance at being correct, and > also potentially be more optimised for each architecture (it > only has to provide release consistency). Yes that is what I attempted to do with the write barrier. To my knowledge there are no reads that could bleed out and I wanted to avoid a full fence instruction there. > I have attached the patch here just for reference, but actually > I am submitting it properly as part of a patch series today, now > that the base bit lock patches have been sent upstream. Good. Andrew: Drop my patch when this goes in. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: SLUB: Avoid atomic operation for slab_unlock
[sorry, I read and replied to my inbox before mailing lists...
please ignore the last mail on this patch, and reply to this
one which is properly threaded]
On Friday 19 October 2007 08:15, Christoph Lameter wrote:
> Currently page flags are only modified in SLUB under page lock. This means
> that we do not need an atomic operation to release the lock since there
> is nothing we can race against that is modifying page flags. We can simply
> clear the bit without the use of an atomic operation and make sure that
> this change becomes visible after the other changes to slab metadata
> through a memory barrier.
>
> The performance of slab_free() increases 10-15% (SMP configuration doing
> a long series of remote frees).
Ah, thanks, but can we just use my earlier patch that does the
proper __bit_spin_unlock which is provided by
bit_spin_lock-use-lock-bitops.patch
This primitive should have a better chance at being correct, and
also potentially be more optimised for each architecture (it
only has to provide release consistency).
I have attached the patch here just for reference, but actually
I am submitting it properly as part of a patch series today, now
that the base bit lock patches have been sent upstream.
> mm/slub.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff -puN mm/slub.c~slub-avoid-atomic-operation-for-slab_unlock mm/slub.c
> --- a/mm/slub.c~slub-avoid-atomic-operation-for-slab_unlock
> +++ a/mm/slub.c
> @@ -1181,9 +1181,22 @@ static __always_inline void slab_lock(st
> bit_spin_lock(PG_locked, &page->flags);
> }
>
> +/*
> + * Slab unlock version that avoids having to use atomic operations
> + * (echos some of the code of bit_spin_unlock!)
> + */
> static __always_inline void slab_unlock(struct page *page)
> {
> - bit_spin_unlock(PG_locked, &page->flags);
> +#ifdef CONFIG_SMP
> + unsigned long flags;
> +
> + flags = page->flags & ~(1 << PG_locked);
> +
> + smp_wmb();
> + page->flags = flags;
> +#endif
> + preempt_enable();
> + __release(bitlock);
> }
This looks wrong, because it would allow the store unlocking
flags to pass a load within the critical section.
stores aren't allowed to pass loads in x86 (only vice versa),
so you might have been confused by looking at x86's spinlocks
into thinking this will work. However on powerpc and sparc, I
don't think it gives you the right types of barriers.
Slub can use the non-atomic version to unlock because other flags will not
get modified with the lock held.
Signed-off-by: Nick Piggin <[EMAIL PROTECTED]>
---
mm/slub.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/mm/slub.c
===
--- linux-2.6.orig/mm/slub.c
+++ linux-2.6/mm/slub.c
@@ -1185,7 +1185,7 @@ static __always_inline void slab_lock(st
static __always_inline void slab_unlock(struct page *page)
{
- bit_spin_unlock(PG_locked, &page->flags);
+ __bit_spin_unlock(PG_locked, &page->flags);
}
static __always_inline int slab_trylock(struct page *page)
