Message-Id: <ca4c18b1-da35-4b9a-9eef-bbdab19c9592@app.fastmail.com>
Date: Sat, 04 Jan 2025 11:33:52 +0000
From: "Jiaxun Yang" <jiaxun.yang@...goat.com>
To: "Xi Ruoyao" <xry111@...111.site>, "Huacai Chen" <chenhuacai@...nel.org>,
 "Xuerui Wang" <kernel@...0n.name>
Cc: "Arnd Bergmann" <arnd@...db.de>, loongarch@...ts.linux.dev,
 linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org
Subject: Re: [PATCH 2/3] loongarch: Introduce sys_loongarch_flush_icache syscall



在2025年1月4日一月 上午9:31，Xi Ruoyao写道：
> On Thu, 2025-01-02 at 18:34 +0000, Jiaxun Yang wrote:
>
> /* snip */
>
>> Sadly many user space applications are assuming ICACHET support, we can't
>> recall those binaries. So we'd better get UAPI for cacheflush ready soonish
>> and encourage application to start using it.
>
> To encourage the developers changing ibar to loongarch_flush_icache, we
> should minimize the extra overhead on mainstream systems.  We can add an
> vDSO layer so if the CPU has ICACHET:

I'm a little bit confused as that's exactly what I'm doing in PATCH 3.

>
> int vdso_loongarch_flush_icache(...)
> {
>   asm ("ibar 0");
>   return 0;
> }
>
> And otherwise the vDSO wrapper invokes the real syscall.  I've
> implemented the boot-time alternative runtime patching for vDSO at
> https://lore.kernel.org/loongarch/20240816110717.10249-3-xry111@xry111.site/.

Thanks! Noted.

>
>> The syscall resolves to a ibar for now, it should be revised when we have
>> actual non-ICACHET support in kernel.
>
> /* snip */
>
>> diff --git a/arch/loongarch/include/asm/cacheflush.h b/arch/loongarch/include/asm/cacheflush.h
>> index f8754d08a31ab07490717c31b9253871668b9a76..94f4a47f00860977db0b360965a22ff0a461c098 100644
>> --- a/arch/loongarch/include/asm/cacheflush.h
>> +++ b/arch/loongarch/include/asm/cacheflush.h
>> @@ -80,6 +80,12 @@ static inline void flush_cache_line(int leaf, unsigned long addr)
>>  	}
>>  }
>>  
>> +/*
>> + * Bits in sys_loongarch_flush_icache()'s flags argument.
>> + */
>> +#define SYS_LOONGARCH_FLUSH_ICACHE_LOCAL 1UL
>> +#define SYS_LOONGARCH_FLUSH_ICACHE_ALL   (SYS_LOONGARCH_FLUSH_ICACHE_LOCAL)
>
> Not a UAPI header so not usable by the user?  How would they specify
> flags then?

We are following the RISC-V's convention on not exposing flags in UAPI
header for now as it's not really ready.

>
> If you meant to add them for UAPI, it would be very problematic.  When a
> new cache type emerges in the hardware implementations, we need to grow
> SYS_LOONGARCH_FLUSH_ICACHE_ALL in the UAPI header, but we cannot change
> the already compiled JIT applications.  Thus all JIT applications have
> to be recompiled with the latest UAPI header.  This just seems an
> unnecessary severe burden to the packagers.

The _LOCAL flag not meant to be hardware cache level but the scope.
(i.e. all threads or just the caller). Vast majority of applications
shouldn't need this level of granularity, so just setting flags to zero.

However, for application want fine-grained optimisations they should
probe availability of flags before using it. Thus kernel should reject
all unknown flags to assist application probing.

>
> Instead IMO it's better not to expose so much details to the userspace.
> Just remove the flags argument and flush all the icaches the kernel
> knows, so with a new cache type the user (and distro) just need to
> update or patch their kernel, w/o recompiling all JIT apps.

There is no need to change anything in user space usage when a new cache
type emerge. See explanations above.

Thanks
>
> -- 
> Xi Ruoyao <xry111@...111.site>
> School of Aerospace Science and Technology, Xidian University

-- 
- Jiaxun

Powered by blists - more mailing lists