Re: [PATCH v2] arch/cacheflush: Introduce flush_all_caches()

From: Davidlohr Bueso <dave@stgolabs.net>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Christoph Hellwig <hch@infradead.org>,
	linux-arch@vger.kernel.org, peterz@infradead.org,
	mark.rutland@arm.com, dave.jiang@intel.com,
	Jonathan.Cameron@huawei.com, a.manzanares@samsung.com,
	bwidawsk@kernel.org, alison.schofield@intel.com,
	ira.weiny@intel.com, linux-cxl@vger.kernel.org,
	nvdimm@lists.linux.dev, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] arch/cacheflush: Introduce flush_all_caches()
Date: Tue, 23 Aug 2022 08:37:37 -0700	[thread overview]
Message-ID: <20220823153737.7p7lpkqsu4otraxh@offworld> (raw)
In-Reply-To: <6303c7f4bb650_1b322947f@dwillia2-xfh.jf.intel.com.notmuch>

On Mon, 22 Aug 2022, Dan Williams wrote:

>Davidlohr Bueso wrote:
>> On Sun, 21 Aug 2022, Christoph Hellwig wrote:
>>
>> >On Fri, Aug 19, 2022 at 10:10:24AM -0700, Davidlohr Bueso wrote:
>> >> index b192d917a6d0..ac4d4fd4e508 100644
>> >> --- a/arch/x86/include/asm/cacheflush.h
>> >> +++ b/arch/x86/include/asm/cacheflush.h
>> >> @@ -10,4 +10,8 @@
>> >>
>> >>  void clflush_cache_range(void *addr, unsigned int size);
>> >>
>> >> +/* see comments in the stub version */
>> >> +#define flush_all_caches() \
>> >> +	do { wbinvd_on_all_cpus(); } while(0)
>> >
>> >Yikes.  This is just a horrible, horrible name and placement for a bad
>> >hack that should have no generic relevance.
>>
>> Why does this have no generic relevance? There's already been discussions
>> on how much wbinv is hated[0].
>>
>> >Please fix up the naming to make it clear that this function is for a
>> >very specific nvdimm use case, and move it to a nvdimm-specific header
>> >file.
>>
>> Do you have any suggestions for a name? And, as the changelog describes,
>> this is not nvdimm specific anymore, and the whole point of all this is
>> volatile memory components for cxl, hence nvdimm namespace is bogus.
>>
>> [0] https://lore.kernel.org/all/Yvtc2u1J%2Fqip8za9@worktop.programming.kicks-ass.net/
>
>While it is not nvdimm specific anymore, it's still specific to "memory
>devices that can bulk invalidate a physical address space". I.e. it's
>not as generic as its location in arch/x86/include/asm/cacheflush.h
>would imply. So, similar to arch_invalidate_pmem(), lets keep it in a
>device-driver-specific header file, because hch and peterz are right, we
>need to make this much more clear that it is not for general
>consumption.

Fine, I won't argue - although I don't particularly agree, at least wrt
the naming. Imo my naming does _exactly_ what it should do and is much
easier to read than arch_has_flush_memregion() which is counter intuitive
when we are in fact flushing everything. This does not either make it
any more clearer about virt vs physical mappings either (except that
it's no longer associated to cacheflush). But, excepting arm cacheflush.h's
rare arch with braino cache users get way too much credit in their namespace
usage.

But yes there is no doubt that my version is more inviting than it should be,
which made me think of naming it to flush_all_caches_careful() so the user
is forced to at least check the function (or one would hope).

Anyway, I'll send a new version based on the below - I particularly agree
with the hypervisor bits.

Thanks,
Davidlohr

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel