lkml.org 
[lkml]   [2014]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] lib/vsprintf: add %pT format specifier
From
Date
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);
> > +}
> > +


\
 
 \ /
  Last update: 2014-01-11 03:21    [W:0.134 / U:0.860 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site