[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 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: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, structxen_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 _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |