Re: [PATCH RESEND 1/4] sched: /proc/sched_stat fails on very verylarge machines.

[Andrew Morton]

On Tue, 15 Jan 2013 15:46:09 -0600
Nathan Zimmer <nzimmer@xxxxxxx> wrote:

> On systems with 4096 cores doing a cat /proc/sched_stat fails.
> We are trying to push all the data into a single kmalloc buffer.
> The issue is on these very large machines all the data will not fit in 4mb.
> 
> A better solution is to not us the single_open mechanism but to provide
> our own seq_operations.
> 
> The output should be identical to previous version and thus not need the
> version number.
> 
> ...
>
> index 903ffa9..33a85c9 100644
> --- a/kernel/sched/stats.c
> +++ b/kernel/sched/stats.c
> @@ -21,9 +21,13 @@ static int show_schedstat(struct seq_file *seq, void *v)
>  	if (mask_str == NULL)
>  		return -ENOMEM;
>  
> -	seq_printf(seq, "version %d\n", SCHEDSTAT_VERSION);
> -	seq_printf(seq, "timestamp %lu\n", jiffies);
> -	for_each_online_cpu(cpu) {
> +	if (v == (void *)1) {

The magic-numbers-in-pointers at least need comments, please.  Or nice
and meaningful #defines.

> +		seq_printf(seq, "version %d\n", SCHEDSTAT_VERSION);
> +		seq_printf(seq, "timestamp %lu\n", jiffies);

The code leaks the memory at mask_str here.

> +	} else {
> +
> +		cpu = (unsigned long)(v - 2);
> +
>  		struct rq *rq = cpu_rq(cpu);
>  #ifdef CONFIG_SMP
>  		struct sched_domain *sd;
> @@ -72,35 +76,64 @@ static int show_schedstat(struct seq_file *seq, void *v)
>  		}
>  		rcu_read_unlock();
>  #endif
> +		kfree(mask_str);
>  	}
> -	kfree(mask_str);
>  	return 0;
>  }

Undoing this change will fix the leak.

The schedstats code (both the original and after the patch) appears to
be racy against cpu hotplug?  What prevents the rq from vanishing while
we're playing with it?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/