Re: [PATCH 5/5] xen: clean up VPF flags macros

[Juergen Gross <jgross@suse.com>]

On 09/28/2015 09:52 AM, Jan Beulich wrote:
>>>> On 28.09.15 at 09:29, <JGross@suse.com> wrote:
>> On 09/28/2015 08:22 AM, Jan Beulich wrote:
>>>>>> On 28.09.15 at 07:23, <JGross@suse.com> wrote:
>>>> On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>>>>>> On 25.09.15 at 13:54, <JGross@suse.com> wrote:
>>>>>> Per-VCPU pause flags in sched.h are defined as bit positions and as
>>>>>> values derived from the bit defines. There is only one user of a value
>>>>>> which can be easily converted to use a bit number as well.
>>>>>
>>>>> I'm not convinced:
>>>>>
>>>>>> --- a/xen/common/domctl.c
>>>>>> +++ b/xen/common/domctl.c
>>>>>> @@ -172,7 +172,7 @@ void getdomaininfo(struct domain *d, struct
>>>> xen_domctl_getdomaininfo *info)
>>>>>>             info->max_vcpu_id = v->vcpu_id;
>>>>>>             if ( !test_bit(_VPF_down, &v->pause_flags) )
>>>>>>             {
>>>>>> -            if ( !(v->pause_flags & VPF_blocked) )
>>>>>> +            if ( !test_bit(_VPF_blocked, &v->pause_flags) )
>>>>>
>>>>> test_bit() is quite a bit more complex an operation than a simple &,
>>>>> and with (on x86) even constant_test_bit() involving a cast to
>>>>> a pointer to volatile I'm afraid we can't even hope that compilers
>>>>> would produce identical code for both in cases like this one (as that
>>>>> casts limits freedom of the compiler). IOW I'd rather see other
>>>>> test_bit(_VPF_...) uses converted the inverse way (which as a nice
>>>>> but minor side effect would yield slightly smaller source code).
>>>>
>>>> What about introducing __test_bit() being a variant which can be
>>>> reordered by omitting the volatile modifier? I think this would have
>>>> the same effect.
>>>
>>> I'm not convinced it always would - the inline function is still more
>>> complex than the plain operation.
>>
>> Depends on the way it is done. What about:
>>
>> #define __test_bit(nr, addr) ({                         \
>>       if ( bitop_bad_size(addr) ) __bitop_bad_size();     \
>>       (__builtin_constant_p(nr) ?                         \
>>        !!(*(addr) & ((typeof)(*(addr))1 << (nr))) :       \
>>        __variable_test_bit((nr),(addr)));                 \
>> })
>
> But that's not correct - addr may point to wider than a single entry
> array, irrespective of whether nr is a compile time constant.
>
>> It would even be possible to drop the test for bitop_bad_size(addr) in
>> the constant case.
>
> In which case 1 << nr may reference a bit beyond the type
> of *addr.

Hmm, yes, you are right, of course.

It could be fixed, however.

The question is: does it make sense to follow this path any longer,
or would you reject it even in case of correctness? I wouldn't mind
either way, I just don't want to waste time (mine and yours).

>>>> And we could still get rid of many double definitions
>>>> of the same bit. Even if the mask definition of a bit is not error prone
>>>> by relying on the definition of the bit position, it makes it harder to
>>>> find all users of this bit.
>>>
>>> Why so? Just omit the leading underscore when grep-ing, and you'll
>>> find all instances (less preprocessor token concatenation, but that's
>>> orthogonal).
>>
>> I do use grep for this purpose occasionally, but I prefer tools like
>> cscope. BTW: IMO using grep the way you are suggesting here is annoying
>> for cases where the search string is contained in other items.
>
> There may be cases, yes, but surely not this one: How likely is it for
> some other variable name to include, say, VPF_blocked?

I think we both agree that [_]VPF_blocked poses no problem using grep.

It's more a matter of taste and what people are used to. If someone
is using cscope for that purpose on a regular basis he will either have
to search for two different variables or he will have to use another
tool, possibly after seeing the definition of VPF_blocked depending on
_VPF_blocked. Both cases will be annoying as an extra action is required
for him.


Juergen