LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Waiman Long <longman@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Davidlohr Bueso <dave@stgolabs.net>,
	"Theodore Y. Ts'o" <tytso@mit.edu>,
	Oleg Nesterov <oleg@redhat.com>,
	Amir Goldstein <amir73il@gmail.com>, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH v3 2/2] locking/percpu-rwsem: Annotate rwsem ownership transfer by setting RWSEM_OWNER_UNKNOWN
Date: Tue, 15 May 2018 14:21:30 -0700	[thread overview]
Message-ID: <20180515212130.GA12204@bombadil.infradead.org> (raw)
In-Reply-To: <4a3fbd3c-3cfa-f9c2-c73c-fa6d9c55c2d5@redhat.com>

On Tue, May 15, 2018 at 02:45:12PM -0400, Waiman Long wrote:
> On 05/15/2018 02:02 PM, Matthew Wilcox wrote:
> > On Tue, May 15, 2018 at 07:58:05PM +0200, Peter Zijlstra wrote:
> >> On Tue, May 15, 2018 at 01:38:04PM -0400, Waiman Long wrote:
> >>> +/*
> >>> + * Owner value to indicate the rwsem's owner is not currently known.
> >>> + */
> >>> +#define RWSEM_OWNER_UNKNOWN	((struct task_struct *)-1)
> >> It might be nice to comment that this works and relies on having that
> >> ANON_OWNER bit set.
> > I'd rather change the definition to be ((struct task_struct *)2)
> > otherwise this is both reader-owned and anonymously-owned which doesn't
> > make much sense.
> 
> Thinking about it a bit more. I can actually just use one special bit
> (bit 0) to designate an unknown owner. So for a reader-owned lock, it is
> just owner == 1 as the owners are unknown for a reader owned lock. For a
> lock owned by an unknown writer, it is (owner & 1) && (owner != 1). That
> will justify the use of -1L and save bit 1 for future extension.

To quote from your patch:

- * In essence, the owner field now has the following 3 states:
+ * In essence, the owner field now has the following 4 states:
  *  1) 0
  *     - lock is free or the owner hasn't set the field yet
  *  2) RWSEM_READER_OWNED
  *     - lock is currently or previously owned by readers (lock is free
  *	  or not set by owner yet)
- *  3) Other non-zero value
- *     - a writer owns the lock
+ *  3) RWSEM_ANONYMOUSLY_OWNED
+ *     - lock is owned by an anonymous writer, so spinning on the lock
+ *	  owner should be disabled.
+ *  4) Other non-zero value
+ *     - a writer owns the lock and other writers can spin on the lock owner.

I'd leave these as 0, 1, 2, other.  It's not really worth messing with
testing bits.

Actually, if you change them to all be values -- s/NULL/RWSEM_NO_OWNER/

then you could define them as:

RWSEM_READER_OWNED	0
RWSEM_ANON_OWNED	1
RWSEM_NO_OWNER		2

and rwsem_should_spin() is just sem->owner > 1.

  reply	other threads:[~2018-05-15 21:21 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-15 17:38 [PATCH v3 0/2] locking/rwsem: Fix DEBUG_RWSEM warning from thaw_super() Waiman Long
2018-05-15 17:38 ` [PATCH v3 1/2] locking/rwsem: Add a new RWSEM_ANONYMOUSLY_OWNED flag Waiman Long
2018-05-15 17:46   ` Peter Zijlstra
2018-05-15 17:48     ` Waiman Long
2018-05-15 17:48   ` Peter Zijlstra
2018-05-15 17:52     ` Waiman Long
2018-05-15 17:55       ` Peter Zijlstra
2018-05-15 17:56         ` Waiman Long
2018-05-15 18:02   ` Peter Zijlstra
2018-05-15 17:38 ` [PATCH v3 2/2] locking/percpu-rwsem: Annotate rwsem ownership transfer by setting RWSEM_OWNER_UNKNOWN Waiman Long
2018-05-15 17:58   ` Peter Zijlstra
2018-05-15 18:02     ` Waiman Long
2018-05-15 18:05       ` Amir Goldstein
2018-05-15 18:45         ` Waiman Long
2018-05-15 18:05       ` Peter Zijlstra
2018-05-15 18:35         ` Waiman Long
2018-05-15 18:02     ` Matthew Wilcox
2018-05-15 18:07       ` Peter Zijlstra
2018-05-15 18:45       ` Waiman Long
2018-05-15 21:21         ` Matthew Wilcox [this message]
2018-05-15 21:30           ` Waiman Long

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=20180515212130.GA12204@bombadil.infradead.org \
    --to=willy@infradead.org \
    --cc=amir73il@gmail.com \
    --cc=dave@stgolabs.net \
    --cc=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=longman@redhat.com \
    --cc=mingo@redhat.com \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=tytso@mit.edu \
    --subject='Re: [PATCH v3 2/2] locking/percpu-rwsem: Annotate rwsem ownership transfer by setting RWSEM_OWNER_UNKNOWN' \
    /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

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).