linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dmitry Vyukov <dvyukov@google.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Clark Williams <williams@redhat.com>,
	Alexander Potapenko <glider@google.com>,
	kasan-dev <kasan-dev@googlegroups.com>,
	Linux-MM <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-rt-users@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH] kasan: convert kasan/quarantine_lock to raw_spinlock
Date: Wed, 10 Oct 2018 10:25:42 +0200	[thread overview]
Message-ID: <CACT4Y+ZB38pKvT8+BAjDZ1t4ZjXQQKoya+ytXT+ASQxHUkWwnA@mail.gmail.com> (raw)
In-Reply-To: <20181009142742.ikh7xv2dn5skjjbe@linutronix.de>

On Tue, Oct 9, 2018 at 4:27 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 2018-10-08 11:15:57 [+0200], Dmitry Vyukov wrote:
>> Hi Sebastian,
> Hi Dmitry,
>
>> This seems to beak quarantine_remove_cache( ) in the sense that some
>> object from the cache may still be in quarantine when
>> quarantine_remove_cache() returns. When quarantine_remove_cache()
>> returns all objects from the cache must be purged from quarantine.
>> That srcu and irq trickery is there for a reason.
>
> That loop should behave like your on_each_cpu() except it does not
> involve the remote CPU.


The problem is that it can squeeze in between:

+               spin_unlock(&q->lock);

                spin_lock(&quarantine_lock);

as far as I see. And then some objects can be left in the quarantine.


>> This code is also on hot path of kmallock/kfree, an additional
>> lock/unlock per operation is expensive. Adding 2 locked RMW per
>> kmalloc is not something that should be done only out of refactoring
>> reasons.
> But this is debug code anyway, right? And it is highly complex imho.
> Well, maybe only for me after I looked at it for the first time…

It is debug code - yes.
Nothing about its performance matters - no.

That's the way to produce unusable debug tools.
With too much overhead timeouts start to fire and code behaves not the
way it behaves in production.
The tool is used in continuous integration and developers wait for
test results before merging code.
The tool is used on canary devices and directly contributes to usage experience.

We of course don't want to trade a page of assembly code for cutting
few cycles here (something that could make sense for some networking
code maybe). But otherwise let's not introduce spinlocks on fast paths
just for refactoring reasons.


>> The original message from Clark mentions that the problem can be fixed
>> by just changing type of spinlock. This looks like a better and
>> simpler way to resolve the problem to me.
>
> I usually prefer to avoid adding raw_locks everywhere if it can be
> avoided. However given that this is debug code and a few additional us
> shouldn't matter here, I have no problem with Clark's initial patch
> (also the mem-free in irq-off region works in this scenario).
> Can you take it as-is or should I repost it with an acked-by?

Perhaps it's the problem with the way RT kernel changes things then?
This is not specific to quarantine, right? Should that mutex detect
that IRQs are disabled and not try to schedule? If this would happen
in some networking code, what would we do?

  reply	other threads:[~2018-10-10  8:26 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-18 15:29 [PATCH RT] rt: convert mm/kasan/quarantine_lock to raw_spinlock Clark Williams
2018-09-18 20:34 ` kbuild test robot
2018-10-05 16:37   ` Sebastian Andrzej Siewior
2018-10-18  9:04     ` [kbuild-all] " Rong Chen
2018-10-05 16:30 ` [PATCH] kasan: convert kasan/quarantine_lock " Sebastian Andrzej Siewior
2018-10-05 16:33   ` Sebastian Andrzej Siewior
2018-10-08  0:47     ` Clark Williams
2018-10-08  9:15     ` Dmitry Vyukov
2018-10-09 14:27       ` Sebastian Andrzej Siewior
2018-10-10  8:25         ` Dmitry Vyukov [this message]
2018-10-10  9:29           ` Sebastian Andrzej Siewior
2018-10-10  9:45             ` Dmitry Vyukov
2018-10-10  9:53               ` Sebastian Andrzej Siewior
2018-10-10  9:57                 ` Dmitry Vyukov
2018-10-10 21:49                   ` [PATCH] mm/kasan: make quarantine_lock a raw_spinlock_t Sebastian Andrzej Siewior
2018-10-11  8:03                     ` Dmitry Vyukov
2018-10-12 23:56                     ` Andrew Morton
2018-10-13 13:50                       ` Peter Zijlstra
2018-10-15 23:35                         ` Andrew Morton
2018-10-19 10:58                           ` Peter Zijlstra

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=CACT4Y+ZB38pKvT8+BAjDZ1t4ZjXQQKoya+ytXT+ASQxHUkWwnA@mail.gmail.com \
    --to=dvyukov@google.com \
    --cc=bigeasy@linutronix.de \
    --cc=glider@google.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=peterz@infradead.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).