linux-kernel - Re: [PATCH 17/19] fs: buffer: Do not use unnecessary atomic operations when discarding buffers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140513140127.GC2485@laptop.programming.kicks-ass.net>
Date:	Tue, 13 May 2014 16:01:27 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Mel Gorman <mgorman@...e.de>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Johannes Weiner <hannes@...xchg.org>,
	Vlastimil Babka <vbabka@...e.cz>, Jan Kara <jack@...e.cz>,
	Michal Hocko <mhocko@...e.cz>, Hugh Dickins <hughd@...gle.com>,
	Dave Hansen <dave.hansen@...el.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Linux-MM <linux-mm@...ck.org>,
	Linux-FSDevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH 17/19] fs: buffer: Do not use unnecessary atomic
 operations when discarding buffers

On Tue, May 13, 2014 at 01:50:07PM +0100, Mel Gorman wrote:
> > Anyway, nothing wrong with this patch, however, you could, if you really
> > wanted to push things, also include BH_Lock in that clear :-)
> 
> That's a bold strategy Cotton.

:-)

> Untested patch on top
> 
> ---8<---
> diff --git a/fs/buffer.c b/fs/buffer.c
> index e80012d..42fcb6d 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1490,6 +1490,8 @@ static void discard_buffer(struct buffer_head * bh)
>  	lock_buffer(bh);
>  	clear_buffer_dirty(bh);
>  	bh->b_bdev = NULL;
> +
> +	smp_mb__before_clear_bit();

Not needed.

>  	b_state = bh->b_state;
>  	for (;;) {
>  		b_state_old = cmpxchg(&bh->b_state, b_state, (b_state & ~BUFFER_FLAGS_DISCARD));
> @@ -1497,7 +1499,13 @@ static void discard_buffer(struct buffer_head * bh)
>  			break;
>  		b_state = b_state_old;
>  	}
> -	unlock_buffer(bh);
> +
> +	/*
> +	 * BUFFER_FLAGS_DISCARD include BH_lock so it has been cleared so the
> +	 * wake_up_bit is the last part of a unlock_buffer
> +	 */
> +	smp_mb__after_clear_bit();

Similarly superfluous.

> +	wake_up_bit(&bh->b_state, BH_Lock);
>  }

The thing is that cmpxchg() guarantees full barrier semantics before and
after the op, and since the loop guarantees at least one cmpxchg() call
its all good.

Now just to confuse everyone, you could have written the loop like:

	b_state = bh->b_state;
	for (;;) {
		b_state_new = b_state & ~BUFFER_FLAGS_DISCARD;
		if (b_state == b_state_new)
			break;
		b_state = cmpxchg(&bh->b_state, b_state, b_state_new);
	}

Which is 'similar' but doesn't guarantee that cmpxchg() gets called.
If you expect the initial value to match the new state, the above form
is slightly faster, but the lack of barrier guarantees can still spoil
the fun.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/