Messages in this thread Patch in this message | | | Subject | Re: [PATCH] sched: deduplicate atomic operations in select_nohz_load_balancer() | From | Steven Rostedt <> | Date | Tue, 31 May 2011 16:21:09 -0400 |
| |
On Wed, 2011-05-25 at 20:48 +0800, Hillf Danton wrote: > Two atomic-related operations, atomic_read and atomic_cmpxchg, are > used here to do the job. Since comparison is already included in the > later, the former is folded in the later, and operation is shorter. > > Signed-off-by: Hillf Danton <dhillf@gmail.com> > --- > > --- tip-git/kernel/sched_fair.c Sat May 14 15:21:56 2011 > +++ sched_fair.c Wed May 25 20:15:34 2011 > @@ -3806,35 +3806,25 @@ void select_nohz_load_balancer(int stop_ > > if (stop_tick) { > if (!cpu_active(cpu)) { > - if (atomic_read(&nohz.load_balancer) != cpu) > - return; > - > /* > * If we are going offline and still the leader, > * give up! > */ > - if (atomic_cmpxchg(&nohz.load_balancer, cpu, > - nr_cpu_ids) != cpu) > - BUG(); > + atomic_cmpxchg(&nohz.load_balancer, cpu, nr_cpu_ids);
OK, I can handle this change.
> > return; > } > > cpumask_set_cpu(cpu, nohz.idle_cpus_mask); > > - if (atomic_read(&nohz.first_pick_cpu) == cpu) > - atomic_cmpxchg(&nohz.first_pick_cpu, cpu, nr_cpu_ids); > - if (atomic_read(&nohz.second_pick_cpu) == cpu) > - atomic_cmpxchg(&nohz.second_pick_cpu, cpu, nr_cpu_ids); > + atomic_cmpxchg(&nohz.first_pick_cpu, cpu, nr_cpu_ids); > + atomic_cmpxchg(&nohz.second_pick_cpu, cpu, nr_cpu_ids);
This too, but we are doing a lot of cmpxchg's here when we probably don't need to. But this does not look like a fast path so it may not be that big of a deal.
> > - if (atomic_read(&nohz.load_balancer) >= nr_cpu_ids) { > + /* make me the ilb owner */ > + if (nr_cpu_ids == > + atomic_cmpxchg(&nohz.load_balancer, nr_cpu_ids, cpu)) {
Hmm, I see what you are doing here. But to fit the rest of the code, it should be:
if (atomic_cmpxchg(&nohz.load_balancer, nr_cpu_ids, cpu) == nr_cpu_ids) {
> int new_ilb; > > - /* make me the ilb owner */ > - if (atomic_cmpxchg(&nohz.load_balancer, nr_cpu_ids, > - cpu) != nr_cpu_ids) > - return; > - > /* > * Check to see if there is a more power-efficient > * ilb. > @@ -3853,10 +3843,7 @@ void select_nohz_load_balancer(int stop_ > > cpumask_clear_cpu(cpu, nohz.idle_cpus_mask); > > - if (atomic_read(&nohz.load_balancer) == cpu) > - if (atomic_cmpxchg(&nohz.load_balancer, cpu, > - nr_cpu_ids) != cpu) > - BUG(); > + atomic_cmpxchg(&nohz.load_balancer, cpu, nr_cpu_ids); > } > return; > }
The only other thing I can see that might be of issue, but not a big one, is that we are removing BUG checks. We may have been able to detect a bug before by seeing that the load_balancer was one thing, and suddenly changed when it shouldn't have. This may not be a big deal, but I just wanted to bring that up.
This change needs to be acked by Peter. I don't work much with the sched_fair code.
-- Steve
| |