Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1403258 > unrolled thread
| Started by | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| First post | 2016-05-18 22:00 +0200 |
| Last post | 2016-05-21 20:50 +0200 |
| Articles | 7 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. Steven Rostedt <rostedt@goodmis.org> - 2016-05-18 22:00 +0200
RE: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. "N, Soumya P" <soumya.p.n@hpe.com> - 2016-05-19 11:30 +0200
Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. Steven Rostedt <rostedt@goodmis.org> - 2016-05-19 15:20 +0200
Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. Steven Rostedt <rostedt@goodmis.org> - 2016-05-19 16:00 +0200
RE: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. "N, Soumya P" <soumya.p.n@hpe.com> - 2016-05-19 16:30 +0200
Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. Steven Rostedt <rostedt@goodmis.org> - 2016-05-20 19:30 +0200
RE: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. "N, Soumya P" <soumya.p.n@hpe.com> - 2016-05-21 20:50 +0200
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-05-18 22:00 +0200 |
| Subject | Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. |
| Message-ID | <rAfNn-6pu-21@gated-at.bofh.it> |
-ERUNONSUBJECT
-- Steve
On Tue, 17 May 2016 21:31:14 +0530
Soumya PN <soumya.p.n@hpe.com> wrote:
> In ftrace.c inside the function alloc_retstack_tasklist()(which will be
> invoked when function_graph tracing is on) the tasklist_lock is being
> held as reader while iterating through list of threads. Here the lock
> is being held as reader with irqs disabled. The tasklist_lock is never
> write_locked in interrupt context so it is safe to not disable interrupts
> for the duration of read_lock in this block which, can be significant,
> given the block of code iterates through all threads. Hence changing the
> code to call read_lock() and read_unlock() instead of read_lock_irqsave()
> and read_unlock_irqrestore(). Seen the same change is made in
> "kernel/tracepoint.c" and "kernel/sched/core.c"files with commit ids
> 'commit 8063e41d2ffc ("tracing: Change syscall_*regfunc() to check
> PF_KTHREAD and use for_each_process_thread()")'
> 'commit 3472eaa1f12e ("sched: normalize_rt_tasks(): Don't use _irqsave
> for tasklist_lock, use task_rq_lock()")'
>
> Stopped the irqbalance service and bound the interrupts and processes
> being traced to the same cpu core with function_graph tracing on. I/Os
> completed successfully. Verified the trace output and seen the expected
> results.
>
> Signed-off-by: Soumya PN <soumya.p.n@hpe.com>
> ---
> kernel/trace/ftrace.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index b1870fb..a680482 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5713,7 +5713,6 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
> {
> int i;
> int ret = 0;
> - unsigned long flags;
> int start = 0, end = FTRACE_RETSTACK_ALLOC_SIZE;
> struct task_struct *g, *t;
>
> @@ -5729,7 +5728,7 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
> }
> }
>
> - read_lock_irqsave(&tasklist_lock, flags);
> + read_lock(&tasklist_lock);
> do_each_thread(g, t) {
> if (start == end) {
> ret = -EAGAIN;
> @@ -5747,7 +5746,7 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
> } while_each_thread(g, t);
>
> unlock:
> - read_unlock_irqrestore(&tasklist_lock, flags);
> + read_unlock(&tasklist_lock);
> free:
> for (i = start; i < end; i++)
> kfree(ret_stack_list[i]);
[toc] | [next] | [standalone]
| From | "N, Soumya P" <soumya.p.n@hpe.com> |
|---|---|
| Date | 2016-05-19 11:30 +0200 |
| Subject | RE: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. |
| Message-ID | <rAsrg-6ir-5@gated-at.bofh.it> |
| In reply to | #1403258 |
Hi Steve,
Could you please explain what this error means?
Is it related to length of subject?
I have run checkpatch.pl on patch and didn't show any error.
Thanks,
Soumya.
-----Original Message-----
From: Steven Rostedt [mailto:rostedt@goodmis.org]
Sent: Thursday, May 19, 2016 1:25 AM
To: N, Soumya P <soumya.p.n@hpe.com>
Cc: mingo@redhat.com; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts.
-ERUNONSUBJECT
-- Steve
On Tue, 17 May 2016 21:31:14 +0530
Soumya PN <soumya.p.n@hpe.com> wrote:
> In ftrace.c inside the function alloc_retstack_tasklist()(which will
> be invoked when function_graph tracing is on) the tasklist_lock is
> being held as reader while iterating through list of threads. Here the
> lock is being held as reader with irqs disabled. The tasklist_lock is
> never write_locked in interrupt context so it is safe to not disable
> interrupts for the duration of read_lock in this block which, can be
> significant, given the block of code iterates through all threads.
> Hence changing the code to call read_lock() and read_unlock() instead
> of read_lock_irqsave() and read_unlock_irqrestore(). Seen the same
> change is made in "kernel/tracepoint.c" and "kernel/sched/core.c"files
> with commit ids 'commit 8063e41d2ffc ("tracing: Change
> syscall_*regfunc() to check PF_KTHREAD and use for_each_process_thread()")'
> 'commit 3472eaa1f12e ("sched: normalize_rt_tasks(): Don't use _irqsave
> for tasklist_lock, use task_rq_lock()")'
>
> Stopped the irqbalance service and bound the interrupts and processes
> being traced to the same cpu core with function_graph tracing on. I/Os
> completed successfully. Verified the trace output and seen the
> expected results.
>
> Signed-off-by: Soumya PN <soumya.p.n@hpe.com>
> ---
> kernel/trace/ftrace.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index
> b1870fb..a680482 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -5713,7 +5713,6 @@ static int alloc_retstack_tasklist(struct
> ftrace_ret_stack **ret_stack_list) {
> int i;
> int ret = 0;
> - unsigned long flags;
> int start = 0, end = FTRACE_RETSTACK_ALLOC_SIZE;
> struct task_struct *g, *t;
>
> @@ -5729,7 +5728,7 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
> }
> }
>
> - read_lock_irqsave(&tasklist_lock, flags);
> + read_lock(&tasklist_lock);
> do_each_thread(g, t) {
> if (start == end) {
> ret = -EAGAIN;
> @@ -5747,7 +5746,7 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
> } while_each_thread(g, t);
>
> unlock:
> - read_unlock_irqrestore(&tasklist_lock, flags);
> + read_unlock(&tasklist_lock);
> free:
> for (i = start; i < end; i++)
> kfree(ret_stack_list[i]);
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-05-19 15:20 +0200 |
| Message-ID | <rAw1Q-gk-9@gated-at.bofh.it> |
| In reply to | #1403564 |
On Thu, 19 May 2016 08:53:17 +0000 "N, Soumya P" <soumya.p.n@hpe.com> wrote: > Hi Steve, > > Could you please explain what this error means? > Is it related to length of subject? > I have run checkpatch.pl on patch and didn't show any error. > > Thanks, > Soumya. > > -----Original Message----- > From: Steven Rostedt [mailto:rostedt@goodmis.org] > Sent: Thursday, May 19, 2016 1:25 AM > To: N, Soumya P <soumya.p.n@hpe.com> > Cc: mingo@redhat.com; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. > > > -ERUNONSUBJECT A subject (and this includes normal net etiquette as well) should be a short description of what the email (or patch) is about, preferably under 80 characters. What you supplied is an abstract. it should have been something as simple as: ftrace: Don't disable interrupts when taking read_lock() for graph tracer And don't rely on checkpatch.pl. Read Documentation/SubmittingPatches in the kernel proper. -- Steve
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-05-19 16:00 +0200 |
| Message-ID | <rAwEy-vI-43@gated-at.bofh.it> |
| In reply to | #1403731 |
On Thu, 19 May 2016 13:49:16 +0000 "N, Soumya P" <soumya.p.n@hpe.com> wrote: > Hi Steve, > > Thanks for the explanation. > I will take care of your comments and send v2 of the same patch. > I'll also note that in our forum (LKML), we do not do "top posting". We practice "inline replying" and "trimming". See https://en.wikipedia.org/wiki/Posting_style -- Steve
[toc] | [prev] | [next] | [standalone]
| From | "N, Soumya P" <soumya.p.n@hpe.com> |
|---|---|
| Date | 2016-05-19 16:30 +0200 |
| Subject | RE: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. |
| Message-ID | <rAwEy-vI-45@gated-at.bofh.it> |
| In reply to | #1403731 |
Hi Steve, Thanks for the explanation. I will take care of your comments and send v2 of the same patch. Thanks, Soumya. -----Original Message----- From: Steven Rostedt [mailto:rostedt@goodmis.org] Sent: Thursday, May 19, 2016 6:41 PM To: N, Soumya P <soumya.p.n@hpe.com> Cc: mingo@redhat.com; linux-kernel@vger.kernel.org Subject: Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. On Thu, 19 May 2016 08:53:17 +0000 "N, Soumya P" <soumya.p.n@hpe.com> wrote: > Hi Steve, > > Could you please explain what this error means? > Is it related to length of subject? > I have run checkpatch.pl on patch and didn't show any error. > > Thanks, > Soumya. > > -----Original Message----- > From: Steven Rostedt [mailto:rostedt@goodmis.org] > Sent: Thursday, May 19, 2016 1:25 AM > To: N, Soumya P <soumya.p.n@hpe.com> > Cc: mingo@redhat.com; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. > > > -ERUNONSUBJECT A subject (and this includes normal net etiquette as well) should be a short description of what the email (or patch) is about, preferably under 80 characters. What you supplied is an abstract. it should have been something as simple as: ftrace: Don't disable interrupts when taking read_lock() for graph tracer And don't rely on checkpatch.pl. Read Documentation/SubmittingPatches in the kernel proper. -- Steve
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2016-05-20 19:30 +0200 |
| Message-ID | <rAWpj-8u4-15@gated-at.bofh.it> |
| In reply to | #1403780 |
On Thu, 19 May 2016 13:49:16 +0000 "N, Soumya P" <soumya.p.n@hpe.com> wrote: > Hi Steve, > > Thanks for the explanation. > I will take care of your comments and send v2 of the same patch. > No need, I just pulled in your patch and made the updates to the change log and subject myself. I'm starting my tests on it now. -- Steve
[toc] | [prev] | [next] | [standalone]
| From | "N, Soumya P" <soumya.p.n@hpe.com> |
|---|---|
| Date | 2016-05-21 20:50 +0200 |
| Subject | RE: [PATCH] ftrace: As disabling interrupts is costly and write_lock variant of tasklist_lock is not held from interrupt context it is not necessary to disable interrupts. |
| Message-ID | <rBk8i-74Y-25@gated-at.bofh.it> |
| In reply to | #1404633 |
On Thu, 19 May 2016 13:49:16 +0000 "N, Soumya P" <soumya.p.n@hpe.com> wrote: > Hi Steve, > > Thanks for the explanation. > I will take care of your comments and send v2 of the same patch. > >>No need, I just pulled in your patch and made the updates to the change log and subject myself. I'm starting my tests on it now. > Thanks Steve.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web