Message-ID: <878u7okzfe.fsf@rasmusvillemoes.dk>
Date:	Wed, 30 Sep 2015 11:51:33 +0200
From:	Rasmus Villemoes <linux@...musvillemoes.dk>
To:	Mark Brown <broonie@...nel.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] regmap: debugfs: improve regmap_reg_ranges_read_file()

On Tue, Sep 29 2015, Mark Brown <broonie@...nel.org> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists