Received-SPF: pass (google.com: domain of linux-kernel+bounces-170763-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <18342368-ce79-9820-26be-01834876fc9e@kylinos.cn>
Date: Tue, 7 May 2024 14:38:19 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] LoongArch: Update the flush cache policy
Content-Language: en-US
To: Xi Ruoyao <xry111@xry111.site>, chenhuacai@kernel.org, kernel@xen0n.name,
 lvjianmin@loongson.cn, dongbiao@loongson.cn, zhangbaoqi@loongson.cn
Cc: loongarch@lists.linux.dev, linux-kernel@vger.kernel.org
References: <20240506092419.4109941-1-lijun01@kylinos.cn>
 <8a8135eb0f1dc724cfbb4402dc6daf08db5b0bc7.camel@xry111.site>
 <cbb24599-8b40-cd27-6ce7-215476c0ddf4@kylinos.cn>
 <cbd6ed9d5be1d7112d69117a72e0cb0081f9b64b.camel@xry111.site>
 <8809f5a7-de6e-0794-feab-726c26f87344@kylinos.cn>
 <cf5df629b88fe38ae04cfa5714b9de2a44792704.camel@xry111.site>
From: lijun <lijun01@kylinos.cn>
In-Reply-To: <cf5df629b88fe38ae04cfa5714b9de2a44792704.camel@xry111.site>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

I guess, final the value of addr is not important, just all of addr must 
read once is very important,

so use two 'for()' and 'volatile' to flush all of addr  's cache, 
exactly as the name of the function is

"flush_cache_last_level".

在 2024/5/7 11:55, Xi Ruoyao 写道:
> On Tue, 2024-05-07 at 08:53 +0800, lijun wrote:
>> The value of addr changes very very quickly, and 'volatile' ensures that
>> every change can be read
> No, volatile has nothing to do with changing quickly or not.
>
> It's only useful when the compiler cannot know the change, for example
> it's changed by the hardware or another thread.
>
> And in the Linux kernel memory model for the hardware change you should
> use READ_ONCE/WRITE_ONCE instead (they are actually wrappers of volatile
> so in the kernel you should almost never need to directly use volatile),
> for the change from another thread using volatile is just wrong and you
> should use some atomic or locked operation instead.
>
> See
> https://www.kernel.org/doc/html/latest/process/volatile-considered-harmful.html.
>
> In this case I'd like to ask first: why won't a simple addr += cdesc-
>> linesz * cdesc->sets * cdesc->ways * 3 work?  Which value(s) of addr,
> cdesc, or cdesc->{linesz,sets,ways} may change w/o the compiler's
> knowledge?
>
>> 在 2024/5/6 18:17, Xi Ruoyao 写道:
>>> On Mon, 2024-05-06 at 18:08 +0800, lijun wrote:
>>>> volatile prevents compiler optimization by allowing the compiler
>>>>
>>>>     to reread the address value of addr every time
>>> But why is this ever needed?  What's wrong if the compiler optimizes
>>> it?
>>>
>>> If the problem is the compiler may optimize it to cdesc->ways * 3 *
>>> cdesc->sets * cdesc->linesz, unknowing cdesc->ways etc may magically
>>> change, you should use READ_ONCE(cdesc->ways) etc.
>>>
>>> I.e. use READ_ONCE on the expression which may magically change,
>>> instead
>>> of hacking addr.  addr won't magically change.
>>>
>>>> 在 2024/5/6 17:28, Xi Ruoyao 写道:
>>>>> On Mon, 2024-05-06 at 17:24 +0800, Li Jun wrote:
>>>>>> fix when LoongArch s3 resume, Can't find image information
>>>>>>
>>>>>> Signed-off-by: Li Jun <lijun01@kylinos.cn>
>>>>>> Signed-off-by: Baoqi Zhang <zhangbaoqi@loongson.cn>
>>>>>> Signed-off-by: Jianmin Lv <lvjianmin@loongson.cn>
>>>>>> Signed-off-by: Biao Dong <dongbiao@loongson.cn>
>>>>>> ---
>>>>>>     arch/loongarch/mm/cache.c | 24 +++++++++++++++++++++++-
>>>>>>     1 file changed, 23 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/loongarch/mm/cache.c
>>>>>> b/arch/loongarch/mm/cache.c
>>>>>> index 6be04d36ca07..52872fa0e5d8 100644
>>>>>> --- a/arch/loongarch/mm/cache.c
>>>>>> +++ b/arch/loongarch/mm/cache.c
>>>>>> @@ -63,6 +63,28 @@ static void flush_cache_leaf(unsigned int
>>>>>> leaf)
>>>>>>     	} while (--nr_nodes > 0);
>>>>>>     }
>>>>>>     
>>>>>> +static void flush_cache_last_level(unsigned int leaf)
>>>>>> +{
>>>>>> +	u64 addr;
>>>>>> +	int i, j, nr_nodes, way_size;
>>>>>> +	struct cache_desc *cdesc =
>>>>>> current_cpu_data.cache_leaves
>>>>>> +
>>>>>> leaf;
>>>>>> +
>>>>>> +	nr_nodes = loongson_sysconf.nr_nodes;
>>>>>> +
>>>>>> +	addr = CSR_DMW1_BASE;
>>>>>> +	iocsr_write32(0x1, 0x280);
>>>>>> +	way_size = cdesc->sets * cdesc->linesz;
>>>>>> +	do {
>>>>>> +		for (i = 0; i < (cdesc->ways * 3); i++) {
>>>>>> +			for (j = 0; j < (cdesc->sets); j++) {
>>>>>> +				*(volatile u32 *)addr;
>>>>> ??? what does this line do?
>>>>>