| Message ID | 150290193547.24854.2362617593969664852.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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);
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(-)