Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] linux/bitmap.h: (buildbot-only) check if we have any
 compile-time zero-size bitmaps
To:     Yury Norov <ynorov@caviumnetworks.com>
Cc:     linux-kernel@vger.kernel.org,
        Matthew Wilcox <mawilcox@microsoft.com>,
        akpm@linux-foundation.org,
        Andy Shevchenko <andy.shevchenko@gmail.com>
References: <20180815085539.27485-1-linux@rasmusvillemoes.dk>
 <20180816232125.GA6119@yury-thinkpad>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <a9227924-1311-3171-2f50-c5a617677558@rasmusvillemoes.dk>
Date:   Fri, 17 Aug 2018 09:45:56 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180816232125.GA6119@yury-thinkpad>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 2018-08-17 01:21, Yury Norov wrote:
> Hi Rasmus,
> 
> On Wed, Aug 15, 2018 at 10:55:39AM +0200, Rasmus Villemoes wrote:
>> Most of the inline bitmap functions are buggy if passed a compile-time
>> constant nbits==0.
> 
> I think it's bad wording. Functions are OK. The problem is in users.

Why shouldn't we handle a zero-size bitmap gracefully? In any case, if
you believe nbits==0 is an implicit violation of the API, all the more
reason to detect if we have such users.

> Also, run-time nbits==0 is buggy as well.

No, not according to my reading of lib/bitmap.c. Can you point out any
function in there that doesn't behave correctly when passed nbits==0?

 On the other hand, we have a rule
> in include/linux/bitmap.h:
>   * Note that nbits should be always a compile time evaluable constant.
>   * Otherwise many inlines will generate horrible code.
> So runtime-defined nbits is bad thing by itself.

Yeah, that ship has sailed long ago. We have lots of users where for
whatever reason nbits is not constant and/or is bigger than
BITS_PER_LONG, and a function call isn't really that horrible. Probably
that piece of doc should be removed or relaxed. I have a small series
that removes another piece of wrong doc, I'll tweak this as well.

One thing that might be worth looking into is to see if we have some
places where nbits is not compile-time const, but is compile-time known
to be in 1 <= nbits <= BITS_PER_LONG, and use the inline branches in
those cases as well. Matthew already did something like that with
2c6deb01525, where we use the inline case when nbits are known to be a
multiple of 8.

>>
>> Not-really-signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>
>> +int const_zero_size_bitmaps_are_buggy(void);
> 
> It introduces undefined symbol and uses it in pretty unusual way. I think
> it worth to add comment about it.

So I thought the subject and the not-really-signed-off made it clear
enough: I don't mean for this patch to get upstream, it was only for the
buildbot to chew on and send me a build error report if we did indeed
have any such users.

The real fix(es) I'm planning to send is in
https://github.com/Villemoes/linux/tree/4.18_bitmap-zero-fix , but I'm
still waiting for the buildbot to send a report for the fake patch.

>>  #define small_const_nbits(nbits) \
>> -       (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG)
>> +       (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG && \
>> +        ((nbits) > 0 || const_zero_size_bitmaps_are_buggy()))
> 
> Some functions in bitmap API has signed type for nbits. (This is wrong
> by itself. I think I'll write patch for it.) 

I thought I fixed that years ago, but looking again, it seems
2fbad29917c98 failed to update bitmap_shift_right properly. There may be
other extern functions with signed nbits, but I think bitmap_shift_right
is the only inline that passes a signed nbits to small_const_nbits().
I'll include a fix in the above series.

So in fact you check here
> that nbits is not negative as well. Would it be better name like
> const_nonpositive_size_bitmaps_are_buggy?

The name of the undefined function is irrelevant per the above.

Rasmus