Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 22 Apr 2022 18:59:22 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Jakob Koschel <jakobkoschel@gmail.com>
Cc:     Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org,
        Mike Rapoport <rppt@kernel.org>,
        "Brian Johannesmeyer" <bjohannesmeyer@gmail.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>
Subject: Re: [PATCH v2 1/4] tracing: Remove usage of list iterator after the
 loop body
Message-ID: <20220422185922.15fe4614@gandalf.local.home>
In-Reply-To: <20220402103341.1763932-2-jakobkoschel@gmail.com>
References: <20220402103341.1763932-1-jakobkoschel@gmail.com>
        <20220402103341.1763932-2-jakobkoschel@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat,  2 Apr 2022 12:33:38 +0200
Jakob Koschel <jakobkoschel@gmail.com> wrote:

> In preparation to limit the scope of the list iterator variable to the
> traversal loop, use a dedicated pointer to point to the found element
> [1].
> 
> Before, the code implicitly used the head when no element was found
> when using &pos->list. Since the new variable is only set if an
> element was found, the head needs to be used explicitly if the
> variable is NULL.
> 
> Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=EHreAsk5SqXPwr9Y7k9sA6cWXJ6w@mail.gmail.com/ [1]
> Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com>
> ---
>  kernel/trace/trace_output.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
> index 8aa493d25c73..733a4d6c20e2 100644
> --- a/kernel/trace/trace_output.c
> +++ b/kernel/trace/trace_output.c
> @@ -692,7 +692,7 @@ static LIST_HEAD(ftrace_event_list);
>  
>  static int trace_search_list(struct list_head **list)
>  {
> -	struct trace_event *e;
> +	struct trace_event *e = NULL, *iter;
>  	int next = __TRACE_LAST_TYPE;
>  
>  	if (list_empty(&ftrace_event_list)) {
> @@ -704,9 +704,11 @@ static int trace_search_list(struct list_head **list)
>  	 * We used up all possible max events,
>  	 * lets see if somebody freed one.
>  	 */
> -	list_for_each_entry(e, &ftrace_event_list, list) {
> -		if (e->type != next)
> +	list_for_each_entry(iter, &ftrace_event_list, list) {
> +		if (iter->type != next) {
> +			e = iter;
>  			break;
> +		}
>  		next++;
>  	}
>  
> @@ -714,7 +716,10 @@ static int trace_search_list(struct list_head **list)
>  	if (next > TRACE_EVENT_TYPE_MAX)
>  		return 0;
>  
> -	*list = &e->list;
> +	if (e)

Technically, if e was NULL, then next would be too big. But as that's very
subtle with the algorithm, I'm fine adding this.

-- Steve


> +		*list = &e->list;
> +	else
> +		*list = &ftrace_event_list;
>  	return next;
>  }
>