Re: [Xen-devel] [PATCH] x86: re-order struct arch_domain fields

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.01.15 at 18:52, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 19/01/15 15:41, Jan Beulich wrote:
>> ... to reduce padding holes. While doing this I noticed vtsc_usercount
>> is a PV-only thing, so it gets moved straight to struct pv_domain.
> 
> The vtsc_{user,kernel}count split is curious.  They are both for stats
> purposes alone, but there is nothing pv specific about the usercount. 
> It frankly looks as if it has been mis-implemented for HVM, despite the
> split appearing to be deliberate when it was introduced in c/s
> bf2c44f8b469.  I am really not sure what to make of it.

It's clearly used for PV only at present. Anyone wanting to change
this (including if the current implementation was really buggy) could
easily move the field back out of struct pv_domain.

>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1767,7 +1767,7 @@ void pv_soft_rdtsc(struct vcpu *v, struc
>>      if ( guest_kernel_mode(v, regs) )
>>          d->arch.vtsc_kerncount++;
>>      else
>> -        d->arch.vtsc_usercount++;
>> +        d->arch.pv_domain.vtsc_usercount++;
>>  
>>      if ( (int64_t)(now - d->arch.vtsc_last) > 0 )
>>          d->arch.vtsc_last = now;
>> @@ -2020,17 +2020,15 @@ static void dump_softtsc(unsigned char k
>>              printk(",khz=%"PRIu32, d->arch.tsc_khz);
>>          if ( d->arch.incarnation )
>>              printk(",inc=%"PRIu32, d->arch.incarnation);
>> -        if ( !(d->arch.vtsc_kerncount | d->arch.vtsc_usercount) )
>> -        {
>> -            printk("\n");
>> -            continue;
>> -        }
>> -        if ( is_hvm_domain(d) )
>> +        if ( is_hvm_domain(d) && d->arch.vtsc_kerncount )
>>              printk(",vtsc count: %"PRIu64" total\n",
>>                     d->arch.vtsc_kerncount);
>> -        else
>> +        else if ( is_pv_domain(d) &&
>> +                  (d->arch.vtsc_kerncount | 
>> d->arch.pv_domain.vtsc_usercount) )
> 
> I realise you are simply rearanging the logic from before, but this
> should really be a logic or rather than a bitwise or, and is probably
> worth tweaking with the movement.

I disagree - for the purpose here, | vs || doesn't matter at all, and
personally (working with compilers other than gcc) I prefer | where
possible as not all compilers know to optimize || to | in cases like
this on architectures where logical machine instructions produce
usable status flags (in fact, none of the three different vendor
Windows compilers I just tried do).

Jan


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