[Qemu-devel] [PATCH v2 0/3] bitops patches

[Qemu-devel] [PATCH v2 0/3] bitops patches

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

Improve bit operations.

v1->v2
Split from single patch.

Blue Swirl (3):
  bitops: fix types
  bitops: drop volatile qualifier
  bitops: use bool

 bitops.h |   42 ++++++++++++++++++++++--------------------
 1 files changed, 22 insertions(+), 20 deletions(-)

-- 
1.7.2.5

[Qemu-devel] [PATCH v2 1/3] bitops: fix types

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

Use 'unsigned int' for bit numbers instead of 'unsigned long' or
'int'. Adjust asserts.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 bitops.h |   46 ++++++++++++++++++++++++++--------------------
 1 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/bitops.h b/bitops.h
index b967ef3..f6ac721 100644
--- a/bitops.h
+++ b/bitops.h
@@ -28,7 +28,7 @@
  *
  * Undefined if no bit exists, so code should check against 0 first.
  */
-static unsigned long bitops_ffsl(unsigned long word)
+static unsigned int bitops_ffsl(unsigned long word)
 {
 	int num = 0;
 
@@ -66,7 +66,7 @@ static unsigned long bitops_ffsl(unsigned long word)
  *
  * Undefined if no set bit exists, so code should check against 0 first.
  */
-static inline unsigned long bitops_flsl(unsigned long word)
+static inline unsigned int bitops_flsl(unsigned long word)
 {
 	int num = BITS_PER_LONG - 1;
 
@@ -104,7 +104,7 @@ static inline unsigned long bitops_flsl(unsigned long word)
  *
  * Undefined if no zero exists, so code should check against ~0UL first.
  */
-static inline unsigned long ffz(unsigned long word)
+static inline unsigned int ffz(unsigned long word)
 {
     return bitops_ffsl(~word);
 }
@@ -114,7 +114,7 @@ static inline unsigned long ffz(unsigned long word)
  * @nr: the bit to set
  * @addr: the address to start counting from
  */
-static inline void set_bit(int nr, volatile unsigned long *addr)
+static inline void set_bit(unsigned int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -127,7 +127,7 @@ static inline void set_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to start counting from
  */
-static inline void clear_bit(int nr, volatile unsigned long *addr)
+static inline void clear_bit(unsigned int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -140,7 +140,7 @@ static inline void clear_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to start counting from
  */
-static inline void change_bit(int nr, volatile unsigned long *addr)
+static inline void change_bit(unsigned int nr, volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -153,7 +153,8 @@ static inline void change_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to set
  * @addr: Address to count from
  */
-static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
+static inline int test_and_set_bit(unsigned int nr,
+                                   volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -168,7 +169,8 @@ static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to count from
  */
-static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
+static inline int test_and_clear_bit(unsigned int nr,
+                                     volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -183,7 +185,8 @@ static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to count from
  */
-static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
+static inline int test_and_change_bit(unsigned int nr,
+                                      volatile unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -198,7 +201,8 @@ static inline int test_and_change_bit(int nr, volatile unsigned long *addr)
  * @nr: bit number to test
  * @addr: Address to start counting from
  */
-static inline int test_bit(int nr, const volatile unsigned long *addr)
+static inline int test_bit(unsigned int nr,
+                           const volatile unsigned long *addr)
 {
 	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }
@@ -282,9 +286,10 @@ static inline unsigned long hweight_long(unsigned long w)
  *
  * Returns: the value of the bit field extracted from the input value.
  */
-static inline uint32_t extract32(uint32_t value, int start, int length)
+static inline uint32_t extract32(uint32_t value, unsigned int start,
+                                 unsigned int length)
 {
-    assert(start >= 0 && length > 0 && length <= 32 - start);
+    assert(start < 32 && length > 0 && length <= 32 && start + length <= 32);
     return (value >> start) & (~0U >> (32 - length));
 }
 
@@ -301,9 +306,10 @@ static inline uint32_t extract32(uint32_t value, int start, int length)
  *
  * Returns: the value of the bit field extracted from the input value.
  */
-static inline uint64_t extract64(uint64_t value, int start, int length)
+static inline uint64_t extract64(uint64_t value, unsigned int start,
+                                 unsigned int length)
 {
-    assert(start >= 0 && length > 0 && length <= 64 - start);
+    assert(start < 64 && length > 0 && length <= 64 && start + length <= 64);
     return (value >> start) & (~0ULL >> (64 - length));
 }
 
@@ -324,11 +330,11 @@ static inline uint64_t extract64(uint64_t value, int start, int length)
  *
  * Returns: the modified @value.
  */
-static inline uint32_t deposit32(uint32_t value, int start, int length,
-                                 uint32_t fieldval)
+static inline uint32_t deposit32(uint32_t value, unsigned int start,
+                                 unsigned int length, uint32_t fieldval)
 {
     uint32_t mask;
-    assert(start >= 0 && length > 0 && length <= 32 - start);
+    assert(start < 32 && length > 0 && length <= 32 && start + length <= 32);
     mask = (~0U >> (32 - length)) << start;
     return (value & ~mask) | ((fieldval << start) & mask);
 }
@@ -350,11 +356,11 @@ static inline uint32_t deposit32(uint32_t value, int start, int length,
  *
  * Returns: the modified @value.
  */
-static inline uint64_t deposit64(uint64_t value, int start, int length,
-                                 uint64_t fieldval)
+static inline uint64_t deposit64(uint64_t value, unsigned int start,
+                                 unsigned int length, uint64_t fieldval)
 {
     uint64_t mask;
-    assert(start >= 0 && length > 0 && length <= 64 - start);
+    assert(start < 64 && length > 0 && length <= 64 && start + length <= 64);
     mask = (~0ULL >> (64 - length)) << start;
     return (value & ~mask) | ((fieldval << start) & mask);
 }
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 2/3] bitops: drop volatile qualifier

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

Qualifier 'volatile' is not useful for applications, it's too strict
for single threaded code but does not give the real atomicity guarantees
needed for multithreaded code.

Drop them.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 bitops.h |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/bitops.h b/bitops.h
index f6ac721..bc99727 100644
--- a/bitops.h
+++ b/bitops.h
@@ -114,7 +114,7 @@ static inline unsigned int ffz(unsigned long word)
  * @nr: the bit to set
  * @addr: the address to start counting from
  */
-static inline void set_bit(unsigned int nr, volatile unsigned long *addr)
+static inline void set_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -127,7 +127,7 @@ static inline void set_bit(unsigned int nr, volatile unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to start counting from
  */
-static inline void clear_bit(unsigned int nr, volatile unsigned long *addr)
+static inline void clear_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -140,7 +140,7 @@ static inline void clear_bit(unsigned int nr, volatile unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to start counting from
  */
-static inline void change_bit(unsigned int nr, volatile unsigned long *addr)
+static inline void change_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -153,8 +153,7 @@ static inline void change_bit(unsigned int nr, volatile unsigned long *addr)
  * @nr: Bit to set
  * @addr: Address to count from
  */
-static inline int test_and_set_bit(unsigned int nr,
-                                   volatile unsigned long *addr)
+static inline int test_and_set_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -169,8 +168,7 @@ static inline int test_and_set_bit(unsigned int nr,
  * @nr: Bit to clear
  * @addr: Address to count from
  */
-static inline int test_and_clear_bit(unsigned int nr,
-                                     volatile unsigned long *addr)
+static inline int test_and_clear_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -185,8 +183,7 @@ static inline int test_and_clear_bit(unsigned int nr,
  * @nr: Bit to change
  * @addr: Address to count from
  */
-static inline int test_and_change_bit(unsigned int nr,
-                                      volatile unsigned long *addr)
+static inline int test_and_change_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -201,8 +198,7 @@ static inline int test_and_change_bit(unsigned int nr,
  * @nr: bit number to test
  * @addr: Address to start counting from
  */
-static inline int test_bit(unsigned int nr,
-                           const volatile unsigned long *addr)
+static inline int test_bit(unsigned int nr, const unsigned long *addr)
 {
 	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }
-- 
1.7.2.5

[Qemu-devel] [PATCH v2 3/3] bitops: use bool

[blauwirbel]

From: Blue Swirl <blauwirbel@gmail.com>

Use 'bool' type for return value of bit test functions.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 bitops.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/bitops.h b/bitops.h
index bc99727..1cecf00 100644
--- a/bitops.h
+++ b/bitops.h
@@ -153,7 +153,7 @@ static inline void change_bit(unsigned int nr, unsigned long *addr)
  * @nr: Bit to set
  * @addr: Address to count from
  */
-static inline int test_and_set_bit(unsigned int nr, unsigned long *addr)
+static inline bool test_and_set_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -168,7 +168,7 @@ static inline int test_and_set_bit(unsigned int nr, unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to count from
  */
-static inline int test_and_clear_bit(unsigned int nr, unsigned long *addr)
+static inline bool test_and_clear_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -183,7 +183,7 @@ static inline int test_and_clear_bit(unsigned int nr, unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to count from
  */
-static inline int test_and_change_bit(unsigned int nr, unsigned long *addr)
+static inline bool test_and_change_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -198,7 +198,7 @@ static inline int test_and_change_bit(unsigned int nr, unsigned long *addr)
  * @nr: bit number to test
  * @addr: Address to start counting from
  */
-static inline int test_bit(unsigned int nr, const unsigned long *addr)
+static inline bool test_bit(unsigned int nr, const unsigned long *addr)
 {
 	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH v2 3/3] bitops: use bool

[Markus Armbruster]

blauwirbel@gmail.com writes:

> From: Blue Swirl <blauwirbel@gmail.com>
>
> Use 'bool' type for return value of bit test functions.

Matter of taste.  'bool' makes sense if you think of these functions as
predicates (ugly ones, with side effects).  'int' makes sense if you
think of them as returning the bit value (a bit is not a truth value in
my book).

In my opinion, this is just unnecessary churn, and unnecessary deviation
from established kernel practice.

Re: [Qemu-devel] [PATCH v2 2/3] bitops: drop volatile qualifier

[Peter Maydell]

On 8 July 2012 20:22,  <blauwirbel@gmail.com> wrote:
> From: Blue Swirl <blauwirbel@gmail.com>
>
> Qualifier 'volatile' is not useful for applications, it's too strict
> for single threaded code but does not give the real atomicity guarantees
> needed for multithreaded code.
>
> Drop them.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
(though of course it won't apply without patch 1 and I disagree with
patch 1, so you'll need to fix up this patch if you want to apply
it on its own.)

-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Markus Armbruster]

blauwirbel@gmail.com writes:

> From: Blue Swirl <blauwirbel@gmail.com>
>
> Use 'unsigned int' for bit numbers instead of 'unsigned long' or
> 'int'. Adjust asserts.

I'd like to lodge a formal objection to this part.

There is no consensus.  I recognize the power of maintainers to force a
change even without consensus.  Use it wisely.

Re: [Qemu-devel] [PATCH v2 3/3] bitops: use bool

[Peter Maydell]

On 8 July 2012 20:22,  <blauwirbel@gmail.com> wrote:
> From: Blue Swirl <blauwirbel@gmail.com>
>
> Use 'bool' type for return value of bit test functions.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

I'm 'meh' about the necessity for this change, but it looks safe
and functionally correct to me -- all these functions are either
already returning the result of a comparison, or (in the test_bit
case) a result that's only 0 or 1, so changing the result type
to bool can't break any callers.

-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Blue Swirl]

On Mon, Jul 9, 2012 at 7:49 AM, Markus Armbruster <armbru@redhat.com> wrote:
> blauwirbel@gmail.com writes:
>
>> From: Blue Swirl <blauwirbel@gmail.com>
>>
>> Use 'unsigned int' for bit numbers instead of 'unsigned long' or
>> 'int'. Adjust asserts.
>
> I'd like to lodge a formal objection to this part.
>
> There is no consensus.  I recognize the power of maintainers to force a
> change even without consensus.  Use it wisely.

I thought I refuted all concrete arguments except performance.
However, Avi kindly provided this part.

Re: [Qemu-devel] [PATCH v2 3/3] bitops: use bool

[Blue Swirl]

On Mon, Jul 9, 2012 at 7:43 AM, Markus Armbruster <armbru@redhat.com> wrote:
> blauwirbel@gmail.com writes:
>
>> From: Blue Swirl <blauwirbel@gmail.com>
>>
>> Use 'bool' type for return value of bit test functions.
>
> Matter of taste.  'bool' makes sense if you think of these functions as
> predicates (ugly ones, with side effects).  'int' makes sense if you
> think of them as returning the bit value (a bit is not a truth value in
> my book).

The documentation actually talks about returning the old value.

But I think the kernel version (with the same documentation) however
returns a truth value:
static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
{
	int oldbit;

	asm volatile(LOCK_PREFIX "btr %2,%1\n\t"
		     "sbb %0,%0"
		     : "=r" (oldbit), ADDR : "Ir" (nr) : "memory");

	return oldbit;
}

SBB oldbit, oldbit with oldbit = 0 initially and carry set will produce -1, no?

>
> In my opinion, this is just unnecessary churn, and unnecessary deviation
> from established kernel practice.

We use for example NetBSD queue macros with heavy modification. If we
have a need to modify code derived from Linux kernel, we should do it
as well. In this case the kernel practice seems to be inconsistent and
worth fixing.

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Peter Maydell]

On 10 July 2012 20:18, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Jul 9, 2012 at 7:49 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> There is no consensus.  I recognize the power of maintainers to force a
>> change even without consensus.  Use it wisely.
>
> I thought I refuted all concrete arguments except performance.

No, you made various claims that Markus and I at least
disagreed with. (Conversely, we have made various claims
that you disagree with -- this is what "no consensus" means...)

> However, Avi kindly provided this part.

One extra instruction, which isn't a load/store or branch? This is the
worst kind of premature microoptimisation. If we're changing this
it should be rooted in arguments about maintainability or similar,
or alternatively if it's really a performance improvement I'm
sure you can produce the benchmarks that demonstrate that :-)

[Plus the extract/deposit ops aren't doing array accesses!]

-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Blue Swirl]

On Tue, Jul 10, 2012 at 7:37 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 July 2012 20:18, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Mon, Jul 9, 2012 at 7:49 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> There is no consensus.  I recognize the power of maintainers to force a
>>> change even without consensus.  Use it wisely.
>>
>> I thought I refuted all concrete arguments except performance.
>
> No, you made various claims that Markus and I at least
> disagreed with. (Conversely, we have made various claims
> that you disagree with -- this is what "no consensus" means...)

You did not present any concrete arguments. In this review you have
pointed at bugs in the assert() expression, thanks.

>
>> However, Avi kindly provided this part.
>
> One extra instruction, which isn't a load/store or branch? This is the
> worst kind of premature microoptimisation. If we're changing this
> it should be rooted in arguments about maintainability or similar,

I think signed and unsigned are pretty much equal in this respect.
Both signed and unsigned have corner cases. Unsigned has the advantage
of being more natural range for values which can't be negative by
their nature, like bit numbers. There's also the security argument
like Avi mentioned.

> or alternatively if it's really a performance improvement I'm
> sure you can produce the benchmarks that demonstrate that :-)
>
> [Plus the extract/deposit ops aren't doing array accesses!]

It's a clear benefit which signed integers can't provide, especially
on x86_64 which is an important platform. For me this clearly tips the
balance.

>
> -- PMM

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Peter Maydell]

On 10 July 2012 21:01, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Jul 10, 2012 at 7:37 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 10 July 2012 20:18, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Mon, Jul 9, 2012 at 7:49 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> There is no consensus.  I recognize the power of maintainers to force a
>>>> change even without consensus.  Use it wisely.
>>>
>>> I thought I refuted all concrete arguments except performance.
>>
>> No, you made various claims that Markus and I at least
>> disagreed with. (Conversely, we have made various claims
>> that you disagree with -- this is what "no consensus" means...)
>
> You did not present any concrete arguments. In this review you have
> pointed at bugs in the assert() expression, thanks.

No, a bug in your assert is an indication of why there are
downsides to making random changes, not a concrete argument
against making the changes. The major problem here
is (a) there is no really good reason to make this change
(b) it's moving away from the existing tested code we have
(and that the kernel has). Basically 'int' has more natural
behaviour for reasoning about than 'unsigned' in ranges
where it's usually used (ie small ones).

-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Kevin Wolf]

Am 08.07.2012 21:22, schrieb blauwirbel@gmail.com:
> From: Blue Swirl <blauwirbel@gmail.com>
> 
> Use 'unsigned int' for bit numbers instead of 'unsigned long' or
> 'int'. Adjust asserts.
> 
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

I haven't followed the original discussion and therefore don't know what
the controversy is about (nor do I feel like reading it up), but if
there is no consensus, I would expect even more than already for normal
patches that the commit message doesn't only state the obvious change,
but also the reasons for the change.

This message isn't much different from the famous "i++; /* increase i by
one */" code comment.

Kevin

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Blue Swirl]

On Tue, Jul 10, 2012 at 9:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 10 July 2012 21:01, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Tue, Jul 10, 2012 at 7:37 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 10 July 2012 20:18, Blue Swirl <blauwirbel@gmail.com> wrote:
>>>> On Mon, Jul 9, 2012 at 7:49 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> There is no consensus.  I recognize the power of maintainers to force a
>>>>> change even without consensus.  Use it wisely.
>>>>
>>>> I thought I refuted all concrete arguments except performance.
>>>
>>> No, you made various claims that Markus and I at least
>>> disagreed with. (Conversely, we have made various claims
>>> that you disagree with -- this is what "no consensus" means...)
>>
>> You did not present any concrete arguments. In this review you have
>> pointed at bugs in the assert() expression, thanks.
>
> No, a bug in your assert is an indication of why there are
> downsides to making random changes, not a concrete argument
> against making the changes. The major problem here
> is (a) there is no really good reason to make this change

There are inconsistencies in the API that need fixing (unsigned long
vs. int) and a technically improved solution (unsigned int in x86_64)
exists. These are good and valid reasons.

> (b) it's moving away from the existing tested code we have
> (and that the kernel has).

Keeping the code intact should not stop progress: fix inconsistencies
and use a better solution. The distance from kernel is questionable
value BTW: have you checked how little bitops.h resembles the kernel
versions?

> Basically 'int' has more natural
> behaviour for reasoning about than 'unsigned' in ranges
> where it's usually used (ie small ones).

But 'unsigned' is much more naturally suited to values that can't be
negative. This part is bikeshedding, your point of view against mine.
Currently some functions use 'unsigned long' for bit numbers,
shouldn't they be changed to 'int' in your logic?

>
> -- PMM

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Blue Swirl]

On Wed, Jul 11, 2012 at 12:49 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 08.07.2012 21:22, schrieb blauwirbel@gmail.com:
>> From: Blue Swirl <blauwirbel@gmail.com>
>>
>> Use 'unsigned int' for bit numbers instead of 'unsigned long' or
>> 'int'. Adjust asserts.
>>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>
> I haven't followed the original discussion and therefore don't know what
> the controversy is about (nor do I feel like reading it up), but if
> there is no consensus, I would expect even more than already for normal
> patches that the commit message doesn't only state the obvious change,
> but also the reasons for the change.
>
> This message isn't much different from the famous "i++; /* increase i by
> one */" code comment.

The message could be improved by vast amounts, but in my view it is
sufficient for such a simple change.

>
> Kevin

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Peter Maydell]

On 12 July 2012 21:18, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Jul 10, 2012 at 9:36 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Basically 'int' has more natural
>> behaviour for reasoning about than 'unsigned' in ranges
>> where it's usually used (ie small ones).
>
> But 'unsigned' is much more naturally suited to values that can't be
> negative. This part is bikeshedding, your point of view against mine.

I agree that this is all veering quite close to bikeshedding, so
this email is my last on the subject.

So, both 'unsigned' and 'int' have two discontinuities, at the extreme
ends of their ranges. For 'unsigned' these are at 0 and UINT_MAX;
for 'int' they're at INT_MIN and INT_MAX. Discontinuities are bad
because they break our intuitive reasoning about the behaviour of
integers and result in bugs which can easily slip through code review.
Any time you might be near a discontinuity you have to think carefully
about what is actually going to happen. Sometimes that's unavoidable
whatever type you pick (eg in situations where values are coming from
an untrusted source which might be trying to find a security hole).
But often values are from another trusted area of the code (or even
obviously small by inspection of the immediate area). In this case
'int' is good because it keeps the discontinuities well away from the
range you'll be working in. Because 'unsigned' has a discontinuity
near 0 it's much easier to bump into it even for numbers in the
expected use range. So when you're really just dealing with "small
numbers that happen to be positive" and don't need the unsigned
arithmetic semantics, 'int' is the better choice.
I think the maintainability/reduced-bugginess argument hugely
outweighs minor details of one insn more or less in some operations.

(If we care about that, we should be much more in favour of using 'int'
returns rather than 'bool', since the bool return forces the compiler
to insert code which normalises a zero-or-nonzero value to zero-or-one.
But there too the readability and maintainability benefits are worth
taking the unmeasurably tiny extra execution time.)

> Currently some functions use 'unsigned long' for bit numbers,
> shouldn't they be changed to 'int' in your logic?

I think that falls into the category of things I would recommend
changing in code review, or if fixing some related kind of
overflow bug, but would not change existing working code for.

-- PMM

Re: [Qemu-devel] [PATCH v2 1/3] bitops: fix types

[Kevin Wolf]

Am 12.07.2012 22:21, schrieb Blue Swirl:
> On Wed, Jul 11, 2012 at 12:49 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 08.07.2012 21:22, schrieb blauwirbel@gmail.com:
>>> From: Blue Swirl <blauwirbel@gmail.com>
>>>
>>> Use 'unsigned int' for bit numbers instead of 'unsigned long' or
>>> 'int'. Adjust asserts.
>>>
>>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>>
>> I haven't followed the original discussion and therefore don't know what
>> the controversy is about (nor do I feel like reading it up), but if
>> there is no consensus, I would expect even more than already for normal
>> patches that the commit message doesn't only state the obvious change,
>> but also the reasons for the change.
>>
>> This message isn't much different from the famous "i++; /* increase i by
>> one */" code comment.
> 
> The message could be improved by vast amounts, but in my view it is
> sufficient for such a simple change.

No, it's not. The change is simple (so you don't necessarily need to
repeat what has changed, I see it in the diff), but the reasons aren't
obvious. So please state them even for totally simple mechanical changes.

Kevin