| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1292258544.2063.189.camel@holzheu-laptop>
Date: Mon, 13 Dec 2010 17:42:24 +0100
From: Michael Holzheu <holzheu@...ux.vnet.ibm.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Shailabh Nagar <nagar1234@...ibm.com>,
Andrew Morton <akpm@...ux-foundation.org>,
Peter Zijlstra <a.p.zijlstra@...llo.nl>,
John stultz <johnstul@...ibm.com>,
Thomas Gleixner <tglx@...utronix.de>,
Balbir Singh <balbir@...ux.vnet.ibm.com>,
Martin Schwidefsky <schwidefsky@...ibm.com>,
Heiko Carstens <heiko.carstens@...ibm.com>,
Roland McGrath <roland@...hat.com>, Valdis.Kletnieks@...edu,
linux-kernel@...r.kernel.org, linux-s390@...r.kernel.org
Subject: Re: [patch v2 4/4] taskstats: Export "cdata_wait" CPU times with
taskstats
On Mon, 2010-12-13 at 15:33 +0100, Oleg Nesterov wrote:
> > --- a/include/linux/taskstats_kern.h
> > +++ b/include/linux/taskstats_kern.h
> > @@ -21,7 +21,8 @@ static inline void taskstats_tgid_free(s
> > kmem_cache_free(taskstats_cache, sig->stats);
> > }
> >
> > -extern void taskstats_exit(struct task_struct *, int group_dead);
> > +extern void taskstats_exit_notify(struct task_struct *, int group_dead);
> > +extern void taskstats_exit_add_thread(struct task_struct *);
> You forgot to update the !CONFIG_TASKSTATS case ;)
... of course, thanks!
[snip]
> Well. I do not like the fact we take ->siglock unconditionally.
> And _irqsave is not needed. And we take it twice if sig->stats == NULL.
> And "if (!tsk->signal->stats)" under ->siglock in
> taskstats_exit_add_thread() looks a bit ugly...
Yes I also saw that, but I didn't want to change too much. In fact your
code is much better. I added it to the patch and also replaced
_add_thread with _add_task, because task seems to be the term for thread
used in the taskstats code. I also did some testing. Everything still
seems to work well.
The new patch is attached below. Balbir, do you agree?
Michael
---
include/linux/taskstats_kern.h | 8 +++-
kernel/exit.c | 3 +
kernel/taskstats.c | 67 ++++++++++++++---------------------------
3 files changed, 32 insertions(+), 46 deletions(-)
--- a/include/linux/taskstats_kern.h
+++ b/include/linux/taskstats_kern.h
@@ -21,10 +21,14 @@ static inline void taskstats_tgid_free(s
kmem_cache_free(taskstats_cache, sig->stats);
}
-extern void taskstats_exit(struct task_struct *, int group_dead);
+extern void taskstats_exit_notify(struct task_struct *tsk, int group_dead);
+extern void taskstats_exit_add_task(struct task_struct *tsk);
extern void taskstats_init_early(void);
#else
-static inline void taskstats_exit(struct task_struct *tsk, int group_dead)
+static inline void taskstats_exit_notify(struct task_struct *tsk,
+ int group_dead)
+{}
+static inline void taskstats_exit_add_task(struct task_struct *tsk);
{}
static inline void taskstats_tgid_free(struct signal_struct *sig)
{}
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1030,6 +1030,7 @@ NORET_TYPE void do_exit(long code)
/* sync mm's RSS info before statistics gathering */
if (tsk->mm)
sync_mm_rss(tsk, tsk->mm);
+ taskstats_exit_add_task(tsk);
group_dead = atomic_dec_and_test(&tsk->signal->live);
if (group_dead) {
struct cdata *tcd = &tsk->signal->cdata_threads;
@@ -1045,7 +1046,7 @@ NORET_TYPE void do_exit(long code)
audit_free(tsk);
tsk->exit_code = code;
- taskstats_exit(tsk, group_dead);
+ taskstats_exit_notify(tsk, group_dead);
exit_mm(tsk);
--- a/kernel/taskstats.c
+++ b/kernel/taskstats.c
@@ -263,24 +263,35 @@ out:
return rc;
}
-static void fill_tgid_exit(struct task_struct *tsk)
+void taskstats_exit_add_task(struct task_struct *tsk)
{
- unsigned long flags;
+ struct taskstats *stats = NULL;
- spin_lock_irqsave(&tsk->sighand->siglock, flags);
- if (!tsk->signal->stats)
- goto ret;
+ if (!tsk->signal->stats) {
+ if (thread_group_empty(tsk))
+ return;
+ stats = kmem_cache_zalloc(taskstats_cache, GFP_KERNEL);
+ if (!stats)
+ return; /* Bad luck, we will loose this task */
+ }
+
+ spin_lock_irq(&tsk->sighand->siglock);
+ if (!tsk->signal->stats) {
+ tsk->signal->stats = stats;
+ stats = NULL;
+ }
/*
* Each accounting subsystem calls its functions here to
* accumalate its per-task stats for tsk, into the per-tgid structure
*
- * per-task-foo(tsk->signal->stats, tsk);
+ * per-task-foo(tsk->signal->stats, tsk);
*/
delayacct_add_tsk(tsk->signal->stats, tsk);
-ret:
- spin_unlock_irqrestore(&tsk->sighand->siglock, flags);
- return;
+ spin_unlock_irq(&tsk->sighand->siglock);
+
+ if (unlikely(stats))
+ kmem_cache_free(taskstats_cache, stats);
}
static int add_del_listener(pid_t pid, const struct cpumask *mask, int isadd)
@@ -530,39 +541,14 @@ static int taskstats_user_cmd(struct sk_
return -EINVAL;
}
-static struct taskstats *taskstats_tgid_alloc(struct task_struct *tsk)
-{
- struct signal_struct *sig = tsk->signal;
- struct taskstats *stats;
-
- if (sig->stats || thread_group_empty(tsk))
- goto ret;
-
- /* No problem if kmem_cache_zalloc() fails */
- stats = kmem_cache_zalloc(taskstats_cache, GFP_KERNEL);
-
- spin_lock_irq(&tsk->sighand->siglock);
- if (!sig->stats) {
- sig->stats = stats;
- stats = NULL;
- }
- spin_unlock_irq(&tsk->sighand->siglock);
-
- if (stats)
- kmem_cache_free(taskstats_cache, stats);
-ret:
- return sig->stats;
-}
-
/* Send pid data out on exit */
-void taskstats_exit(struct task_struct *tsk, int group_dead)
+void taskstats_exit_notify(struct task_struct *tsk, int group_dead)
{
int rc;
struct listener_list *listeners;
struct taskstats *stats;
struct sk_buff *rep_skb;
size_t size;
- int is_thread_group;
if (!family_registered)
return;
@@ -573,13 +559,8 @@ void taskstats_exit(struct task_struct *
size = nla_total_size(sizeof(u32)) +
nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
- is_thread_group = !!taskstats_tgid_alloc(tsk);
- if (is_thread_group) {
- /* PID + STATS + TGID + STATS */
- size = 2 * size;
- /* fill the tsk->signal->stats structure */
- fill_tgid_exit(tsk);
- }
+ if (group_dead && tsk->signal->stats)
+ size = 2 * size; /* PID + STATS + TGID + STATS */
listeners = &__raw_get_cpu_var(listener_array);
if (list_empty(&listeners->list))
@@ -598,7 +579,7 @@ void taskstats_exit(struct task_struct *
/*
* Doesn't matter if tsk is the leader or the last group member leaving
*/
- if (!is_thread_group || !group_dead)
+ if (!group_dead || tsk->signal->stats == NULL)
goto send;
stats = mk_reply(rep_skb, TASKSTATS_TYPE_TGID, tsk->tgid);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists