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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.09.15 at 09:29, <JGross@xxxxxxxx> wrote:
> On 09/28/2015 08:22 AM, Jan Beulich wrote:
>>>>> On 28.09.15 at 07:23, <JGross@xxxxxxxx> wrote:
>>> On 09/25/2015 05:42 PM, Jan Beulich wrote:
>>>>>>> On 25.09.15 at 13:54, <JGross@xxxxxxxx> 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.

>>> 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?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel