From patchwork Sun Sep 25 19:56:17 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Linus Torvalds X-Patchwork-Id: 9349903 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 BDD2D6077B for ; Sun, 25 Sep 2016 19:56:39 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B06F028BC1 for ; Sun, 25 Sep 2016 19:56:39 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id A4E3828BD1; Sun, 25 Sep 2016 19:56:39 +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.3 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RCVD_IN_SORBS_SPAM,T_DKIM_INVALID,T_TVD_MIME_EPI autolearn=unavailable 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 4515228BC1 for ; Sun, 25 Sep 2016 19:56:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966260AbcIYT4U (ORCPT ); Sun, 25 Sep 2016 15:56:20 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:33665 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966064AbcIYT4T (ORCPT ); Sun, 25 Sep 2016 15:56:19 -0400 Received: by mail-oi0-f68.google.com with SMTP id w11so12358525oia.0; Sun, 25 Sep 2016 12:56:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=KJySagfplDx2iEDQfIKYbFf6V2DJYDv1XQlYi2+gpRg=; b=fsVmlt+Cf+576hasvERGScVLMl7oTkspmuQqsNsQUMHou5a4DDcB3aF/g8mQRRfvsi +AOXqJo1MbJNT8flofLZW1bYFTPhYqG2VqS80j+JyZ4mQ6BSvXKMlJAz/EpoNjbPl/I9 2+iY9p89X3cBnpZ4nKgU/5C3fVsyKTWvm1Hwz7mOSi8hIvCJvzBJC/qdxCMeWNY2qamb 1Ep8FlVS9duOKjDExy5KKXQbJ4fqX0orQKMCLP5oCyKRN//fj8JyrFxzI8xyPYEvBuul aQX29J2IxTYTd+rp0WzuMQi5rkG9twcKyBuhKSbiIjKiwdrI0HQm7z4SPxwWFAkhCSaR 9+YQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=KJySagfplDx2iEDQfIKYbFf6V2DJYDv1XQlYi2+gpRg=; b=G0QEKDHgev49BysftBKYIrCcX44mYxsEN737F775yVwFmGLuPa9Zr8L5gqUHEQtkGw fSjWAT5sDPGZlIFAqZfx37GRcATsgrKYLP9vidqCqWklii0Eh1y2g0UXg/Yd2kbWOM6h 0Gta9MVCf0cqW8rwRsL7xTERe/axnmQmzeHu6U2fBzQ7ewJsIbSP0BGzUue87MTMUpmM BHVGzQIEClhho+C4le3A7rXKBhSVturUZY99dPExazJ705+uEe8DqRL9WbvSRaPlsxwC 7YzyQF6C5cG7QYBj92PIaEl9L9lWY/EuMZeBMdXC6OVP79/OJuMOkV97SwxIjHT5UGjn BgtA== X-Gm-Message-State: AE9vXwPjcsZjgBn14oEIMH/68qvpLMvP2jVVzrvOggKySTFe+xvGNslijls9XHBaDZpQisPu4T5OlB0jflcdBg== X-Received: by 10.202.181.212 with SMTP id e203mr23985381oif.1.1474833378141; Sun, 25 Sep 2016 12:56:18 -0700 (PDT) MIME-Version: 1.0 Received: by 10.182.119.104 with HTTP; Sun, 25 Sep 2016 12:56:17 -0700 (PDT) In-Reply-To: References: <1474570415-14938-1-git-send-email-mawilcox@linuxonhyperv.com> <1474570415-14938-3-git-send-email-mawilcox@linuxonhyperv.com> From: Linus Torvalds Date: Sun, 25 Sep 2016 12:56:17 -0700 X-Google-Sender-Auth: KmAzM3kS-hjSSSv3NjS_I71zpNA Message-ID: Subject: Re: [PATCH 2/2] radix-tree: Fix optimisation problem To: Cedric Blancher Cc: Ross Zwisler , Matthew Wilcox , Konstantin Khlebnikov , Andrew Morton , "Kirill A. Shutemov" , linux-fsdevel , Linux MM , Linux Kernel Mailing List , Matthew Wilcox Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Sun, Sep 25, 2016 at 12:04 PM, Linus Torvalds wrote: > It gets rid of > the ad-hoc arithmetic in radix_tree_descend(), and just makes all that > be inside the is_sibling_entry() logic instead. Which got renamed and > made to actually return the main sibling. Sadly, it looks like gcc generates bad code for this approach. Looks like it ends up testing the resulting sibling pointer twice (because we explicitly disable -fno-delete-null-pointer-checks in the kernel, and we have no way to say "look, I know this pointer I'm returning is non-null"). So a smaller patch that keeps the old boolean "is_sibling_entry()" but then actually *uses* that inside radix_tree_descend() and then tries to make the nasty cast to "void **" more legible by making it use a temporary variable seems to be a reasonable balance. At least I feel like I can still read the code, but admittedly by now that may be because I've stared at those few lines so much that I feel like I know what's going on. So maybe the code isn't actually any more legible after all. .. and unlike my previous patch, it actually generates better code than the original (while still passing the fixed test-suite, of course). The reason seems to be exactly that temporary variable, allowing us to just do entry = rcu_dereference_raw(*sibentry); rather than doing entry = rcu_dereference_raw(parent->slots[offset]); with the re-computed offset. So I think I'll commit this unless somebody screams. Linus Acked-by: Matthew Wilcox lib/radix-tree.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/radix-tree.c b/lib/radix-tree.c index 1b7bf7314141..91f0727e3cad 100644 --- a/lib/radix-tree.c +++ b/lib/radix-tree.c @@ -105,10 +105,10 @@ static unsigned int radix_tree_descend(struct radix_tree_node *parent, #ifdef CONFIG_RADIX_TREE_MULTIORDER if (radix_tree_is_internal_node(entry)) { - unsigned long siboff = get_slot_offset(parent, entry); - if (siboff < RADIX_TREE_MAP_SIZE) { - offset = siboff; - entry = rcu_dereference_raw(parent->slots[offset]); + if (is_sibling_entry(parent, entry)) { + void **sibentry = (void **) entry_to_node(entry); + offset = get_slot_offset(parent, sibentry); + entry = rcu_dereference_raw(*sibentry); } } #endif