Re: recursion handling: Re: [PATCH next v2 3/3] printk: remove logbuf_lock, add syslog_lock

[Sergey Senozhatsky]

On (20/12/04 17:10), Petr Mladek wrote:
[..]
> char *get_printk_counter_by_ctx()
> {
> 	int ctx = 0;
> 
> 	if (in_nmi)
> 		ctx = 1;
> 
> 	if (!printk_percpu_data_ready())
> 		return &printk_count_early[ctx];
> 
> 	return this_cpu_ptr(printk_count[ctx]);
> }
>
> > +
> > +	return count;
> > +}
> > +
> > +static bool printk_enter(unsigned long *flags)
> > +{
> > +	char *count;
> > +
> > +	local_irq_save(*flags);
> > +	count = get_printk_count();
> > +	/* Only 1 level of recursion allowed. */
> 
> We should allow at least some level of recursion. Otherwise, we would
> not see warnings printed from vsprintf code.

One level of recursion seems reasonable on one hand, but on the other
hand I also wonder if we can get useful info from recursion levels 2
and higher. Would be great to maybe list possible scenarios. vsprintf()
still call re-enter printk() -- WARN_ONCE()-s in the code -- external
format specifiers handlers also can. Would we need to let two levels of
recursion printk->vsprintf->printk->???->printk or one is just enough?

It also would make sense to add the lost messages counter to per-CPU
recursion counter struct, to count the number of times we bailout
of printk due to recursion limit. So that we'll at least have
"%d recursive printk messages lost" hints.


Overall...
I wonder where does the "limit printk recursion" come from? printk_safe
doesn't impose any strict limits (print_context is limited, but still)
and we've been running it for years now; have we ever seen any reports
of printk recursion overflows?

> > +	if (*count > 1) {
> > +		local_irq_restore(*flags);
> > +		return false;
> > +	}
> > +	(*count)++;
> > +
> > +	return true;
> > +}
> 
> This should be unified with printk_context, printk_nmi_enter(),
> printk_nmi_exit(). It does not make sense to have two separate
> printk context counters.

Agreed.

> Or is there any plan to remove printk_safe and printk_context?

That's a good point. This patch set and printk_safe answer the
same question in different ways, as far as I understand it. The
question is "Why do we want to track printk recursion"? This patch
set merely wants to, correct me if I'm wrong, avoid the very deep
vprintk_store() recursion stacks (which is a subset of printk()
recursion superset):

	vprintk_store()
	{
		if (!printk_enter())
			return;

		vsprintf/prb
		print_exit();
	}

And that's pretty much it, at least for the time being.

printk_safe()'s answer is - we don't want to re-enter parts of
the kernel that sit in the core, behind the scenes, and that are
not ready to be re-entered. Things like

	printk()
	 down_console_sem()
	  down()
	   raw_spin_lock_irqsave(&sem->lock)
	    printk()
	     down_console_sem()
	      down()
	       raw_spin_lock_irqsave(&sem->lock)

	-ss