From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Clark Williams <williams@redhat.com>,
Steven Rostedt <rostedt@goodmis.org>,
Dmitry Vyukov <dvyukov@google.com>,
Andrey Konovalov <andreyknvl@gmail.com>,
kasan-dev@googlegroups.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH PREEMPT_RT] kcov: fix locking splat from kcov_remote_start()
Date: Wed, 11 Aug 2021 11:00:33 +0200 [thread overview]
Message-ID: <20210811090033.wijh4v37wlnny3ox@linutronix.de> (raw)
In-Reply-To: <87sfzhox15.ffs@tglx>
On 2021-08-10 22:38:30 [+0200], Thomas Gleixner wrote:
> On Tue, Aug 10 2021 at 11:50, Sebastian Andrzej Siewior wrote:
> > On 2021-08-09 15:59:09 [-0500], Clark Williams wrote:
> >> Saw the following splat on 5.14-rc4-rt5 with:
> > …
> >> Change kcov_remote_lock from regular spinlock_t to raw_spinlock_t so that
> >> we don't get "sleeping function called from invalid context" on PREEMPT_RT kernel.
> >
> > I'm not entirely happy with that:
> > - kcov_remote_start() decouples spin_lock_irq() and does local_irq_save()
> > + spin_lock() which shouldn't be done as per
> > Documentation/locking/locktypes.rst
> > I would prefer to see the local_irq_save() replaced by
> > local_lock_irqsave() so we get a context on what is going on.
>
> Which does not make it raw unless we create a raw_local_lock.
But why raw? I was thinking about local_lock_irqsave() instead of
local_irq_save() and keeping the spinlock_t.
> > - kcov_remote_reset() has a kfree() with that irq-off lock acquired.
>
> That free needs to move out obviously
>
> > - kcov_remote_add() has a kmalloc() and is invoked with that irq-off
> > lock acquired.
>
> So does the kmalloc.
>
> > - kcov_remote_area_put() uses INIT_LIST_HEAD() for no reason (just
> > happen to notice).
> >
> > - kcov_remote_stop() does local_irq_save() + spin_lock(&kcov->lock);.
> > This should also create a splat.
> >
> > - With lock kcov_remote_lock acquired there is a possible
> > hash_for_each_safe() and list_for_each() iteration. I don't know what
> > the limits are here but with a raw_spinlock_t it will contribute to
> > the maximal latency.
>
> And that matters because? kcov has a massive overhead and with that
> enabled you care as much about latencies as you do when running with
> lockdep enabled.
I wasn't aware of that. However, with that local_irq_save() ->
local_lock_irqsave() swap and that first C code from
Documentation/dev-tools/kcov.rst I don't see any spike in cyclictest's
results. Maybe I'm not using it right…
> Thanks,
>
> tglx
Sebastian
next prev parent reply other threads:[~2021-08-11 9:00 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-09 20:59 [PATCH PREEMPT_RT] kcov: fix locking splat from kcov_remote_start() Clark Williams
2021-08-10 9:42 ` Marco Elver
2021-08-10 9:50 ` Sebastian Andrzej Siewior
2021-08-10 19:14 ` Steven Rostedt
2021-08-10 20:38 ` Thomas Gleixner
2021-08-11 9:00 ` Sebastian Andrzej Siewior [this message]
2021-08-11 12:25 ` Thomas Gleixner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210811090033.wijh4v37wlnny3ox@linutronix.de \
--to=bigeasy@linutronix.de \
--cc=andreyknvl@gmail.com \
--cc=dvyukov@google.com \
--cc=kasan-dev@googlegroups.com \
--cc=linux-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=williams@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.