[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 5/5] xen: clean up VPF flags macros
>>> On 28.09.15 at 10:15, <JGross@xxxxxxxx> wrote: > On 09/28/2015 09:52 AM, Jan Beulich wrote: >>>>> 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: >>>>>>> --- 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. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |