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.

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]

-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]);

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]

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]);

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]

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

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]

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

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]

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

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]

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

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]

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.

[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.

[Soumya PN]

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 <soumya.p.n@hpe.com>
---
 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

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.

[Maloy Ghosh]

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 <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
> 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 <soumya.p.n@hpe.com>
> ---
>  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)