On Mon, 20 Jul 2026 15:43:34 -0700, [email protected] wrote: > AI review might have found some things, one "critical": > > https://sashiko.dev/#/patchset/[email protected] > > --- Patch [6]: [PATCH v7 6/9] string: introduce memcpy_nontemporal() --- > commit 48cf815dd31950a95bb4752e1e8e62594a090364 > Author: Li Zhe <[email protected]> > string: introduce memcpy_nontemporal() > Introduce memcpy_nontemporal() for write-once copy sites that want a > named non-temporal copy primitive. > > diff --git a/arch/x86/include/asm/string_64.h > > b/arch/x86/include/asm/string_64.h > > index 4635616863f53..21ae515ae35a3 100644 > > --- a/arch/x86/include/asm/string_64.h > > +++ b/arch/x86/include/asm/string_64.h > > @@ -100,6 +100,18 @@ static __always_inline void memcpy_flushcache(void > > *dst, const void *src, size_t > > } > > __memcpy_flushcache(dst, src, cnt); > > } > > + > > +#define memcpy_nontemporal memcpy_nontemporal > > +/* > > + * Reuse the existing x86 flushcache backend as the non-temporal copy > > + * primitive. > > + */ > > +static __always_inline void memcpy_nontemporal(void *dst, const void *src, > > + size_t cnt) > > +{ > > + memcpy_flushcache(dst, src, cnt); > > +} > Does this missing fencing introduce a memory ordering hazard on x86_64? > Because memcpy_flushcache() uses weakly-ordered movnti instructions, it > bypasses the x86 Total Store Order (TSO) guarantees. If generic code > follows memcpy_nontemporal() with smp_store_release() or smp_wmb(), it > could result in data corruption since those standard SMP barriers only > compile to compiler barriers on x86_64. > Is there a plan to add a built-in sfence here, or update the > documentation to warn callers that wmb() is strictly required?
On x86 it intentionally follows the existing memcpy_flushcache() style: it performs the non-temporal copy, but it does not imply a drain or a publication barrier. Callers that use memcpy_nontemporal() as part of a producer-consumer or device-visible handoff must add the required ordering at the handoff point. On x86, that means a real write barrier such as wmb() or pmem_wmb(); smp_wmb() and smp_store_release() are not sufficient for ordering non-temporal stores before such a handoff. This patchset does not need such a drain in the ZONE_DEVICE template-copy path. The copy is followed by more local struct page initialization on the same CPU, not by a publication store to another CPU or device. > Will this bypass KASAN memory safety instrumentation and FORTIFY_SOURCE > bounds checking on x86_64? > The inline wrapper doesn't use __FORTIFY_INLINE, and calling > memcpy_flushcache() directly skips the usual kasan_check_write() and > kasan_check_read() protections that the generic memcpy macro preserves. On x86, memcpy_nontemporal() is only a thin wrapper around the existing memcpy_flushcache() backend. I would prefer to keep the wrapper at that level and not add a separate FORTIFY_SOURCE layer here, since the backend it wraps does not provide one either. For this series, bypassing KASAN/KMSAN instrumentation in the final ZONE_DEVICE template-copy path is expected and is called out in the final cleanup patch. > > > diff --git a/include/linux/string.h b/include/linux/string.h > > index 5702daca4326b..9815c08867d72 100644 > > --- a/include/linux/string.h > > +++ b/include/linux/string.h > [ ... ] > > @@ -278,6 +278,18 @@ static inline void memcpy_flushcache(void *dst, const > > void *src, size_t cnt) > > } > > #endif > > > > +#ifndef memcpy_nontemporal > > +/* > > + * memcpy_nontemporal() requests a non-temporal copy when the > > + * architecture has a suitable backend. Architectures without a > > + * specialized backend fall back to memcpy(). Keep this as a macro alias > > + * so the compiler can still see the original memcpy() call site and > > + * preserve the usual FORTIFY coverage when object sizes remain visible > > + * there. > > + */ > > +#define memcpy_nontemporal memcpy > > +#endif > Could this macro definition cause build failures due to a return type > mismatch across architectures? > The generic fallback here aliases to memcpy, which implicitly returns > void *, while the x86_64 backend explicitly returns void. If a caller > attempts to use the return value on x86_64, the build will fail with a > void value error, whereas it will compile cleanly on fallback > architectures. > Should this macro cast the result to void to enforce a uniform API? > For example: #define memcpy_nontemporal(d, s, l) ((void)memcpy(d, s, l)) Agreed. The return value should not be part of the memcpy_nontemporal() API. I will make the generic fallback a void function-like macro. This keeps the API consistent with memcpy_flushcache(), which also returns void, and makes accidental use of a return value fail consistently across architectures. > --- Patch [7]: [PATCH v7 7/9] x86/string: extend memcpy_flushcache() > fixed-size fastpaths --- > commit 882d29aec39f30f4e6b01548ec298e08ca1ab787 > Author: Li Zhe <[email protected]> > x86/string: extend memcpy_flushcache() fixed-size fastpaths > This commit adds larger fixed-size fast paths (32 to 96 bytes) to > memcpy_flushcache() using inline movnti helpers. It also explicitly removes > the 8-byte destination alignment check for these sizes based on microbenchmark > results showing improved performance for unaligned copies. > > diff --git a/arch/x86/include/asm/string_64.h > > b/arch/x86/include/asm/string_64.h > > --- a/arch/x86/include/asm/string_64.h > > +++ b/arch/x86/include/asm/string_64.h > [ ... ] > > +static __always_inline void movnti_8(void *dst, const void *src) > > +{ > > + asm volatile("movntiq %1, %0" > > + : "=m"(*(u64 *)dst) > > + : "r"(*(const u64 *)src) > Does casting an unaligned void pointer to a strictly-aligned u64 pointer and > dereferencing it invoke undefined behavior here? > Since this fast path handles unaligned addresses, the compiler might assume > the > memory operand is naturally 8-byte aligned based on the u64 type. Could this > allow the compiler to incorrectly optimize away subsequent alignment checks or > emit invalid aligned optimizations? I do not see an 8-byte alignment requirement for MOVNTI in the Intel SDM. This also matches the existing memcpy_flushcache() fixed-size cases: the 8-byte and 16-byte cases already use MOVNTI without a separate alignment check. So I do not think this series needs to add an alignment fallback for the new 32/48/64/80/96-byte fixed-size cases. Please correct me if I am wrong. > > + : "memory"); > Is this global "memory" clobber necessary given the specific "=m" output > constraint? > When this helper is nested to create larger copies like movnti_64(), it > generates up to 8 consecutive full compiler memory barriers. Does this force > the compiler to unnecessarily spill and reload locally cached variables from > registers, preventing instruction scheduling across each 8-byte chunk? The "memory" clobber is intentional here. The goal is to keep the fixed-size copy as a compact sequence of MOVNTI stores and avoid the compiler moving other memory accesses into the middle of that streaming store sequence. The "=m" operand describes the destination memory operand, but it does not express that broader scheduling constraint. The largest fixed-size copy added by this patch is still small, and the measured result for the final memcpy_flushcache() path is still favorable for the ZONE_DEVICE struct page copies that motivated this series. So I would prefer to keep the conservative clobber in this version unless there is a demonstrated code generation or performance regression from it. Please correct me if I am wrong. > > +} > [ ... ] > > +static __always_inline void memcpy_flushcache(void *dst, const void *src, > > + size_t cnt) > > { > [ ... ] > > + case 32: > > + movnti_32(dst, src); > > + return; > Does skipping the 8-byte alignment check here break the cache bypass guarantee > required for persistent memory (PMEM) durability? > The memcpy_flushcache() API guarantees that written data bypasses the CPU > cache > so PMEM updates are durable without requiring manual clflush. On x86, > unaligned > non-temporal stores (MOVNTI) crossing cache lines silently fall back to > temporal Read-For-Ownership cache allocations. > Because this inline fast path bypasses __memcpy_flushcache() (which correctly > aligns the destination using normal memcpy and clflushopt), will the unaligned > data silently sit in the CPU L1/L2 cache instead of reaching the PMEM module? > Is it possible that the microbenchmark speedup mentioned in the commit message > (0.410us vs 0.962us for unaligned offsets) was achieved precisely because the > stores hit the fast L1 cache instead of actually flushing to memory? I do not see anything in the Intel SDM saying that an unaligned non-temporal MOVNTI store that crosses a cache line silently falls back to temporal RFO cache allocations. The existing 4/8/16-byte memcpy_flushcache() fixed-size cases already use MOVNTI without such an alignment check. Please correct me if I am wrong. The microbenchmark is only used as the performance motivation for keeping these small constant-size copies in the inline path. It is not intended as a durability proof. > --- Patch [8]: [PATCH v7 8/9] mm: use memcpy_nontemporal() in zone-device > template copies --- > commit ebc6b81ea2e4b12a555515b9468c89fd0bdfe4f1 > Author: Li Zhe <[email protected]> > mm: use memcpy_nontemporal() in zone-device template copies > The template fast path currently uses memcpy() for the actual struct page > copy. Switch zone_device_page_init_from_template() to memcpy_nontemporal() > to reduce write-allocate cache traffic. > > diff --git a/mm/mm_init.c b/mm/mm_init.c > > index 2668acef4fe60..2a723a518f413 100644 > > --- a/mm/mm_init.c > > +++ b/mm/mm_init.c > [ ... ] > > @@ -1110,7 +1120,7 @@ static void > > zone_device_page_init_from_template(struct page *page, > > * to the destination page. > > */ > > zone_device_page_update_template(template, pfn); > > - memcpy(page, template, sizeof(*page)); > > + memcpy_nontemporal(page, template, sizeof(*page)); > Could this mix of non-temporal and temporal stores to the same memory location > cause data corruption on x86? > During DAX device binding, memmap_init_zone_device() copies the template to > the head page using this memcpy_nontemporal() call, and then immediately > calls memmap_init_compound(): > mm/mm_init.c:memmap_init_zone_device() { > ... > } else { > zone_device_page_init_from_template(page, pfn, > &template); > } > ... > memmap_init_compound(page, pfn, zone_idx, nid, pgmap, > compound_nr_pages(pfn, altmap, pgmap), > use_template); > ... > } > Inside memmap_init_compound(), a normal temporal store is applied to the > head page: > mm/mm_init.c:memmap_init_compound() { > ... > /* > * We have to initialize the pages, including setting up page links. > * prep_compound_page() does not take care of that, so instead we > * open-code prep_compound_page() so we can take care of initializing > * the pages in the same go. > */ > __SetPageHead(head); > ... > } > The commit message states: > "The ordinary stores that follow in this path, such as compound-page setup, > are part of the same CPU's initialization sequence; they are not used as a > publication store that tells another CPU or device to consume data written > by the non-temporal copy. Therefore this call site does not need a helper- > level drain for correctness." > However, because x86 does not maintain coherency between write-combining (WC) > buffers and temporal cache lines for the same address, a pending WC buffer > flush can unpredictably overwrite the L1 cache updates from the same CPU. This > could cause the PG_head bit to be lost. > Does this path actually require an intervening memory barrier (like sfence) > before the temporal writes to guarantee correctness? I do not think an sfence is required between the template copy and the following compound-page setup. The weak-ordering concern for non-temporal stores is about when those stores become visible to other agents, and about ordering them before an external handoff. The following __SetPageHead() store is not such a handoff. It is still part of the same CPU's construction of the same struct page metadata. I do not think the x86 memory-ordering rules imply that a later same-CPU temporal update to the same struct page can be silently lost behind an earlier non-temporal store. The required fence is for ordering non-temporal stores before an external observer or handoff. So the relevant ordering point would be before publishing data written by non-temporal stores to another CPU or device. This path does not do that between the memcpy_nontemporal() call and the following compound-page stores. It is only continuing local initialization of the struct page. Thanks, Zhe
