From patchwork Tue Apr 12 16:50:42 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Christoph Hellwig X-Patchwork-Id: 701291 Received: from mx4-phx2.redhat.com (mx4-phx2.redhat.com [209.132.183.25]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p3CGqjkZ008145 for ; Tue, 12 Apr 2011 16:53:06 GMT Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by mx4-phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p3CGopFJ000427; Tue, 12 Apr 2011 12:50:51 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p3CGoojp024434 for ; Tue, 12 Apr 2011 12:50:50 -0400 Received: from mx1.redhat.com (ext-mx13.extmail.prod.ext.phx2.redhat.com [10.5.110.18]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id p3CGoisC030184; Tue, 12 Apr 2011 12:50:44 -0400 Received: from bombadil.infradead.org (bombadil.infradead.org [18.85.46.34]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p3CGohhl014519; Tue, 12 Apr 2011 12:50:44 -0400 Received: from hch by bombadil.infradead.org with local (Exim 4.72 #1 (Red Hat Linux)) id 1Q9gna-0006uN-Ii; Tue, 12 Apr 2011 16:50:42 +0000 Date: Tue, 12 Apr 2011 12:50:42 -0400 From: "hch@infradead.org" To: Jens Axboe Message-ID: <20110412165042.GA23764@infradead.org> References: <20110411205928.13915719@notabene.brown> <4DA2E03A.2080607@fusionio.com> <20110411212635.7959de70@notabene.brown> <4DA2E7F0.9010904@fusionio.com> <20110411220505.1028816e@notabene.brown> <4DA2F00E.6010907@fusionio.com> <20110411223623.4278fad1@notabene.brown> <4DA2F8AD.1060605@fusionio.com> <20110412011255.GA29236@infradead.org> <4DA40F0E.1070903@fusionio.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4DA40F0E.1070903@fusionio.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org See http://www.infradead.org/rpr.html X-RedHat-Spam-Score: -5.01 (RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Scanned-By: MIMEDefang 2.68 on 10.5.110.18 X-loop: dm-devel@redhat.com Cc: "linux-raid@vger.kernel.org" , "dm-devel@redhat.com" , "linux-kernel@vger.kernel.org" , Mike Snitzer Subject: Re: [dm-devel] [PATCH 05/10] block: remove per-queue plugging X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Tue, 12 Apr 2011 16:53:07 +0000 (UTC) On Tue, Apr 12, 2011 at 10:36:30AM +0200, Jens Axboe wrote: > The existance and out-of-line is for the scheduler() hook. It should be > an unlikely event to schedule with a plug held, normally the plug should > have been explicitly unplugged before that happens. I still don't think unlikely() is the right thing to do. The static branch prediction hints cause a real massive slowdown if taken. For things like this that happen during normal operation you're much better off leaving the dynamic branch prediction in the CPU predicting what's going on. And I don't think it's all that unlikely - e.g. for all the metadata during readpages/writepages schedule/io_schedule will be the unplugging point right now. I'll see if I can run an I/O workload with Steve's likely/unlikely profiling turned on. > > void __blk_flush_plug(struct task_struct *tsk, struct blk_plug *plug) > > { > > flush_plug_list(plug); > > if (plug == tsk->plug) > > tsk->plug = NULL; > > tsk->plug = plug; > > } > > > > it would seem much smarted to just call flush_plug_list directly. > > In fact it seems like the tsk->plug is not nessecary at all and > > all remaining __blk_flush_plug callers could be replaced with > > flush_plug_list. > > It depends on whether this was an explicit unplug (eg > blk_finish_plug()), or whether it was an implicit event (eg on > schedule()). If we do it on schedule(), then we retain the plug after > the flush. Otherwise we clear it. blk_finish_plug doesn't got through this codepath. This is an untested patch how the area should look to me: --- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel diff --git a/block/blk-core.c b/block/blk-core.c index 90f22cc..6fa5ba1 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -2668,7 +2668,7 @@ static int plug_rq_cmp(void *priv, struct list_head *a, struct list_head *b) return !(rqa->q <= rqb->q); } -static void flush_plug_list(struct blk_plug *plug) +void blk_flush_plug_list(struct blk_plug *plug) { struct request_queue *q; unsigned long flags; @@ -2716,29 +2716,16 @@ static void flush_plug_list(struct blk_plug *plug) BUG_ON(!list_empty(&plug->list)); local_irq_restore(flags); } - -static void __blk_finish_plug(struct task_struct *tsk, struct blk_plug *plug) -{ - flush_plug_list(plug); - - if (plug == tsk->plug) - tsk->plug = NULL; -} +EXPORT_SYMBOL_GPL(blk_flush_plug_list); void blk_finish_plug(struct blk_plug *plug) { - if (plug) - __blk_finish_plug(current, plug); + blk_flush_plug_list(plug); + if (plug == current->plug) + current->plug = NULL; } EXPORT_SYMBOL(blk_finish_plug); -void __blk_flush_plug(struct task_struct *tsk, struct blk_plug *plug) -{ - __blk_finish_plug(tsk, plug); - tsk->plug = plug; -} -EXPORT_SYMBOL(__blk_flush_plug); - int __init blk_dev_init(void) { BUILD_BUG_ON(__REQ_NR_BITS > 8 * diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 32176cc..fa6a4e1 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -862,14 +862,14 @@ struct blk_plug { extern void blk_start_plug(struct blk_plug *); extern void blk_finish_plug(struct blk_plug *); -extern void __blk_flush_plug(struct task_struct *, struct blk_plug *); +extern void blk_flush_plug_list(struct blk_plug *); static inline void blk_flush_plug(struct task_struct *tsk) { struct blk_plug *plug = tsk->plug; - if (unlikely(plug)) - __blk_flush_plug(tsk, plug); + if (plug) + blk_flush_plug_list(plug); } static inline bool blk_needs_flush_plug(struct task_struct *tsk)