Re: [PATCH] lib/vsprintf: add %pT format specifier

[Tetsuo Handa]

Andrew Morton wrote:
> > This patch introduces %pT format specifier for printing task_struct->comm.
> > Currently %pT does not provide consistency. I'm planning to change to use RCU
> > in the future. By using RCU, the comm name read from task_struct->comm will be
> > guaranteed to be consistent.
> 
> Not completely accurate - RCU won't protect code which accesses ->comm
> from interrupts.  Printing current->comm from irq is quite daft, but I
> bet there's code that does it :(
> 
> As long as the kernel doesn't crash or otherwise misbehave when this
> happens, I think we're OK.
> 
> (And I guess there's also non-daft code which accesses current->comm
> from interrupt context: oops, panic, etc).

Excuse me, but are interrupts relevant?

I think that readers that do

  rcu_read_lock();
  use p->comm here
  rcu_read_unlock();

will be protected from writers that wait freeing p->comm using
synchronize_rcu() or call_rcu().

Is synchronize_rcu() or call_rcu() insufficient for protecting readers that do

  rcu_read_lock();
  use p->comm here
  rcu_read_unlock();

 from interrupts?

> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1179,6 +1179,21 @@ char *address_val(char *buf, char *end, const void *addr,
> >  	return number(buf, end, num, spec);
> >  }
> >  
> > +static noinline_for_stack
> 
> hm, does noinline_for_stack actually do anything useful here?  I
> suspect it *increases* stack depth in the way comm_name() is used here.
> 
I just added noinline_for_stack as with other functions does.
But indeed, stack used by name[] is only 16 bytes but stack used by function
arguments are larger than 16 bytes. We should remove noinline_for_stack ?

> > +char *comm_name(char *buf, char *end, struct task_struct *tsk,
> > +		struct printf_spec spec, const char *fmt)
> > +{
> > +	char name[TASK_COMM_LEN];
> > +
> > +	/* Caller can pass NULL instead of current. */
> > +	if (!tsk)
> > +		tsk = current;
> > +	/* Not using get_task_comm() in case I'm in IRQ context. */
> > +	memcpy(name, tsk->comm, TASK_COMM_LEN);
> > +	name[sizeof(name) - 1] = '\0';
> 
> get_task_comm() uses strncpy()?

I think unconditionally copying 16 bytes using memcpy() is faster than
conditionally copying until '\0'.

> 
> > +	return string(buf, end, name, spec);
> > +}
> > +
--
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/