Tetsuo Handa wrote:
> This is a draft patch which changes task_struct->comm to use RCU.

Changes from previous draft version:

  Changed "struct rcu_comm" to use copy-on-write approach. Those multi-thread
  or multi-process applications which do not change comm name will consume
  memory for only one "struct rcu_comm".

  Changed do_commset() not to sleep.

  Changed to tolerate loss of consistency when memory allocation failed.

  Fixed race condition in copy_process().

Regards.
----------
>From ada6c4d94f5afda36c7c21869d38b7111a6fe9bc Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
Date: Mon, 17 Feb 2014 14:32:11 +0900
Subject: [PATCH] Change task_struct->comm to use RCU.

This patch changes task_struct->comm to be updated using RCU
(unless memory allocation fails).

Signed-off-by: Tetsuo Handa <penguin-ker...@i-love.sakura.ne.jp>
---
 fs/exec.c                 |  145 +++++++++++++++++++++++++++++++++++++++------
 include/linux/init_task.h |    4 +-
 include/linux/sched.h     |   37 ++++++++++--
 kernel/fork.c             |    9 +++
 kernel/kthread.c          |    4 +-
 kernel/sched/core.c       |    2 +-
 6 files changed, 173 insertions(+), 28 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a781dec..8a68ab3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -263,6 +263,11 @@ extern char ___assert_task_state[1 - 2*!!(
 
 /* Task command name length */
 #define TASK_COMM_LEN 16
+struct rcu_comm {
+       char name[TASK_COMM_LEN];
+       atomic_t usage;
+       struct rcu_head rcu;
+};
 
 #include <linux/spinlock.h>
 
@@ -1317,10 +1322,12 @@ struct task_struct {
                                         * credentials (COW) */
        const struct cred __rcu *cred;  /* effective (overridable) subjective 
task
                                         * credentials (COW) */
-       char comm[TASK_COMM_LEN]; /* executable name excluding path
-                                    - access with [gs]et_task_comm (which lock
-                                      it with task_lock())
-                                    - initialized normally by setup_new_exec */
+       struct rcu_comm __rcu *rcu_comm; /* executable name excluding path (COW)
+                                           - update with set_task_comm() or
+                                             do_set_task_comm[_fmt]()
+                                           - read with commcpy() or %pT
+                                           - initialized normally by
+                                             setup_new_exec */
 /* file system info */
        int link_count, total_link_count;
 #ifdef CONFIG_SYSVIPC
@@ -1792,6 +1799,23 @@ static inline cputime_t task_gtime(struct task_struct *t)
 extern void task_cputime_adjusted(struct task_struct *p, cputime_t *ut, 
cputime_t *st);
 extern void thread_group_cputime_adjusted(struct task_struct *p, cputime_t 
*ut, cputime_t *st);
 
+/**
+ * commcpy - Copy task_struct->comm to buffer.
+ *
+ * @buf: Buffer to copy @tsk->comm which must be at least TASK_COMM_LEN bytes.
+ * @tsk: Pointer to "struct task_struct".
+ *
+ * Returns @buf .
+ */
+static inline char *commcpy(char *buf, const struct task_struct *tsk)
+{
+       rcu_read_lock();
+       memcpy(buf, rcu_dereference(tsk->rcu_comm)->name, TASK_COMM_LEN);
+       rcu_read_unlock();
+       buf[TASK_COMM_LEN - 1] = '\0';
+       return buf;
+}
+
 /*
  * Per process flags
  */
@@ -2320,7 +2344,10 @@ struct task_struct *fork_idle(int);
 extern pid_t kernel_thread(int (*fn)(void *), void *arg, unsigned long flags);
 
 extern void set_task_comm(struct task_struct *tsk, char *from);
-extern char *get_task_comm(char *to, struct task_struct *tsk);
+extern void do_set_task_comm(struct task_struct *tsk, const char *buf);
+extern void do_set_task_comm_fmt(struct task_struct *tsk, const char *fmt, ...)
+       __printf(2, 3);
+extern void put_commname(struct rcu_comm *comm);
 
 #ifdef CONFIG_SMP
 void scheduler_ipi(void);
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 6df7f9f..b217f8c 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -154,7 +154,7 @@ extern struct task_group root_task_group;
 # define INIT_VTIME(tsk)
 #endif
 
-#define INIT_TASK_COMM "swapper"
+extern struct rcu_comm init_rcu_comm;
 
 #ifdef CONFIG_RT_MUTEXES
 # define INIT_RT_MUTEXES(tsk)                                          \
@@ -201,7 +201,7 @@ extern struct task_group root_task_group;
        .group_leader   = &tsk,                                         \
        RCU_POINTER_INITIALIZER(real_cred, &init_cred),                 \
        RCU_POINTER_INITIALIZER(cred, &init_cred),                      \
-       .comm           = INIT_TASK_COMM,                               \
+       .rcu_comm       = &init_rcu_comm,                               \
        .thread         = INIT_THREAD,                                  \
        .fs             = &init_fs,                                     \
        .files          = &init_files,                                  \
diff --git a/fs/exec.c b/fs/exec.c
index 3d78fcc..85c57a6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1026,30 +1026,11 @@ killed:
        return -EAGAIN;
 }
 
-char *get_task_comm(char *buf, struct task_struct *tsk)
-{
-       /* buf must be at least sizeof(tsk->comm) in size */
-       task_lock(tsk);
-       strncpy(buf, tsk->comm, sizeof(tsk->comm));
-       task_unlock(tsk);
-       return buf;
-}
-EXPORT_SYMBOL_GPL(get_task_comm);
-
 /*
  * These functions flushes out all traces of the currently running executable
  * so that a new one can be started
  */
 
-void set_task_comm(struct task_struct *tsk, char *buf)
-{
-       task_lock(tsk);
-       trace_task_rename(tsk, buf);
-       strlcpy(tsk->comm, buf, sizeof(tsk->comm));
-       task_unlock(tsk);
-       perf_event_comm(tsk);
-}
-
 static void filename_to_taskname(char *tcomm, const char *fn, unsigned int len)
 {
        int i, ch;
@@ -1626,3 +1607,129 @@ asmlinkage long compat_sys_execve(const char __user * 
filename,
        return compat_do_execve(getname(filename), argv, envp);
 }
 #endif
+
+/* Task's comm name handler functions. */
+static struct kmem_cache *commname_cachep __read_mostly;
+
+/* comm name for init_task. */
+struct rcu_comm init_rcu_comm = {
+       .name = "swapper",
+       .usage = ATOMIC_INIT(INT_MAX) /* Mark this object static. */
+};
+
+/**
+ * set_task_comm - Change task_struct->comm with tracer and perf hooks called.
+ *
+ * @tsk: Pointer to "struct task_struct".
+ * @buf: New comm name.
+ *
+ * Returns nothing.
+ */
+void set_task_comm(struct task_struct *tsk, char *buf)
+{
+       /*
+        * Question: Do we need to use task_lock()/task_unlock() ?
+        */
+       task_lock(tsk);
+       trace_task_rename(tsk, buf);
+       task_unlock(tsk);
+       do_set_task_comm(tsk, buf);
+       perf_event_comm(tsk);
+}
+
+/**
+ * do_set_task_comm_fmt - Change task_struct->comm.
+ *
+ * @tsk: Pointer to "struct task_struct".
+ * @fmt: Format string, followed by arguments.
+ *
+ * Returns nothing.
+ */
+void do_set_task_comm_fmt(struct task_struct *tsk, const char *fmt, ...)
+{
+       va_list args;
+       char name[TASK_COMM_LEN];
+
+       va_start(args, fmt);
+       vsnprintf(name, TASK_COMM_LEN, fmt, args);
+       va_end(args);
+       do_set_task_comm(tsk, name);
+}
+
+/**
+ * do_set_task_comm - Change task_struct->comm.
+ *
+ * @tsk: Pointer to "struct task_struct".
+ * @buf: New comm name.
+ *
+ * Returns nothing.
+ */
+void do_set_task_comm(struct task_struct *tsk, const char *buf)
+{
+       struct rcu_comm *comm;
+
+       comm = kmem_cache_zalloc(commname_cachep, GFP_ATOMIC | __GFP_NOWARN);
+       if (likely(comm)) {
+               atomic_set(&comm->usage, 1);
+               strncpy(comm->name, buf, TASK_COMM_LEN - 1);
+               smp_wmb(); /* Behave like rcu_assign_pointer(). */
+               comm = xchg(&tsk->rcu_comm, comm);
+               put_commname(comm);
+       } else {
+               /*
+                * Memory allocation failed. We have to tolerate loss of
+                * consistency.
+                *
+                * Question 1: Do we want to reserve some amount of static
+                * "struct rcu_comm" arrays for use upon allocation failures?
+                *
+                * Question 2: Do we perfer unchanged comm name over
+                * overwritten comm name because comm name is copy-on-write ?
+                */
+               WARN_ONCE(1, "Failed to allocate memory for comm name.\n");
+               rcu_read_lock();
+               do {
+                       comm = rcu_dereference(tsk->rcu_comm);
+               } while (!atomic_read(&comm->usage));
+               strncpy(comm->name, buf, TASK_COMM_LEN - 1);
+               rcu_read_unlock();
+       }
+}
+
+/**
+ * free_commname - Release "struct rcu_comm".
+ *
+ * @rcu: Pointer to "struct rcu_head".
+ *
+ * Returns nothing.
+ */
+static void free_commname(struct rcu_head *rcu)
+{
+       struct rcu_comm *comm = container_of(rcu, struct rcu_comm, rcu);
+       kmem_cache_free(commname_cachep, comm);
+}
+
+/**
+ * put_commname - Release "struct rcu_comm".
+ *
+ * @comm: Pointer to "struct rcu_comm".
+ *
+ * Returns nothing.
+ */
+void put_commname(struct rcu_comm *comm)
+{
+       if (atomic_dec_and_test(&comm->usage))
+               call_rcu(&comm->rcu, free_commname);
+}
+
+/**
+ * commname_init - Initialize "struct rcu_comm".
+ *
+ * Returns nothing.
+ */
+void __init commname_init(void)
+{
+       commname_cachep = kmem_cache_create("commname",
+                                           sizeof(struct rcu_comm),
+                                           0, SLAB_PANIC, NULL);
+}
diff --git a/kernel/fork.c b/kernel/fork.c
index a17621c..23be1ef 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -214,6 +214,7 @@ void free_task(struct task_struct *tsk)
        ftrace_graph_exit_task(tsk);
        put_seccomp_filter(tsk);
        arch_release_task_struct(tsk);
+       put_commname(tsk->rcu_comm);
        free_task_struct(tsk);
 }
 EXPORT_SYMBOL(free_task);
@@ -249,6 +250,8 @@ EXPORT_SYMBOL_GPL(__put_task_struct);
 
 void __init __weak arch_task_cache_init(void) { }
 
+extern void __init commname_init(void);
+
 void __init fork_init(unsigned long mempages)
 {
 #ifndef CONFIG_ARCH_TASK_STRUCT_ALLOCATOR
@@ -260,6 +263,7 @@ void __init fork_init(unsigned long mempages)
                kmem_cache_create("task_struct", sizeof(struct task_struct),
                        ARCH_MIN_TASKALIGN, SLAB_PANIC | SLAB_NOTRACK, NULL);
 #endif
+       commname_init();
 
        /* do the arch specific task caches init */
        arch_task_cache_init();
@@ -1191,6 +1195,11 @@ static struct task_struct *copy_process(unsigned long 
clone_flags,
        p = dup_task_struct(current);
        if (!p)
                goto fork_out;
+       rcu_read_lock();
+       do {
+               p->rcu_comm = rcu_dereference(current->rcu_comm);
+       } while (!atomic_inc_not_zero(&p->rcu_comm->usage));
+       rcu_read_unlock();
 
        ftrace_graph_init_task(p);
        get_seccomp_filter(p);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index b5ae3ee..e83b67c 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -309,10 +309,12 @@ struct task_struct *kthread_create_on_node(int 
(*threadfn)(void *data),
        if (!IS_ERR(task)) {
                static const struct sched_param param = { .sched_priority = 0 };
                va_list args;
+               char name[TASK_COMM_LEN];
 
                va_start(args, namefmt);
-               vsnprintf(task->comm, sizeof(task->comm), namefmt, args);
+               vsnprintf(name, TASK_COMM_LEN, namefmt, args);
                va_end(args);
+               do_set_task_comm(task, name);
                /*
                 * root may have changed our (kthreadd's) priority or CPU mask.
                 * The kernel thread should not inherit these properties.
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index b46131e..bbb7c94 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4488,7 +4488,7 @@ void init_idle(struct task_struct *idle, int cpu)
        ftrace_graph_init_idle_task(idle, cpu);
        vtime_init_idle(idle, cpu);
 #if defined(CONFIG_SMP)
-       sprintf(idle->comm, "%s/%d", INIT_TASK_COMM, cpu);
+       do_set_task_comm_fmt(idle, "swapper/%d", cpu);
 #endif
 }
 
-- 
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to