Re: [RFC PATCH 0/7] Introduce cache_is_aliasing() to fix DAX regression

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2024-01-29 16:22, Dan Williams wrote:
> Mathieu Desnoyers wrote:
>> This commit introduced in v5.13 prevents building FS_DAX on 32-bit ARM,
>> even on ARMv7 which does not have virtually aliased dcaches:
>>
>> commit d92576f1167c ("dax: does not work correctly with virtual aliasing caches")
>>
>> It used to work fine before: I have customers using dax over pmem on
>> ARMv7, but this regression will likely prevent them from upgrading their
>> kernel.
>>
>> The root of the issue here is the fact that DAX was never designed to
>> handle virtually aliased dcache (VIVT and VIPT with aliased dcache). It
>> touches the pages through their linear mapping, which is not consistent
>> with the userspace mappings on virtually aliased dcaches.
>>
>> This patch series introduces cache_is_aliasing() with new Kconfig
>> options:
>>
>>    * ARCH_HAS_CACHE_ALIASING
>>    * ARCH_HAS_CACHE_ALIASING_DYNAMIC
>>
>> and implements it for all architectures. The "DYNAMIC" implementation
>> implements cache_is_aliasing() as a runtime check, which is what is
>> needed on architectures like 32-bit ARMV6 and ARMV6K.
>>
>> With this we can basically narrow down the list of architectures which
>> are unsupported by DAX to those which are really affected.
>>
>> Feedback is welcome,
> 
> Hi Mathieu, this looks good overall, just some quibbling about the
> ordering.

Thanks for having a look !

> 
> I would introduce dax_is_supported() with the current overly broad
> interpretation of "!(ARM || MIPS || SPARC)" using IS_ENABLED(), then
> fixup the filesystems to use the new helper, and finally go back and
> convert dax_is_supported() to use cache_is_aliasing() internally.

Will do.

> 
> Separately, it is not clear to me why ARCH_HAS_CACHE_ALIASING_DYNAMIC
> needs to exist. As long as all paths that care are calling
> cache_is_aliasing() then whether it is dynamic or not is something only
> the compiler cares about. If those dynamic archs do not want to pay the
> .text size increase they can always do CONFIG_FS_DAX=n, right?

Good point. It will help reduce complexity and improve test coverage.

I also intend to rename "cache_is_aliasing()" to "dcache_is_aliasing()",
so if we introduce an "icache_is_aliasing()" in the future, it won't be
confusing. Having aliasing icache-dcache but not dcache-dcache seems to
be fairly common.

So basically:

If an arch selects ARCH_HAS_CACHE_ALIASING, it implements
dcache_is_aliasing() (for now), and eventually we can implement
icache_is_aliasing() as well.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com