linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linux-kernel@vger.kernel.org,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Ingo Molnar <mingo@kernel.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Jiri Kosina <jikos@kernel.org>, Miroslav Benes <mbenes@suse.cz>,
	Petr Mladek <pmladek@suse.com>
Subject: Re: [PATCH 03/11 v3] ftrace: Optimize testing what context current is in
Date: Mon, 9 Nov 2020 13:17:08 +0100	[thread overview]
Message-ID: <20201109121708.GK2594@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20201106023546.558881845@goodmis.org>

On Thu, Nov 05, 2020 at 09:32:38PM -0500, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> The preempt_count() is not a simple location in memory, it could be part of
> per_cpu code or more. Each access to preempt_count(), or one of its accessor
> functions (like in_interrupt()) takes several cycles. By reading
> preempt_count() once, and then doing tests to find the context against the
> value return is slightly faster than using in_nmi() and in_interrupt().
> 
> Link: https://lkml.kernel.org/r/20201028115612.780796355@goodmis.org
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>  include/linux/trace_recursion.h | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/trace_recursion.h b/include/linux/trace_recursion.h
> index f2a949dbfec7..ac3d73484cb2 100644
> --- a/include/linux/trace_recursion.h
> +++ b/include/linux/trace_recursion.h
> @@ -117,22 +117,29 @@ enum {
>  
>  #define TRACE_CONTEXT_MASK	TRACE_LIST_MAX
>  
> +/*
> + * Used for setting context
> + *  NMI     = 0
> + *  IRQ     = 1
> + *  SOFTIRQ = 2
> + *  NORMAL  = 3
> + */
> +enum {
> +	TRACE_CTX_NMI,
> +	TRACE_CTX_IRQ,
> +	TRACE_CTX_SOFTIRQ,
> +	TRACE_CTX_NORMAL,
> +};
> +
>  static __always_inline int trace_get_context_bit(void)
>  {
> -	int bit;
> -
> -	if (in_interrupt()) {
> -		if (in_nmi())
> -			bit = 0;
> -
> -		else if (in_irq())
> -			bit = 1;
> -		else
> -			bit = 2;
> -	} else
> -		bit = 3;
> +	unsigned long pc = preempt_count();
>  
> -	return bit;
> +	if (!(pc & (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET)))
> +		return TRACE_CTX_NORMAL;
> +	else
> +		return pc & NMI_MASK ? TRACE_CTX_NMI :
> +			pc & HARDIRQ_MASK ? TRACE_CTX_IRQ : TRACE_CTX_SOFTIRQ;
>  }

This patch is misleading, it doesn't optimize it nearly as much as is
possible and actually fixes the softirq case, which isn't at all
mentioned.

Let me go do that other patch.

  reply	other threads:[~2020-11-09 12:17 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-06  2:32 [PATCH 00/11 v3] ftrace: Have callbacks handle their own recursion Steven Rostedt
2020-11-06  2:32 ` [PATCH 01/11 v3] ftrace: Move the recursion testing into global headers Steven Rostedt
2020-11-06  2:32 ` [PATCH 02/11 v3] ftrace: Add ftrace_test_recursion_trylock() helper function Steven Rostedt
2020-11-06  2:32 ` [PATCH 03/11 v3] ftrace: Optimize testing what context current is in Steven Rostedt
2020-11-09 12:17   ` Peter Zijlstra [this message]
2020-11-09 16:51     ` Steven Rostedt
2020-11-06  2:32 ` [PATCH 04/11 v3] pstore/ftrace: Add recursion protection to the ftrace callback Steven Rostedt
2020-11-06  2:32 ` [PATCH 05/11 v3] kprobes/ftrace: " Steven Rostedt
2020-11-06 10:25   ` Masami Hiramatsu
2020-11-06  2:32 ` [PATCH 06/11 v3] livepatch/ftrace: " Steven Rostedt
2020-11-06 10:05   ` Miroslav Benes
2020-11-06  2:32 ` [PATCH 07/11 v3] livepatch: Trigger WARNING if livepatch function fails due to recursion Steven Rostedt
2020-11-06 10:07   ` Miroslav Benes
2020-11-06  2:32 ` [PATCH 08/11 v3] perf/ftrace: Add recursion protection to the ftrace callback Steven Rostedt
2020-11-06  2:32 ` [PATCH 09/11 v3] perf/ftrace: Check for rcu_is_watching() in callback function Steven Rostedt
2020-11-06  2:32 ` [PATCH 10/11 v3] ftrace: Reverse what the RECURSION flag means in the ftrace_ops Steven Rostedt
2020-11-06  2:32 ` [PATCH 11/11 v3] ftrace: Add recording of functions that caused recursion Steven Rostedt
2020-11-06 13:13   ` Petr Mladek
2020-11-06 13:41     ` Steven Rostedt
2020-11-06 14:27       ` Petr Mladek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201109121708.GK2594@hirez.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=jikos@kernel.org \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mbenes@suse.cz \
    --cc=mhiramat@kernel.org \
    --cc=mingo@kernel.org \
    --cc=pmladek@suse.com \
    --cc=rostedt@goodmis.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).