From patchwork Wed Dec 20 20:18:47 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Zijlstra X-Patchwork-Id: 10126539 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id D9D0A60390 for ; Wed, 20 Dec 2017 20:18:55 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C9B2D298CC for ; Wed, 20 Dec 2017 20:18:55 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id BB9B9298E0; Wed, 20 Dec 2017 20:18:55 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 5F146298CC for ; Wed, 20 Dec 2017 20:18:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756142AbdLTUSw (ORCPT ); Wed, 20 Dec 2017 15:18:52 -0500 Received: from bombadil.infradead.org ([65.50.211.133]:46713 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755839AbdLTUSv (ORCPT ); Wed, 20 Dec 2017 15:18:51 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=vj1w0hbJ2gnfgIipeB0jtng6vxH6wycnUSjv7Vqr3v0=; b=WkMHFXeIRgRc2xTQ3FhcdZkhI beP+4uRCoHTC9U5UsgzBhbaff2OouTwaxXm2/suCMvfJomZGDUnKY3128VL1tQdhCI9ozQ770U6P5 xOFWdej5E2apglc1sRs9gGQSW3FrkF0z81CCOIuPoYiPuwpF0KcmORu9N6Vx2/NRDMflz4ltfoHcA 27E72ell3+SF3ATcszeJl9SB7GapFAPvXrTyy0WGxgwVkMZD35CIJIYjTmRR1DwUQi/n0tIilZDI/ 08oqBTdQ6j1SwydlBlzlenIu4vdG99cLhmjTz3TPSv6dN/57dha7QCw0A6orJp6l8mj675DsgEkoO JDc5b9b2Q==; Received: from j217100.upc-j.chello.nl ([24.132.217.100] helo=hirez.programming.kicks-ass.net) by bombadil.infradead.org with esmtpsa (Exim 4.89 #1 (Red Hat Linux)) id 1eRkpE-0003bF-RP; Wed, 20 Dec 2017 20:18:49 +0000 Received: by hirez.programming.kicks-ass.net (Postfix, from userid 1000) id 099172024D44B; Wed, 20 Dec 2017 21:18:47 +0100 (CET) Date: Wed, 20 Dec 2017 21:18:47 +0100 From: Peter Zijlstra To: Jens Axboe Cc: "linux-block@vger.kernel.org" , Huang Ying , Ingo Molnar Subject: Re: random call_single_data alignment Message-ID: <20171220201846.7pbwf5vq57luzg5q@hirez.programming.kicks-ass.net> References: <3875cde5-a193-6aa6-1f9e-561cc66110f2@kernel.dk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <3875cde5-a193-6aa6-1f9e-561cc66110f2@kernel.dk> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-block-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, Dec 20, 2017 at 12:40:25PM -0700, Jens Axboe wrote: > On 12/20/17 12:10 PM, Jens Axboe wrote: > > For some reason, commit 966a967116e6 was added to the tree without > > CC'ing relevant maintainers, even though it's touching random subsystems. > > One example is struct request, a core structure in the block layer. > > After this change, struct request grows from 296 to 320 bytes on my > > setup. > https://marc.info/?l=linux-block&m=151379793913822&w=2 Does that actually matter though?, kmalloc is likely to over-allocate in any case. Sure it introduces a weird hole in the data structure, but that can be easily fixed by rearranging the thing. struct request { struct list_head { struct list_head * next; /* 0 8 */ struct list_head * prev; /* 8 8 */ } queuelist; /* 0 16 */ /* XXX 16 bytes hole, try to pack */ union { /* typedef call_single_data_t */ struct __call_single_data { struct llist_node { struct llist_node * next; /* 32 8 */ } llist; /* 32 8 */ /* typedef smp_call_func_t */ void (*func)(void *); /* 40 8 */ void * info; /* 48 8 */ unsigned int flags; /* 56 4 */ } csd; /* 32 */ /* typedef u64 */ long long unsigned int fifo_time; /* 8 */ }; /* 32 32 */ /* --- cacheline 1 boundary (64 bytes) --- */ .... } Gets you the exact same size back. > In the future, please CC the relevant folks before making (and > committing) changes like that. Yeah, I usually do, sorry about that :/ diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 8089ca17db9a..9d6fb6d59268 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -133,11 +133,11 @@ typedef __u32 __bitwise req_flags_t; * especially blk_mq_rq_ctx_init() to take care of the added fields. */ struct request { - struct list_head queuelist; union { call_single_data_t csd; u64 fifo_time; }; + struct list_head queuelist; struct request_queue *q; struct blk_mq_ctx *mq_ctx; > > Why are we blindly aligning to 32 bytes? The comment says to avoid > > it spanning two cache lines - but if that's the case, look at the > > actual use case and see if that's actually a problem. For struct > > request, it is not. > > > > Seems to me, this should have been applied in the specific area > > where it was needed. Keep struct call_single_data (instead of some > > __ version), and just manually align it where it matters. Without enforcement of some kind, its too easy to get wrong. > https://marc.info/?l=linux-block&m=151379849914002&w=2 diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c index ccb9975a97fa..e0c44e4efa44 100644 --- a/drivers/block/null_blk.c +++ b/drivers/block/null_blk.c @@ -33,9 +33,9 @@ static inline u64 mb_per_tick(int mbps) } struct nullb_cmd { + call_single_data_t csd; struct list_head list; struct llist_node ll_list; - call_single_data_t csd; struct request *rq; struct bio *bio; unsigned int tag;