Message-ID: <c817ca87-1961-ffc2-cc40-8dc4932e0afc@huawei.com>
Date:   Thu, 9 Mar 2023 14:47:24 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.0
Subject: Re: [PATCH] ftrace: Add ftrace_page to list after the index is
 calculated
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     <linux-trace-kernel@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <mhiramat@kernel.org>,
        <mark.rutland@arm.com>
References: <20230308070844.58180-1-chenzhongjin@huawei.com>
 <20230308095336.67f9368a@gandalf.local.home>
From:   Chen Zhongjin <chenzhongjin@huawei.com>
In-Reply-To: <20230308095336.67f9368a@gandalf.local.home>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2023/3/8 22:53, Steven Rostedt wrote:
>
>> -	} else {
>> -		if (!ftrace_pages)
>> -			goto out;
>> -
>> -		if (WARN_ON(ftrace_pages->next)) {
>> -			/* Hmm, we have free pages? */
>> -			while (ftrace_pages->next)
>> -				ftrace_pages = ftrace_pages->next;
>> -		}
>> -
>> -		ftrace_pages->next = start_pg;
> Basically, what you are saying is that once we add ftrace_pages->next to
> point to the new start_pg, it becomes visible to others and that could be a
> problem. And moving this code around is not really going to solve that, as
> then we would need to add memory barriers.
>
>> -	}
>> -
>>   	p = start;
>>   	pg = start_pg;
>>   	while (p < end) {
>> @@ -6855,6 +6841,21 @@ static int ftrace_process_locs(struct module *mod,
>>   	/* We should have used all pages */
>>   	WARN_ON(pg->next);
>>   
>> +	/* Add pages to ftrace_pages list */
>> +	if (!mod) {
>> +		WARN_ON(ftrace_pages || ftrace_pages_start);
>> +		/* First initialization */
>> +		ftrace_pages_start = start_pg;
>> +	} else {
>> +		if (WARN_ON(ftrace_pages->next)) {
>> +			/* Hmm, we have free pages? */
>> +			while (ftrace_pages->next)
>> +				ftrace_pages = ftrace_pages->next;
>> +		}
>> +
>> +		ftrace_pages->next = start_pg;
>> +	}
>> +
>>   	/* Assign the last page to ftrace_pages */
>>   	ftrace_pages = pg;
>>   
> Why not just test for it?
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 29baa97d0d53..9b2803c7a18f 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1564,7 +1564,8 @@ static struct dyn_ftrace *lookup_rec(unsigned long start, unsigned long end)
>   	key.flags = end;	/* overload flags, as it is unsigned long */
>   
>   	for (pg = ftrace_pages_start; pg; pg = pg->next) {
> -		if (end < pg->records[0].ip ||
> +		if (pg->index == 0 ||
> +		    end < pg->records[0].ip ||
>   		    start >= (pg->records[pg->index - 1].ip + MCOUNT_INSN_SIZE))
>   			continue;
>   		rec = bsearch(&key, pg->records, pg->index,
>
>
> -- Steve
Thanks for review!

At first I'm worried that it will cause lookup_rec to return an 
incorrect result
if it issearching a module address going to be added.
But now I found that records are added at MODULE_STATE_UNFORMED, the module
address won't  be searched at this point in any case.

Let's do it this way. I'll send another patch.