On Thu, Oct 17, 2013 at 12:00:34PM -0400, Don Zickus wrote:
> On Thu, Oct 17, 2013 at 11:41:45AM +0200, Peter Zijlstra wrote:
> > On Thu, Oct 17, 2013 at 01:07:12AM +0200, Peter Zijlstra wrote:
> > > On Wed, Oct 16, 2013 at 11:03:19PM +0200, Peter Zijlstra wrote:
> > > > Anyway; if you want to have a go at this, feel free.
> > >
> > > OK, couldn't help myself; completely untested patch below.
> > >
> > > I think the full once copy it best for the decode as even with the below
> > > interface you'd end up doing a lot of duplicate copying due to the
> > > variable size insn mess.
> >
> > Duh, a very small tweak would make it work for that and avoid most of
> > the memcpy()s.
>
> Hmm, for some reason, even though copy_from_user_nmi_iter is super fast
> now, the while(to < ip) count increased dramatically and so did my
> latency. :-(
I take that back the copy_from_user_nmi_iter is not super fast, I just had
a bug in how I accumulate total time. So some how this approach is slower
that yesterdays.
Cheers,
Don
>
> Not sure what happened between your pretty patch yesterday and this
> direction.
>
> Cheers,
> Don
>
> >
> > ---
> > arch/x86/include/asm/uaccess.h | 13 +++++
> > arch/x86/kernel/cpu/perf_event.c | 32 +++++------
> > arch/x86/kernel/cpu/perf_event_intel_ds.c | 21 ++++---
> > arch/x86/lib/usercopy.c | 91
> > ++++++++++++++++++++++++++++++-
> > arch/x86/mm/gup.c | 63 +++++++++++++--------
> > 5 files changed, 165 insertions(+), 55 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> > index 5838fa911aa0..a341de0eadd1 100644
> > --- a/arch/x86/include/asm/uaccess.h
> > +++ b/arch/x86/include/asm/uaccess.h
> > @@ -516,6 +516,19 @@ struct __large_struct { unsigned long buf[100]; };
> >
> > extern unsigned long
> > copy_from_user_nmi(void *to, const void __user *from, unsigned long n);
> > +
> > +struct copy_from_user_nmi_state {
> > + void *map;
> > + unsigned long address;
> > + unsigned long flags;
> > +};
> > +
> > +extern void *
> > +copy_from_user_nmi_iter(void *to, const void __user *from,
> > + unsigned long n, struct copy_from_user_nmi_state
> > *state);
> > +extern void
> > +copy_from_user_nmi_end(struct copy_from_user_nmi_state *state);
> > +
> > extern __must_check long
> > strncpy_from_user(char *dst, const char __user *src, long count);
> >
> > diff --git a/arch/x86/kernel/cpu/perf_event.c
> > b/arch/x86/kernel/cpu/perf_event.c
> > index 19c9d86d2f04..c917fe470861 100644
> > --- a/arch/x86/kernel/cpu/perf_event.c
> > +++ b/arch/x86/kernel/cpu/perf_event.c
> > @@ -1979,8 +1979,9 @@ static inline int
> > perf_callchain_user32(struct pt_regs *regs, struct perf_callchain_entry
> > *entry)
> > {
> > /* 32-bit process in 64-bit kernel. */
> > + struct copy_from_user_nmi_state state = { NULL };
> > unsigned long ss_base, cs_base;
> > - struct stack_frame_ia32 frame;
> > + struct stack_frame_ia32 frame, *f;
> > const void __user *fp;
> >
> > if (!test_thread_flag(TIF_IA32))
> > @@ -1991,20 +1992,17 @@ perf_callchain_user32(struct pt_regs *regs, struct
> > perf_callchain_entry *entry)
> >
> > fp = compat_ptr(ss_base + regs->bp);
> > while (entry->nr < PERF_MAX_STACK_DEPTH) {
> > - unsigned long bytes;
> > - frame.next_frame = 0;
> > - frame.return_address = 0;
> > -
> > - bytes = copy_from_user_nmi(&frame, fp, sizeof(frame));
> > - if (bytes != sizeof(frame))
> > + f = copy_from_user_nmi_iter(&frame, fp, sizeof(frame), &state);
> > + if (!f)
> > break;
> >
> > if (!valid_user_frame(fp, sizeof(frame)))
> > break;
> >
> > - perf_callchain_store(entry, cs_base + frame.return_address);
> > - fp = compat_ptr(ss_base + frame.next_frame);
> > + perf_callchain_store(entry, cs_base + f->return_address);
> > + fp = compat_ptr(ss_base + f->next_frame);
> > }
> > + copy_from_user_nmi_end(&state);
> > return 1;
> > }
> > #else
> > @@ -2018,7 +2016,8 @@ perf_callchain_user32(struct pt_regs *regs, struct
> > perf_callchain_entry *entry)
> > void
> > perf_callchain_user(struct perf_callchain_entry *entry, struct pt_regs
> > *regs)
> > {
> > - struct stack_frame frame;
> > + struct copy_from_user_nmi_state state = { NULL };
> > + struct stack_frame frame, *f;
> > const void __user *fp;
> >
> > if (perf_guest_cbs && perf_guest_cbs->is_in_guest()) {
> > @@ -2043,20 +2042,17 @@ perf_callchain_user(struct perf_callchain_entry
> > *entry, struct pt_regs *regs)
> > return;
> >
> > while (entry->nr < PERF_MAX_STACK_DEPTH) {
> > - unsigned long bytes;
> > - frame.next_frame = NULL;
> > - frame.return_address = 0;
> > -
> > - bytes = copy_from_user_nmi(&frame, fp, sizeof(frame));
> > - if (bytes != sizeof(frame))
> > + f = copy_from_user_nmi_iter(&frame, fp, sizeof(frame), &state);
> > + if (!f)
> > break;
> >
> > if (!valid_user_frame(fp, sizeof(frame)))
> > break;
> >
> > - perf_callchain_store(entry, frame.return_address);
> > - fp = frame.next_frame;
> > + perf_callchain_store(entry, f->return_address);
> > + fp = f->next_frame;
> > }
> > + copy_from_user_nmi_end(&state);
> > }
> >
> > /*
> > diff --git a/arch/x86/kernel/cpu/perf_event_intel_ds.c
> > b/arch/x86/kernel/cpu/perf_event_intel_ds.c
> > index 32e9ed81cd00..5bd3f2091da9 100644
> > --- a/arch/x86/kernel/cpu/perf_event_intel_ds.c
> > +++ b/arch/x86/kernel/cpu/perf_event_intel_ds.c
> > @@ -725,10 +725,14 @@ void intel_pmu_pebs_disable_all(void)
> > static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
> > {
> > struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> > + struct copy_from_user_nmi_state state = { NULL };
> > unsigned long from = cpuc->lbr_entries[0].from;
> > unsigned long old_to, to = cpuc->lbr_entries[0].to;
> > unsigned long ip = regs->ip;
> > + u8 buf[MAX_INSN_SIZE];
> > + struct insn insn;
> > int is_64bit = 0;
> > + void *kaddr;
> >
> > /*
> > * We don't need to fixup if the PEBS assist is fault like
> > @@ -764,19 +768,12 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs
> > *regs)
> > }
> >
> > do {
> > - struct insn insn;
> > - u8 buf[MAX_INSN_SIZE];
> > - void *kaddr;
> > -
> > old_to = to;
> > if (!kernel_ip(ip)) {
> > - int bytes, size = MAX_INSN_SIZE;
> > -
> > - bytes = copy_from_user_nmi(buf, (void __user *)to,
> > size);
> > - if (bytes != size)
> > - return 0;
> > -
> > - kaddr = buf;
> > + kaddr = copy_from_user_nmi_iter(buf, (void __user *)to,
> > + MAX_INSN_SIZE, &state);
> > + if (!kaddr)
> > + break;
> > } else
> > kaddr = (void *)to;
> >
> > @@ -788,6 +785,8 @@ static int intel_pmu_pebs_fixup_ip(struct pt_regs *regs)
> > to += insn.length;
> > } while (to < ip);
> >
> > + copy_from_user_nmi_end(&state);
> > +
> > if (to == ip) {
> > set_linear_ip(regs, old_to);
> > return 1;
> > diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
> > index 4f74d94c8d97..da6c36a8b842 100644
> > --- a/arch/x86/lib/usercopy.c
> > +++ b/arch/x86/lib/usercopy.c
> > @@ -10,6 +10,8 @@
> > #include <asm/word-at-a-time.h>
> > #include <linux/sched.h>
> >
> > +extern int ___get_user_pages_fast(unsigned long start, int nr_pages, int
> > flags,
> > + struct page **pages);
> > /*
> > * best effort, GUP based copy_from_user() that is NMI-safe
> > */
> > @@ -18,6 +20,7 @@ copy_from_user_nmi(void *to, const void __user *from,
> > unsigned long n)
> > {
> > unsigned long offset, addr = (unsigned long)from;
> > unsigned long size, len = 0;
> > + unsigned long flags;
> > struct page *page;
> > void *map;
> > int ret;
> > @@ -26,9 +29,12 @@ copy_from_user_nmi(void *to, const void __user *from,
> > unsigned long n)
> > return len;
> >
> > do {
> > - ret = __get_user_pages_fast(addr, 1, 0, &page);
> > - if (!ret)
> > + local_irq_save(flags);
> > + ret = ___get_user_pages_fast(addr, 1, 0, &page);
> > + if (!ret) {
> > + local_irq_restore(flags);
> > break;
> > + }
> >
> > offset = addr & (PAGE_SIZE - 1);
> > size = min(PAGE_SIZE - offset, n - len);
> > @@ -36,7 +42,7 @@ copy_from_user_nmi(void *to, const void __user *from,
> > unsigned long n)
> > map = kmap_atomic(page);
> > memcpy(to, map+offset, size);
> > kunmap_atomic(map);
> > - put_page(page);
> > + local_irq_restore(flags);
> >
> > len += size;
> > to += size;
> > @@ -47,3 +53,82 @@ copy_from_user_nmi(void *to, const void __user *from,
> > unsigned long n)
> > return len;
> > }
> > EXPORT_SYMBOL_GPL(copy_from_user_nmi);
> > +
> > +void *copy_from_user_nmi_iter(void *to, const void __user *from,
> > + unsigned long n, struct copy_from_user_nmi_state *state)
> > +{
> > + unsigned long offset, addr = (unsigned long)from;
> > + unsigned long size, len = 0;
> > + unsigned long flags;
> > + struct page *page;
> > + void *map, *_to = to;
> > + int ret;
> > +
> > + if (__range_not_ok(from, n, TASK_SIZE))
> > + return NULL;
> > +
> > + if (state->map) {
> > + if ((state->address >> PAGE_SHIFT) ==
> > + (addr >> PAGE_SHIFT)) {
> > + flags = state->flags;
> > + map = state->map;
> > + goto got_page;
> > + }
> > + kunmap_atomic(state->map);
> > + local_irq_restore(state->flags);
> > + }
> > +
> > + for (;;) {
> > + local_irq_save(flags);
> > + ret = ___get_user_pages_fast(addr, 1, 0, &page);
> > + if (!ret) {
> > + local_irq_restore(flags);
> > + state->map = NULL;
> > + return NULL;
> > + }
> > +
> > + map = kmap_atomic(page);
> > +
> > +got_page:
> > + offset = addr & (PAGE_SIZE - 1);
> > + size = min(PAGE_SIZE - offset, n - len);
> > +
> > + /*
> > + * If the entire desired range falls within the one page
> > + * avoid the copy and return a pointer into the kmap.
> > + */
> > + if (size == n) {
> > + _to = map + offset;
> > + break;
> > + }
> > +
> > + memcpy(to, map+offset, size);
> > + len += size;
> > +
> > + if (len == n)
> > + break;
> > +
> > + to += size;
> > + addr += size;
> > +
> > + kunmap_atomic(map);
> > + local_irq_restore(flags);
> > + }
> > +
> > + state->address = addr;
> > + state->flags = flags;
> > + state->map = map;
> > +
> > + return _to;
> > +}
> > +EXPORT_SYMBOL_GPL(copy_from_user_nmi_iter);
> > +
> > +void copy_from_user_nmi_end(struct copy_from_user_nmi_state *state)
> > +{
> > + if (state->map) {
> > + kunmap_atomic(state->map);
> > + local_irq_restore(state->flags);
> > + state->map = NULL;
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(copy_from_user_nmi_end);
> > diff --git a/arch/x86/mm/gup.c b/arch/x86/mm/gup.c
> > index dd74e46828c0..e383caf323e4 100644
> > --- a/arch/x86/mm/gup.c
> > +++ b/arch/x86/mm/gup.c
> > @@ -63,19 +63,22 @@ static inline pte_t gup_get_pte(pte_t *ptep)
> > #endif
> > }
> >
> > +#define GUPF_GET 0x01
> > +#define GUPF_WRITE 0x02
> > +
> > /*
> > * The performance critical leaf functions are made noinline otherwise gcc
> > * inlines everything into a single function which results in too much
> > * register pressure.
> > */
> > static noinline int gup_pte_range(pmd_t pmd, unsigned long addr,
> > - unsigned long end, int write, struct page **pages, int *nr)
> > + unsigned long end, int flags, struct page **pages, int *nr)
> > {
> > unsigned long mask;
> > pte_t *ptep;
> >
> > mask = _PAGE_PRESENT|_PAGE_USER;
> > - if (write)
> > + if (flags & GUPF_WRITE)
> > mask |= _PAGE_RW;
> >
> > ptep = pte_offset_map(&pmd, addr);
> > @@ -89,7 +92,8 @@ static noinline int gup_pte_range(pmd_t pmd, unsigned
> > long addr,
> > }
> > VM_BUG_ON(!pfn_valid(pte_pfn(pte)));
> > page = pte_page(pte);
> > - get_page(page);
> > + if (flags & GUPF_GET)
> > + get_page(page);
> > SetPageReferenced(page);
> > pages[*nr] = page;
> > (*nr)++;
> > @@ -109,7 +113,7 @@ static inline void get_head_page_multiple(struct page
> > *page, int nr)
> > }
> >
> > static noinline int gup_huge_pmd(pmd_t pmd, unsigned long addr,
> > - unsigned long end, int write, struct page **pages, int *nr)
> > + unsigned long end, int flags, struct page **pages, int *nr)
> > {
> > unsigned long mask;
> > pte_t pte = *(pte_t *)&pmd;
> > @@ -117,7 +121,7 @@ static noinline int gup_huge_pmd(pmd_t pmd, unsigned
> > long addr,
> > int refs;
> >
> > mask = _PAGE_PRESENT|_PAGE_USER;
> > - if (write)
> > + if (flags & GUPF_WRITE)
> > mask |= _PAGE_RW;
> > if ((pte_flags(pte) & mask) != mask)
> > return 0;
> > @@ -131,19 +135,20 @@ static noinline int gup_huge_pmd(pmd_t pmd, unsigned
> > long addr,
> > do {
> > VM_BUG_ON(compound_head(page) != head);
> > pages[*nr] = page;
> > - if (PageTail(page))
> > + if ((flags & GUPF_GET) && PageTail(page))
> > get_huge_page_tail(page);
> > (*nr)++;
> > page++;
> > refs++;
> > } while (addr += PAGE_SIZE, addr != end);
> > - get_head_page_multiple(head, refs);
> > + if (flags & GUPF_GET)
> > + get_head_page_multiple(head, refs);
> >
> > return 1;
> > }
> >
> > static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
> > - int write, struct page **pages, int *nr)
> > + int flags, struct page **pages, int *nr)
> > {
> > unsigned long next;
> > pmd_t *pmdp;
> > @@ -167,10 +172,10 @@ static int gup_pmd_range(pud_t pud, unsigned long
> > addr, unsigned long end,
> > if (pmd_none(pmd) || pmd_trans_splitting(pmd))
> > return 0;
> > if (unlikely(pmd_large(pmd))) {
> > - if (!gup_huge_pmd(pmd, addr, next, write, pages, nr))
> > + if (!gup_huge_pmd(pmd, addr, next, flags, pages, nr))
> > return 0;
> > } else {
> > - if (!gup_pte_range(pmd, addr, next, write, pages, nr))
> > + if (!gup_pte_range(pmd, addr, next, flags, pages, nr))
> > return 0;
> > }
> > } while (pmdp++, addr = next, addr != end);
> > @@ -179,7 +184,7 @@ static int gup_pmd_range(pud_t pud, unsigned long addr,
> > unsigned long end,
> > }
> >
> > static noinline int gup_huge_pud(pud_t pud, unsigned long addr,
> > - unsigned long end, int write, struct page **pages, int *nr)
> > + unsigned long end, int flags, struct page **pages, int *nr)
> > {
> > unsigned long mask;
> > pte_t pte = *(pte_t *)&pud;
> > @@ -187,7 +192,7 @@ static noinline int gup_huge_pud(pud_t pud, unsigned
> > long addr,
> > int refs;
> >
> > mask = _PAGE_PRESENT|_PAGE_USER;
> > - if (write)
> > + if (flags & GUPF_WRITE)
> > mask |= _PAGE_RW;
> > if ((pte_flags(pte) & mask) != mask)
> > return 0;
> > @@ -201,19 +206,20 @@ static noinline int gup_huge_pud(pud_t pud, unsigned
> > long addr,
> > do {
> > VM_BUG_ON(compound_head(page) != head);
> > pages[*nr] = page;
> > - if (PageTail(page))
> > + if ((flags & GUPF_GET) && PageTail(page))
> > get_huge_page_tail(page);
> > (*nr)++;
> > page++;
> > refs++;
> > } while (addr += PAGE_SIZE, addr != end);
> > - get_head_page_multiple(head, refs);
> > + if (flags & GUPF_GET)
> > + get_head_page_multiple(head, refs);
> >
> > return 1;
> > }
> >
> > static int gup_pud_range(pgd_t pgd, unsigned long addr, unsigned long end,
> > - int write, struct page **pages, int *nr)
> > + int flags, struct page **pages, int *nr)
> > {
> > unsigned long next;
> > pud_t *pudp;
> > @@ -226,10 +232,10 @@ static int gup_pud_range(pgd_t pgd, unsigned long
> > addr, unsigned long end,
> > if (pud_none(pud))
> > return 0;
> > if (unlikely(pud_large(pud))) {
> > - if (!gup_huge_pud(pud, addr, next, write, pages, nr))
> > + if (!gup_huge_pud(pud, addr, next, flags, pages, nr))
> > return 0;
> > } else {
> > - if (!gup_pmd_range(pud, addr, next, write, pages, nr))
> > + if (!gup_pmd_range(pud, addr, next, flags, pages, nr))
> > return 0;
> > }
> > } while (pudp++, addr = next, addr != end);
> > @@ -241,13 +247,12 @@ static int gup_pud_range(pgd_t pgd, unsigned long
> > addr, unsigned long end,
> > * Like get_user_pages_fast() except its IRQ-safe in that it won't fall
> > * back to the regular GUP.
> > */
> > -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > +int ___get_user_pages_fast(unsigned long start, int nr_pages, int flags,
> > struct page **pages)
> > {
> > struct mm_struct *mm = current->mm;
> > unsigned long addr, len, end;
> > unsigned long next;
> > - unsigned long flags;
> > pgd_t *pgdp;
> > int nr = 0;
> >
> > @@ -255,7 +260,7 @@ int __get_user_pages_fast(unsigned long start, int
> > nr_pages, int write,
> > addr = start;
> > len = (unsigned long) nr_pages << PAGE_SHIFT;
> > end = start + len;
> > - if (unlikely(!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
> > + if (unlikely(!access_ok((flags & GUPF_WRITE) ? VERIFY_WRITE :
> > VERIFY_READ,
> > (void __user *)start, len)))
> > return 0;
> >
> > @@ -277,7 +282,6 @@ int __get_user_pages_fast(unsigned long start, int
> > nr_pages, int write,
> > * (which we do on x86, with the above PAE exception), we can follow the
> > * address down to the the page and take a ref on it.
> > */
> > - local_irq_save(flags);
> > pgdp = pgd_offset(mm, addr);
> > do {
> > pgd_t pgd = *pgdp;
> > @@ -285,14 +289,27 @@ int __get_user_pages_fast(unsigned long start, int
> > nr_pages, int write,
> > next = pgd_addr_end(addr, end);
> > if (pgd_none(pgd))
> > break;
> > - if (!gup_pud_range(pgd, addr, next, write, pages, &nr))
> > + if (!gup_pud_range(pgd, addr, next, flags, pages, &nr))
> > break;
> > } while (pgdp++, addr = next, addr != end);
> > - local_irq_restore(flags);
> >
> > return nr;
> > }
> >
> > +int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > + struct page **pages)
> > +{
> > + unsigned long flags;
> > + int ret;
> > +
> > + local_irq_save(flags);
> > + ret = ___get_user_pages_fast(start, nr_pages,
> > + GUPF_GET | (write ? GUPF_WRITE : 0), pages);
> > + local_irq_restore(flags);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * get_user_pages_fast() - pin user pages in memory
> > * @start: starting user address
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
