Re: [Xen-devel] [PATCH v2 1/6] xen: in do_softirq() sample smp_processor_id() once and for all.
Il 29 Ago 2017 4:03 PM, George Dunlapha scritto: On Wed, Aug 16, 2017 at 5:45 PM, 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 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 > Reviewed-by: George Dunlap > --- > Cc: Andrew Cooper > Cc: Jan Beulich > Cc: Konrad Rzeszutek Wilk > Cc: Stefano Stabellini > Cc: Julien Grall > Cc: Tim Deegan > --- > 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 1/6] xen: in do_softirq() sample smp_processor_id() once and for all.
On Wed, Aug 16, 2017 at 5:45 PM, Dario Faggioliwrote: > 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 > Reviewed-by: George Dunlap > --- > Cc: Andrew Cooper > Cc: Jan Beulich > Cc: Konrad Rzeszutek Wilk > Cc: Stefano Stabellini > Cc: Julien Grall > Cc: Tim Deegan > --- > 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
[Xen-devel] [PATCH v2 1/6] xen: in do_softirq() sample smp_processor_id() once and for all.
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 FaggioliReviewed-by: George Dunlap --- Cc: Andrew Cooper Cc: Jan Beulich Cc: Konrad Rzeszutek Wilk Cc: Stefano Stabellini Cc: Julien Grall Cc: Tim Deegan --- 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