Re: [Patch 3/8] cpu delays

[Andrew Morton <akpm@osdl.org>]

Shailabh Nagar <nagar@watson.ibm.com> wrote:
>
> delayacct-schedstats.patch
> 
> Make the task-related schedstats functions
> callable by delay accounting even if schedstats
> collection isn't turned on. This removes the
> dependency of delay accounting on schedstats and allows
> cpu delays to be exported.
> 
> ..
>
> Index: linux-2.6.16/include/linux/sched.h
> ===================================================================
> --- linux-2.6.16.orig/include/linux/sched.h	2006-03-29 18:13:13.000000000 -0500
> +++ linux-2.6.16/include/linux/sched.h	2006-03-29 18:13:15.000000000 -0500
> @@ -525,7 +525,7 @@ typedef struct prio_array prio_array_t;
>  struct backing_dev_info;
>  struct reclaim_state;
> 
> -#ifdef CONFIG_SCHEDSTATS
> +#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
>  struct sched_info {
>  	/* cumulative counters */
>  	unsigned long	cpu_time,	/* time spent on the cpu */
> @@ -537,10 +537,14 @@ struct sched_info {
>  			last_queued;	/* when we were last queued to run */
>  };
> 
> +#ifdef CONFIG_SCHEDSTATS
>  extern struct file_operations proc_schedstat_operations;
> -#endif
> +#endif /* CONFIG_SCHEDSTATS */
> +#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */
> 
>  #ifdef CONFIG_TASK_DELAY_ACCT
> +extern int delayacct_on;
> +

This was already declared in delayacct.h and nothing in sched.h needs it. 
So it's better if .c files pick this declaration up from delayacct.h.

> 
> +static inline void rq_sched_info_arrive(struct runqueue *rq,
> +						unsigned long diff)
> +{
> +	if (rq) {
> +		rq->rq_sched_info.run_delay += diff;
> +		rq->rq_sched_info.pcnt++;
> +	}
> +}

The nonatomic updates need locking.  I assume it's runqueue.lock, but a
comment describing the rules would be good.

> +static inline void rq_sched_info_depart(struct runqueue *rq,
> +						unsigned long diff)
> +{
> +	if (rq)
> +		rq->rq_sched_info.cpu_time += diff;
> +}

Ditto.

>  static inline void sched_info_queued(task_t *t)
>  {
> -	if (!t->sched_info.last_queued)
> -		t->sched_info.last_queued = jiffies;
> +	if (unlikely(sched_info_on()))
> +		if (!t->sched_info.last_queued)
> +			t->sched_info.last_queued = jiffies;
>  }

It might be better to put the unlikely() into sched_info_on() itself.