From: Andreas Mohr <andi@lisas.de>
To: Peter Zijlstra <peterz@infradead.org>
Cc: der.herr@hofr.at, Al Viro <viro@ZenIV.linux.org.uk>,
mingo@redhat.com, torvalds@linux-foundation.org,
dave@stgolabs.net, Oleg Nesterov <oleg@redhat.com>,
riel@redhat.com, tj@kernel.org, paulmck@linux.vnet.ibm.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem
Date: Tue, 6 Sep 2016 03:58:00 +0200 [thread overview]
Message-ID: <20160906015800.GA31162@rhlx01.hs-esslingen.de> (raw)
Hi,
[no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]
https://lkml.org/lkml/2016/9/5/832
Two thoughts:
***multiple locks
Don't have much insight into this
(didn't spend much thinking on this),
but of course it's unfortunate
that two lock types need to be serviced
rather than having the atomic handling exchange area/section
be sufficiently guarded by one lock only
(the question here could possibly be:
what kind of currently existing
structural disadvantage/layer distribution
prevents us from being able to
have things simply serviced
within one granular lock area only?)
***lock handling
> + percpu_down_read(&file_rwsem);
> spin_lock(&ctx->flc_lock);
> spin_unlock(&ctx->flc_lock);
> + percpu_up_read(&file_rwsem);
These are repeated multiple times in this commit, thus error-prone.
A possibly good way to commit-micro-manage this would be:
1. commit shoves things into a newly created encapsulation/wrapper helper
stuff_lock(&flc_lock); /* <---- naming surely can be improved here */
2. [this commit]
extend encapsulation/wrapper helper
to be servicing file_rwsem, too:
stuff_lock(&flc_lock, &file_rwsem);
That way it is pretty much guaranteed that:
a) neither one nor the other lock type
will get forgotten later, at a specific use site
b) lock order will be correctly maintained at all times
(AB-BA deadlock......)
[or, IOW, you get some symmetry/consistency malus points
for having provided
a relatively large amount of LOC extensions
which put certain amounts of stress on
code implementation quality/maintainability ;)]
HTH,
Andreas Mohr
next reply other threads:[~2016-09-06 1:58 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-06 1:58 Andreas Mohr [this message]
2016-09-06 8:23 ` [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem Peter Zijlstra
2016-09-06 8:36 ` Andreas Mohr
2016-09-06 8:59 ` Peter Zijlstra
-- strict thread matches above, loose matches on Subject: below --
2016-09-05 19:40 [PATCH 0/7] perpcu rwsem, fs/locks and killing lglocks Peter Zijlstra
2016-09-05 19:40 ` [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem 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=20160906015800.GA31162@rhlx01.hs-esslingen.de \
--to=andi@lisas.de \
--cc=dave@stgolabs.net \
--cc=der.herr@hofr.at \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=oleg@redhat.com \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=riel@redhat.com \
--cc=tj@kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=viro@ZenIV.linux.org.uk \
/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).