[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH 5/5] xen: clean up VPF flags macros
- To: Jan Beulich <JBeulich@xxxxxxxx>
- From: Juergen Gross <jgross@xxxxxxxx>
- Date: Mon, 28 Sep 2015 10:44:24 +0200
- Cc: tim@xxxxxxx, wei.liu2@xxxxxxxxxx, Ian.Campbell@xxxxxxxxxx, stefano.stabellini@xxxxxxxxxxxxx, andrew.cooper3@xxxxxxxxxx, ian.jackson@xxxxxxxxxxxxx, xen-devel@xxxxxxxxxxxxx, david.vrabel@xxxxxxxxxx, keir@xxxxxxx
- Delivery-date: Mon, 28 Sep 2015 08:44:36 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
On 09/28/2015 10:34 AM, Jan Beulich wrote:
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.
Okay, I'll follow that route then.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
