On Sun, 28 Oct 2007, Pekka J Enberg wrote: > It would be easier to review the actual locking changes if you did the > SlabXXX removal in a separate patch.
There are no locking changes. > > -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? Its making sure that the changes to page flags are made visible after all other changes. > > > + page->flags = state; > > + preempt_enable(); > > We don't need preempt_enable for CONFIG_SMP, right? preempt_enable is needed if preemption is enabled. > > > + __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? Probably. > > +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? I think that is reasonably clear. Having code duplicated is not good either. Well we may have to clean this up a bit. - 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/