Re: [PATCH v2 1/6] xen: in do_softirq() sample smp_processor_id() once and for all.

Re: [PATCH v2 1/6] xen: in do_softirq() sample smp_processor_id() once and for all.

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2392 bytes --]

Il 29 Ago 2017 4:03 PM, George Dunlap <George.Dunlap@eu.citrix.com> ha scritto:
On Wed, Aug 16, 2017 at 5:45 PM, Dario Faggioli
<dario.faggioli@citrix.com> 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 only once, and remove the comment;
> - 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@citrix.com>
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Tim Deegan <tim@xen.org>
> ---
> This has been submitted already, as a part of another series. Discussion is here:
>  https://lists.xen.org/archives/html/xen-devel/2017-06/msg00102.html
>
> For the super lazy, Jan's latest word in that thread were these:
>  "I've voiced my opinion, but I don't mean to block the patch. After
>   all there's no active issue the change introduces."
>  (https://lists.xen.org/archives/html/xen-devel/2017-06/msg00797.html)
>
> Since then:
> - changed "once and for all" with "only once", as requested by George (and
>   applied his Reviewed-by, as he said I could).


The commit message, but forgot to change the title. :-)

Indeed. I focused on the body of the changelog, and didn't even recall/notice, that it was in the present in the subject line as well! :-(

Sorry,
Dario


[-- Attachment #1.2: Type: text/html, Size: 3999 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 1/6] xen: in do_softirq() sample smp_processor_id() once and for all.

[George Dunlap]

On Wed, Aug 16, 2017 at 5:45 PM, Dario Faggioli
<dario.faggioli@citrix.com> 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 only once, and remove the comment;
> - 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@citrix.com>
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Tim Deegan <tim@xen.org>
> ---
> This has been submitted already, as a part of another series. Discussion is here:
>  https://lists.xen.org/archives/html/xen-devel/2017-06/msg00102.html
>
> For the super lazy, Jan's latest word in that thread were these:
>  "I've voiced my opinion, but I don't mean to block the patch. After
>   all there's no active issue the change introduces."
>  (https://lists.xen.org/archives/html/xen-devel/2017-06/msg00797.html)
>
> Since then:
> - changed "once and for all" with "only once", as requested by George (and
>   applied his Reviewed-by, as he said I could).


The commit message, but forgot to change the title. :-)  That can be
addressed on check-in if need be.

 -George

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

[PATCH v2 1/6] xen: in do_softirq() sample smp_processor_id() once and for all.

[Dario Faggioli]

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 only once, and remove the comment;
- 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@citrix.com>
Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Tim Deegan <tim@xen.org>
---
This has been submitted already, as a part of another series. Discussion is here:
 https://lists.xen.org/archives/html/xen-devel/2017-06/msg00102.html

For the super lazy, Jan's latest word in that thread were these:
 "I've voiced my opinion, but I don't mean to block the patch. After
  all there's no active issue the change introduces."
 (https://lists.xen.org/archives/html/xen-devel/2017-06/msg00797.html)

Since then:
- changed "once and for all" with "only once", as requested by George (and
  applied his Reviewed-by, as he said I could).
---
 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@lists.xen.org
https://lists.xen.org/xen-devel