| Message ID | 148469943088.19533.8330155575788944646.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/01/17 00:30, Dario Faggioli wrote: > Since we are doing cpumask manipulation already, clear a bit > in the mask at once. Doing that will save us an if, later in > the code. > > No functional change intended. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > --- > xen/common/sched_credit2.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c > index ef8e0d8..d086264 100644 > --- a/xen/common/sched_credit2.c > +++ b/xen/common/sched_credit2.c > @@ -985,7 +985,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now) > cpumask_andnot(&mask, &rqd->active, &rqd->idle); > cpumask_andnot(&mask, &mask, &rqd->tickled); > cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); > - if ( cpumask_test_cpu(cpu, &mask) ) > + if ( __cpumask_test_and_clear_cpu(cpu, &mask) ) Since we're micro-optimizing -- isn't test-and-clear a locked operation? Would that be more expensive than the if() statement below? -George
>>> On 18.01.17 at 11:21, <george.dunlap@citrix.com> wrote: > On 18/01/17 00:30, Dario Faggioli wrote: >> index ef8e0d8..d086264 100644 >> --- a/xen/common/sched_credit2.c >> +++ b/xen/common/sched_credit2.c >> @@ -985,7 +985,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now) >> cpumask_andnot(&mask, &rqd->active, &rqd->idle); >> cpumask_andnot(&mask, &mask, &rqd->tickled); >> cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); >> - if ( cpumask_test_cpu(cpu, &mask) ) >> + if ( __cpumask_test_and_clear_cpu(cpu, &mask) ) > > Since we're micro-optimizing -- isn't test-and-clear a locked operation? > Would that be more expensive than the if() statement below? cpumask_test_and_clear_cpu() is, but __cpumask_test_and_clear_cpu() isn't. Jan
On Wed, 2017-01-18 at 03:30 -0700, Jan Beulich wrote: > > > > On 18.01.17 at 11:21, <george.dunlap@citrix.com> wrote: > > On 18/01/17 00:30, Dario Faggioli wrote: > > > > > > --- a/xen/common/sched_credit2.c > > > +++ b/xen/common/sched_credit2.c > > > @@ -985,7 +985,7 @@ runq_tickle(const struct scheduler *ops, > > > struct csched2_vcpu *new, s_time_t now) > > > cpumask_andnot(&mask, &rqd->active, &rqd->idle); > > > cpumask_andnot(&mask, &mask, &rqd->tickled); > > > cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); > > > - if ( cpumask_test_cpu(cpu, &mask) ) > > > + if ( __cpumask_test_and_clear_cpu(cpu, &mask) ) > > > > Since we're micro-optimizing -- isn't test-and-clear a locked > > operation? > > Would that be more expensive than the if() statement below? > > cpumask_test_and_clear_cpu() is, but __cpumask_test_and_clear_cpu() > isn't. > As Jan said. And, FWIW, I personally like how the code looks after this patch better, even leaving aside performance. I find it cleaner (probably because dislike 'continue'), and more in line with what we do in the rest of the file. Thanks and Regadrs, Dario
On Wed, 2017-01-18 at 03:30 -0700, Jan Beulich wrote: > > > > On 18.01.17 at 11:21, <george.dunlap@citrix.com> wrote: > > On 18/01/17 00:30, Dario Faggioli wrote: > > > index ef8e0d8..d086264 100644 > > > --- a/xen/common/sched_credit2.c > > > +++ b/xen/common/sched_credit2.c > > > @@ -985,7 +985,7 @@ runq_tickle(const struct scheduler *ops, > > > struct csched2_vcpu *new, s_time_t now) > > > cpumask_andnot(&mask, &rqd->active, &rqd->idle); > > > cpumask_andnot(&mask, &mask, &rqd->tickled); > > > cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); > > > - if ( cpumask_test_cpu(cpu, &mask) ) > > > + if ( __cpumask_test_and_clear_cpu(cpu, &mask) ) > > > > Since we're micro-optimizing -- isn't test-and-clear a locked > > operation? > > Would that be more expensive than the if() statement below? > > cpumask_test_and_clear_cpu() is, but __cpumask_test_and_clear_cpu() > isn't. > George, ping? Thanks and Regards, Dario
On 26/01/17 01:00, Dario Faggioli wrote: > On Wed, 2017-01-18 at 03:30 -0700, Jan Beulich wrote: >>>>> On 18.01.17 at 11:21, <george.dunlap@citrix.com> wrote: >>> On 18/01/17 00:30, Dario Faggioli wrote: >>>> index ef8e0d8..d086264 100644 >>>> --- a/xen/common/sched_credit2.c >>>> +++ b/xen/common/sched_credit2.c >>>> @@ -985,7 +985,7 @@ runq_tickle(const struct scheduler *ops, >>>> struct csched2_vcpu *new, s_time_t now) >>>> cpumask_andnot(&mask, &rqd->active, &rqd->idle); >>>> cpumask_andnot(&mask, &mask, &rqd->tickled); >>>> cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); >>>> - if ( cpumask_test_cpu(cpu, &mask) ) >>>> + if ( __cpumask_test_and_clear_cpu(cpu, &mask) ) >>> >>> Since we're micro-optimizing -- isn't test-and-clear a locked >>> operation? >>> Would that be more expensive than the if() statement below? >> >> cpumask_test_and_clear_cpu() is, but __cpumask_test_and_clear_cpu() >> isn't. >> > George, ping? Yes, this looks fine then. But it didn't apply cleanly when I tried to apply it -- please re-send it with the other patches you have outstanding. Thanks. -George
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index ef8e0d8..d086264 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -985,7 +985,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now) cpumask_andnot(&mask, &rqd->active, &rqd->idle); cpumask_andnot(&mask, &mask, &rqd->tickled); cpumask_and(&mask, &mask, new->vcpu->cpu_hard_affinity); - if ( cpumask_test_cpu(cpu, &mask) ) + if ( __cpumask_test_and_clear_cpu(cpu, &mask) ) { cur = CSCHED2_VCPU(curr_on_cpu(cpu)); burn_credits(rqd, cur, now); @@ -1001,8 +1001,7 @@ runq_tickle(const struct scheduler *ops, struct csched2_vcpu *new, s_time_t now) for_each_cpu(i, &mask) { /* Already looked at this one above */ - if ( i == cpu ) - continue; + ASSERT(i != cpu); cur = CSCHED2_VCPU(curr_on_cpu(i));
Since we are doing cpumask manipulation already, clear a bit in the mask at once. Doing that will save us an if, later in the code. No functional change intended. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> --- xen/common/sched_credit2.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)