On Fri, Jun 12, 2020 at 1:20 PM afzal mohammed <afzal.mohd.ma@xxxxxxxxx> wrote:
>
> copy_{from,to}_user() uaccess helpers are implemented by user page
> pinning, followed by temporary kernel mapping & then memcpy(). This
> helps to achieve user page copy when current virtual address mapping
> of the CPU excludes user pages.
>
> Performance wise, results are not encouraging, 'dd' on tmpfs results,
>
> ARM Cortex-A8, BeagleBone White (256MiB RAM):
> w/o series - ~29.5 MB/s
> w/ series - ~20.5 MB/s
> w/ series & highmem disabled - ~21.2 MB/s
>
> On Cortex-A15(2GiB RAM) in QEMU:
> w/o series - ~4 MB/s
> w/ series - ~2.6 MB/s
>
> Roughly a one-third drop in performance. Disabling highmem improves
> performance only slightly.
>
> 'hackbench' also showed a similar pattern.
>
> uaccess routines using page pinning & temporary kernel mapping is not
> something new, it has been done long long ago by Ingo [1] as part of
> 4G/4G user/kernel mapping implementation on x86, though not merged in
> mainline.
>
> [1] https://lore.kernel.org/lkml/Pine.LNX.4.44.0307082332450.17252-100000@localhost.localdomain/
Some comments (more related to generic things).
...
> +// Started from arch/um/kernel/skas/uaccess.c
Does it mean you will deduplicate it there?
...
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/highmem.h>
> +#include <linux/mm.h>
Perhaps ordered?
> +static int do_op_one_page(unsigned long addr, int len,
> + int (*op)(unsigned long addr, int len, void *arg), void *arg,
> + struct page *page)
Maybe typedef for the func() ?
> +{
> + int n;
> +
> + addr = (unsigned long) kmap_atomic(page) + (addr & ~PAGE_MASK);
I don't remember about this one...
> + n = (*op)(addr, len, arg);
> + kunmap_atomic((void *)addr);
> +
> + return n;
> +}
> +
> +static long buffer_op(unsigned long addr, int len,
> + int (*op)(unsigned long, int, void *), void *arg,
> + struct page **pages)
> +{
> + long size, remain, n;
> +
> + size = min(PAGE_ALIGN(addr) - addr, (unsigned long) len);
...but here seems to me you can use helpers (offset_in_page() or how
it's called).
Also consider to use macros like PFN_DOWN(), PFN_UP(), etc in your code.
> + remain = len;
> + if (size == 0)
> + goto page_boundary;
> +
> + n = do_op_one_page(addr, size, op, arg, *pages);
> + if (n != 0) {
> + remain = (n < 0 ? remain : 0);
Why duplicate three times (!) this line, if you can move it to under 'out'?
> + goto out;
> + }
> +
> + pages++;
> + addr += size;
> + remain -= size;
> +
> +page_boundary:
> + if (remain == 0)
> + goto out;
> + while (addr < ((addr + remain) & PAGE_MASK)) {
> + n = do_op_one_page(addr, PAGE_SIZE, op, arg, *pages);
> + if (n != 0) {
> + remain = (n < 0 ? remain : 0);
> + goto out;
> + }
> +
> + pages++;
> + addr += PAGE_SIZE;
> + remain -= PAGE_SIZE;
> + }
Sounds like this can be refactored to iterate over pages rather than addresses.
> + if (remain == 0)
> + goto out;
> +
> + n = do_op_one_page(addr, remain, op, arg, *pages);
> + if (n != 0) {
> + remain = (n < 0 ? remain : 0);
> + goto out;
> + }
> +
> + return 0;
> +out:
> + return remain;
> +}
...
> +static int copy_chunk_from_user(unsigned long from, int len, void *arg)
> +{
> + unsigned long *to_ptr = arg, to = *to_ptr;
> +
> + memcpy((void *) to, (void *) from, len);
What is the point in the casting to void *?
> + *to_ptr += len;
> + return 0;
> +}
> +
> +static int copy_chunk_to_user(unsigned long to, int len, void *arg)
> +{
> + unsigned long *from_ptr = arg, from = *from_ptr;
> +
> + memcpy((void *) to, (void *) from, len);
> + *from_ptr += len;
Ditto.
> + return 0;
> +}
> +
> +unsigned long gup_kmap_copy_from_user(void *to, const void __user *from, unsigned long n)
> +{
> + struct page **pages;
> + int num_pages, ret, i;
> +
> + if (uaccess_kernel()) {
> + memcpy(to, (__force void *)from, n);
> + return 0;
> + }
> +
> + num_pages = DIV_ROUND_UP((unsigned long)from + n, PAGE_SIZE) -
> + (unsigned long)from / PAGE_SIZE;
PFN_UP() ?
> + pages = kmalloc_array(num_pages, sizeof(*pages), GFP_KERNEL | __GFP_ZERO);
> + if (!pages)
> + goto end;
> +
> + ret = get_user_pages_fast((unsigned long)from, num_pages, 0, pages);
> + if (ret < 0)
> + goto free_pages;
> +
> + if (ret != num_pages) {
> + num_pages = ret;
> + goto put_pages;
> + }
> +
> + n = buffer_op((unsigned long) from, n, copy_chunk_from_user, &to, pages);
> +
> +put_pages:
> + for (i = 0; i < num_pages; i++)
> + put_page(pages[i]);
> +free_pages:
> + kfree(pages);
> +end:
> + return n;
> +}
...
I think you can clean up the code a bit after you will get the main
functionality working.
--
With Best Regards,
Andy Shevchenko
