From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.6 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3985EC433E1 for ; Thu, 4 Jun 2020 03:33:17 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id F1C99206DC for ; Thu, 4 Jun 2020 03:33:16 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="czpHzZfJ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F1C99206DC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=infradead.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 7605280009; Wed, 3 Jun 2020 23:33:16 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 711F880007; Wed, 3 Jun 2020 23:33:16 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 6003180009; Wed, 3 Jun 2020 23:33:16 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0249.hostedemail.com [216.40.44.249]) by kanga.kvack.org (Postfix) with ESMTP id 4661180007 for ; Wed, 3 Jun 2020 23:33:16 -0400 (EDT) Received: from smtpin15.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 0B3F32497 for ; Thu, 4 Jun 2020 03:33:16 +0000 (UTC) X-FDA: 76890108792.15.sky81_56ad3b7a5f10e Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin15.hostedemail.com (Postfix) with ESMTP id D970F1814B0CC for ; Thu, 4 Jun 2020 03:33:15 +0000 (UTC) X-HE-Tag: sky81_56ad3b7a5f10e X-Filterd-Recvd-Size: 4253 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) by imf41.hostedemail.com (Postfix) with ESMTP for ; Thu, 4 Jun 2020 03:33:15 +0000 (UTC) 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; bh=NeWDl+dUFaAgbo2rgQrEyhwG3FwcRQeFQWfmPWBSuo8=; b=czpHzZfJxlsSO9ZBia9eluIgMr gTHBfc5gHw3efNtrlwybcAH7wdTPb2ErPXTXJsByBNyFLruXk2QmJMDQvZic9KS5SHTjHKsWiWu6S l9Oid5xzQtxzvebHsKU4wMimmdbSYRarh8/89cCw2Ni4OwYM594/OphsJeP0qNBSdUMX+KMLPYzGb aEtz14V6DNFyM59FyEAOyDUqbCZU2QwHf28ZeY7HpcHEgQzJz0CYcgxrCdp0Nm0VD96oNnR26BR/3 wwx9p1LyTG2ZLduiGBb2tspLCd0FkTdeMZLy/8gniR4J+WbM2RXYjkpZ6CA7g470cxND9jW8XjfRh 9C5w2TQg==; Received: from willy by bombadil.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1jggcv-0003xl-Oh; Thu, 04 Jun 2020 03:33:09 +0000 Date: Wed, 3 Jun 2020 20:33:09 -0700 From: Matthew Wilcox To: William Kucharski Cc: Andrew Morton , linux-mm@kvack.org Subject: Re: [PATCH] vmalloc: Convert to XArray Message-ID: <20200604033309.GS19604@bombadil.infradead.org> References: <20200603171448.5894-1-willy@infradead.org> <3CE9C10C-2C1E-4363-94A6-BD4724881926@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3CE9C10C-2C1E-4363-94A6-BD4724881926@oracle.com> X-Rspamd-Queue-Id: D970F1814B0CC X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam05 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Jun 03, 2020 at 11:35:24AM -0600, William Kucharski wrote: > > - err = radix_tree_preload(gfp_mask); > > - if (unlikely(err)) { > > + err = xa_insert(&vmap_blocks, vb_idx, vb, gfp_mask); > > + if (err) { > > Should the "(err)" here be "unlikely(err)" as the radix tree version was? That's a good question. In previous years, GCC used to be stupider and we had to help it out by annotating which paths were more or less likely to be taken. Now it generally makes the right decision (eg understanding that an "early exit" which unwinds state from a function is likely to be an error case and thus the slow path), so we no longer need to mark nearly as much code with unlikely() as we used to. Similar things are true for prefetch() calls. I took a look at the disassembly of this code with and without the unlikely() and I also compared if (err) with if (err < 0). In the end, it makes no difference to the control flow (all variants jump to the end of the function) although it changes the register allocation decisions a little (!) What did make a difference was moving all the error handling to the end of the function; reduced the size of the function by 48 bytes. This is with gcc-9.3. I can submit this patch as a follow-up since it's basically unrelated to the other change. diff --git a/mm/vmalloc.c b/mm/vmalloc.c index 375bbb410a94..3d5b5c32c840 100644 --- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -1569,10 +1569,8 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask) va = alloc_vmap_area(VMAP_BLOCK_SIZE, VMAP_BLOCK_SIZE, VMALLOC_START, VMALLOC_END, node, gfp_mask); - if (IS_ERR(va)) { - kfree(vb); - return ERR_CAST(va); - } + if (IS_ERR(va)) + goto free_vb; vaddr = vmap_block_vaddr(va->va_start, 0); spin_lock_init(&vb->lock); @@ -1587,11 +1585,8 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask) vb_idx = addr_to_vb_idx(va->va_start); err = xa_insert(&vmap_blocks, vb_idx, vb, gfp_mask); - if (err) { - kfree(vb); - free_vmap_area(va); - return ERR_PTR(err); - } + if (err < 0) + goto free_va; vbq = &get_cpu_var(vmap_block_queue); spin_lock(&vbq->lock); @@ -1600,6 +1595,13 @@ static void *new_vmap_block(unsigned int order, gfp_t gfp_mask) put_cpu_var(vmap_block_queue); return vaddr; + +free_va: + free_vmap_area(va); + va = ERR_PTR(err); +free_vb: + kfree(vb); + return ERR_CAST(va); } static void free_vmap_block(struct vmap_block *vb) > Nice change and simplifies the code quite a bit. > > Reviewed-by: William Kucharski