linux-kernel - 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.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AT5PR84MB0036A69B641B4291B72A5BE0CA4A0@AT5PR84MB0036.NAMPRD84.PROD.OUTLOOK.COM>
Date:	Thu, 19 May 2016 08:53:17 +0000
From:	"N, Soumya P" <soumya.p.n@....com>
To:	Steven Rostedt <rostedt@...dmis.org>
CC:	"mingo@...hat.com" <mingo@...hat.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.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.

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@...dmis.org] 
Sent: Thursday, May 19, 2016 1:25 AM
To: N, Soumya P <soumya.p.n@....com>
Cc: mingo@...hat.com; linux-kernel@...r.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@....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@....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]);