Messages in this thread | | | Subject | Re: [PATCH] lib/vsprintf: add %pT format specifier | From | Tetsuo Handa <> | Date | Sat, 11 Jan 2014 10:59:54 +0900 |
| |
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); > > +} > > +
| |