Bump!
On Monday, February 10, 2020 at 12:14:52 AM UTC-5, Waldek Kozaczuk wrote:
>
> Nadav,
>
> What do you think about this patch? Also, do you think that the deadlock
> issue related to *vma_list_mutex *I discovered and had to overcome (see
> my previous email in this chain) is the only one we need to worry about or
> are there any potential hidden problems like that I have missed to notice?
>
> Shall I make this lazy-stack feature activated with #ifdef and only if we
> feel confident we could make it permanent?
>
> Waldek
>
> On Sunday, February 2, 2020 at 11:15:30 AM UTC-5, Waldek Kozaczuk wrote:
>>
>> As the issue #143 explains, currently applications stacks are eagerly
>> allocated (aka pre-populated) which may lead to substantial memory waste.
>> We want those stacks to be lazily allocated instead, so that physical
>> memory behind the stack gets allocated as needed by standard fault
>> handling mechanism.
>>
>> The page fault handling logic requires that both interrupts and
>> preemption
>> are enabled. Therefore this patch implements simple strategy to read
>> single byte on a stack
>> one page (4K) deeper before disabling interrupts or preemption.
>>
>> It turns out we cannot read that single byte "blindly" because in some
>> scenarios
>> interrupts might be already disabled when we are about to disable
>> preemption and vice versa.
>> Also similarly, which is more speculative, disabling preemption (and
>> possibly interrupts) may nest.
>> Therefore we need a "conditional" read of that byte on stack. More
>> specifically
>> we should ONLY read the byte IF both interrupts and preemption are
>> enabled.
>>
>> This patch achieves this by adding extra thread-local counter to track
>> interrupts disabling/enabling
>> that operates in a similar way the preemption counter does. So every time
>> interrupts
>> are disabled we increment the counter and decrement it everytime
>> interrupts are enabled back.
>> Then everytime before we disable interrupts or preemption, we check if
>> both counters
>> are 0 and only then try to read a byte to potentially trigger a fault.
>>
>> Please note that performance-wise this patch is slighy more expensive
>> than it
>> was originally hoped. As the disassembled snippets illustrate it costs
>> extra 3-4 instructions
>> extra every time we disable preemption or interrupts.
>>
>> A. Disabling preemption in __tls_get_addr before the patch:
>> ```
>> 4035cae8: e8 83 f6 ff ff callq 4035c170 <elf::get_program()>
>> 4035caed: 4c 8b 2d 6c 40 51 00 mov 0x51406c(%rip),%r13 #
>> 40870b60 <sched::preempt_counter+0x40870b60>
>> 4035caf4: 4c 8b 33 mov (%rbx),%r14
>> 4035caf7: 64 41 83 45 00 01 addl $0x1,%fs:0x0(%r13)
>> 4035cafd: e8 6e f6 ff ff callq 4035c170 <elf::get_program()>
>> ```
>>
>> B. Disabling preemption in __tls_get_addr after the patch:
>> ```
>> 4035fdd8: e8 63 f6 ff ff callq 4035f440 <elf::get_program()>
>> 4035fddd: 4c 8b 2d a4 4d 51 00 mov 0x514da4(%rip),%r13 #
>> 40874b88 <arch::irq_preempt_counters+0x40874b88>
>> 4035fde4: 4c 8b 33 mov (%rbx),%r14
>> 4035fde7: 64 49 83 7d 00 00 cmpq $0x0,%fs:0x0(%r13)
>> 4035fded: 75 07 jne 4035fdf6
>> <__tls_get_addr+0x76>
>> 4035fdef: 8a 84 24 00 f0 ff ff mov -0x1000(%rsp),%al
>> 4035fdf6: 64 41 83 45 00 01 addl $0x1,%fs:0x0(%r13)
>> 4035fdfc: e8 3f f6 ff ff callq 4035f440 <elf::get_program()>
>> ```
>>
>> As an example of memory saving, tomcat using ~ 300 thread ended up 365MB
>> instead
>> of 620MB before the patch.
>>
>> Fixes #143
>>
>> Signed-off-by: Matthew Pabst <pabstmatt...@gmail.com>
>> Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
>> ---
>> arch/aarch64/arch.hh | 5 ++++
>> arch/x64/arch-switch.hh | 12 ++++++++++
>> arch/x64/arch.hh | 34 ++++++++++++++++++++++++++
>> arch/x64/mmu.cc | 5 ++++
>> core/mmu.cc | 24 +++++++++++++++++++
>> core/sched.cc | 6 ++++-
>> include/osv/counters.hh | 53 +++++++++++++++++++++++++++++++++++++++++
>> include/osv/irqlock.hh | 4 ++++
>> include/osv/mmu-defs.hh | 3 +--
>> include/osv/sched.hh | 10 ++++----
>> libc/mman.cc | 9 ++++---
>> libc/pthread.cc | 10 +++++++-
>> 12 files changed, 162 insertions(+), 13 deletions(-)
>> create mode 100644 include/osv/counters.hh
>>
>> diff --git a/arch/aarch64/arch.hh b/arch/aarch64/arch.hh
>> index 855f1987..473e84e9 100644
>> --- a/arch/aarch64/arch.hh
>> +++ b/arch/aarch64/arch.hh
>> @@ -20,6 +20,11 @@ namespace arch {
>> #define INSTR_SIZE_MIN 4
>> #define ELF_IMAGE_START (OSV_KERNEL_BASE + 0x10000)
>>
>> +inline void ensure_next_stack_page()
>> +{
>> + //TODO: Implement lazy stack check for AARCH64
>> +}
>> +
>> inline void irq_disable()
>> {
>> processor::irq_disable();
>> diff --git a/arch/x64/arch-switch.hh b/arch/x64/arch-switch.hh
>> index 6803498f..3e52f7f8 100644
>> --- a/arch/x64/arch-switch.hh
>> +++ b/arch/x64/arch-switch.hh
>> @@ -148,6 +148,13 @@ void thread::init_stack()
>> _state.rip = reinterpret_cast<void*>(thread_main);
>> _state.rsp = stacktop;
>> _state.exception_stack = _arch.exception_stack +
>> sizeof(_arch.exception_stack);
>> +
>> + if (stack.lazy) {
>> + // If the thread stack is setup to get lazily allocated and
>> given the thread initially starts
>> + // running with preemption disabled, we need to pre-fault the
>> first stack page.
>> + volatile char r = *((char *) (stacktop - 1));
>> + (void) r; // trick the compiler into thinking that r is used
>> + }
>> }
>>
>> void thread::setup_tcb()
>> @@ -311,6 +318,11 @@ void thread::free_tcb()
>>
>> void thread_main_c(thread* t)
>> {
>> + // Disable lazy stack pre-fault logic for this thread
>> + // if stack is not lazy (most likely kernel thread)
>> + if (t->get_stack_info().lazy) {
>> +
>>
>> arch::irq_preempt_counters._data.set_lazy_flags_bit(arch::lazy_stack_disable,
>>
>> false);
>> + }
>> arch::irq_enable();
>> #ifdef CONF_preempt
>> preempt_enable();
>> diff --git a/arch/x64/arch.hh b/arch/x64/arch.hh
>> index 17df5f5c..74b728dc 100644
>> --- a/arch/x64/arch.hh
>> +++ b/arch/x64/arch.hh
>> @@ -10,6 +10,8 @@
>>
>> #include "processor.hh"
>> #include "msr.hh"
>> +#include <osv/barrier.hh>
>> +#include <osv/counters.hh>
>>
>> // namespace arch - architecture independent interface for architecture
>> // dependent operations (e.g. irq_disable vs. cli)
>> @@ -20,8 +22,33 @@ namespace arch {
>> #define INSTR_SIZE_MIN 1
>> #define ELF_IMAGE_START OSV_KERNEL_BASE
>>
>> +inline void ensure_next_stack_page() {
>> + // Because both irq and preempt counters and lazy stack
>> + // flags are co-located within same 8-bytes long union
>> + // we can check single field for non-zero value to determine
>> + // if we should trigger pre-fault on lazy stack
>> + if (irq_preempt_counters.lazy_stack_no_pre_fault) {
>> + return;
>> + }
>> +
>> + char i;
>> + asm volatile("movb -4096(%%rsp), %0" : "=r"(i));
>> +}
>> +
>> +inline void ensure_next_two_stack_pages() {
>> + if (irq_preempt_counters.lazy_stack_no_pre_fault) {
>> + return;
>> + }
>> +
>> + char i;
>> + asm volatile("movb -4096(%%rsp), %0" : "=r"(i));
>> + asm volatile("movb -8192(%%rsp), %0" : "=r"(i));
>> +}
>> +
>> inline void irq_disable()
>> {
>> + ensure_next_stack_page();
>> + ++irq_preempt_counters._data.irq;
>> processor::cli();
>> }
>>
>> @@ -36,11 +63,15 @@ inline void irq_disable_notrace()
>> inline void irq_enable()
>> {
>> processor::sti();
>> + --irq_preempt_counters._data.irq;
>> + barrier();
>> }
>>
>> inline void wait_for_interrupt()
>> {
>> processor::sti_hlt();
>> + --irq_preempt_counters._data.irq;
>> + barrier();
>> }
>>
>> inline void halt_no_interrupts()
>> @@ -78,12 +109,15 @@ private:
>>
>> inline void irq_flag_notrace::save()
>> {
>> + ensure_next_stack_page();
>> + ++irq_preempt_counters._data.irq;
>> asm volatile("sub $128, %%rsp; pushfq; popq %0; add $128, %%rsp" :
>> "=r"(_rflags));
>> }
>>
>> inline void irq_flag_notrace::restore()
>> {
>> asm volatile("sub $128, %%rsp; pushq %0; popfq; add $128, %%rsp" : :
>> "r"(_rflags));
>> + --irq_preempt_counters._data.irq;
>> }
>>
>> inline bool irq_flag_notrace::enabled() const
>> diff --git a/arch/x64/mmu.cc b/arch/x64/mmu.cc
>> index 24da5caa..ae619a9f 100644
>> --- a/arch/x64/mmu.cc
>> +++ b/arch/x64/mmu.cc
>> @@ -37,6 +37,11 @@ void page_fault(exception_frame *ef)
>> assert(sched::preemptable());
>> assert(ef->rflags & processor::rflags_if);
>>
>> + // Even though we are on exception stack that is not lazy
>> + // for pre-caution we are disabling the pre-fault logic
>> + // for the time the fault is being handled
>> + arch::lazy_stack_flags_guard
>> lazy_stack_guard(arch::lazy_stack_fault_disable);
>> +
>> // And since we may sleep, make sure interrupts are enabled.
>> DROP_LOCK(irq_lock) { // irq_lock is acquired by HW
>> mmu::vm_fault(addr, ef);
>> diff --git a/core/mmu.cc b/core/mmu.cc
>> index ff3fab47..b6e7cc36 100644
>> --- a/core/mmu.cc
>> +++ b/core/mmu.cc
>> @@ -42,6 +42,18 @@ extern const char text_start[], text_end[];
>>
>> namespace mmu {
>>
>> +static void prevent_stack_page_fault() {
>> + // We need to ensure that lazy stack is populated
>> + // deeply enough (2 pages) for all cases where
>> + // the vma_list_mutex is taken for write so that we can disable
>> + // the pre-fault check (arch::ensure_next_stack_page) which,
>> + // if page fault triggered, would make page-fault handling logic
>> + // attempt to take same vma_list_mutex fo read and end up with
>> deadlock
>> +#ifndef AARCH64_PORT_STUB
>> + arch::ensure_next_two_stack_pages();
>> +#endif
>> +}
>> +
>> namespace bi = boost::intrusive;
>>
>> class vma_compare {
>> @@ -1183,6 +1195,8 @@ static void nohugepage(void* addr, size_t length)
>>
>> error advise(void* addr, size_t size, int advice)
>> {
>> + prevent_stack_page_fault();
>> + arch::lazy_stack_flags_guard
>> lazy_stack_guard(arch::lazy_stack_mmu_disable);
>> WITH_LOCK(vma_list_mutex.for_write()) {
>> if (!ismapped(addr, size)) {
>> return make_error(ENOMEM);
>> @@ -1215,6 +1229,8 @@ void* map_anon(const void* addr, size_t size,
>> unsigned flags, unsigned perm)
>> size = align_up(size, mmu::page_size);
>> auto start = reinterpret_cast<uintptr_t>(addr);
>> auto* vma = new mmu::anon_vma(addr_range(start, start + size), perm,
>> flags);
>> + prevent_stack_page_fault();
>> + arch::lazy_stack_flags_guard
>> lazy_stack_guard(arch::lazy_stack_mmu_disable);
>> SCOPE_LOCK(vma_list_mutex.for_write());
>> auto v = (void*) allocate(vma, start, size, search);
>> if (flags & mmap_populate) {
>> @@ -1241,6 +1257,8 @@ void* map_file(const void* addr, size_t size,
>> unsigned flags, unsigned perm,
>> auto start = reinterpret_cast<uintptr_t>(addr);
>> auto *vma = f->mmap(addr_range(start, start + size), flags |
>> mmap_file, perm, offset).release();
>> void *v;
>> + prevent_stack_page_fault();
>> + arch::lazy_stack_flags_guard
>> lazy_stack_guard(arch::lazy_stack_mmu_disable);
>> WITH_LOCK(vma_list_mutex.for_write()) {
>> v = (void*) allocate(vma, start, size, search);
>> if (flags & mmap_populate) {
>> @@ -1607,6 +1625,8 @@ ulong map_jvm(unsigned char* jvm_addr, size_t size,
>> size_t align, balloon_ptr b)
>>
>> auto* vma = new mmu::jvm_balloon_vma(jvm_addr, start, start + size,
>> b, v->perm(), v->flags());
>>
>> + prevent_stack_page_fault();
>> + arch::lazy_stack_flags_guard
>> lazy_stack_guard(arch::lazy_stack_mmu_disable);
>> WITH_LOCK(vma_list_mutex.for_write()) {
>> // This means that the mapping that we had before was a balloon
>> mapping
>> // that was laying around and wasn't updated to an anon mapping.
>> If we
>> @@ -1868,6 +1888,8 @@ void free_initial_memory_range(uintptr_t addr,
>> size_t size)
>>
>> error mprotect(const void *addr, size_t len, unsigned perm)
>> {
>> + prevent_stack_page_fault();
>> + arch::lazy_stack_flags_guard
>> lazy_stack_guard(arch::lazy_stack_mmu_disable);
>> SCOPE_LOCK(vma_list_mutex.for_write());
>>
>> if (!ismapped(addr, len)) {
>> @@ -1879,6 +1901,8 @@ error mprotect(const void *addr, size_t len,
>> unsigned perm)
>>
>> error munmap(const void *addr, size_t length)
>> {
>> + prevent_stack_page_fault();
>> + arch::lazy_stack_flags_guard
>> lazy_stack_guard(arch::lazy_stack_mmu_disable);
>> SCOPE_LOCK(vma_list_mutex.for_write());
>>
>> length = align_up(length, mmu::page_size);
>> diff --git a/core/sched.cc b/core/sched.cc
>> index 06f849d1..4510a85e 100644
>> --- a/core/sched.cc
>> +++ b/core/sched.cc
>> @@ -42,6 +42,11 @@ extern char _percpu_start[], _percpu_end[];
>> using namespace osv;
>> using namespace osv::clock::literals;
>>
>> +namespace arch {
>> +// By default disable lazy stack pre-fault logic
>> +counters __thread irq_preempt_counters = {{1, 1, 1}};
>> +}
>> +
>> namespace sched {
>>
>> TRACEPOINT(trace_sched_idle, "");
>> @@ -69,7 +74,6 @@ std::vector<cpu*> cpus
>> __attribute__((init_priority((int)init_prio::cpus)));
>> thread __thread * s_current;
>> cpu __thread * current_cpu;
>>
>> -unsigned __thread preempt_counter = 1;
>> bool __thread need_reschedule = false;
>>
>> elf::tls_data tls;
>> diff --git a/include/osv/counters.hh b/include/osv/counters.hh
>> new file mode 100644
>> index 00000000..b862f32c
>> --- /dev/null
>> +++ b/include/osv/counters.hh
>> @@ -0,0 +1,53 @@
>> +/*
>> + * Copyright (C) 2020 Waldemar Kozaczuk
>> + *
>> + * This work is open source software, licensed under the terms of the
>> + * BSD license as described in the LICENSE file in the top-level
>> directory.
>> + */
>> +
>> +#ifndef OSV_COUNTERS_HH
>> +#define OSV_COUNTERS_HH
>> +
>> +namespace arch {
>> +
>> +// Both preempt and irq counters are co-located next to each other
>> +// along with the the flags that drive lazy stack related logic
>> +// union by design. It is so that compiler can optimize
>> ensure_next_stack_page()
>> +// to test against single 64-bit lazy_stack_no_pre_fault field
>> +union counters {
>> + struct data {
>> + unsigned preempt;
>> + u16 irq;
>> + u16 lazy_stack_flags;
>> +
>> + inline void set_lazy_flags_bit(unsigned nr, bool v) {
>> + lazy_stack_flags &= ~(u64(1) << nr);
>> + lazy_stack_flags |= u64(v) << nr;
>> + }
>> + } _data;
>> + uint64_t lazy_stack_no_pre_fault;
>> +};
>> +
>> +extern counters __thread irq_preempt_counters;
>> +
>> +class lazy_stack_flags_guard {
>> + unsigned _flag_nr;
>> +public:
>> + lazy_stack_flags_guard(unsigned flag_nr) {
>> + _flag_nr = flag_nr;
>> + irq_preempt_counters._data.set_lazy_flags_bit(_flag_nr, true);
>> + }
>> + ~lazy_stack_flags_guard() {
>> + irq_preempt_counters._data.set_lazy_flags_bit(_flag_nr, false);
>> + }
>> +};
>> +
>> +enum {
>> + lazy_stack_disable = 0,
>> + lazy_stack_mmu_disable = 1,
>> + lazy_stack_fault_disable = 2,
>> +};
>> +
>> +}
>> +
>> +#endif //OSV_COUNTERS_HH
>> diff --git a/include/osv/irqlock.hh b/include/osv/irqlock.hh
>> index 2d5372fc..1e0ab6fa 100644
>> --- a/include/osv/irqlock.hh
>> +++ b/include/osv/irqlock.hh
>> @@ -34,6 +34,10 @@ inline void irq_save_lock_type::lock()
>> inline void irq_save_lock_type::unlock()
>> {
>> _flags.restore();
>> +#ifndef AARCH64_PORT_STUB
>> + --arch::irq_preempt_counters._data.irq;
>> + barrier();
>> +#endif
>> }
>>
>> extern irq_lock_type irq_lock;
>> diff --git a/include/osv/mmu-defs.hh b/include/osv/mmu-defs.hh
>> index 18edf441..a731ed6e 100644
>> --- a/include/osv/mmu-defs.hh
>> +++ b/include/osv/mmu-defs.hh
>> @@ -15,8 +15,6 @@
>>
>> struct exception_frame;
>>
>> -struct exception_frame;
>> -
>> namespace mmu {
>>
>> constexpr uintptr_t page_size = 4096;
>> @@ -84,6 +82,7 @@ enum {
>> mmap_small = 1ul << 5,
>> mmap_jvm_balloon = 1ul << 6,
>> mmap_file = 1ul << 7,
>> + mmap_stack = 1ul << 8,
>> };
>>
>> enum {
>> diff --git a/include/osv/sched.hh b/include/osv/sched.hh
>> index 0fb44f77..96453a8b 100644
>> --- a/include/osv/sched.hh
>> +++ b/include/osv/sched.hh
>> @@ -25,6 +25,7 @@
>> #include <osv/rcu.hh>
>> #include <osv/clock.hh>
>> #include <osv/timer-set.hh>
>> +#include "osv/counters.hh"
>> #include <string.h>
>>
>> typedef float runtime_t;
>> @@ -335,6 +336,7 @@ public:
>> void* begin;
>> size_t size;
>> void (*deleter)(stack_info si); // null: don't delete
>> + bool lazy = false;
>> static void default_deleter(stack_info si);
>> };
>> struct attr {
>> @@ -978,14 +980,13 @@ inline void release(mutex_t* mtx)
>> }
>> }
>>
>> -extern unsigned __thread preempt_counter;
>> extern bool __thread need_reschedule;
>>
>> #ifdef __OSV_CORE__
>> inline unsigned int get_preempt_counter()
>> {
>> barrier();
>> - return preempt_counter;
>> + return arch::irq_preempt_counters._data.preempt;
>> }
>>
>> inline bool preemptable()
>> @@ -1005,14 +1006,15 @@ inline void preempt()
>>
>> inline void preempt_disable()
>> {
>> - ++preempt_counter;
>> + arch::ensure_next_stack_page();
>> + ++arch::irq_preempt_counters._data.preempt;
>> barrier();
>> }
>>
>> inline void preempt_enable()
>> {
>> barrier();
>> - --preempt_counter;
>> + --arch::irq_preempt_counters._data.preempt;
>> if (preemptable() && need_reschedule && arch::irq_enabled()) {
>> cpu::schedule();
>> }
>> diff --git a/libc/mman.cc b/libc/mman.cc
>> index d0803ac4..d00befc3 100644
>> --- a/libc/mman.cc
>> +++ b/libc/mman.cc
>> @@ -38,12 +38,11 @@ unsigned libc_flags_to_mmap(int flags)
>> mmap_flags |= mmu::mmap_populate;
>> }
>> if (flags & MAP_STACK) {
>> - // OSv currently requires that stacks be pinned (see issue
>> #143). So
>> - // if an application wants to mmap() a stack for
>> pthread_attr_setstack
>> - // and did us the courtesy of telling this to ue (via
>> MAP_STACK),
>> - // let's return the courtesy by returning pre-faulted memory.
>> - // FIXME: If issue #143 is fixed, this workaround should be
>> removed.
>> +#ifdef AARCH64_PORT_STUB
>> mmap_flags |= mmu::mmap_populate;
>> +#else
>> + mmap_flags |= mmu::mmap_stack;
>> +#endif
>> }
>> if (flags & MAP_SHARED) {
>> mmap_flags |= mmu::mmap_shared;
>> diff --git a/libc/pthread.cc b/libc/pthread.cc
>> index 8c976bf6..2279ac0c 100644
>> --- a/libc/pthread.cc
>> +++ b/libc/pthread.cc
>> @@ -140,10 +140,18 @@ namespace pthread_private {
>> return {attr.stack_begin, attr.stack_size};
>> }
>> size_t size = attr.stack_size;
>> - void *addr = mmu::map_anon(nullptr, size, mmu::mmap_populate,
>> mmu::perm_rw);
>> +#ifdef AARCH64_PORT_STUB
>> + unsigned stack_flags = mmu::mmap_populate;
>> +#else
>> + unsigned stack_flags = mmu::mmap_stack;
>> +#endif
>> + void *addr = mmu::map_anon(nullptr, size, stack_flags,
>> mmu::perm_rw);
>> mmu::mprotect(addr, attr.guard_size, 0);
>> sched::thread::stack_info si{addr, size};
>> si.deleter = free_stack;
>> +#ifndef AARCH64_PORT_STUB
>> + si.lazy = true;
>> +#endif
>> return si;
>> }
>>
>> --
>> 2.20.1
>>
>>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/97486200-a8f5-4e69-a464-15678882bb40%40googlegroups.com.
