Re: [PATCH] arp_queue: serializing unlink + kfree_skb

David S. Miller
2005-02-03T22:29:52Z

On Fri, 4 Feb 2005 07:30:10 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Fri, Feb 04, 2005 at 01:27:05AM +1100, Anton Blanchard wrote:
> >
> > Architectures should guarantee that any of the atomics and bitops that
> > return values order in both directions. So you dont need the
> > smp_mb__before_atomic_dec here.
>
> I wasn't aware of this requirement before. However, if this is so,
> why don't we get rid of the smp_mb__* macros?

They are for cases where you want strict ordering even for the
non-return-value-giving atomic_t ops.

Actually.... Herbert has a point. By Anton's specification, several
uses in 2.6.x I see of these smp_mb__*() routines are bogus. Case
in point, look at mm/filemap.c:

void fastcall unlock_page(struct page *page)
{
smp_mb__before_clear_bit();
if (!TestClearPageLocked(page))
BUG();
smp_mb__after_clear_bit();
wake_up_page(page, PG_locked);
}

TestClearPageLocked() uses one of the bitops returning a value, so
must be providing the explicit memory barriers in it's implementation.

void end_page_writeback(struct page *page)
{
if (!TestClearPageReclaim(page) || rotate_reclaimable_page(page)) {
if (!test_clear_page_writeback(page))
BUG();
}
smp_mb__after_clear_bit();
wake_up_page(page, PG_writeback);
}

Same thing there.

Looking at include/linux/sunrpc/sched.h, those uses are legitimate,
correct, and needed. As is the put_bh() use in include/linux/buffer_head.h
There are several other correct and necessary uses in:

include/linux/interrupt.h
include/linux/netdevice.h
include/linux/nfs_page.h
include/linux/spinlock.h
net/core/dev.c
net/sunrpc/sched.c
sound/pci/bt87x.c
fs/buffer.c
fs/nfs/pagelist.c
drivers/block/ll_rw_blk.c
arch/ppc64/kernel/smp.c
arch/i386/kernel/smp.c
arch/i386/mach-voyager/smp.c

I'm working on a rough but rather complete draft Anton said needs
to be written to explicitly spell out the atomic_t and bitops
stuff.