[Qemu-devel] [PATCH v3 0/2] bitops patches

[Qemu-devel] [PATCH v3 0/2] bitops patches

[Blue Swirl]

Improve bit operations.

v2->v3
1/2: drop also now useless casts.
2/2: unify to 'unsigned long' which is used by kernel.
Drop bool patch (3).

v1->v2
Split from single patch.

Blue Swirl (2):
  bitops: drop volatile qualifier
  bitops: fix types

 bitops.h |   52 +++++++++++++++++++++++++++-------------------------
 1 files changed, 27 insertions(+), 25 deletions(-)

-- 
1.7.2.5

[Qemu-devel] [PATCH v3 1/2] bitops: drop volatile qualifier

[Blue Swirl]

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 and now useless casts.

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

diff --git a/bitops.h b/bitops.h
index c456232..74e14e5 100644
--- a/bitops.h
+++ b/bitops.h
@@ -114,10 +114,10 @@ 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(int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+        unsigned long *p = addr + BIT_WORD(nr);
 
 	*p  |= mask;
 }
@@ -127,10 +127,10 @@ 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(int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+        unsigned long *p = addr + BIT_WORD(nr);
 
 	*p &= ~mask;
 }
@@ -140,10 +140,10 @@ 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(int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+        unsigned long *p = addr + BIT_WORD(nr);
 
 	*p ^= mask;
 }
@@ -153,10 +153,10 @@ 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(int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+        unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old = *p;
 
 	*p = old | mask;
@@ -168,10 +168,10 @@ 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(int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+        unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old = *p;
 
 	*p = old & ~mask;
@@ -183,10 +183,10 @@ 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(int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
-	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
+        unsigned long *p = addr + BIT_WORD(nr);
 	unsigned long old = *p;
 
 	*p = old ^ mask;
@@ -198,7 +198,7 @@ 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(int nr, const unsigned long *addr)
 {
 	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }
-- 
1.7.2.5

[Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Blue Swirl]

bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.

Unify to 'unsigned long' because it generates better code on x86_64.
Adjust asserts accordingly.

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

diff --git a/bitops.h b/bitops.h
index 74e14e5..4ad0219 100644
--- a/bitops.h
+++ b/bitops.h
@@ -30,7 +30,7 @@
  */
 static unsigned long bitops_ffsl(unsigned long word)
 {
-	int num = 0;
+        unsigned long num = 0;
 
 #if LONG_MAX > 0x7FFFFFFF
 	if ((word & 0xffffffff) == 0) {
@@ -68,7 +68,7 @@ static unsigned long bitops_ffsl(unsigned long word)
  */
 static inline unsigned long bitops_flsl(unsigned long word)
 {
-	int num = BITS_PER_LONG - 1;
+        unsigned long num = BITS_PER_LONG - 1;
 
 #if LONG_MAX > 0x7FFFFFFF
 	if (!(word & (~0ul << 32))) {
@@ -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, unsigned long *addr)
+static inline void set_bit(unsigned long nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
         unsigned long *p = addr + BIT_WORD(nr);
@@ -127,7 +127,7 @@ static inline void set_bit(int nr, unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to start counting from
  */
-static inline void clear_bit(int nr, unsigned long *addr)
+static inline void clear_bit(unsigned long nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
         unsigned long *p = addr + BIT_WORD(nr);
@@ -140,7 +140,7 @@ static inline void clear_bit(int nr, unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to start counting from
  */
-static inline void change_bit(int nr, unsigned long *addr)
+static inline void change_bit(unsigned long nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
         unsigned long *p = addr + BIT_WORD(nr);
@@ -153,7 +153,7 @@ static inline void change_bit(int nr, unsigned long *addr)
  * @nr: Bit to set
  * @addr: Address to count from
  */
-static inline int test_and_set_bit(int nr, unsigned long *addr)
+static inline int test_and_set_bit(unsigned long nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
         unsigned long *p = addr + BIT_WORD(nr);
@@ -168,7 +168,7 @@ static inline int test_and_set_bit(int nr, unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to count from
  */
-static inline int test_and_clear_bit(int nr, unsigned long *addr)
+static inline int test_and_clear_bit(unsigned long nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
         unsigned long *p = addr + BIT_WORD(nr);
@@ -183,7 +183,7 @@ static inline int test_and_clear_bit(int nr, unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to count from
  */
-static inline int test_and_change_bit(int nr, unsigned long *addr)
+static inline int test_and_change_bit(unsigned long nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
         unsigned long *p = addr + BIT_WORD(nr);
@@ -198,7 +198,7 @@ static inline int test_and_change_bit(int nr, unsigned long *addr)
  * @nr: bit number to test
  * @addr: Address to start counting from
  */
-static inline int test_bit(int nr, const unsigned long *addr)
+static inline int test_bit(unsigned long nr, const unsigned long *addr)
 {
 	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }
@@ -282,9 +282,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 long start,
+                                 unsigned long 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 +302,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 long start,
+                                 unsigned long 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 +326,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 long start,
+                                 unsigned long 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 +352,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 long start,
+                                 unsigned long 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

Re: [Qemu-devel] [PATCH v3 1/2] bitops: drop volatile qualifier

[Peter Maydell]

On 14 July 2012 13:34, Blue Swirl <blauwirbel@gmail.com> wrote:
> 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 and now useless casts.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH v3 1/2] bitops: drop volatile qualifier

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On 07/14/2012 06:34 AM, Blue Swirl wrote:
> 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 and now useless casts.
> 

> -static inline void set_bit(int nr, volatile unsigned long *addr)
> +static inline void set_bit(int nr, unsigned long *addr)
>  {
>  	unsigned long mask = BIT_MASK(nr);
> -	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
> +        unsigned long *p = addr + BIT_WORD(nr);

The diff looks weird, because you are converting TAB to space on your
affected lines but not cleaning up the neighboring lines.  Is it worth a
preliminary patch to whitespace-sanitize things?

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Avi Kivity]

On 07/14/2012 03:34 PM, Blue Swirl wrote:
> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
> 
> Unify to 'unsigned long' because it generates better code on x86_64.
> Adjust asserts accordingly.
> 


Actually, plain unsigned generates the best code.  unsigned longs
require an extra byte for many instructions.  It also requires more
stack space if spilled.  Unsigned does not, but is compatible with
unsigned long in case it needs to be used in pointer arithmetic (unlike
signed int which needs a sign extension instruction).

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Peter Maydell]

On 14 July 2012 13:34, Blue Swirl <blauwirbel@gmail.com> wrote:
> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
>
> Unify to 'unsigned long' because it generates better code on x86_64.
> Adjust asserts accordingly.

Still disagree with this patch, for the record.

-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 14 July 2012 13:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
>>
>> Unify to 'unsigned long' because it generates better code on x86_64.
>> Adjust asserts accordingly.
>
> Still disagree with this patch, for the record.

So do I.

Changing tried-and-true code for some unproven performance gain is a bad
idea.

In this particular case, it additionally deviates from the code's
source.

If you feel your patch is a worthwhile improvement, please take it to
LKML, so the kernel and future borrowers of this code can profit.
Copying free code without at least trying to contribute improvements
back to the source isn't proper.

Re: [Qemu-devel] [PATCH v3 1/2] bitops: drop volatile qualifier

[Blue Swirl]

On Mon, Jul 16, 2012 at 2:40 PM, Eric Blake <eblake@redhat.com> wrote:
> On 07/14/2012 06:34 AM, Blue Swirl wrote:
>> 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 and now useless casts.
>>
>
>> -static inline void set_bit(int nr, volatile unsigned long *addr)
>> +static inline void set_bit(int nr, unsigned long *addr)
>>  {
>>       unsigned long mask = BIT_MASK(nr);
>> -     unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
>> +        unsigned long *p = addr + BIT_WORD(nr);
>
> The diff looks weird, because you are converting TAB to space on your
> affected lines but not cleaning up the neighboring lines.  Is it worth a
> preliminary patch to whitespace-sanitize things?

That is close to reformatting, this is the usual way these days.

>
> --
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Blue Swirl]

On Tue, Jul 17, 2012 at 12:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 14 July 2012 13:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
>>>
>>> Unify to 'unsigned long' because it generates better code on x86_64.
>>> Adjust asserts accordingly.
>>
>> Still disagree with this patch, for the record.
>
> So do I.
>
> Changing tried-and-true code for some unproven performance gain is a bad
> idea.

No it isn't and that is not the case.

>
> In this particular case, it additionally deviates from the code's
> source.

I'm getting a strong feeling that it's a bad idea to reuse any Linux
kernel sources since they are seen as divine and untouchable, unlike
for example BSD queue macros. Perhaps BSD have also bit field ops
which we could use instead? There's also the licensing problem with
GPLv2only in some cases.

>
> If you feel your patch is a worthwhile improvement, please take it to
> LKML, so the kernel and future borrowers of this code can profit.
> Copying free code without at least trying to contribute improvements
> back to the source isn't proper.

The original kernel functions use 'unsigned long'. The bit field
functions introduced by Peter use 'int' but those are not in the
kernel tree, so there's nothing to fix (except the two first hunks)
from the kernel point of view. Also 'volatile' makes sense when kernel
is banging HW registers.

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Peter Maydell]

On 23 July 2012 18:33, Blue Swirl <blauwirbel@gmail.com> wrote:
> I'm getting a strong feeling that it's a bad idea to reuse any Linux
> kernel sources since they are seen as divine and untouchable, unlike
> for example BSD queue macros.

We should also try to avoid deviations in our queue macros,
and I think we do (eg commit 6095aa8 added functionality by
moving us closer into sync with the BSD macros rather than
by reinventing the wheel which was IIRC what the initial pre-code-review
patch did).

-- PMM

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 23 July 2012 18:33, Blue Swirl <blauwirbel@gmail.com> wrote:
>> I'm getting a strong feeling that it's a bad idea to reuse any Linux
>> kernel sources since they are seen as divine and untouchable, unlike
>> for example BSD queue macros.

Reusing good code that solves the problem at hand can be a bad idea if
you can't resist the temptation to tinker with it, yet can't be bothered
to upstream your improvements.  Then you might as well build your own
bikeshed from scratch :)

> We should also try to avoid deviations in our queue macros,

Agree.

> and I think we do (eg commit 6095aa8 added functionality by
> moving us closer into sync with the BSD macros rather than
> by reinventing the wheel which was IIRC what the initial pre-code-review
> patch did).

Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types

[Blue Swirl]

On Tue, Jul 24, 2012 at 9:26 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 23 July 2012 18:33, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> I'm getting a strong feeling that it's a bad idea to reuse any Linux
>>> kernel sources since they are seen as divine and untouchable, unlike
>>> for example BSD queue macros.
>
> Reusing good code that solves the problem at hand can be a bad idea if
> you can't resist the temptation to tinker with it, yet can't be bothered
> to upstream your improvements.  Then you might as well build your own
> bikeshed from scratch :)

There's nothing wrong in tinkering with reused good code. As I
explained, there's little point to upstream these changes, so 'not
bothering' is false accusation.

>
>> We should also try to avoid deviations in our queue macros,
>
> Agree.

Avoiding "deviations" can be secondary to many other needs.

>
>> and I think we do (eg commit 6095aa8 added functionality by
>> moving us closer into sync with the BSD macros rather than
>> by reinventing the wheel which was IIRC what the initial pre-code-review
>> patch did).