From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mel Gorman Subject: Re: [PATCH 17/19] fs: buffer: Do not use unnecessary atomic operations when discarding buffers Date: Tue, 13 May 2014 15:46:42 +0100 Message-ID: <20140513144642.GV23991@suse.de> References: <1399974350-11089-1-git-send-email-mgorman@suse.de> <1399974350-11089-18-git-send-email-mgorman@suse.de> <20140513110951.GB30445@twins.programming.kicks-ass.net> <20140513125007.GQ23991@suse.de> <20140513140127.GC2485@laptop.programming.kicks-ass.net> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Cc: Andrew Morton , Johannes Weiner , Vlastimil Babka , Jan Kara , Michal Hocko , Hugh Dickins , Dave Hansen , Linux Kernel , Linux-MM , Linux-FSDevel To: Peter Zijlstra Return-path: Content-Disposition: inline In-Reply-To: <20140513140127.GC2485@laptop.programming.kicks-ass.net> Sender: owner-linux-mm@kvack.org List-Id: linux-fsdevel.vger.kernel.org On Tue, May 13, 2014 at 04:01:27PM +0200, Peter Zijlstra wrote: > 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. > Of course, thanks for pointing that out. I was only thinking of it in terms of it being a clear_bit operation which was dumb. > 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. I do not really expect the initial value to match the new state. At the very least I would expect BH_mapped to be routinely cleared during this operation so I doubt it's worth the effort trying to deal with conditional buffers. -- Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761012AbaEMOqu (ORCPT ); Tue, 13 May 2014 10:46:50 -0400 Received: from cantor2.suse.de ([195.135.220.15]:57792 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760155AbaEMOqs (ORCPT ); Tue, 13 May 2014 10:46:48 -0400 Date: Tue, 13 May 2014 15:46:42 +0100 From: Mel Gorman To: Peter Zijlstra Cc: Andrew Morton , Johannes Weiner , Vlastimil Babka , Jan Kara , Michal Hocko , Hugh Dickins , Dave Hansen , Linux Kernel , Linux-MM , Linux-FSDevel Subject: Re: [PATCH 17/19] fs: buffer: Do not use unnecessary atomic operations when discarding buffers Message-ID: <20140513144642.GV23991@suse.de> References: <1399974350-11089-1-git-send-email-mgorman@suse.de> <1399974350-11089-18-git-send-email-mgorman@suse.de> <20140513110951.GB30445@twins.programming.kicks-ass.net> <20140513125007.GQ23991@suse.de> <20140513140127.GC2485@laptop.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <20140513140127.GC2485@laptop.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 13, 2014 at 04:01:27PM +0200, Peter Zijlstra wrote: > 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. > Of course, thanks for pointing that out. I was only thinking of it in terms of it being a clear_bit operation which was dumb. > 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. I do not really expect the initial value to match the new state. At the very least I would expect BH_mapped to be routinely cleared during this operation so I doubt it's worth the effort trying to deal with conditional buffers. -- Mel Gorman SUSE Labs