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

[Juergen Gross <jgross@suse.com>]

On 09/28/2015 10:34 AM, Jan Beulich wrote:
>>>> On 28.09.15 at 10:15, <JGross@suse.com> wrote:
>> 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:
>>>>>>>> --- 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).
>
> I continue to think that the better route would be to get rid of the
> unnecessary test_bit() uses in favor of the shorter and less
> restrictive (to the compiler) & operation.

Okay, I'll follow that route then.


Juergen