linux-kernel - Re: [PATCH v1 4/5] seqlock: seqcount_LOCKTYPE_t: Introduce PREEMPT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200828093130.GA7672@lx-t490>
Date:   Fri, 28 Aug 2020 11:31:30 +0200
From:   "Ahmed S. Darwish" <a.darwish@...utronix.de>
To:     peterz@...radead.org
Cc:     Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "Sebastian A. Siewior" <bigeasy@...utronix.de>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 4/5] seqlock: seqcount_LOCKTYPE_t: Introduce
 PREEMPT_RT support

On Fri, Aug 28, 2020 at 10:59:38AM +0200, peterz@...radead.org wrote:
> On Fri, Aug 28, 2020 at 03:07:09AM +0200, Ahmed S. Darwish wrote:
> > +#define __SEQ_RT	IS_ENABLED(CONFIG_PREEMPT_RT)
> > +
> > +SEQCOUNT_LOCKTYPE(raw_spinlock, raw_spinlock_t,  false,    s->lock,        raw_spin, raw_spin_lock(s->lock))
> > +SEQCOUNT_LOCKTYPE(spinlock,     spinlock_t,      __SEQ_RT, s->lock,        spin,     spin_lock(s->lock))
> > +SEQCOUNT_LOCKTYPE(rwlock,       rwlock_t,        __SEQ_RT, s->lock,        read,     read_lock(s->lock))
> > +SEQCOUNT_LOCKTYPE(mutex,        struct mutex,    true,     s->lock,        mutex,    mutex_lock(s->lock))
> > +SEQCOUNT_LOCKTYPE(ww_mutex,     struct ww_mutex, true,     &s->lock->base, ww_mutex, ww_mutex_lock(s->lock, NULL))
>
> Ooh, reading is hard, but ^^^^ you already have that.
>

Haha, I was just answering the other mail :)

> > +/*
> > + * Automatically disable preemption for seqcount_LOCKTYPE_t writers, if the
> > + * associated lock does not implicitly disable preemption.
> > + *
> > + * Don't do it for PREEMPT_RT. Check __SEQ_LOCK().
> > + */
> > +#define __seq_enforce_preemption_protection(s)				\
> > +	(!IS_ENABLED(CONFIG_PREEMPT_RT) && __seqcount_lock_preemptible(s))
>
> Then what is this doing ?!? I'm confused now.

There were a number of call sites (at DRM mainly) that protected their
seqcount_t write sections with mutex and ww_mutex. So, they manually
disabled preemption before entering the seqcount_t write sections.

Unfortunately these write sections were big, and spinlocks were also
acquired inside them.  This was all very bad for RT...

So the idea was to relieve call sites from the responsibility of
disabling/enabling preemption upon entering a seqcount_LOCKNAME_t write
section, and let seqlock.h automatically handle it if LOCKNAME_t is
preemptible (the macro's condition, second part).

Having seqlock.h control the preempt disable/enable allows us to never
disable preemption for RT, which is exactly what is needed since we
already handle any possible livelock through the mechanisms described at
__SEQ_LOCK (the macro's condition test, first part).

Thanks,

--
Ahmed S. Darwish
Linutronix GmbH