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

[Mathieu Desnoyers]

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