[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 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)));                 \
})

It would even be possible to drop the test for bitop_bad_size(addr) in
the constant case.

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.

Juergen


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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.