lkml.org 
[lkml]   [2019]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH V2 0/5] vhost: accelerate metadata access through vmap()
    From
    Date
    On Tue, 2019-03-12 at 18:50 -0400, Andrea Arcangeli wrote:
    > On Tue, Mar 12, 2019 at 03:02:54PM -0700, James Bottomley wrote:
    > > I'm sure there must be workarounds elsewhere in the other arch code
    > > otherwise things like this, which appear all over drivers/,
    > > wouldn't
    > > work:
    > >
    > > drivers/scsi/isci/request.c:1430
    > >
    > > kaddr = kmap_atomic(page);
    > > memcpy(kaddr + sg->offset, src_addr, copy_len);
    > > kunmap_atomic(kaddr);
    > >
    >
    > Are you sure "page" is an userland page with an alias address?
    >
    > sg->page_link = (unsigned long)virt_to_page(addr);

    Yes, it's an element of a scatter gather list, which may be either a
    kernel page or a user page, but is usually the latter.

    > page_link seems to point to kernel memory.
    >
    > I found an apparent solution like parisc on arm 32bit:
    >
    > void __kunmap_atomic(void *kvaddr)
    > {
    > unsigned long vaddr = (unsigned long) kvaddr & PAGE_MASK;
    > int idx, type;
    >
    > if (kvaddr >= (void *)FIXADDR_START) {
    > type = kmap_atomic_idx();
    > idx = FIX_KMAP_BEGIN + type + KM_TYPE_NR *
    > smp_processor_id();
    >
    > if (cache_is_vivt())
    > __cpuc_flush_dcache_area((void *)vaddr,
    > PAGE_SIZE);
    >
    > However on arm 64bit kunmap_atomic is not implemented at all and
    > other 32bit implementations don't do it, for example sparc seems to
    > do the cache flush too if the kernel is built with
    > CONFIG_DEBUG_HIGHMEM (which makes the flushing conditional to the
    > debug option).
    >
    > The kunmap_atomic where fixmap is used, is flushing the tlb lazily so
    > even on 32bit you can't even be sure if there was a tlb flush for
    > each single page you unmapped, so it's hard to see how the above can
    > work safe, is "page" would have been an userland page mapped with
    > aliased CPU cache.
    >
    > > the sequence dirties the kernel virtual address but doesn't flush
    > > before doing kunmap. There are hundreds of other examples which is
    > > why I think adding flush_kernel_dcache_page() is an already lost
    > > cause.
    >
    > In lots of cases kmap is needed to just modify kernel memory not to
    > modify userland memory (where get/put_user is more commonly used
    > instead..), there's no cache aliasing in such case.

    That's why I picked drivers/ The use case in there is mostly kmap to
    put a special value into a scatter gather list entry.

    > > Actually copy_user_page() is unused in the main kernel. The big
    > > problem is copy_user_highpage() but that's mostly highly optimised
    > > by the VIPT architectures (in other words you can fiddle with kmap
    > > without impacting it).
    >
    > copy_user_page is not unused, it's called precisely by
    > copy_user_highpage, which is why the cache flushes are done inside
    > copy_user_page.
    >
    > static inline void copy_user_highpage(struct page *to, struct page
    > *from,
    > unsigned long vaddr, struct vm_area_struct *vma)
    > {
    > char *vfrom, *vto;
    >
    > vfrom = kmap_atomic(from);
    > vto = kmap_atomic(to);
    > copy_user_page(vto, vfrom, vaddr, to);
    > kunmap_atomic(vto);
    > kunmap_atomic(vfrom);
    > }

    That's the asm/generic implementation. Most VIPT architectures
    override it.

    James

    \
     
     \ /
      Last update: 2019-03-12 23:58    [W:3.591 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site