Messages in this thread | | | Date | Sun, 28 Oct 2007 17:10:36 +0200 (EET) | From | Pekka J Enberg <> | Subject | Re: [patch 09/10] SLUB: Do our own locking via slab_lock and slab_unlock. |
| |
Hi Christoph,
On Sat, 27 Oct 2007, Christoph Lameter wrote: > Too many troubles with the bitlocks and we really do not need > to do any bitops. Bitops do not effectively retrieve the old > value which we want. So use a cmpxchg instead on the arches > that allow it.
> -static inline int SlabFrozen(struct page *page) > -{ > - return page->flags & FROZEN; > -} > - > -static inline void SetSlabFrozen(struct page *page) > -{ > - page->flags |= FROZEN; > -}
[snip]
It would be easier to review the actual locking changes if you did the SlabXXX removal in a separate patch.
> +#ifdef __HAVE_ARCH_CMPXCHG > /* > * Per slab locking using the pagelock > */ > -static __always_inline void slab_lock(struct page *page) > +static __always_inline void slab_unlock(struct page *page, > + unsigned long state) > { > - bit_spin_lock(PG_locked, &page->flags); > + smp_wmb();
Memory barriers deserve a comment. I suppose this is protecting page->flags but against what?
> + page->flags = state; > + preempt_enable();
We don't need preempt_enable for CONFIG_SMP, right?
> + __release(bitlock);
This needs a less generic name and maybe a comment explaining that it's not annotating a proper lock? Or maybe we can drop it completely?
> +static __always_inline unsigned long slab_trylock(struct page *page) > +{ > + unsigned long state; > + > + preempt_disable(); > + state = page->flags & ~LOCKED; > +#ifdef CONFIG_SMP > + if (cmpxchg(&page->flags, state, state | LOCKED) != state) { > + preempt_enable(); > + return 0; > + } > +#endif
This is hairy. Perhaps it would be cleaner to have totally separate functions for SMP and UP instead?
> -static __always_inline void slab_unlock(struct page *page) > +static __always_inline unsigned long slab_lock(struct page *page) > { > - bit_spin_unlock(PG_locked, &page->flags); > + unsigned long state; > + > + preempt_disable(); > +#ifdef CONFIG_SMP > + do { > + state = page->flags & ~LOCKED; > + } while (cmpxchg(&page->flags, state, state | LOCKED) != state); > +#else > + state = page->flags & ~LOCKED; > +#endif
Same here.
Pekka - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |