lkml.org 
[lkml]   [2015]   [May]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 3/6] x86, pmem: add PMEM API for persistent memory
    From
    On Thu, May 28, 2015 at 4:20 PM, H. Peter Anvin <hpa@zytor.com> wrote:
    > On 05/28/2015 03:35 PM, Ross Zwisler wrote:
    >> Add a new PMEM API to x86, and allow for architectures that do not
    >> implement this API. Architectures that implement the PMEM API should
    >> define ARCH_HAS_PMEM_API in their kernel configuration and must provide
    >> implementations for persistent_copy(), persistent_flush() and
    >> persistent_sync().
    >
    >>
    >> void clflush_cache_range(void *addr, unsigned int size);
    >>
    >
    > No, no, no, no, no. Include the proper header file.
    >
    >> +static inline void arch_persistent_flush(void *vaddr, size_t size)
    >> +{
    >> + clflush_cache_range(vaddr, size);
    >> +}
    >
    > Shouldn't this really be using clwb() -- we really need a
    > clwb_cache_range() I guess?
    >
    > Incidentally, clflush_cache_range() seems to have the same flaw as the
    > proposed use case for clwb() had... if the buffer is aligned it will
    > needlessly flush the last line twice. It should really look something
    > like this (which would be a good standalone patch):
    >
    > void clflush_cache_range(void *vaddr, unsigned int size)
    > {
    > void *vend = vaddr + size - 1;
    >
    > mb();
    >
    > vaddr = (void *)
    > ((unsigned long)vaddr
    > & ~(boot_cpu_data.x86_clflush_size - 1));
    >
    > for (; vaddr < vend; vaddr += boot_cpu_data.x86_clflush_size)
    > clflushopt(vaddr);
    >
    > mb();
    > }
    > EXPORT_SYMBOL_GPL(clflush_cache_range);
    >
    > I also note that with your implementation we have a wmb() in
    > arch_persistent_sync() and an mb() in arch_persistent_flush()... surely
    > one is redundant?

    Hmm, yes, but I believe Ross (on vacation now) was following the
    precedent set by commit cd8ddf1a2800 "x86: clflush_page_range needs
    mfence" whereby the api handles all necessary fencing internally.
    Shall we introduce something like __unordered_clflush_cache_range()
    for arch_persistent_flush() to use with the understanding it will be
    following up with the wmb() in arch_persistent_sync()?


    \
     
     \ /
      Last update: 2015-05-29 02:21    [W:2.146 / U:1.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site