Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types
On Tue, Jul 24, 2012 at 9:26 AM, Markus Armbruster arm...@redhat.com wrote: Peter Maydell peter.mayd...@linaro.org writes: On 23 July 2012 18:33, Blue Swirl blauwir...@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).
Re: [Qemu-devel] [PATCH v3 2/2] bitops: fix types
Peter Maydell peter.mayd...@linaro.org writes: On 23 July 2012 18:33, Blue Swirl blauwir...@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
On Tue, Jul 17, 2012 at 12:36 PM, Markus Armbruster arm...@redhat.com wrote: Peter Maydell peter.mayd...@linaro.org writes: On 14 July 2012 13:34, Blue Swirl blauwir...@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
On 23 July 2012 18:33, Blue Swirl blauwir...@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
Peter Maydell peter.mayd...@linaro.org writes: On 14 July 2012 13:34, Blue Swirl blauwir...@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 2/2] bitops: fix types
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
On 14 July 2012 13:34, Blue Swirl blauwir...@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
[Qemu-devel] [PATCH v3 2/2] bitops: fix types
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 blauwir...@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 0x7FFF
if ((word 0x) == 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 0x7FFF
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,
+
