On Tue, Aug 30, 2022 at 6:59 PM Commit Bot <b...@cloudius-systems.com> wrote:
> From: Waldemar Kozaczuk <jwkozac...@gmail.com>
> Committer: Waldemar Kozaczuk <jwkozac...@gmail.com>
> Branch: master
>
> trace: do not use malloc/free when interrupts are disabled
>
> This patch fixes a subtle bug found when working on lazy stack changes
> and trying to establish some invariants in relevant places in the kernel
> code.
> One of the findings was that the memory allocation logic or more
> specifically the functions malloc() and free() require both interrupts and
> preemption enabled to function correctly.
>
> If one starts analysis with memory::pool::alloc(), he/she will notice
> a DROP_LOCK(preempt_lock) executed under WITH_LOCK(preempt) if
> the pool is empty. This means that the code assumes the preemption must
> be enabled before calling alloc(), otherwise the code in DROP_LOCK would
> run with preemption disabled which is contrary to the intention.
> Also, after drilling down the add_page() calling path, one eventually
> would find the call to reclaimers::wait() which calls
> thread::wait_until(). The
> thread::do_wait_until() called by the former enforces that both interrupts
> and preemption needs to be enabled with assertion.
>
> The bottom line of the analysis above is that both interrupts and
> preemption
> must be enabled before calling malloc() (same applies to free()). This
> effectively
> means that trying to call malloc() after disabling interrupts might not
> work (which
> would only happen if relevent pool was out of free pages AND l1 pool was
> empty and
> had to refill from l2 pool which probably is quite rare).
>
> So this patch changes the code in trace::create_trace_dump() to avoid
> calling new/malloc() (see copies.emplace_back()) and instead makes it
> preallocate
> the vector ahead of time and then simply copy the trace buffer to a
> relevant
> spot in the vector.
>
> Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
>
> ---
> diff --git a/core/trace.cc b/core/trace.cc
> --- a/core/trace.cc
> +++ b/core/trace.cc
> @@ -697,7 +697,7 @@ std::string
> trace::create_trace_dump()
> {
> semaphore signal(0);
> - std::vector<trace_buf> copies;
> + std::vector<trace_buf> copies(sched::cpus.size());
>
> auto is_valid_tracepoint = [](const tracepoint_base * tp_test) {
> for (auto & tp : tracepoint_base::tp_list) {
> @@ -717,7 +717,7 @@ trace::create_trace_dump()
> irq.save();
> arch::irq_disable_notrace();
> auto * tbp = percpu_trace_buffer.for_cpu(cpu);
> - copies.emplace_back(*tbp);
> + copies[i] = trace_buf(*tbp);
>
This change is good, but I think you didn't reall have to change the
emplace_back() call - you could have kept the emplace_back() and instead of
the constructor in the beginning, use copies.reserve(sched::cpus.size()).
irq.restore();
> signal.post();
> }, sched::thread::attr().pin(cpu)));
>
> --
> 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/0000000000000da02b05e7777708%40google.com
> .
>
--
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/CANEVyjvonrc5fPy-SDWY3r6VfYjhCHDYmgd38%2ByGHdY1GVnC1g%40mail.gmail.com.
