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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.11.13 at 16:28, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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.

> * 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.

> @@ -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, ...

> +    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.

> @@ -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


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