Patchwork [v3,2/2] bitops: fix types

login
register
mail settings
Submitter Blue Swirl
Date July 14, 2012, 12:34 p.m.
Message ID <0e5d35008c438b250c66b1ab4e774bd718521d8c.1342269249.git.blauwirbel@gmail.com>
Download mbox | patch
Permalink /patch/171001/
State New
Headers show

Comments

Blue Swirl - July 14, 2012, 12:34 p.m.
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(-)
Avi Kivity - July 16, 2012, 3:25 p.m.
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).
Peter Maydell - July 16, 2012, 3:34 p.m.
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
Markus Armbruster - July 17, 2012, 12:36 p.m.
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.
Blue Swirl - July 23, 2012, 5:33 p.m.
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.
Peter Maydell - July 23, 2012, 5:44 p.m.
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
Markus Armbruster - July 24, 2012, 9:26 a.m.
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).
Blue Swirl - July 24, 2012, 7:51 p.m.
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).

Patch

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);
 }