Taking a very raw attempt at this, I scratched out the following simple
implementation. I'd appreciate any review or suggestions for
improvements. I'm not at all certain the passing of the thread pid_t
through the unsigned long is valid, for instance, or if
same_thread_group() is the right check to make sure we only change
siblings and not tid from other processes. So any advice on better
approaches would be great.
thanks
-john
========================
Setting a thread's comm to be something unique is a very useful ability
and is helpful for debugging complicated threaded applications. However
currently the only way to set a thread name is for the thread to name
itself via the PR_SET_NAME prctl.
However, there may be situations where it would be advantageous for a
thread dispatcher to be naming the threads its managing, rather then
having the threads self-describe themselves. This sort of behavior is
available on other systems via the pthread_setname_np() interface.
This patch allows a thread to name its sibling threads via a new
PR_SET_THREAD_NAME prctrl.
Signed-off-by: John Stultz <[email protected]>
diff --git a/include/linux/prctl.h b/include/linux/prctl.h
index 9311505..2726527 100644
--- a/include/linux/prctl.h
+++ b/include/linux/prctl.h
@@ -52,6 +52,7 @@
#define PR_SET_NAME 15 /* Set process name */
#define PR_GET_NAME 16 /* Get process name */
+#define PR_SET_THREAD_NAME 17 /* Set sibling thread name */
/* Get/set process endian */
#define PR_GET_ENDIAN 19
diff --git a/kernel/sys.c b/kernel/sys.c
index 255475d..d25851a 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -1506,6 +1506,27 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
sizeof(comm)))
return -EFAULT;
return 0;
+ case PR_SET_THREAD_NAME:
+ {
+ struct task_struct *tsk;
+ pid_t tid;
+
+ comm[sizeof(me->comm)-1] = 0;
+ tid = (pid_t)arg2;
+ if (strncpy_from_user(comm, (char __user *)arg3,
+ sizeof(me->comm) - 1) < 0)
+ return -EFAULT;
+
+ tsk = get_pid_task(find_get_pid(tid), PIDTYPE_PID);
+ if (!tsk)
+ return -EINVAL;
+
+ if (!same_thread_group(me, tsk))
+ return -EINVAL;
+
+ set_task_comm(tsk, comm);
+ return 0;
+ }
case PR_GET_ENDIAN:
error = GET_ENDIAN(me, arg2);
break;
On Wed, Oct 21, 2009 at 04:21:37PM -0700, john stultz wrote:
>
> Taking a very raw attempt at this, I scratched out the following simple
> implementation. I'd appreciate any review or suggestions for
> improvements. I'm not at all certain the passing of the thread pid_t
> through the unsigned long is valid, for instance, or if
> same_thread_group() is the right check to make sure we only change
> siblings and not tid from other processes. So any advice on better
> approaches would be great.
First though that comes to mind is that this should not be in prctl()
-Andi
On Thu, 2009-10-22 at 02:28 +0200, Andi Kleen wrote:
> On Wed, Oct 21, 2009 at 04:21:37PM -0700, john stultz wrote:
> >
> > Taking a very raw attempt at this, I scratched out the following simple
> > implementation. I'd appreciate any review or suggestions for
> > improvements. I'm not at all certain the passing of the thread pid_t
> > through the unsigned long is valid, for instance, or if
> > same_thread_group() is the right check to make sure we only change
> > siblings and not tid from other processes. So any advice on better
> > approaches would be great.
>
> First though that comes to mind is that this should not be in prctl()
So it deserves a new syscall? Any other thoughts?
thanks
-john
On Wed, Oct 21, 2009 at 05:42:15PM -0700, john stultz wrote:
> On Thu, 2009-10-22 at 02:28 +0200, Andi Kleen wrote:
> > On Wed, Oct 21, 2009 at 04:21:37PM -0700, john stultz wrote:
> > >
> > > Taking a very raw attempt at this, I scratched out the following simple
> > > implementation. I'd appreciate any review or suggestions for
> > > improvements. I'm not at all certain the passing of the thread pid_t
> > > through the unsigned long is valid, for instance, or if
> > > same_thread_group() is the right check to make sure we only change
> > > siblings and not tid from other processes. So any advice on better
> > > approaches would be great.
> >
> > First though that comes to mind is that this should not be in prctl()
>
> So it deserves a new syscall? Any other thoughts?
I would probably just put it into /proc/pid and use the normal ptrace
access checks.
-Andi
--
[email protected] -- Speaking for myself only.
On Wed, 21 Oct 2009 16:21:37 -0700
john stultz <[email protected]> wrote:
>
> Taking a very raw attempt at this, I scratched out the following
> simple implementation. I'd appreciate any review or suggestions for
> improvements. I'm not at all certain the passing of the thread pid_t
> through the unsigned long is valid, for instance, or if
> same_thread_group() is the right check to make sure we only change
> siblings and not tid from other processes. So any advice on better
> approaches would be great.
>
> + return -EINVAL;
> +
> + set_task_comm(tsk, comm);
you're pretty much the first now who touches ->comm from
not-the-thread-itself.... are you sure that is safe?
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
> you're pretty much the first now who touches ->comm from
> not-the-thread-itself.... are you sure that is safe?
It's not, there is no locking. On the other hand nothing should crash,
just users might see half rewritten data.
-Andi
--
[email protected] -- Speaking for myself only.
On Wed, 2009-10-21 at 17:48 -0700, Arjan van de Ven wrote:
> On Wed, 21 Oct 2009 16:21:37 -0700
> john stultz <[email protected]> wrote:
>
> >
> > Taking a very raw attempt at this, I scratched out the following
> > simple implementation. I'd appreciate any review or suggestions for
> > improvements. I'm not at all certain the passing of the thread pid_t
> > through the unsigned long is valid, for instance, or if
> > same_thread_group() is the right check to make sure we only change
> > siblings and not tid from other processes. So any advice on better
> > approaches would be great.
> >
> > + return -EINVAL;
> > +
> > + set_task_comm(tsk, comm);
>
>
> you're pretty much the first now who touches ->comm from
> not-the-thread-itself.... are you sure that is safe?
No, I'm not sure at all :)
Thanks for pointing this out. I'll see whats needed in set_task_comm().
thanks again
-john
On Wed, 21 Oct 2009 17:52:24 -0700 john stultz <[email protected]> wrote:
> On Wed, 2009-10-21 at 17:48 -0700, Arjan van de Ven wrote:
> > On Wed, 21 Oct 2009 16:21:37 -0700
> > john stultz <[email protected]> wrote:
> >
> > >
> > > Taking a very raw attempt at this, I scratched out the following
> > > simple implementation. I'd appreciate any review or suggestions for
> > > improvements. I'm not at all certain the passing of the thread pid_t
> > > through the unsigned long is valid, for instance, or if
> > > same_thread_group() is the right check to make sure we only change
> > > siblings and not tid from other processes. So any advice on better
> > > approaches would be great.
> > >
> > > + return -EINVAL;
> > > +
> > > + set_task_comm(tsk, comm);
> >
> >
> > you're pretty much the first now who touches ->comm from
> > not-the-thread-itself.... are you sure that is safe?
>
> No, I'm not sure at all :)
>
> Thanks for pointing this out. I'll see whats needed in set_task_comm().
>
set_task_comm() is OK. The problem will be the unwritten rule that
processes can read *their own* ->comm without task_lock(), because nobody
ever alters ->comm apart from tack which owns it.
You've changed that, so all the open-coded accesses to current->comm are
now racy.
Also, you appear to be running set_task_comm() against a task_struct
without holding a reference on that task. Will a well-timed exit() cause a
modify-after-free?
On Thu, 22 Oct 2009 02:49:19 +0200
Andi Kleen <[email protected]> wrote:
> > you're pretty much the first now who touches ->comm from
> > not-the-thread-itself.... are you sure that is safe?
>
> It's not, there is no locking. On the other hand nothing should crash,
> just users might see half rewritten data.
.. with strings this is tricky though.. if the new string is longer
than the old one the terminating zero might just be missed etc.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Wed, Oct 21, 2009 at 07:48:22PM -0700, Arjan van de Ven wrote:
> On Thu, 22 Oct 2009 02:49:19 +0200
> Andi Kleen <[email protected]> wrote:
>
> > > you're pretty much the first now who touches ->comm from
> > > not-the-thread-itself.... are you sure that is safe?
> >
> > It's not, there is no locking. On the other hand nothing should crash,
> > just users might see half rewritten data.
>
> .. with strings this is tricky though.. if the new string is longer
> than the old one the terminating zero might just be missed etc.
Good point.
He needs to first set the last byte to zero to avoid that.
But better probably to do a proper lock on all readers. Or not add
this feature at all (does it have a strong use case?)
-Andi
--
[email protected] -- Speaking for myself only.
On Sat, 2009-10-24 at 05:54 +0200, Andi Kleen wrote:
> On Wed, Oct 21, 2009 at 07:48:22PM -0700, Arjan van de Ven wrote:
> > On Thu, 22 Oct 2009 02:49:19 +0200
> > Andi Kleen <[email protected]> wrote:
> >
> > > > you're pretty much the first now who touches ->comm from
> > > > not-the-thread-itself.... are you sure that is safe?
> > >
> > > It's not, there is no locking. On the other hand nothing should crash,
> > > just users might see half rewritten data.
> >
> > .. with strings this is tricky though.. if the new string is longer
> > than the old one the terminating zero might just be missed etc.
>
> Good point.
> He needs to first set the last byte to zero to avoid that.
> But better probably to do a proper lock on all readers. Or not add
> this feature at all (does it have a strong use case?)
Thread naming is really helpful for debugging a large multi-threaded
application. But currently it requires the threads to name themselves.
In some applications there may be a dispatch thread that spawns off the
siblings, and it has more context for naming the threads then the
threads do themselves.
So in that case, currently in order to have named threads, the
application's dispatch thread assigns names, and the threads have to
occasionally poll to see if they need to name themselves to something.
So they can get it to work, but its ugly and a big pain. Other OSes
support pthread_set_name_np() functionality, which makes things easy for
them, so they've requested to see if Linux can't do something similar.
thanks
-john
> On Wed, 21 Oct 2009 17:52:24 -0700 john stultz <[email protected]> wrote:
>
> > On Wed, 2009-10-21 at 17:48 -0700, Arjan van de Ven wrote:
> > > On Wed, 21 Oct 2009 16:21:37 -0700
> > > john stultz <[email protected]> wrote:
> > >
> > > >
> > > > Taking a very raw attempt at this, I scratched out the following
> > > > simple implementation. I'd appreciate any review or suggestions for
> > > > improvements. I'm not at all certain the passing of the thread pid_t
> > > > through the unsigned long is valid, for instance, or if
> > > > same_thread_group() is the right check to make sure we only change
> > > > siblings and not tid from other processes. So any advice on better
> > > > approaches would be great.
> > > >
> > > > + return -EINVAL;
> > > > +
> > > > + set_task_comm(tsk, comm);
> > >
> > >
> > > you're pretty much the first now who touches ->comm from
> > > not-the-thread-itself.... are you sure that is safe?
> >
> > No, I'm not sure at all :)
> >
> > Thanks for pointing this out. I'll see whats needed in set_task_comm().
> >
>
> set_task_comm() is OK. The problem will be the unwritten rule that
> processes can read *their own* ->comm without task_lock(), because nobody
> ever alters ->comm apart from tack which owns it.
>
> You've changed that, so all the open-coded accesses to current->comm are
> now racy.
>
> Also, you appear to be running set_task_comm() against a task_struct
> without holding a reference on that task. Will a well-timed exit() cause a
> modify-after-free?
John, I'd prefer to suggested another design.
How about this?
1. remove pid argument from prctl
2. cancel pthread_setname_np()
3. instead, create pthread_attr_setname_np()
4. pthread_create() change own thread name by pthread_attr.
It avoid many racy problem automatically.
KOSAKI Motohiro wrote:
> John, I'd prefer to suggested another design.
> How about this?
>
> 1. remove pid argument from prctl
> 2. cancel pthread_setname_np()
> 3. instead, create pthread_attr_setname_np()
> 4. pthread_create() change own thread name by pthread_attr.
>
>
> It avoid many racy problem automatically.
Perhaps, but it also greatly reduces the flexibility of the
implementation by restricting name changes to create time.
--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
> KOSAKI Motohiro wrote:
>
> > John, I'd prefer to suggested another design.
> > How about this?
> >
> > 1. remove pid argument from prctl
> > 2. cancel pthread_setname_np()
> > 3. instead, create pthread_attr_setname_np()
> > 4. pthread_create() change own thread name by pthread_attr.
> >
> > It avoid many racy problem automatically.
>
> Perhaps, but it also greatly reduces the flexibility of the
> implementation by restricting name changes to create time.
Hm.
if your program really need to change another thread name, can you please tell us
why it is necessary and when it is used?
KOSAKI Motohiro wrote:
>> KOSAKI Motohiro wrote:
>>
>>> John, I'd prefer to suggested another design.
>>> How about this?
>>>
>>> 1. remove pid argument from prctl
>>> 2. cancel pthread_setname_np()
>>> 3. instead, create pthread_attr_setname_np()
>>> 4. pthread_create() change own thread name by pthread_attr.
>>>
>>> It avoid many racy problem automatically.
>> Perhaps, but it also greatly reduces the flexibility of the
>> implementation by restricting name changes to create time.
>
> Hm.
> if your program really need to change another thread name, can you please tell us
> why it is necessary and when it is used?
>
I think John's previous mails covered that pretty well. As for doing the
name change at create time, or sometime later, it just seems to me that
the flexibility of doing so later is worth having. While I know we don't
have to follow other systems implementations, in this case
pthread_setname_np() seems a reasonable model to follow to me.
--
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team
> KOSAKI Motohiro wrote:
> >> KOSAKI Motohiro wrote:
> >>
> >>> John, I'd prefer to suggested another design.
> >>> How about this?
> >>>
> >>> 1. remove pid argument from prctl
> >>> 2. cancel pthread_setname_np()
> >>> 3. instead, create pthread_attr_setname_np()
> >>> 4. pthread_create() change own thread name by pthread_attr.
> >>>
> >>> It avoid many racy problem automatically.
> >> Perhaps, but it also greatly reduces the flexibility of the
> >> implementation by restricting name changes to create time.
> >
> > Hm.
> > if your program really need to change another thread name, can you please tell us
> > why it is necessary and when it is used?
>
> I think John's previous mails covered that pretty well. As for doing the
> name change at create time, or sometime later, it just seems to me that
> the flexibility of doing so later is worth having. While I know we don't
> have to follow other systems implementations, in this case
> pthread_setname_np() seems a reasonable model to follow to me.
You only said your mode is more flexible. but I want to know _why_ this flexibiliby is
necessay. please tell us concrete use-case.
Sean Foley
J9 Real-Time Java Ottawa Technical Lead
[email protected] (613)356-5012
KOSAKI Motohiro <[email protected]> wrote on 11/05/2009
12:42:23 AM:
> > KOSAKI Motohiro wrote:
> > >> KOSAKI Motohiro wrote:
> > >>
> > >>> John, I'd prefer to suggested another design.
> > >>> How about this?
> > >>>
> > >>> 1. remove pid argument from prctl
> > >>> 2. cancel pthread_setname_np()
> > >>> 3. instead, create pthread_attr_setname_np()
> > >>> 4. pthread_create() change own thread name by pthread_attr.
> > >>>
> > >>> It avoid many racy problem automatically.
> > >> Perhaps, but it also greatly reduces the flexibility of the
> > >> implementation by restricting name changes to create time.
> > >
> > > Hm.
> > > if your program really need to change another thread name, can
> you please tell us
> > > why it is necessary and when it is used?
> >
> > I think John's previous mails covered that pretty well. As for doing
the
> > name change at create time, or sometime later, it just seems to me
that
> > the flexibility of doing so later is worth having. While I know we
don't
> > have to follow other systems implementations, in this case
> > pthread_setname_np() seems a reasonable model to follow to me.
>
> You only said your mode is more flexible. but I want to know _why_
> this flexibiliby is
> necessay. please tell us concrete use-case.
>
Kosaki,
Here are a couple of use cases previously posted to this thread on the
linux kernel mailing list:
dispatch thread adds context to thread names:
http://marc.info/?l=linux-kernel&m=125660141231348&w=2
java language support:
http://marc.info/?l=linux-kernel&m=125666430720863&w=2
Here are some various specific use cases from the web:
Attaching additional info to thread names when used for different
purposes:
http://osdir.com/ml/java.jsr.166-concurrency/2006-12/msg00105.html
Threads obtained from thread pools being reassigned new names:
http://haacked.com/archive/2004/06/07/546.aspx
http://bytes.com/topic/c-sharp/answers/637152-naming-backgroundworker-thread
Renaming threads scattered across third-party libraries by enumerating
them and renaming them dynamically:
http://stackoverflow.com/questions/467224/renaming-threads-in-java
On Thu, 2009-11-05 at 14:42 +0900, KOSAKI Motohiro wrote:
> > KOSAKI Motohiro wrote:
> > >> KOSAKI Motohiro wrote:
> > >>
> > >>> John, I'd prefer to suggested another design.
> > >>> How about this?
> > >>>
> > >>> 1. remove pid argument from prctl
> > >>> 2. cancel pthread_setname_np()
> > >>> 3. instead, create pthread_attr_setname_np()
> > >>> 4. pthread_create() change own thread name by pthread_attr.
> > >>>
> > >>> It avoid many racy problem automatically.
> > >> Perhaps, but it also greatly reduces the flexibility of the
> > >> implementation by restricting name changes to create time.
> > >
> > > Hm.
> > > if your program really need to change another thread name, can you please tell us
> > > why it is necessary and when it is used?
> >
> > I think John's previous mails covered that pretty well. As for doing the
> > name change at create time, or sometime later, it just seems to me that
> > the flexibility of doing so later is worth having. While I know we don't
> > have to follow other systems implementations, in this case
> > pthread_setname_np() seems a reasonable model to follow to me.
>
> You only said your mode is more flexible. but I want to know _why_ this flexibiliby is
> necessay. please tell us concrete use-case.
You can read Sean's example from this thread here:
http://lkml.org/lkml/2009/10/27/259
thanks
-john
Hi Sean, John,
> Kosaki,
> Here are a couple of use cases previously posted to this thread on the linux kernel mailing list:
>
> dispatch thread adds context to thread names:
> http://marc.info/?l=linux-kernel&m=125660141231348&w=2
>
> java language support:
> http://marc.info/?l=linux-kernel&m=125666430720863&w=2
>
>
>
> Here are some various specific use cases from the web:
>
> Attaching additional info to thread names when used for different purposes:
> http://osdir.com/ml/java.jsr.166-concurrency/2006-12/msg00105.html
>
> Threads obtained from thread pools being reassigned new names:
> http://haacked.com/archive/2004/06/07/546.aspx
> http://bytes.com/topic/c-sharp/answers/637152-naming-backgroundworker-thread
>
> Renaming threads scattered across third-party libraries by enumerating them and renaming them dynamically:
> http://stackoverflow.com/questions/467224/renaming-threads-in-java
Okey, good explanation. thanks!
So, I would suggested to extend /proc/{pid}/cmdline instead using task->comm.
because
- task->comm has nasty locking rule. It is harder to change SMP safe.
- ps (and other procps tools) already support /proc/{pid}/cmdline.
- task->comm is restrected 16 character length, /proc/cmdline isn't.
You can see prctl-add-pr_set_proctitle_area-option.patch in -mm tree
as enhancement example at first step.
On Tue, 2009-11-10 at 14:27 +0900, KOSAKI Motohiro wrote:
> Hi Sean, John,
>
> > Kosaki,
> > Here are a couple of use cases previously posted to this thread on the linux kernel mailing list:
> >
> > dispatch thread adds context to thread names:
> > http://marc.info/?l=linux-kernel&m=125660141231348&w=2
> >
> > java language support:
> > http://marc.info/?l=linux-kernel&m=125666430720863&w=2
> >
> >
> >
> > Here are some various specific use cases from the web:
> >
> > Attaching additional info to thread names when used for different purposes:
> > http://osdir.com/ml/java.jsr.166-concurrency/2006-12/msg00105.html
> >
> > Threads obtained from thread pools being reassigned new names:
> > http://haacked.com/archive/2004/06/07/546.aspx
> > http://bytes.com/topic/c-sharp/answers/637152-naming-backgroundworker-thread
> >
> > Renaming threads scattered across third-party libraries by enumerating them and renaming them dynamically:
> > http://stackoverflow.com/questions/467224/renaming-threads-in-java
>
> Okey, good explanation. thanks!
>
> So, I would suggested to extend /proc/{pid}/cmdline instead using task->comm.
> because
> - task->comm has nasty locking rule. It is harder to change SMP safe.
I cc'ed you on the updated patch which addresses this. Please let me
know if you have specific concerns there.
> - ps (and other procps tools) already support /proc/{pid}/cmdline.
> - task->comm is restrected 16 character length, /proc/cmdline isn't.
Part of the reason to use comm is that most tools like perf or oprofile,
use comm, instead of cmd. Additionally, looking at cmdline, that's per
mm not per task, right? So it wouldn't really work for thread names.
Please correct me if I'm not seeing what you're really suggesting.
thanks
-john
> On Tue, 2009-11-10 at 14:27 +0900, KOSAKI Motohiro wrote:
> > Hi Sean, John,
> >
> > > Kosaki,
> > > Here are a couple of use cases previously posted to this thread on the linux kernel mailing list:
> > >
> > > dispatch thread adds context to thread names:
> > > http://marc.info/?l=linux-kernel&m=125660141231348&w=2
> > >
> > > java language support:
> > > http://marc.info/?l=linux-kernel&m=125666430720863&w=2
> > >
> > >
> > >
> > > Here are some various specific use cases from the web:
> > >
> > > Attaching additional info to thread names when used for different purposes:
> > > http://osdir.com/ml/java.jsr.166-concurrency/2006-12/msg00105.html
> > >
> > > Threads obtained from thread pools being reassigned new names:
> > > http://haacked.com/archive/2004/06/07/546.aspx
> > > http://bytes.com/topic/c-sharp/answers/637152-naming-backgroundworker-thread
> > >
> > > Renaming threads scattered across third-party libraries by enumerating them and renaming them dynamically:
> > > http://stackoverflow.com/questions/467224/renaming-threads-in-java
> >
> > Okey, good explanation. thanks!
> >
> > So, I would suggested to extend /proc/{pid}/cmdline instead using task->comm.
> > because
> > - task->comm has nasty locking rule. It is harder to change SMP safe.
>
> I cc'ed you on the updated patch which addresses this. Please let me
> know if you have specific concerns there.
>
> > - ps (and other procps tools) already support /proc/{pid}/cmdline.
> > - task->comm is restrected 16 character length, /proc/cmdline isn't.
>
> Part of the reason to use comm is that most tools like perf or oprofile,
> use comm, instead of cmd.
Ah, good reason. Okey, I will revew your new patch.
thanks.
> Additionally, looking at cmdline, that's per
> mm not per task, right? So it wouldn't really work for thread names.
Currently, yes. I meaned I think you can enhance it ;)
> Please correct me if I'm not seeing what you're really suggesting.