On Tue, May 22 2018 at 3:27pm -0400, Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > On Tue, May 22, 2018 at 12:19 PM, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > > On Tue, May 22 2018 at 3:00pm -0400, > > Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > > > >> On Tue, May 22, 2018 at 11:41 AM, Mike Snitzer <snitzer@xxxxxxxxxx> wrote: > >> > On Tue, May 22 2018 at 2:39am -0400, > >> > Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > >> > > >> >> On Sat, May 19, 2018 at 07:25:07AM +0200, Mikulas Patocka wrote: > >> >> > Use new API for flushing persistent memory. > >> >> > >> >> The sentence doesnt make much sense. 'A new API', 'A better > >> >> abstraction' maybe? > >> >> > >> >> > > >> >> > The problem is this: > >> >> > * on X86-64, non-temporal stores have the best performance > >> >> > * ARM64 doesn't have non-temporal stores, so we must flush cache. We > >> >> > should flush cache as late as possible, because it performs better this > >> >> > way. > >> >> > > >> >> > We introduce functions pmem_memcpy, pmem_flush and pmem_commit. To commit > >> >> > data persistently, all three functions must be called. > >> >> > > >> >> > The macro pmem_assign may be used instead of pmem_memcpy. pmem_assign > >> >> > (unlike pmem_memcpy) guarantees that 8-byte values are written atomically. > >> >> > > >> >> > On X86, pmem_memcpy is memcpy_flushcache, pmem_flush is empty and > >> >> > pmem_commit is wmb. > >> >> > > >> >> > On ARM64, pmem_memcpy is memcpy, pmem_flush is arch_wb_cache_pmem and > >> >> > pmem_commit is empty. > >> >> > >> >> All these should be provided by the pmem layer, and be properly > >> >> documented. And be sorted before adding your new target that uses > >> >> them. > >> > > >> > I don't see that as a hard requirement. Mikulas did the work to figure > >> > out what is more optimal on x86_64 vs amd64. It makes a difference for > >> > his target and that is sufficient to carry it locally until/when it is > >> > either elevated to pmem. > >> > > >> > We cannot even get x86 and swait maintainers to reply to repeat requests > >> > for review. Stacking up further deps on pmem isn't high on my list. > >> > > >> > >> Except I'm being responsive. > > > > Except you're looking to immediately punt to linux-arm-kernel ;) > > Well, I'm not, not really. I'm saying drop ARM support, it's not ready. > > > > >> I agree with Christoph that we should > >> build pmem helpers at an architecture level and not per-driver. Let's > >> make this driver depend on ARCH_HAS_PMEM_API and require ARM to catch > >> up to x86 in this space. We already have PowerPC enabling PMEM API, so > >> I don't see an unreasonable barrier to ask the same of ARM. This patch > >> is not even cc'd to linux-arm-kernel. Has the subject been broached > >> with them? > > > > No idea. Not by me. > > > > The thing is, I'm no expert in pmem. You are. Coordinating the change > > with ARM et al feels unnecessarily limiting and quicky moves outside my > > control. > > > > Serious question: Why can't this code land in this dm-writecache target > > and then be lifted (or obsoleted)? > > Because we already have an API, and we don't want to promote local > solutions to global problems, or carry unnecessary technical debt. > > > > > But if you think it worthwhile to force ARM to step up then fine. That > > does limit the availability of using writecache on ARM while they get > > the PMEM API together. > > > > I'll do whatever you want.. just put the smack down and tell me how it > > is ;) > > I'd say just control the variables you can control. Drop the ARM > support if you want to move forward and propose extensions / updates > to the pmem api for x86 and I'll help push those since I was involved > in pushing the x86 pmem api in the first instance. That way you don't > need to touch this driver as new archs add their pmem api enabling. Looking at Mikulas' wrapper API that you and hch are calling into question: For ARM it is using arch/arm64/mm/flush.c:arch_wb_cache_pmem(). (And ARM does seem to be providing CONFIG_ARCH_HAS_PMEM_API.) Whereas x86_64 is using memcpy_flushcache() as provided by CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE. (Yet ARM does provide arch/arm64/lib/uaccess_flushcache.c:memcpy_flushcache) Just seems this isn't purely about ARM lacking on an API level (given on x86_64 Mikulas isn't only using CONFIG_ARCH_HAS_PMEM_API). Seems this is more to do with x86_64 having efficient Non-temporal stores? Anyway, I'm still trying to appreciate the details here before I can make any forward progress. Mike -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
