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 13:49:16 + "N, Soumya P" 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.
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 13:49:16 + "N, Soumya P" 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
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.
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:rost...@goodmis.org] Sent: Thursday, May 19, 2016 6:41 PM To: N, Soumya P Cc: mi...@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 + "N, Soumya P" 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:rost...@goodmis.org] > Sent: Thursday, May 19, 2016 1:25 AM > To: N, Soumya P > Cc: mi...@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
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 13:49:16 + "N, Soumya P" 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
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 + "N, Soumya P" 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:rost...@goodmis.org] > Sent: Thursday, May 19, 2016 1:25 AM > To: N, Soumya P > Cc: mi...@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
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.
Hi,
I guess (from the error code) this is to do with your patch not having
short one line subject before the detailed explanation of what your
patch does. The automated script might be treating your patch detailed
description as subject line. More can be found in the patch submitting
guideline.
Regards,
On Thu, May 19, 2016 at 6:10 PM, Soumya PN 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
> 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()")'
>
> Done basic testing by enabling function graph tracing in x86_64.
>
> Signed-off-by: Soumya PN
> ---
> kernel/trace/ftrace.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index b1870fb..45bc72f 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;
>
> @@ -5728,8 +5727,7 @@ static int alloc_retstack_tasklist(struct
> ftrace_ret_stack **ret_stack_list)
> goto free;
> }
> }
> -
> - read_lock_irqsave(&tasklist_lock, flags);
> + read_lock(&tasklist_lock);
> do_each_thread(g, t) {
> if (start == end) {
> ret = -EAGAIN;
> @@ -5747,7 +5745,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]);
> --
> 1.8.3.1
>
--
--
Maloy Ghosh
Research Engineer
Next Generation Network (NGN)
Center for Development of Telematics (CDoT)
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.
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:rost...@goodmis.org]
Sent: Thursday, May 19, 2016 1:25 AM
To: N, Soumya P
Cc: mi...@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 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
> ---
> 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]);
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 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
> ---
> 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]);
