From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755402AbbI3Jvn (ORCPT ); Wed, 30 Sep 2015 05:51:43 -0400 Received: from mail-la0-f48.google.com ([209.85.215.48]:33706 "EHLO mail-la0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755360AbbI3Jvg (ORCPT ); Wed, 30 Sep 2015 05:51:36 -0400 From: Rasmus Villemoes To: Mark Brown Cc: Greg Kroah-Hartman , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] regmap: debugfs: improve regmap_reg_ranges_read_file() Organization: D03 References: <1443479342-31621-1-git-send-email-linux@rasmusvillemoes.dk> <1443479342-31621-2-git-send-email-linux@rasmusvillemoes.dk> <20150929183024.GC30445@sirena.org.uk> X-Hashcash: 1:20:150930:broonie@kernel.org::x2bXuMK86qazsi5e:00000000000000000000000000000000000000000001IcO X-Hashcash: 1:20:150930:gregkh@linuxfoundation.org::AUUoIo0xZJMmH0PX:000000000000000000000000000000000006NBw X-Hashcash: 1:20:150930:linux-kernel@vger.kernel.org::9ki2b1zAkSeC1Fdg:00000000000000000000000000000000095z0 Date: Wed, 30 Sep 2015 11:51:33 +0200 In-Reply-To: <20150929183024.GC30445@sirena.org.uk> (Mark Brown's message of "Tue, 29 Sep 2015 19:30:24 +0100") Message-ID: <878u7okzfe.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 29 2015, Mark Brown wrote: > On Tue, Sep 29, 2015 at 12:29:02AM +0200, Rasmus Villemoes wrote: > > This patch is an example of why SubmittingPatches recommends splitting > things up into one change per patch, it would be much easier to read and > review as a series (especially given that there's very few collisions). It's a balance. Splitting into too small pieces and one gets accused of trying to inflate one's commit stats. But I actually agree with you (and SubmittingPatches), and it would be up to the maintainer to squash the patches if they're deemed too small. >> * A page is a bit much for two integers and a bit of punctuation. 64 >> bytes should suffice. > > Right, the reason PAGE_SIZE was chosen is that it's a natural unit for > the allocator and is clearly absurdly large for the data. For 64 bytes > I have to think for a moment if it's suitably large. Please leave it at > PAGE_SIZE, it's not like this hangs around for any length of time. OK, but it's not that one-sided. When I read the code, I spent time thinking if there might be any case where we'd need anywhere near that much (which also requires checking all places that entry is used in the function). Absurd choices can also be confusing. >> * Calling strlen() on entry no less than three times is silly, >> especially when snprintf() has returned that value (which was just >> thrown away). > > I think we were expecting the compiler would figure out that strlen() is > a pure function and do the right thing here (though I do see it's > missing an annotation). It does, and there's no need for an annotation - gcc knows about the properties and semantics of the standard library string functions, which is why it can optimize strlen("foo") to 3 at compile-time, !strlen(s) to "*s == '\0'", using "i < strlen(s)" in the terminating condition of a for loop doesn't always generate horrible code, etc. HOWEVER, once there are calls to external functions in-between, all bets are off. entry is a pointer to some blob of global memory, and the compiler has absolutely no way of knowing whether some callee changes that memory. So it dutifully does what the programmer told it, which is to compute the strlen() at that point in the code. (Again, even calling it _once_ is one more than necessary.) Aside: This was in fact how my attention was drawn to regmap-debugfs in the first place: I have a patch adding __attribute__((assume_aligned)) (gcc 4.9+) annotations to kmalloc and friends. This generally lead to smaller and more efficient code, but bloat-o-meter said that several functions in regmap-debugfs.c grew by several hundred bytes. This turned out to be caused by gcc deciding to open-code strlen() [using the check-four-bytes-at-a-time-for-a-0 trick] when it knew that the string is sufficiently aligned. Since these strlen calls are entirely redundant, I wanted to try to get rid of those, but then I found the stuff which was slightly more important than reducing .text and saving a few cycles. >> * Transferring entry to the output buffer using snprintf is silly, >> when we know the length. Use memcpy instead. > > Right, that's a legacy of a previous version transferring the maximum > amount of data possible (which got abandoned due to complexity). > However with the changes to use the return value of snprinf() it seems > like the best thing to do here is to go back to this and just fill up as > much of the buffer as we can by using snprintf() to do the transfer. > > That said I think memcpy() is going to be the best way of getting to > that since one of the issues there (which currently doesn't work) is > slicing things off the front and memcpy() handles that nicely. I'm not sure I understand what you want to do. Should I resend as three separate patches (the four bullets minus the PAGE_SIZE thing), and then we can take it from there? Rasmus