All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dmitry Safonov <dima@arista.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org,
	"Dmitry Safonov" <0x7f454c46@gmail.com>,
	"Daniel Axtens" <dja@axtens.net>,
	"Dmitry Vyukov" <dvyukov@google.com>,
	"Michael Neuling" <mikey@neuling.org>,
	"Mikulas Patocka" <mpatocka@redhat.com>,
	"Nathan March" <nathan@gt.net>, "Pasi Kärkkäinen" <pasik@iki.fi>,
	"Peter Hurley" <peter@hurleysoftware.com>,
	"Rong, Chen" <rong.a.chen@intel.com>,
	"Sergey Senozhatsky" <sergey.senozhatsky.work@gmail.com>,
	"Tan Xiaojun" <tanxiaojun@huawei.com>,
	"Tetsuo Handa" <penguin-kernel@I-love.SAKURA.ne.jp>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Jiri Slaby" <jslaby@suse.com>
Subject: Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
Date: Tue, 11 Sep 2018 14:01:58 +0100	[thread overview]
Message-ID: <1536670918.2710.29.camel@arista.com> (raw)
In-Reply-To: <20180911120258.GC19234@hirez.programming.kicks-ass.net>

On Tue, 2018-09-11 at 14:02 +0200, Peter Zijlstra wrote:
> On Tue, Sep 11, 2018 at 02:48:21AM +0100, Dmitry Safonov wrote:
> > It seems like when ldsem_down_read() fails with timeout, it misses
> > update for sem->wait_readers. By that reason, when writer finally
> > releases write end of the semaphore __ldsem_wake_readers() does
> > adjust
> > sem->count with wrong value:
> > sem->wait_readers * (LDSEM_ACTIVE_BIAS - LDSEM_WAIT_BIAS)
> > 
> > I.e, if update comes with 1 missed wait_readers decrement, sem-
> > >count
> > will be 0x100000001 which means that there is active reader and
> > it'll
> > make any further writer to fail in acquiring the semaphore.
> > 
> > It looks like, this is a dead-code, because ldsem_down_read() is
> > never
> > called with timeout different than MAX_SCHEDULE_TIMEOUT, so it
> > might be
> > worth to delete timeout parameter and error path fall-back..
> 
> You might want to think about ditching that ldsem thing entirely, and
> use a regular rwsem ?

Yeah, but AFAICS, regular rwsem will need to have a timeout then (for
write). So, I thought fixing this pile would be simpler than adding
timeout and probably writer-priority to generic rwsem?

And I guess, we still will need fixes for stable for the bugs here..

I expect that timeouts are ABI, while the gain of adding priority may
be measured. I'll give it a shot (adding timeout/priority for linux-
next) to rwsem if you say it's acceptable.

-- 
Thanks,
             Dmitry

  reply	other threads:[~2018-09-11 13:02 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-11  1:48 [PATCHv3 0/6] tty: Hold write ldisc sem in tty_reopen() Dmitry Safonov
2018-09-11  1:48 ` [PATCHv3 1/6] tty: Drop tty->count on tty_reopen() failure Dmitry Safonov
2018-09-11  1:48 ` [PATCHv3 2/6] tty/ldsem: Update waiter->task before waking up reader Dmitry Safonov
2018-09-11  5:04   ` Sergey Senozhatsky
2018-09-11  5:41     ` Sergey Senozhatsky
2018-09-11 11:04       ` Kirill Tkhai
2018-09-11 11:44       ` Peter Zijlstra
2018-09-11 11:43     ` Peter Zijlstra
2018-09-11 11:40   ` Peter Zijlstra
2018-09-11 12:48     ` Dmitry Safonov
2018-09-11  1:48 ` [PATCHv3 3/6] tty: Hold tty_ldisc_lock() during tty_reopen() Dmitry Safonov
2018-09-11  1:48 ` [PATCHv3 4/6] tty/lockdep: Add ldisc_sem asserts Dmitry Safonov
2018-09-11 11:59   ` Peter Zijlstra
2018-09-11 12:01   ` Peter Zijlstra
2018-09-11 12:53     ` Dmitry Safonov
2018-09-11  1:48 ` [PATCHv3 5/6] tty: Simplify tty->count math in tty_reopen() Dmitry Safonov
2018-09-11  1:48 ` [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read() Dmitry Safonov
2018-09-11 12:02   ` Peter Zijlstra
2018-09-11 13:01     ` Dmitry Safonov [this message]
2018-09-11 13:33       ` Dmitry Safonov
2018-09-11 13:50         ` Peter Zijlstra
2018-09-11 15:04           ` Dmitry Safonov
2018-09-11 12:16 ` [PATCHv3 0/6] tty: Hold write ldisc sem in tty_reopen() Mark Rutland
2018-09-11 12:42   ` Dmitry Safonov

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=1536670918.2710.29.camel@arista.com \
    --to=dima@arista.com \
    --cc=0x7f454c46@gmail.com \
    --cc=dja@axtens.net \
    --cc=dvyukov@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jslaby@suse.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mikey@neuling.org \
    --cc=mpatocka@redhat.com \
    --cc=nathan@gt.net \
    --cc=pasik@iki.fi \
    --cc=penguin-kernel@I-love.SAKURA.ne.jp \
    --cc=peter@hurleysoftware.com \
    --cc=peterz@infradead.org \
    --cc=rong.a.chen@intel.com \
    --cc=sergey.senozhatsky.work@gmail.com \
    --cc=tanxiaojun@huawei.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.