From patchwork Sun Mar 18 14:23:27 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lukas Wunner X-Patchwork-Id: 10291185 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 E4D7A60386 for ; Sun, 18 Mar 2018 14:23:43 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id D0DE826E51 for ; Sun, 18 Mar 2018 14:23:43 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C498A2880C; Sun, 18 Mar 2018 14:23:43 +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=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.wl.linuxfoundation.org (Postfix) with SMTP id 42AB626E51 for ; Sun, 18 Mar 2018 14:23:41 +0000 (UTC) Received: (qmail 30442 invoked by uid 550); 18 Mar 2018 14:23:39 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Delivered-To: mailing list kernel-hardening@lists.openwall.com Received: (qmail 30422 invoked from network); 18 Mar 2018 14:23:39 -0000 Date: Sun, 18 Mar 2018 15:23:27 +0100 From: Lukas Wunner To: Rasmus Villemoes Cc: Laura Abbott , Linus Walleij , Kees Cook , linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-hardening@lists.openwall.com, Mathias Duckeck , Nandor Han , Semi Malinen , Patrice Chotard Subject: Re: [PATCH 1/4] gpio: Remove VLA from gpiolib Message-ID: <20180318142327.GA23761@wunner.de> References: <20180310001021.6437-1-labbott@redhat.com> <20180310001021.6437-2-labbott@redhat.com> <20180317082509.GA2579@wunner.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20180317082509.GA2579@wunner.de> User-Agent: Mutt/1.5.23 (2014-03-12) X-Virus-Scanned: ClamAV using ClamSMTP On Sat, Mar 17, 2018 at 09:25:09AM +0100, Lukas Wunner wrote: > On Mon, Mar 12, 2018 at 04:00:36PM +0100, Rasmus Villemoes wrote: > > On 2018-03-10 01:10, Laura Abbott wrote: > > > @@ -2887,14 +2909,30 @@ void gpiod_set_array_value_complex(bool raw, bool can_sleep, > > > > > > while (i < array_size) { > > > struct gpio_chip *chip = desc_array[i]->gdev->chip; > > > - unsigned long mask[BITS_TO_LONGS(chip->ngpio)]; > > > - unsigned long bits[BITS_TO_LONGS(chip->ngpio)]; > > > + unsigned long *mask; > > > + unsigned long *bits; > > > int count = 0; > > > > > > + mask = kmalloc_array(BITS_TO_LONGS(chip->ngpio), > > > + sizeof(*mask), > > > + can_sleep ? GFP_KERNEL : GFP_ATOMIC); > > > + > > > + if (!mask) > > > + return; > > > + > > > + bits = kmalloc_array(BITS_TO_LONGS(chip->ngpio), > > > + sizeof(*bits), > > > + can_sleep ? GFP_KERNEL : GFP_ATOMIC); > > > + > > > + if (!bits) { > > > + kfree(mask); > > > + return; > > > + } > > > + > > > if (!can_sleep) > > > WARN_ON(chip->can_sleep); > > > > > > - memset(mask, 0, sizeof(mask)); > > > + memset(mask, 0, sizeof(*mask)); > > > > Other random thoughts: maybe two allocations for each loop iteration is > > a bit much. Maybe do a first pass over the array and collect the maximal > > chip->ngpio, do the memory allocation and freeing outside the loop (then > > you'd of course need to preserve the memset() with appropriate length > > computed). And maybe even just do one allocation, making bits point at > > the second half. > > I think those are great ideas because the function is kind of a hotpath > and usage of VLAs was motivated by the desire to make it fast. > > I'd go one step further and store the maximum ngpio of all registered > chips in a global variable (and update it in gpiochip_add_data_with_key()), > then allocate 2 * max_ngpio once before entering the loop (as you've > suggested). That would avoid the first pass to determine the maximum > chip->ngpio. In most systems max_ngpio will be < 64, so one or two > unsigned longs depending on the arch's bitness. Actually, scratch that. If ngpio is usually smallish, we can just allocate reasonably sized space for mask and bits on the stack, and fall back to the kcalloc slowpath only if chip->ngpio exceeds that limit. Basically the below (likewise compile-tested only), this is on top of Laura's patch, could be squashed together. Let me know what you think, thanks. -- >8 -- Subject: [PATCH] gpio: Add fastpath to gpiod_get/set_array_value_complex() Signed-off-by: Lukas Wunner --- drivers/gpio/gpiolib.c | 76 ++++++++++++++++++++++++-------------------------- 1 file changed, 37 insertions(+), 39 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 429bc251392b..ffc67b0b866c 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2432,6 +2432,8 @@ static int gpio_chip_get_multiple(struct gpio_chip *chip, return -EIO; } +#define FASTPATH_NGPIO 256 + int gpiod_get_array_value_complex(bool raw, bool can_sleep, unsigned int array_size, struct gpio_desc **desc_array, @@ -2441,27 +2443,24 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, while (i < array_size) { struct gpio_chip *chip = desc_array[i]->gdev->chip; - unsigned long *mask; - unsigned long *bits; + unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)]; + unsigned long *slowpath = NULL, *mask, *bits; int first, j, ret; - mask = kcalloc(BITS_TO_LONGS(chip->ngpio), - sizeof(*mask), - can_sleep ? GFP_KERNEL : GFP_ATOMIC); - - if (!mask) - return -ENOMEM; - - bits = kcalloc(BITS_TO_LONGS(chip->ngpio), - sizeof(*bits), - can_sleep ? GFP_KERNEL : GFP_ATOMIC); - - if (!bits) { - kfree(mask); - return -ENOMEM; + if (likely(chip->ngpio <= FASTPATH_NGPIO)) { + memset(fastpath, 0, sizeof(fastpath)); + mask = fastpath; + bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO); + } else { + slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio), + sizeof(*slowpath), + can_sleep ? GFP_KERNEL : GFP_ATOMIC); + if (!slowpath) + return -ENOMEM; + mask = slowpath; + bits = slowpath + BITS_TO_LONGS(chip->ngpio); } - if (!can_sleep) WARN_ON(chip->can_sleep); @@ -2478,8 +2477,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, ret = gpio_chip_get_multiple(chip, mask, bits); if (ret) { - kfree(bits); - kfree(mask); + if (slowpath) + kfree(slowpath); return ret; } @@ -2493,8 +2492,9 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep, value_array[j] = value; trace_gpio_value(desc_to_gpio(desc), 1, value); } - kfree(bits); - kfree(mask); + + if (slowpath) + kfree(slowpath); } return 0; } @@ -2699,24 +2699,22 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, while (i < array_size) { struct gpio_chip *chip = desc_array[i]->gdev->chip; - unsigned long *mask; - unsigned long *bits; + unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)]; + unsigned long *slowpath = NULL, *mask, *bits; int count = 0; - mask = kcalloc(BITS_TO_LONGS(chip->ngpio), - sizeof(*mask), - can_sleep ? GFP_KERNEL : GFP_ATOMIC); - - if (!mask) - return -ENOMEM; - - bits = kcalloc(BITS_TO_LONGS(chip->ngpio), - sizeof(*bits), - can_sleep ? GFP_KERNEL : GFP_ATOMIC); - - if (!bits) { - kfree(mask); - return -ENOMEM; + if (likely(chip->ngpio <= FASTPATH_NGPIO)) { + memset(fastpath, 0, sizeof(fastpath)); + mask = fastpath; + bits = fastpath + BITS_TO_LONGS(FASTPATH_NGPIO); + } else { + slowpath = kcalloc(2 * BITS_TO_LONGS(chip->ngpio), + sizeof(*slowpath), + can_sleep ? GFP_KERNEL : GFP_ATOMIC); + if (!slowpath) + return -ENOMEM; + mask = slowpath; + bits = slowpath + BITS_TO_LONGS(chip->ngpio); } if (!can_sleep) @@ -2753,8 +2751,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep, if (count != 0) gpio_chip_set_multiple(chip, mask, bits); - kfree(mask); - kfree(bits); + if (slowpath) + kfree(slowpath); } return 0; }