Oleg Nesterov points out that we should check that the task is still alive
before we iterate over the threads. This patch includes a fixup for this.
Also simplify do_io_accounting() implementation.
Signed-off-by: Andrea Righi <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
fs/proc/base.c | 56 ++++++++++++++++++++++----------------------------------
1 files changed, 22 insertions(+), 34 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index a891fe4..346500d 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2381,35 +2381,18 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
u64 rchar, wchar, syscr, syscw;
struct task_io_accounting ioac;
- if (!whole) {
- rchar = task->rchar;
- wchar = task->wchar;
- syscr = task->syscr;
- syscw = task->syscw;
- memcpy(&ioac, &task->ioac, sizeof(ioac));
- } else {
- unsigned long flags;
- struct task_struct *t = task;
- rchar = wchar = syscr = syscw = 0;
- memset(&ioac, 0, sizeof(ioac));
+ rchar = task->rchar;
+ wchar = task->wchar;
+ syscr = task->syscr;
+ syscw = task->syscw;
+ memcpy(&ioac, &task->ioac, sizeof(ioac));
- rcu_read_lock();
- do {
- rchar += t->rchar;
- wchar += t->wchar;
- syscr += t->syscr;
- syscw += t->syscw;
-
- ioac.read_bytes += t->ioac.read_bytes;
- ioac.write_bytes += t->ioac.write_bytes;
- ioac.cancelled_write_bytes +=
- t->ioac.cancelled_write_bytes;
- t = next_thread(t);
- } while (t != task);
- rcu_read_unlock();
+ if (whole) {
+ unsigned long flags;
if (lock_task_sighand(task, &flags)) {
struct signal_struct *sig = task->signal;
+ struct task_struct *t = task;
rchar += sig->rchar;
wchar += sig->wchar;
@@ -2420,11 +2403,20 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
ioac.write_bytes += sig->ioac.write_bytes;
ioac.cancelled_write_bytes +=
sig->ioac.cancelled_write_bytes;
-
+ while_each_thread(task, t) {
+ rchar += t->rchar;
+ wchar += t->wchar;
+ syscr += t->syscr;
+ syscw += t->syscw;
+
+ ioac.read_bytes += t->ioac.read_bytes;
+ ioac.write_bytes += t->ioac.write_bytes;
+ ioac.cancelled_write_bytes +=
+ t->ioac.cancelled_write_bytes;
+ }
unlock_task_sighand(task, &flags);
}
}
-
return sprintf(buffer,
"rchar: %llu\n"
"wchar: %llu\n"
@@ -2433,13 +2425,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
"read_bytes: %llu\n"
"write_bytes: %llu\n"
"cancelled_write_bytes: %llu\n",
- (unsigned long long)rchar,
- (unsigned long long)wchar,
- (unsigned long long)syscr,
- (unsigned long long)syscw,
- (unsigned long long)ioac.read_bytes,
- (unsigned long long)ioac.write_bytes,
- (unsigned long long)ioac.cancelled_write_bytes);
+ rchar, wchar, syscr, syscw,
+ ioac.read_bytes, ioac.write_bytes,
+ ioac.cancelled_write_bytes);
}
static int proc_tid_io_accounting(struct task_struct *task, char *buffer)
--
1.5.4.3
On 07/26, Andrea Righi wrote:
>
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2381,35 +2381,18 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
> u64 rchar, wchar, syscr, syscw;
> struct task_io_accounting ioac;
>
> - if (!whole) {
> - rchar = task->rchar;
> - wchar = task->wchar;
> - syscr = task->syscr;
> - syscw = task->syscw;
> - memcpy(&ioac, &task->ioac, sizeof(ioac));
> - } else {
> - unsigned long flags;
> - struct task_struct *t = task;
> - rchar = wchar = syscr = syscw = 0;
> - memset(&ioac, 0, sizeof(ioac));
> + rchar = task->rchar;
> + wchar = task->wchar;
> + syscr = task->syscr;
> + syscw = task->syscw;
> + memcpy(&ioac, &task->ioac, sizeof(ioac));
>
> - rcu_read_lock();
> - do {
> - rchar += t->rchar;
> - wchar += t->wchar;
> - syscr += t->syscr;
> - syscw += t->syscw;
> -
> - ioac.read_bytes += t->ioac.read_bytes;
> - ioac.write_bytes += t->ioac.write_bytes;
> - ioac.cancelled_write_bytes +=
> - t->ioac.cancelled_write_bytes;
> - t = next_thread(t);
> - } while (t != task);
> - rcu_read_unlock();
> + if (whole) {
> + unsigned long flags;
>
> if (lock_task_sighand(task, &flags)) {
> struct signal_struct *sig = task->signal;
> + struct task_struct *t = task;
>
> rchar += sig->rchar;
> wchar += sig->wchar;
> @@ -2420,11 +2403,20 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
> ioac.write_bytes += sig->ioac.write_bytes;
> ioac.cancelled_write_bytes +=
> sig->ioac.cancelled_write_bytes;
> -
> + while_each_thread(task, t) {
> + rchar += t->rchar;
> + wchar += t->wchar;
> + syscr += t->syscr;
> + syscw += t->syscw;
> +
> + ioac.read_bytes += t->ioac.read_bytes;
> + ioac.write_bytes += t->ioac.write_bytes;
> + ioac.cancelled_write_bytes +=
> + t->ioac.cancelled_write_bytes;
> + }
> unlock_task_sighand(task, &flags);
> }
> }
> -
> return sprintf(buffer,
> "rchar: %llu\n"
> "wchar: %llu\n"
> @@ -2433,13 +2425,9 @@ static int do_io_accounting(struct task_struct *task, char *buffer, int whole)
> "read_bytes: %llu\n"
> "write_bytes: %llu\n"
> "cancelled_write_bytes: %llu\n",
> - (unsigned long long)rchar,
> - (unsigned long long)wchar,
> - (unsigned long long)syscr,
> - (unsigned long long)syscw,
> - (unsigned long long)ioac.read_bytes,
> - (unsigned long long)ioac.write_bytes,
> - (unsigned long long)ioac.cancelled_write_bytes);
> + rchar, wchar, syscr, syscw,
> + ioac.read_bytes, ioac.write_bytes,
> + ioac.cancelled_write_bytes);
> }
Looks good!
And thanks for correcting me, of course we should initialize t
before while_each_thread(task, t).
Oleg.