| Message ID | 20181116083215.7165-1-anton.ivanov@cambridgegreys.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | um: Remove unnecessary kmap_atomic in uaccess on 64bit | expand |
Hi Anton, On Fri, Nov 16, 2018 at 4:32 PM <anton.ivanov@cambridgegreys.com> wrote: > > From: Anton Ivanov <anton.ivanov@cambridgegreys.com> > > On 64 bit architectures it is not necessary to perform highmem > kmap_/kunmap for various copy to/from user operations. The mmu > emulation on its own is sufficient to provide the correct address > offset to execute a memcpy(). > > This results in 3%-7% improvement in various copy_to/from ops. > > Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> > --- > arch/um/kernel/skas/uaccess.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c > index d450797a3a7c..615be3ec98eb 100644 > --- a/arch/um/kernel/skas/uaccess.c > +++ b/arch/um/kernel/skas/uaccess.c > @@ -69,20 +69,26 @@ static int do_op_one_page(unsigned long addr, int len, int is_write, > return -1; > > page = pte_page(*pte); > + current->thread.fault_catcher = &buf; > + faulted = UML_SETJMP(&buf); I'm wondering why should we move these in front of kmap_atomic(), as in a faulted situation this will lead to a second kmap_atomic() execution. Thanks > +#ifndef CONFIG_64BIT > addr = (unsigned long) kmap_atomic(page) + > (addr & ~PAGE_MASK); > +#else > + addr = (unsigned long) page_address(page) + > + (addr & ~PAGE_MASK); > +#endif > > - current->thread.fault_catcher = &buf; > > - faulted = UML_SETJMP(&buf); > if (faulted == 0) > n = (*op)(addr, len, arg); > else > n = -1; > > current->thread.fault_catcher = NULL; > - > +#ifndef CONFIG_64BIT > kunmap_atomic((void *)addr); > +#endif > > return n; > } > -- > 2.11.0 > > > _______________________________________________ > linux-um mailing list > linux-um@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-um
On 11/22/18 2:09 AM, liwei wrote: > Hi Anton, > > On Fri, Nov 16, 2018 at 4:32 PM <anton.ivanov@cambridgegreys.com> wrote: >> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> >> On 64 bit architectures it is not necessary to perform highmem >> kmap_/kunmap for various copy to/from user operations. The mmu >> emulation on its own is sufficient to provide the correct address >> offset to execute a memcpy(). >> >> This results in 3%-7% improvement in various copy_to/from ops. >> >> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> >> --- >> arch/um/kernel/skas/uaccess.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c >> index d450797a3a7c..615be3ec98eb 100644 >> --- a/arch/um/kernel/skas/uaccess.c >> +++ b/arch/um/kernel/skas/uaccess.c >> @@ -69,20 +69,26 @@ static int do_op_one_page(unsigned long addr, int len, int is_write, >> return -1; >> >> page = pte_page(*pte); >> + current->thread.fault_catcher = &buf; >> + faulted = UML_SETJMP(&buf); > I'm wondering why should we move these in front of kmap_atomic(), as > in a faulted situation > this will lead to a second kmap_atomic() execution. Thanks for noting. I think they are needed only for the case where we do a simple page_addr, instead of kmap_atomic because kmap_atomic temporarily turns off paging. So if pte is non-null (as checked by the previous if) there should be no point to do a faulted check. I will double check and re-test all combinations. > Thanks > >> +#ifndef CONFIG_64BIT >> addr = (unsigned long) kmap_atomic(page) + >> (addr & ~PAGE_MASK); >> +#else >> + addr = (unsigned long) page_address(page) + >> + (addr & ~PAGE_MASK); >> +#endif >> >> - current->thread.fault_catcher = &buf; >> >> - faulted = UML_SETJMP(&buf); >> if (faulted == 0) >> n = (*op)(addr, len, arg); >> else >> n = -1; >> >> current->thread.fault_catcher = NULL; >> - >> +#ifndef CONFIG_64BIT >> kunmap_atomic((void *)addr); >> +#endif >> >> return n; >> } >> -- >> 2.11.0 >> >> >> _______________________________________________ >> linux-um mailing list >> linux-um@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-um Brgds,
Hi liwei, On 11/22/18 9:17 AM, Anton Ivanov wrote: > > On 11/22/18 2:09 AM, liwei wrote: >> Hi Anton, >> >> On Fri, Nov 16, 2018 at 4:32 PM <anton.ivanov@cambridgegreys.com> wrote: >>> From: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>> >>> On 64 bit architectures it is not necessary to perform highmem >>> kmap_/kunmap for various copy to/from user operations. The mmu >>> emulation on its own is sufficient to provide the correct address >>> offset to execute a memcpy(). >>> >>> This results in 3%-7% improvement in various copy_to/from ops. >>> >>> Signed-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com> >>> --- >>> arch/um/kernel/skas/uaccess.c | 12 +++++++++--- >>> 1 file changed, 9 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/um/kernel/skas/uaccess.c >>> b/arch/um/kernel/skas/uaccess.c >>> index d450797a3a7c..615be3ec98eb 100644 >>> --- a/arch/um/kernel/skas/uaccess.c >>> +++ b/arch/um/kernel/skas/uaccess.c >>> @@ -69,20 +69,26 @@ static int do_op_one_page(unsigned long addr, >>> int len, int is_write, >>> return -1; >>> >>> page = pte_page(*pte); >>> + current->thread.fault_catcher = &buf; >>> + faulted = UML_SETJMP(&buf); >> I'm wondering why should we move these in front of kmap_atomic(), as >> in a faulted situation >> this will lead to a second kmap_atomic() execution. > > Thanks for noting. > > I think they are needed only for the case where we do a simple > page_addr, instead of kmap_atomic because kmap_atomic temporarily > turns off paging. > > So if pte is non-null (as checked by the previous if) there should be > no point to do a faulted check. > > I will double check and re-test all combinations. I have traced the various paths on 64 and 32 bits and the faulted check should not be there. In fact, I was looking at the wrong culprit - it was the fault check + SETJMP which were the issue, not so much the actual map/unmap. It is actually quite costly and with potentials for error conditions when memory is low. It was re-enabling signals mid-copy without paging being enabled (it was disabled by kmap a few lines further up). So it was possible to end up with a scenario where pending IO released by signals enable would result in an OOM. As a side effect, I am now having a difficulty to make UML go into an OOM situation on low memory and heavy IO. While I cannot say that this kills that issue, it seems to behave better. I have issued a revised patch. In fact it is not so much revised as completely redone. Once again, thanks for noting the issues with the original. > >> Thanks >> >>> +#ifndef CONFIG_64BIT >>> addr = (unsigned long) kmap_atomic(page) + >>> (addr & ~PAGE_MASK); >>> +#else >>> + addr = (unsigned long) page_address(page) + >>> + (addr & ~PAGE_MASK); >>> +#endif >>> >>> - current->thread.fault_catcher = &buf; >>> >>> - faulted = UML_SETJMP(&buf); >>> if (faulted == 0) >>> n = (*op)(addr, len, arg); >>> else >>> n = -1; >>> >>> current->thread.fault_catcher = NULL; >>> - >>> +#ifndef CONFIG_64BIT >>> kunmap_atomic((void *)addr); >>> +#endif >>> >>> return n; >>> } >>> -- >>> 2.11.0 >>> >>> >>> _______________________________________________ >>> linux-um mailing list >>> linux-um@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-um > > Brgds, >
diff --git a/arch/um/kernel/skas/uaccess.c b/arch/um/kernel/skas/uaccess.c index d450797a3a7c..615be3ec98eb 100644 --- a/arch/um/kernel/skas/uaccess.c +++ b/arch/um/kernel/skas/uaccess.c @@ -69,20 +69,26 @@ static int do_op_one_page(unsigned long addr, int len, int is_write, return -1; page = pte_page(*pte); + current->thread.fault_catcher = &buf; + faulted = UML_SETJMP(&buf); +#ifndef CONFIG_64BIT addr = (unsigned long) kmap_atomic(page) + (addr & ~PAGE_MASK); +#else + addr = (unsigned long) page_address(page) + + (addr & ~PAGE_MASK); +#endif - current->thread.fault_catcher = &buf; - faulted = UML_SETJMP(&buf); if (faulted == 0) n = (*op)(addr, len, arg); else n = -1; current->thread.fault_catcher = NULL; - +#ifndef CONFIG_64BIT kunmap_atomic((void *)addr); +#endif return n; }