Re: [patch 1/2] dump_stack: serialize the output from dump_stack()

[Andrew Morton]

On Wed, 08 May 2013 16:01:03 -0500 athorlton@xxxxxxx wrote:

> These patches fix up issues with interspersed output from multiple
> simultaneous calls to warn or dump_stack on multi-cpu systems.
> References: <20130508210102.898396979@xxxxxxxxxxxxxxxxxxxxxxx>
> Content-Disposition: inline; filename=dump-stack-serialize.patch
> 
> This patch adds functionality to serialize the output from dump_stack() to 
> avoid mangling of the output when dump_stack is called simultaneously from
> multiple cpus.
> 
> ...
>
> The original discussion regarding this patch can be found in this thread:
> [PATCH] x86: Avoid intermixing cpu dump_stack output on multi-processor systems

If there was anything useful or interesting in that discussion then it
should be included in this patch's changelog, please.  Don't send
everyone off hunting for emails.  Email to which they can't reply in
the context of this thread...

> +++ linux/lib/dump_stack.c
> @@ -7,6 +7,8 @@
>  #include <linux/export.h>
>  #include <linux/sched.h>
>  
> +static atomic_t dump_lock = ATOMIC_INIT(-1);
> +
>  /**
>   * dump_stack - dump the current task information and its stack trace
>   *
> @@ -14,7 +16,30 @@
>   */
>  void dump_stack(void)
>  {
> +	int was_locked;
> +	int old;
> +	int cpu;
> +
> +	preempt_disable();
> +
> +retry:
> +	cpu = smp_processor_id();
> +	old = atomic_cmpxchg(&dump_lock, -1, cpu);
> +	if (old == -1) {
> +		was_locked = 0;
> +	} else if (old == cpu) {
> +		was_locked = 1;
> +	} else {
> +		cpu_relax();
> +		goto retry;
> +	}
> +
>  	dump_stack_print_info(KERN_DEFAULT);
>  	show_stack(NULL, NULL);
> +
> +	if (!was_locked)
> +		atomic_set(&dump_lock, -1);
> +
> +	preempt_enable();

This would benefit from a comment explaining what it's doing and why. 
"Permit this cpu to perform nested stack dumps while serialising
against other CPUs".

The patch adds a load of goop which is unneeded on uniprocessor
kernels.  I guess a non-messy way of avoiding that is to just have two
versions of dump_stack() in this file.

It would be prudent to toss some more #includes in there.  For
atomic_foo() and cpu_relax(), for example.

--
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/