Re: [PATCH] - Reduce overhead of calc_load

[Nick Piggin <nickpiggin@yahoo.com.au>]

Andrew Morton wrote:
> Jack Steiner <steiner@sgi.com> wrote:
> 
>>+unsigned long nr_active(void)
>>+{
>>+	unsigned long i, running = 0, uninterruptible = 0;
>>+
>>+	for_each_online_cpu(i) {
>>+		running += cpu_rq(i)->nr_running;
>>+		uninterruptible += cpu_rq(i)->nr_uninterruptible;
>>+	}
>>+
>>+	if (unlikely((long)uninterruptible < 0))
>>+		uninterruptible = 0;
>>+
>>+	return running + uninterruptible;
>>+}
> 
> 
> Is that check for (uninterruptible < 0) (copied from nr_uninterruptible)
> really needed?  Can rq->nr_uninterruptible actually go negative?
> 

The sum cannot if there are no concurrent updates, however when
there are concurrent updates then it can go negative.

rq->nr_uninterruptible itself is meaningless because it can be
incremented on one rq and decremented on another.

> Perhaps nr_context_switches() and nr_iowait() should also go into this
> function, then we rename it all to
> 
> 	struct sched_stuff {
> 		unsigned nr_uninterruptible;
> 		unsigned nr_running;
> 		unsigned nr_active;
> 		unsigned long nr_context_switches;
> 	};
> 
> 	void get_sched_stuff(struct sched_stuff *);
> 
> and then convert all those random little counter-upper-callers we have.
> 

Is there a need? Do they (except calc_load) use multiple values at
the same time?

> And then give get_sched_stuff() a hotplug handler (probably unneeded) and

What would the hotplug handler do?

> then scratch our heads over why nr_uninterruptible() iterates across all
> possible CPUs while this new nr_active() iterates over all online CPUs like
> nr_running() and unlike nr_context_switches().
> 

I think it need only iterate over possible CPUs.

> 
> IOW: this code's an inefficient mess and needs some caring for.

What are the performance critical places that call the nr_blah() functions?

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com