Re: [PATCH v2 1/5] x86/hpet: Pre cleanup

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/11/13 09:49, Jan Beulich wrote:
>>>> On 07.11.13 at 16:28, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This is a set of changes which might not make sense alone, but are
>> used/required for the main purpose of the series, and simply the mammoth 
>> patch
>> making it easier to review.
>>
>> * Make hpet_msi_write conditionally disable MSIs while updating the route
>>   register.  This removes the requirement that it must be masked while
>>   writing.
>>
>> * Defer read of cfg register in hpet_setup_msi_irq.  As a result, an 
>> intremap
>>   failure prevents us suffering a pointless MMIO read.
> Rather benign I would say - this isn't a hot code path.
>
>> * Change some instances of per_cpu($foo, cpu) to this_cpu($foo).  It is
>>   cleaner to read, and makes it more obvious when the code is poking around 
>> in
>>   another cpus data.
> Documentation wise this is certainly desirable, but since our this_cpu(),
> other than e.g. Linux'es equivalents, internally does another
> smp_processor_id(), generate code becomes worse.

with an "unsigned int cpu = smp_processor_id()" in context, the
generated code appears to be identical.

>
>> * Convert hpet_next_event() to taking a struct hpet_event_channel *, and
>>   rename to __hpet_set_counter() for a more accurate description of its
>>   actions.
> I very much think we should get away from the habit of prefixing
> symbols with two underscores: Such names are reserved to the
> compiler/platform, and the standard specifically reserves names
> starting with one underscore (and not followed by an upper case
> letter) for use a file scope symbols. This is what I'd like to request
> be done here.

So what would you suggest?  Here, all the __$foo() are designed to end
up similar to large swathes of other Xen code, implying that the
appropriate spinlock should be held by the caller.

I am not too fussed which convention we use, so long as it is used
consistently.

>
>> @@ -697,14 +707,16 @@ void hpet_broadcast_enter(void)
>>  {
>>      unsigned int cpu = smp_processor_id();
>>      struct hpet_event_channel *ch = per_cpu(cpu_bc_channel, cpu);
> And you're not even consistent with your conversion - you don't
> convert the above, ...

Because cpu_bc_channel disappears in the next patch.  As stated in the
description, the primary purpose of this patch is to make the following
patch easier to read, rather than to strictly make sense itself.

>
>> +    s_time_t deadline = this_cpu(timer_deadline);
>> +
>> +    ASSERT(!local_irq_is_enabled());
>>  
>> -    if ( per_cpu(timer_deadline, cpu) == 0 )
> ... but you do convert this one.

Because the following patch needs 'deadline' in the new code added here.

~Andrew

>
>> @@ -725,7 +737,9 @@ void hpet_broadcast_exit(void)
>>      unsigned int cpu = smp_processor_id();
>>      struct hpet_event_channel *ch = per_cpu(cpu_bc_channel, cpu);
>>  
>> -    if ( per_cpu(timer_deadline, cpu) == 0 )
>> +    ASSERT(local_irq_is_enabled());
>> +
>> +    if ( this_cpu(timer_deadline) == 0 )
> Same here. Anyway - I'd prefer these to be left alone.
>
> Jan
>