From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [PATCH 5/5] xen: clean up VPF flags macros Date: Mon, 28 Sep 2015 10:15:25 +0200 Message-ID: <5608F71D.6040608@suse.com> References: <1443182072-15321-1-git-send-email-jgross@suse.com> <1443182072-15321-6-git-send-email-jgross@suse.com> <5605878D02000078000A5BA1@suse.com> <5608CEC1.1080207@suse.com> <5608F8AE02000078000A5F6E@suse.com> <5608EC5B.3060106@suse.com> <56090DDF02000078000A6042@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <56090DDF02000078000A6042@suse.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: tim@xen.org, wei.liu2@citrix.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, david.vrabel@citrix.com, keir@xen.org List-Id: xen-devel@lists.xenproject.org On 09/28/2015 09:52 AM, Jan Beulich wrote: >>>> On 28.09.15 at 09:29, wrote: >> On 09/28/2015 08:22 AM, Jan Beulich wrote: >>>>>> On 28.09.15 at 07:23, wrote: >>>> On 09/25/2015 05:42 PM, Jan Beulich wrote: >>>>>>>> On 25.09.15 at 13:54, 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))); \ >> }) > > 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