Re: [Xen-devel] [PATCH 01/15] xen: in do_softirq() sample smp_processor_id() once and for all.

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 01/06/17 18:33, Dario Faggioli wrote:
> In fact, right now, we read it at every iteration of the loop.
> The reason it's done like this is how context switch was handled
> on IA64 (see commit ae9bfcdc, "[XEN] Various softirq cleanups" [1]).
> 
> However:
> 1) we don't have IA64 any longer, and all the achitectures that
>    we do support, are ok with sampling once and for all;
> 2) sampling at every iteration (slightly) affect performance;
> 3) sampling at every iteration is misleading, as it makes people
>    believe that it is currently possible that SCHEDULE_SOFTIRQ
>    moves the execution flow on another CPU (and the comment,
>    by reinforcing this belief, makes things even worse!).
> 
> Therefore, let's:
> - do the sampling once and for all, and remove the comment;

"Once and for all" has a much stronger sense of finality than you're
using here: reading smp_processor_id() in that function "once and for
all" would mean you would never have to read it on that function, on
that particular core, ever again, until the end of the universe. :-)

I think I'd say, "only once".  The scope of the "only" in that case is
"this function call", not "all calls forever".

With that changed:

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

(Although we till need Jan's acquiescence.)

> - leave an ASSERT() around, so that, if context switching
>   logic changes (in current or new arches), we will notice.
> 
> [1] Some more (historical) information here:
>     
> http://old-list-archives.xenproject.org/archives/html/xen-devel/2006-06/msg01262.html
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>


> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> ---
>  xen/common/softirq.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/softirq.c b/xen/common/softirq.c
> index ac12cf8..67c84ba 100644
> --- a/xen/common/softirq.c
> +++ b/xen/common/softirq.c
> @@ -27,16 +27,12 @@ static DEFINE_PER_CPU(unsigned int, batching);
>  
>  static void __do_softirq(unsigned long ignore_mask)
>  {
> -    unsigned int i, cpu;
> +    unsigned int i, cpu = smp_processor_id();
>      unsigned long pending;
>  
>      for ( ; ; )
>      {
> -        /*
> -         * Initialise @cpu on every iteration: SCHEDULE_SOFTIRQ may move
> -         * us to another processor.
> -         */
> -        cpu = smp_processor_id();
> +        ASSERT(cpu == smp_processor_id());
>  
>          if ( rcu_pending(cpu) )
>              rcu_check_callbacks(cpu);
> 


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