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

[Blue Swirl]

On Tue, Jul 17, 2012 at 12:36 PM, Markus Armbruster <address@hidden> wrote:
> Peter Maydell <address@hidden> writes:
>
>> On 14 July 2012 13:34, Blue Swirl <address@hidden> 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.