From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757589AbbJ2Tf5 (ORCPT ); Thu, 29 Oct 2015 15:35:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33085 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757560AbbJ2Tf4 (ORCPT ); Thu, 29 Oct 2015 15:35:56 -0400 From: Jeff Moyer To: Davidlohr Bueso Cc: Jens Axboe , Steven Rostedt , Ingo Molnar , linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] blktrace: re-write setting q->blk_trace References: <20151027030545.GH27292@linux-uzut.site> <20151027031452.GI27292@linux-uzut.site> <20151029191905.GA7947@linux-uzut.site> X-PGP-KeyID: 1F78E1B4 X-PGP-CertKey: F6FE 280D 8293 F72C 65FD 5A58 1FF8 A7CA 1F78 E1B4 X-PCLoadLetter: What the f**k does that mean? Date: Thu, 29 Oct 2015 15:35:54 -0400 In-Reply-To: <20151029191905.GA7947@linux-uzut.site> (Davidlohr Bueso's message of "Thu, 29 Oct 2015 12:19:05 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Davidlohr Bueso writes: >>The patch itself looks ok, but it doesn't seem to apply to a recent >>kernel tree. It appears as though it is white-space damaged. Would you >>mind re-sending it? > > Hmm sorry about that. I thought I had based it against that day's tip/master. > Anyway, here is v3 which is against tip/master as of 338e29ba93639138fafb9fb5ba946fd99a512aae. Thanks. Reviewed-by: Jeff Moyer > 8<-------------------------------------------------------------------------------- > From: Davidlohr Bueso > Date: Thu, 29 Oct 2015 12:13:24 -0700 > Subject: [PATCH -tip v3] blktrace: re-write setting q->blk_trace > > This is really about simplifying the double xchg patterns into > a single cmpxchg, with the same logic. Other than the immediate > cleanup, there are some subtleties this change deals with: > > (i) While the load of the old bt is fully ordered wrt everything, > ie: > > old_bt = xchg(&q->blk_trace, bt); [barrier] > if (old_bt) > (void) xchg(&q->blk_trace, old_bt); [barrier] > > blk_trace could still be changed between the xchg and the old_bt > load. Note that this description is merely theoretical and afaict > very small, but doing everything in a single context with cmpxchg > closes this potential race. > > (ii) Ordering guarantees are obviously kept with cmpxchg. > > (iii) Gets rid of the hacky-by-nature (void)xchg pattern. > > Signed-off-by: Davidlohr Bueso > --- > v3: rebased ontop of today's -tip. > minor changelog addition. > > kernel/trace/blktrace.c | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c > index 90e72a0..e3a2618 100644 > --- a/kernel/trace/blktrace.c > +++ b/kernel/trace/blktrace.c > @@ -437,7 +437,7 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, > struct block_device *bdev, > struct blk_user_trace_setup *buts) > { > - struct blk_trace *old_bt, *bt = NULL; > + struct blk_trace *bt = NULL; > struct dentry *dir = NULL; > int ret; > > @@ -519,11 +519,8 @@ int do_blk_trace_setup(struct request_queue *q, char *name, dev_t dev, > bt->trace_state = Blktrace_setup; > > ret = -EBUSY; > - old_bt = xchg(&q->blk_trace, bt); > - if (old_bt) { > - (void) xchg(&q->blk_trace, old_bt); > + if (cmpxchg(&q->blk_trace, NULL, bt)) > goto err; > - } > > if (atomic_inc_return(&blk_probes_ref) == 1) > blk_register_tracepoints(); > @@ -1481,7 +1478,7 @@ static int blk_trace_remove_queue(struct request_queue *q) > static int blk_trace_setup_queue(struct request_queue *q, > struct block_device *bdev) > { > - struct blk_trace *old_bt, *bt = NULL; > + struct blk_trace *bt = NULL; > int ret = -ENOMEM; > > bt = kzalloc(sizeof(*bt), GFP_KERNEL); > @@ -1497,12 +1494,9 @@ static int blk_trace_setup_queue(struct request_queue *q, > > blk_trace_setup_lba(bt, bdev); > > - old_bt = xchg(&q->blk_trace, bt); > - if (old_bt != NULL) { > - (void)xchg(&q->blk_trace, old_bt); > - ret = -EBUSY; > + ret = -EBUSY; > + if (cmpxchg(&q->blk_trace, NULL, bt)) > goto free_bt; > - } > > if (atomic_inc_return(&blk_probes_ref) == 1) > blk_register_tracepoints(); > -- > 2.1.4