* Re: [PATCH v3 1/2] mm: add probe_user_read()
From: Michael Ellerman @ 2019-02-08 3:01 UTC (permalink / raw)
To: Jann Horn, Christophe Leroy
Cc: Kees Cook, kernel list, Mike Rapoport, Linux-MM, Paul Mackerras,
Andrew Morton, linuxppc-dev
In-Reply-To: <CAG48ez1gXgsBG6bYGG5+B4Dqkhk_iVaYLqt63RaxURxE0yt9eA@mail.gmail.com>
Jann Horn <jannh@google.com> writes:
> On Thu, Feb 7, 2019 at 10:22 AM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>> In powerpc code, there are several places implementing safe
>> access to user data. This is sometimes implemented using
>> probe_kernel_address() with additional access_ok() verification,
>> sometimes with get_user() enclosed in a pagefault_disable()/enable()
>> pair, etc. :
>> show_user_instructions()
>> bad_stack_expansion()
>> p9_hmi_special_emu()
>> fsl_pci_mcheck_exception()
>> read_user_stack_64()
>> read_user_stack_32() on PPC64
>> read_user_stack_32() on PPC32
>> power_pmu_bhrb_to()
>>
>> In the same spirit as probe_kernel_read(), this patch adds
>> probe_user_read().
>>
>> probe_user_read() does the same as probe_kernel_read() but
>> first checks that it is really a user address.
>>
>> The patch defines this function as a static inline so the "size"
>> variable can be examined for const-ness by the check_object_size()
>> in __copy_from_user_inatomic()
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>
>
>
>> ---
>> v3: Moved 'Returns:" comment after description.
>> Explained in the commit log why the function is defined static inline
>>
>> v2: Added "Returns:" comment and removed probe_user_address()
>>
>> include/linux/uaccess.h | 34 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>>
>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
>> index 37b226e8df13..ef99edd63da3 100644
>> --- a/include/linux/uaccess.h
>> +++ b/include/linux/uaccess.h
>> @@ -263,6 +263,40 @@ extern long strncpy_from_unsafe(char *dst, const void *unsafe_addr, long count);
>> #define probe_kernel_address(addr, retval) \
>> probe_kernel_read(&retval, addr, sizeof(retval))
>>
>> +/**
>> + * probe_user_read(): safely attempt to read from a user location
>> + * @dst: pointer to the buffer that shall take the data
>> + * @src: address to read from
>> + * @size: size of the data chunk
>> + *
>> + * Safely read from address @src to the buffer at @dst. If a kernel fault
>> + * happens, handle that and return -EFAULT.
>> + *
>> + * We ensure that the copy_from_user is executed in atomic context so that
>> + * do_page_fault() doesn't attempt to take mmap_sem. This makes
>> + * probe_user_read() suitable for use within regions where the caller
>> + * already holds mmap_sem, or other locks which nest inside mmap_sem.
>> + *
>> + * Returns: 0 on success, -EFAULT on error.
>> + */
>> +
>> +#ifndef probe_user_read
>> +static __always_inline long probe_user_read(void *dst, const void __user *src,
>> + size_t size)
>> +{
>> + long ret;
>> +
>> + if (!access_ok(src, size))
>> + return -EFAULT;
>
> If this happens in code that's running with KERNEL_DS, the access_ok()
> is a no-op. If this helper is only intended for accessing real
> userspace memory, it would be more robust to add
> set_fs(USER_DS)/set_fs(oldfs) around this thing. Looking at the
> functions you're referring to in the commit message, e.g.
> show_user_instructions() does an explicit `__access_ok(pc,
> NR_INSN_TO_PRINT * sizeof(int), USER_DS)` to get the same effect.
Yeah I raised the same question up thread.
I think we're both right :) - it should explicitly set USER_DS.
There's precedent for that in the code you mentioned and also in the
perf code, eg:
88b0193d9418 ("perf/callchain: Force USER_DS when invoking perf_callchain_user()")
cheers
^ permalink raw reply
* Re: [PATCH v3 1/7] dump_stack: Support adding to the dump stack arch description
From: Sergey Senozhatsky @ 2019-02-08 2:01 UTC (permalink / raw)
To: Michael Ellerman
Cc: linux-arch, pmladek, sergey.senozhatsky, linux-kernel,
Steven Rostedt, linuxppc-dev, tj, akpm, dyoung
In-Reply-To: <20190207124635.3885-1-mpe@ellerman.id.au>
Cc-ing Steven
https://lore.kernel.org/lkml/20190207124635.3885-1-mpe@ellerman.id.au/T/#u
On (02/07/19 23:46), Michael Ellerman wrote:
> Arch code can set a "dump stack arch description string" which is
> displayed with oops output to describe the hardware platform.
>
> It is useful to initialise this as early as possible, so that an early
> oops will have the hardware description.
>
> However in practice we discover the hardware platform in stages, so it
> would be useful to be able to incrementally fill in the hardware
> description as we discover it.
>
> This patch adds that ability, by creating dump_stack_add_arch_desc().
>
> If there is no existing string it behaves exactly like
> dump_stack_set_arch_desc(). However if there is an existing string it
> appends to it, with a leading space.
>
> This makes it easy to call it multiple times from different parts of the
> code and get a reasonable looking result.
>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
You probably can have a __init buffer somewhere in ppc code, append
data to it, step by step, and call dump_stack_set_arch_desc() all
the time.
But no real objections; dump_stack_add_arch_desc() can do.
FWIW,
Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
-ss
^ permalink raw reply
* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
From: Michael Ellerman @ 2019-02-08 1:04 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev, Russell Currey
In-Reply-To: <1549515373.con208q1rq.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Russell Currey's on February 6, 2019 4:28 pm:
>> Without restoring the IAMR after idle, execution prevention on POWER9
>> with Radix MMU is overwritten and the kernel can freely execute userspace without
>> faulting.
>>
>> This is necessary when returning from any stop state that modifies user
>> state, as well as hypervisor state.
>>
>> To test how this fails without this patch, load the lkdtm driver and
>> do the following:
>>
>> echo EXEC_USERSPACE > /sys/kernel/debug/provoke-crash/DIRECT
>>
>> which won't fault, then boot the kernel with powersave=off, where it
>> will fault. Applying this patch will fix this.
>>
>> Fixes: 3b10d0095a1e ("powerpc/mm/radix: Prevent kernel execution of user
>> space")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>
> Good catch and debugging. This really should be a quirk, we don't want
> to have to restore this thing on a thread switch.
I'm not sure I follow. We don't context switch it on Radix, but we do
on hash if pkeys are enabled.
> Can we put it under a CONFIG option if we're not using IAMR?
We'll always be using it with Radix, and we might be using it for pkeys
on hash, unless pkeys are compiled out. But I don't really expect anyone
to be running with pkeys compiled out.
So I think the only case we could optimise is that we're on hash and the
current thread has an IAMR of 0, then we could just not restore
(assuming we come out of idle with IAMR=0).
But maybe I'm not understanding.
cheers
^ permalink raw reply
* Re: [PATCH 1/2] powerpc/64s: Work around spurious warning on old gccs with -fsanitize-coverage
From: Andrew Donnellan @ 2019-02-08 0:34 UTC (permalink / raw)
To: Segher Boessenkool, Nicholas Piggin; +Cc: syzkaller, linuxppc-dev, dvyukov
In-Reply-To: <20190207074947.GT14180@gate.crashing.org>
(+ Nick)
On 7/2/19 6:49 pm, Segher Boessenkool wrote:
> On Thu, Feb 07, 2019 at 05:59:48PM +1100, Andrew Donnellan wrote:
>> On 7/2/19 5:37 pm, Segher Boessenkool wrote:
>>> On Thu, Feb 07, 2019 at 04:33:23PM +1100, Andrew Donnellan wrote:
>>>> Some older gccs (<GCC 7), when invoked with -fsanitize-coverage=trace-pc,
>>>> cause a spurious uninitialised variable warning in dt_cpu_ftrs.c:
>>>>
>>>> arch/powerpc/kernel/dt_cpu_ftrs.c: In function
>>>> ‘cpufeatures_process_feature’:
>>>> arch/powerpc/kernel/dt_cpu_ftrs.c:686:7: warning: ‘m’ may be used
>>>> uninitialized in this function [-Wmaybe-uninitialized]
>>>> if (m->cpu_ftr_bit_mask)
>>>
>>> It seems to me the warning is correct? If enable_unknown is false and no
>>> cpu_feature is found, it will in
>>>
>>> if (m->cpu_ftr_bit_mask)
>>> cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>
>>> enable random features (whatever was last in the table), or indeed access
>>> via NULL if the table is length 0? So maybe this should be
>>>
>>> if (known && m->cpu_ftr_bit_mask)
>>> cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
>>>
>>> instead? (The code would be much clearer if all the known vs. !known
>>> codepath was fully separated here).
>>
>> The table is never length 0, it's a statically defined array.
>
> Sure, and presumably that is why newer GCC doesn't warn. But what about
> the other point? Is this code ever correct? Enabling random features
> (in cur_cpu_spec->cpu_features) when the name isn't found seems wrong.
Now that I'm replying without being 2 minutes before a meeting :)
The warning is still spurious, but the logic looks very suspicious.
I think your solution looks correct, though the whole function could be
cleaned up a bit.
I also notice that if enable_unknown == false, then I think an unknown
feature will still print "enabling" and return true, which seems wrong.
How does something like the following look, which I could send instead
and will probably solve the spurious warnings issues anyway?
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 2192b2114513..0f13048dc0dd 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -658,40 +658,31 @@ static void __init cpufeatures_setup_start(u32 isa)
static bool __init cpufeatures_process_feature(struct dt_cpu_feature *f)
{
- const struct dt_cpu_feature_match *m = NULL;
- bool known = false;
+ const struct dt_cpu_feature_match *m;
int i;
for (i = 0; i < ARRAY_SIZE(dt_cpu_feature_match_table); i++) {
m = &dt_cpu_feature_match_table[i];
if (!strcmp(f->name, m->name)) {
- known = true;
- if (m->enable(f))
- break;
-
- pr_info("not enabling: %s (disabled or
unsupported by kernel)\n",
- f->name);
- return false;
- }
- }
-
- if (!known && enable_unknown) {
- if (!feat_try_enable_unknown(f)) {
- pr_info("not enabling: %s (unknown and
unsupported by kernel)\n",
- f->name);
- return false;
+ if (!m->enable(f)) {
+ pr_info("not enabling: %s (disabled or
unsupported by kernel)\n",
+ f->name);
+ return false;
+ }
+ pr_debug("enabling: %s\n", f->name);
+ cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
+ return true;
}
}
- if (m->cpu_ftr_bit_mask)
- cur_cpu_spec->cpu_features |= m->cpu_ftr_bit_mask;
-
- if (known)
- pr_debug("enabling: %s\n", f->name);
- else
+ if (enable_unknown && feat_try_enable_unknown(f)) {
pr_debug("enabling: %s (unknown)\n", f->name);
-
- return true;
+ return true;
+ } else {
+ pr_info("not enabling: %s (unknown and unsupported by
kernel)\n",
+ f->name);
+ return false;
+ }
}
static __init void cpufeatures_cpu_quirks(void)
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply related
* Re: [PATCH 0/1] Start conversion of PowerPC docs
From: Jonathan Corbet @ 2019-02-08 0:01 UTC (permalink / raw)
To: Tobin C. Harding; +Cc: linuxppc-dev, linux-kernel, linux-doc
In-Reply-To: <20190207060316.3221-1-tobin@kernel.org>
On Thu, 7 Feb 2019 17:03:15 +1100
"Tobin C. Harding" <tobin@kernel.org> wrote:
> As discussed at LCA here is the start to the docs conversion for PowerPC
> to RST.
>
> This applies cleanly on top of the mainline (5.20-rc5) and Jon's tree
> (docs-next branch).
>
> I'm guessing it should go in through the PowerPC tree because I doubt
> you want to review this Jon, it's one big single patch (all blame for
> that falls on mpe ;)
Well, I went and took a look anyway, being a glutton for punishment. So
naturally I do have some comments...
- I don't think this should be a top-level directory full of docs; the top
level is already rather overpopulated. At worst, we should create an
arch/ directory for architecture-specific docs. I kind of think that
this should be thought through a bit more, though, with an eye toward
who the audience is. Some of it is clearly developer documentation, and
some of it is aimed at admins; ptrace.rst is user-space API stuff.
Nobody ever welcomes me saying this, but we should really split things
into the appropriate manuals according to audience.
- It would be good to know how much of this stuff is still relevant.
bootwrapper.txt hasn't been modified since it was added in 2008.
cpu_features.txt predates the git era, as does mpc52xx.txt; hvcs.txt is
nearly as old. And so on. Can we perhaps stop dragging some of those
docs around?
- The use of flat-table in isa-versions.rst totally wrecks the readability
of those tables in the plain-text version. Said tables are pretty close
to being RST in their original form; it would be far better to just fix
anything needing fixing but to keep that form.
- I'm glad you're adding SPDX lines, but do you know that the license is
correct in each case? It's best to be careful with such things.
Thanks,
jon
^ permalink raw reply
* Re: [PATCH 1/1] docs: powerpc: Convert to RST format
From: Randy Dunlap @ 2019-02-07 22:58 UTC (permalink / raw)
To: Tobin C. Harding, Michael Ellerman
Cc: linux-doc, linuxppc-dev, linux-kernel, Jonathan Corbet
In-Reply-To: <20190207060316.3221-2-tobin@kernel.org>
On 2/6/19 10:03 PM, Tobin C. Harding wrote:
> The PowerPC docs have yet to be converted to RST format. Let's kick it
> off by doing all the files that _don't_ contain ASCII art.
>
> - Add SPDX license identifier to each new RST file.
>
> .. SPDX-License-Identifier: GPL-2.0
>
> - User correct heading adornments.
>
> - Make all lines < 72 characters in width.
>
> - Use correct indentation for code blocks, add syntax highlighting
>
> - Sparingly use double ticks if it makes the files easier to parse
> both in text and on the web.
>
> - Fix any super obvious typos (lean towards not making changes so that
> we don't introduce errors).
>
> Edited as text files (obviously) and formatted as HTML to verify
> rendering, no other formats verified.
>
> Convert docs to RST format, adding license.
>
> Signed-off-by: Tobin C. Harding <tobin@kernel.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
Tested-by: Randy Dunlap <rdunlap@infradead.org>
thanks.
> ---
> Documentation/index.rst | 1 +
> Documentation/powerpc/DAWR-POWER9.rst | 60 ++++
> Documentation/powerpc/DAWR-POWER9.txt | 58 ---
> Documentation/powerpc/bootwrapper.rst | 140 ++++++++
> Documentation/powerpc/bootwrapper.txt | 141 --------
> Documentation/powerpc/conf.py | 10 +
> Documentation/powerpc/cpu_features.rst | 62 ++++
> Documentation/powerpc/cpu_features.txt | 56 ---
> .../powerpc/eeh-pci-error-recovery.rst | 319 +++++++++++++++++
> .../powerpc/eeh-pci-error-recovery.txt | 334 ------------------
> Documentation/powerpc/index.rst | 21 ++
> Documentation/powerpc/isa-versions.rst | 234 ++++++++----
> Documentation/powerpc/mpc52xx.rst | 52 +++
> Documentation/powerpc/mpc52xx.txt | 39 --
> Documentation/powerpc/pmu-ebb.rst | 148 ++++++++
> Documentation/powerpc/pmu-ebb.txt | 137 -------
> Documentation/powerpc/ptrace.rst | 177 ++++++++++
> Documentation/powerpc/ptrace.txt | 151 --------
> .../{syscall64-abi.txt => syscall64-abi.rst} | 80 +++--
> .../powerpc/transactional_memory.rst | 259 ++++++++++++++
> .../powerpc/transactional_memory.txt | 244 -------------
> 21 files changed, 1460 insertions(+), 1263 deletions(-)
> create mode 100644 Documentation/powerpc/DAWR-POWER9.rst
> delete mode 100644 Documentation/powerpc/DAWR-POWER9.txt
> create mode 100644 Documentation/powerpc/bootwrapper.rst
> delete mode 100644 Documentation/powerpc/bootwrapper.txt
> create mode 100644 Documentation/powerpc/conf.py
> create mode 100644 Documentation/powerpc/cpu_features.rst
> delete mode 100644 Documentation/powerpc/cpu_features.txt
> create mode 100644 Documentation/powerpc/eeh-pci-error-recovery.rst
> delete mode 100644 Documentation/powerpc/eeh-pci-error-recovery.txt
> create mode 100644 Documentation/powerpc/index.rst
> create mode 100644 Documentation/powerpc/mpc52xx.rst
> delete mode 100644 Documentation/powerpc/mpc52xx.txt
> create mode 100644 Documentation/powerpc/pmu-ebb.rst
> delete mode 100644 Documentation/powerpc/pmu-ebb.txt
> create mode 100644 Documentation/powerpc/ptrace.rst
> delete mode 100644 Documentation/powerpc/ptrace.txt
> rename Documentation/powerpc/{syscall64-abi.txt => syscall64-abi.rst} (58%)
> create mode 100644 Documentation/powerpc/transactional_memory.rst
> delete mode 100644 Documentation/powerpc/transactional_memory.txt
--
~Randy
^ permalink raw reply
* Re: [PATCH] powerpc/powernv/idle: Restore IAMR after idle
From: Russell Currey @ 2019-02-07 22:38 UTC (permalink / raw)
To: Thiago Jung Bauermann; +Cc: linuxppc-dev, Nicholas Piggin
In-Reply-To: <878syrlfnf.fsf@morokweng.localdomain>
On Thu, 2019-02-07 at 14:37 -0200, Thiago Jung Bauermann wrote:
> Russell Currey <ruscur@russell.cc> writes:
> > On Thu, 2019-02-07 at 15:08 +1000, Nicholas Piggin wrote:
> > > Russell Currey's on February 6, 2019 4:28 pm:
> > > >
> > > > Fixes: 3b10d0095a1e ("powerpc/mm/radix: Prevent kernel
> > > > execution of
> > > > user
> > > > space")
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > >
> > > Good catch and debugging. This really should be a quirk, we don't
> > > want
> > > to have to restore this thing on a thread switch.
> > >
> > > Can we put it under a CONFIG option if we're not using IAMR?
> >
> > I don't exactly know when we do or don't use the IAMR (since the
> > only
> > thing I've used it for is radix). When wouldn't we care about
> > restoring it on hash?
>
> On hash it's used for memory protection keys (code is in
> arch/powerpc/mm/pkeys.c). The kernel doesn't use protection keys, but
> userspace apps may use it explicitly via specific syscalls
> (pkey_alloc(), pkey_mprotect, pkey_free()).
>
> Also, the kernel may use a protection key if the process does an
> mmap(PROT_EXEC).
I don't understand how this would work, though - in this case we
wouldn't know on boot if we were going to use the IAMR or not. On
radix (unless booting with nosmep) it would always be used, but on hash
it seems it depends on what userspace does. How exactly would a
runtime toggle of "IAMR in use" work?
With a CONFIG option it would have to depend on PPC_MEM_KEYS ||
PPC_RADIX_MMU, but those are (pretty much) always going to be on in P8
and P9, which I already check for.
>
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
>
^ permalink raw reply
* Re: [PATCH-tip 15/22] locking/rwsem: Merge owner into count on x86-64
From: Waiman Long @ 2019-02-07 20:54 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, Will Deacon,
linux-kernel, Linus Torvalds, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, linux-alpha, sparclinux, Thomas Gleixner,
linuxppc-dev, Andrew Morton, linux-arm-kernel
In-Reply-To: <20190207200848.GH32477@hirez.programming.kicks-ass.net>
On 02/07/2019 03:08 PM, Peter Zijlstra wrote:
> On Thu, Feb 07, 2019 at 02:07:19PM -0500, Waiman Long wrote:
>> On 32-bit architectures, there aren't enough bits to hold both.
>> 64-bit architectures, however, can have enough bits to do that. For
>> x86-64, the physical address can use up to 52 bits. That is 4PB of
>> memory. That leaves 12 bits available for other use. The task structure
>> pointer is also aligned to the L1 cache size. That means another 6 bits
>> (64 bytes cacheline) will be available. Reserving 2 bits for status
>> flags, we will have 16 bits for the reader count. That can supports
>> up to (64k-1) readers.
> 64k readers sounds like a number that is fairly 'easy' to reach, esp. on
> 64bit. These are preemptible locks after all, all we need to do is get
> 64k tasks nested on enough CPUs.
>
> I'm sure there's some willing Java proglet around that spawns more than
> 64k threads just because it can. Run it on a big enough machine (ISTR
> there's a number of >1k CPU systems out there) and voila.
Yes, that can be a problem.
One possible solution is to check if the count goes negative. If so,
fail the read lock and make the readers wait in the wait queue until the
count is in positive territory. That effectively reduces the reader
count to 15 bits, but it will avoid the overflow situation. I will try
to add that support into the next version.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH-tip 00/22] locking/rwsem: Rework rwsem-xadd & enable new rwsem features
From: Waiman Long @ 2019-02-07 20:00 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: linux-arch, linux-xtensa, linux-ia64, Tim Chen, Arnd Bergmann,
linux-sh, Peter Zijlstra, linux-hexagon, x86, Will Deacon,
linux-kernel, Linus Torvalds, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, linux-alpha, sparclinux, Thomas Gleixner,
linuxppc-dev, Andrew Morton, linux-arm-kernel
In-Reply-To: <20190207195119.lglz4lb7cnf4ndn2@linux-r8p5>
On 02/07/2019 02:51 PM, Davidlohr Bueso wrote:
> On Thu, 07 Feb 2019, Waiman Long wrote:
>> 30 files changed, 1197 insertions(+), 1594 deletions(-)
>
> Performance numbers on numerous workloads, pretty please.
>
> I'll go and throw this at my mmap_sem intensive workloads
> I've collected.
>
> Thanks,
> Davidlohr
Thanks for getting some of the performance numbers. This is the initial
draft after more than 1 years of hibernation. I will also get other
performance numbers in subsequent revision of the patch.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH-tip 15/22] locking/rwsem: Merge owner into count on x86-64
From: Waiman Long @ 2019-02-07 19:55 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, Will Deacon,
linux-kernel, Linus Torvalds, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, linux-alpha, sparclinux, Thomas Gleixner,
linuxppc-dev, Andrew Morton, linux-arm-kernel
In-Reply-To: <20190207194542.GG32511@hirez.programming.kicks-ass.net>
On 02/07/2019 02:45 PM, Peter Zijlstra wrote:
> On Thu, Feb 07, 2019 at 02:07:19PM -0500, Waiman Long wrote:
>> On 32-bit architectures, there aren't enough bits to hold both.
>> 64-bit architectures, however, can have enough bits to do that. For
>> x86-64, the physical address can use up to 52 bits. That is 4PB of
>> memory. That leaves 12 bits available for other use. The task structure
>> pointer is also aligned to the L1 cache size. That means another 6 bits
>> (64 bytes cacheline) will be available. Reserving 2 bits for status
>> flags, we will have 16 bits for the reader count. That can supports
>> up to (64k-1) readers.
> *groan*...
>
> So take qrwlock's idea for a queue, then make the count value (similar
> to the new mutex); that is have a bit0 be a r/w bit, when w bits 6-N are
> owner, when r they are reader-count. bit1 can be a pending bit, bit2 a
> handoff bit etc..
>
> That should fit and work on 32bit and 64bit without issue.
>
> I have a half-arsed rwsem-atomic.c somewhere that does just that. I just
> never got around to doing all the optimistic spin and steal crap that
> makes our current rwsem fly.
>
> And that nicely gets rid of that mind bending BIAS crud.
Well, the reason for this compromise is to keep using xadd for readers.
Your scheme will certainly work, but we have to use cmpxchg for readers
too. That will have a performance impact especially with multiple
readers contending which I am trying to avoid.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH-tip 04/22] locking/rwsem: Remove arch specific rwsem files
From: Waiman Long @ 2019-02-07 19:43 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, Will Deacon,
linux-kernel, Linus Torvalds, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, linux-alpha, sparclinux, Thomas Gleixner,
linuxppc-dev, Andrew Morton, linux-arm-kernel
In-Reply-To: <20190207193656.GF32511@hirez.programming.kicks-ass.net>
On 02/07/2019 02:36 PM, Peter Zijlstra wrote:
> On Thu, Feb 07, 2019 at 02:07:08PM -0500, Waiman Long wrote:
>
>> +static inline int __down_read_trylock(struct rw_semaphore *sem)
>> +{
>> + long tmp;
>> +
>> + while ((tmp = atomic_long_read(&sem->count)) >= 0) {
>> + if (tmp == atomic_long_cmpxchg_acquire(&sem->count, tmp,
>> + tmp + RWSEM_ACTIVE_READ_BIAS)) {
>> + return 1;
>> + }
>> + }
> Nah, you're supposed to write that like:
>
> for (;;) {
> val = atomic_long_cond_read_relaxed(&sem->count, VAL < 0);
> if (atomic_long_try_cmpxchg_acquire(&sem->count, &val,
> val + RWSEM_ACTIVE_READ_BIAS))
> break;
> }
>
Ah, you are right. I just took it directly from the current generic
asm.h file and didn't pay much attention to it. I will fix that in the
next iteration.
>> + return 0;
>> +}
>
> Anyway, yuck, you're keeping all that BIAS nonsense :/ I was so hoping
> for a rwsem implementation without that impenetrable crap.
Well, I am going to take out all the BIAS handling in a later patch.
This one is just for removing the architecture specific files.
I can't change the rwsem count encoding before I take out all the arch
specific files.
Cheers,
Longman
^ permalink raw reply
* [PATCH-tip 22/22] locking/rwsem: Ensure an RT task will not spin on reader
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
An RT task can do optimistic spinning only if the lock holder is
actually running. If the state of the lock holder isn't known, there
is a possibility that high priority of the RT task may block forward
progress of the lock holder if they happen to reside on the same CPU.
This will lead to deadlock. So we have to make sure that an RT task
will not spin on a reader-owned rwsem.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 3cf2e84..ac1f6c4 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -349,12 +349,14 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
/*
* Return the folowing three values depending on the lock owner state.
+ * 2 when owner is currently NULL
* 1 when owner changes and no reader is detected yet.
* 0 when owner changes and the new owner may be a reader.
* -1 when optimistic spinning has to stop because either the owner stops
* running, is unknown, or its timeslice has been used up.
*/
enum owner_state {
+ OWNER_NULL = 2,
OWNER_SPINNABLE = 1,
OWNER_READER = 0,
OWNER_NONSPINNABLE = -1,
@@ -405,7 +407,8 @@ static noinline enum owner_state rwsem_spin_on_owner(struct rw_semaphore *sem)
owner = rwsem_get_owner(sem);
if (!is_rwsem_owner_spinnable(owner))
return OWNER_NONSPINNABLE;
- return is_rwsem_owner_reader(owner) ? OWNER_READER : OWNER_SPINNABLE;
+ return !owner ? OWNER_NULL :
+ is_rwsem_owner_reader(owner) ? OWNER_READER : OWNER_SPINNABLE;
}
static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
@@ -442,12 +445,13 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
break;
/*
- * When there's no owner, we might have preempted between the
- * owner acquiring the lock and setting the owner field. If
- * we're an RT task that will live-lock because we won't let
- * the owner complete.
+ * An RT task cannot do optimistic spinning if it cannot
+ * be sure the lock holder is running. When there's no owner
+ * or is reader-owned, an RT task has to stop spinning or
+ * live-lock may happen if the current task and the lock
+ * holder happen to run in the same CPU.
*/
- if (!rwsem_get_owner(sem) &&
+ if ((owner_state != OWNER_SPINNABLE) &&
(need_resched() || rt_task(current)))
break;
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 21/22] locking/rwsem: Wake up all readers in wait queue
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
When the front of the wait queue is a reader, other readers
immediately following the first reader will also be woken up at the
same time. However, if there is a writer in between. Those readers
behind the writer will not be woken up.
Because of optimistic spinning, the lock acquisition order is not FIFO
anyway. The lock handoff mechanism will ensure that lock starvation
will not happen.
Assuming that the lock hold times of the other readers still in the
queue will be about the same as the readers that are being woken up,
there is really not much additional cost other than the additional
latency due to the wakeup of additional tasks by the waker. Therefore
all the readers in the queue are woken up when the first waiter is a
reader to improve reader throughput.
With a locking microbenchmark running on 5.0 based kernel, the total
locking rates (in kops/s) of the benchmark on a 4-socket 56-core x86-64
system with equal numbers of readers and writers before all the reader
spining patches, before this patch and after this patch were as follows:
# of Threads Pre-rspin Pre-Patch Post-patch
------------ --------- --------- ----------
2 1,926 8,057 7,397
4 1,391 7,680 6,161
8 716 7,284 6,405
16 618 6,542 6,768
32 501 1,449 6,550
64 61 480 5,548
112 75 769 5,216
At low contention level, there is a slight drop in performance. At high
contention level, however, this patch gives a big performance boost.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 3beb942..3cf2e84 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -180,16 +180,16 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
}
/*
- * Grant an infinite number of read locks to the readers at the front
- * of the queue. We know that woken will be at least 1 as we accounted
- * for above. Note we increment the 'active part' of the count by the
+ * Grant an infinite number of read locks to all the readers in the
+ * queue. We know that woken will be at least 1 as we accounted for
+ * above. Note we increment the 'active part' of the count by the
* number of readers before waking any processes up.
*/
list_for_each_entry_safe(waiter, tmp, &sem->wait_list, list) {
struct task_struct *tsk;
if (waiter->type == RWSEM_WAITING_FOR_WRITE)
- break;
+ continue;
woken++;
tsk = waiter->task;
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 20/22] locking/rwsem: Enable count-based spinning on reader
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
When the rwsem is owned by reader, writers stop optimistic spinning
simply because there is no easy way to figure out if all the readers
are actively running or not. However, there are scenarios where
the readers are unlikely to sleep and optimistic spinning can help
performance.
This patch provides a simple mechanism for spinning on a reader-owned
rwsem. It is a loop count threshold based spinning where the count will
get reset whenenver the rwsem reader count value changes indicating
that the rwsem is still active. There is another maximum count value
that limits that maximum number of spinnings that can happen.
When the loop or max counts reach 0, a bit will be set in the owner
field to indicate that no more optimistic spinning should be done on
this rwsem until it becomes writer owned again. Not even readers
is allowed to acquire the reader-locked rwsem for better fairness.
The spinning threshold and maximum values can be overridden by
architecture specific header file, if necessary. The current default
threshold value is 512 iterations.
With a locking microbenchmark running on 5.0 based kernel, the total
locking rates (in kops/s) of the benchmark on a 4-socket 56-core x86-64
system with equal numbers of readers and writers before all the reader
spining patches, before this patch and after this patch were as follows:
# of Threads Pre-rspin Pre-Patch Post-patch
------------ --------- --------- ----------
2 1,926 2,120 8,057
4 1,391 1,320 7,680
8 716 694 7,284
16 618 606 6,542
32 501 487 1,449
64 61 57 480
This patch gives a big boost in performance for mixed reader/writer
workloads.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/lock_events_list.h | 1 +
kernel/locking/rwsem-xadd.c | 63 +++++++++++++++++++++++++++++++++++----
kernel/locking/rwsem-xadd.h | 45 +++++++++++++++++++++-------
3 files changed, 94 insertions(+), 15 deletions(-)
diff --git a/kernel/locking/lock_events_list.h b/kernel/locking/lock_events_list.h
index 54b6650..0052534 100644
--- a/kernel/locking/lock_events_list.h
+++ b/kernel/locking/lock_events_list.h
@@ -60,6 +60,7 @@
LOCK_EVENT(rwsem_opt_rlock) /* # of read locks opt-spin acquired */
LOCK_EVENT(rwsem_opt_wlock) /* # of write locks opt-spin acquired */
LOCK_EVENT(rwsem_opt_fail) /* # of failed opt-spinnings */
+LOCK_EVENT(rwsem_opt_nospin) /* # of disabled reader opt-spinnings */
LOCK_EVENT(rwsem_rlock) /* # of read locks acquired */
LOCK_EVENT(rwsem_rlock_fast) /* # of fast read locks acquired */
LOCK_EVENT(rwsem_rlock_fail) /* # of failed read lock acquisitions */
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 015edd6..3beb942 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -95,6 +95,22 @@ enum rwsem_wake_type {
#define RWSEM_WAIT_TIMEOUT ((HZ - 1)/200 + 1)
/*
+ * Reader-owned rwsem spinning threshold and maximum value
+ *
+ * This threshold and maximum values can be overridden by architecture
+ * specific value. The loop count will be reset whenenver the rwsem count
+ * value changes. The max value constrains the total number of reader-owned
+ * lock spinnings that can happen.
+ */
+#ifdef ARCH_RWSEM_RSPIN_THRESHOLD
+# define RWSEM_RSPIN_THRESHOLD ARCH_RWSEM_RSPIN_THRESHOLD
+# define RWSEM_RSPIN_MAX ARCH_RWSEM_RSPIN_MAX
+#else
+# define RWSEM_RSPIN_THRESHOLD (1 << 9)
+# define RWSEM_RSPIN_MAX (1 << 12)
+#endif
+
+/*
* handle the lock release when processes blocked on it that can now run
* - if we come here from up_xxxx(), then the RWSEM_FLAG_WAITERS bit must
* have been set.
@@ -324,7 +340,7 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
owner = rwsem_get_owner(sem);
if (owner) {
ret = is_rwsem_owner_spinnable(owner) &&
- owner_on_cpu(owner, sem);
+ (is_rwsem_owner_reader(owner) || owner_on_cpu(owner, sem));
}
rcu_read_unlock();
lockevent_cond_inc(rwsem_opt_fail, !ret);
@@ -359,7 +375,8 @@ static noinline enum owner_state rwsem_spin_on_owner(struct rw_semaphore *sem)
* This enables the spinner to move forward and do a trylock
* earlier.
*/
- while (owner && (READ_ONCE(sem->owner) == owner)) {
+ while (owner && !is_rwsem_owner_reader(owner)
+ && (READ_ONCE(sem->owner) == owner)) {
/*
* Ensure we emit the owner->on_cpu, dereference _after_
* checking sem->owner still matches owner, if that fails,
@@ -394,6 +411,10 @@ static noinline enum owner_state rwsem_spin_on_owner(struct rw_semaphore *sem)
static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
{
bool taken = false;
+ enum owner_state owner_state;
+ int rspin_cnt = RWSEM_RSPIN_THRESHOLD;
+ int rspin_max = RWSEM_RSPIN_MAX;
+ int old_rcount = 0;
preempt_disable();
@@ -401,14 +422,16 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
if (!osq_lock(&sem->osq))
goto done;
+ if (!is_rwsem_spinnable(sem))
+ rspin_cnt = 0;
+
/*
* Optimistically spin on the owner field and attempt to acquire the
* lock whenever the owner changes. Spinning will be stopped when:
* 1) the owning writer isn't running; or
- * 2) readers own the lock as we can't determine if they are
- * actively running or not.
+ * 2) readers own the lock and spinning count has reached 0.
*/
- while (rwsem_spin_on_owner(sem) == OWNER_SPINNABLE) {
+ while ((owner_state = rwsem_spin_on_owner(sem)) != OWNER_NONSPINNABLE) {
/*
* Try to acquire the lock
*/
@@ -429,6 +452,36 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
break;
/*
+ * We only decremnt rspin_cnt when a writer is trying to
+ * acquire a lock owned by readers. In which case,
+ * rwsem_spin_on_owner() will essentially be a no-op
+ * and we will be spinning in this main loop. The spinning
+ * count will be reset whenever the rwsem count value
+ * changes.
+ */
+ if (wlock && (owner_state == OWNER_READER)) {
+ int rcount;
+
+ if (!rspin_cnt || !rspin_max) {
+ if (is_rwsem_spinnable(sem)) {
+ rwsem_set_nonspinnable(sem);
+ lockevent_inc(rwsem_opt_nospin);
+ }
+ break;
+ }
+
+ rcount = atomic_long_read(&sem->count)
+ >> RWSEM_READER_SHIFT;
+ if (rcount != old_rcount) {
+ old_rcount = rcount;
+ rspin_cnt = RWSEM_RSPIN_THRESHOLD;
+ } else {
+ rspin_cnt--;
+ }
+ rspin_max--;
+ }
+
+ /*
* The cpu_relax() call is a compiler barrier which forces
* everything in this loop to be re-loaded. We don't need
* memory barriers as we'll eventually observe the right
diff --git a/kernel/locking/rwsem-xadd.h b/kernel/locking/rwsem-xadd.h
index eb4ef36..be67dbd 100644
--- a/kernel/locking/rwsem-xadd.h
+++ b/kernel/locking/rwsem-xadd.h
@@ -5,18 +5,20 @@
* - RWSEM_READER_OWNED (bit 0): The rwsem is owned by readers
* - RWSEM_ANONYMOUSLY_OWNED (bit 1): The rwsem is anonymously owned,
* i.e. the owner(s) cannot be readily determined. It can be reader
- * owned or the owning writer is indeterminate.
+ * owned or the owning writer is indeterminate. Optimistic spinning
+ * should be disabled if this flag is set.
*
* When a writer acquires a rwsem, it puts its task_struct pointer
- * into the owner field. It is cleared after an unlock.
+ * into the owner field or the count itself (64-bit only. It should
+ * be cleared after an unlock.
*
* When a reader acquires a rwsem, it will also puts its task_struct
- * pointer into the owner field with both the RWSEM_READER_OWNED and
- * RWSEM_ANONYMOUSLY_OWNED bits set. On unlock, the owner field will
- * largely be left untouched. So for a free or reader-owned rwsem,
- * the owner value may contain information about the last reader that
- * acquires the rwsem. The anonymous bit is set because that particular
- * reader may or may not still own the lock.
+ * pointer into the owner field with the RWSEM_READER_OWNED bit set.
+ * On unlock, the owner field will largely be left untouched. So
+ * for a free or reader-owned rwsem, the owner value may contain
+ * information about the last reader that acquires the rwsem. The
+ * anonymous bit may also be set to permanently disable optimistic
+ * spinning on a reader-own rwsem until a writer comes along.
*
* That information may be helpful in debugging cases where the system
* seems to hang on a reader owned rwsem especially if only one reader
@@ -182,8 +184,7 @@ static inline struct task_struct *rwsem_get_owner(struct rw_semaphore *sem)
static inline void __rwsem_set_reader_owned(struct rw_semaphore *sem,
struct task_struct *owner)
{
- unsigned long val = (unsigned long)owner | RWSEM_READER_OWNED
- | RWSEM_ANONYMOUSLY_OWNED;
+ unsigned long val = (unsigned long)owner | RWSEM_READER_OWNED;
WRITE_ONCE(sem->owner, (struct task_struct *)val);
}
@@ -209,6 +210,14 @@ static inline bool is_rwsem_owner_reader(struct task_struct *owner)
}
/*
+ * Return true if the rwsem is spinnable.
+ */
+static inline bool is_rwsem_spinnable(struct rw_semaphore *sem)
+{
+ return is_rwsem_owner_spinnable(READ_ONCE(sem->owner));
+}
+
+/*
* Return true if the rwsem is owned by a reader.
*/
static inline bool is_rwsem_reader_owned(struct rw_semaphore *sem)
@@ -226,6 +235,22 @@ static inline bool is_rwsem_reader_owned(struct rw_semaphore *sem)
}
/*
+ * Set the RWSEM_ANONYMOUSLY_OWNED flag if the RWSEM_READER_OWNED flag
+ * remains set. Otherwise, the operation will be aborted.
+ */
+static inline void rwsem_set_nonspinnable(struct rw_semaphore *sem)
+{
+ long owner = (long)READ_ONCE(sem->owner);
+
+ while (is_rwsem_owner_reader((struct task_struct *)owner)) {
+ if (!is_rwsem_owner_spinnable((struct task_struct *)owner))
+ break;
+ owner = cmpxchg((long *)&sem->owner, owner,
+ owner | RWSEM_ANONYMOUSLY_OWNED);
+ }
+}
+
+/*
* Return true if rwsem is owned by an anonymous writer or readers.
*/
static inline bool rwsem_has_anonymous_owner(struct task_struct *owner)
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 19/22] locking/rwsem: Enable readers spinning on writer
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
This patch enables readers to optimistically spin on a
rwsem when it is owned by a writer instead of going to sleep
directly. The rwsem_can_spin_on_owner() function is extracted
out of rwsem_optimistic_spin() and is called directly by
__rwsem_down_read_failed_common() and __rwsem_down_write_failed_common().
This patch may actually reduce performance under certain circumstances
for reader-mostly workload as the readers may not be grouped together
in the wait queue anymore. So we may have a number of small reader
groups among writers instead of a large reader group. However, this
change is needed for some of the subsequent patches.
With a locking microbenchmark running on 5.0 based kernel, the total
locking rates (in kops/s) of the benchmark on a 4-socket 56-core x86-64
system with equal numbers of readers and writers before and after the
patch were as follows:
# of Threads Pre-patch Post-patch
------------ --------- ----------
2 1,926 2,120
4 1,391 1,320
8 716 694
16 618 606
32 501 487
64 61 57
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/lock_events_list.h | 1 +
kernel/locking/rwsem-xadd.c | 80 ++++++++++++++++++++++++++++++++++-----
kernel/locking/rwsem-xadd.h | 3 ++
3 files changed, 74 insertions(+), 10 deletions(-)
diff --git a/kernel/locking/lock_events_list.h b/kernel/locking/lock_events_list.h
index 4cde507..54b6650 100644
--- a/kernel/locking/lock_events_list.h
+++ b/kernel/locking/lock_events_list.h
@@ -57,6 +57,7 @@
LOCK_EVENT(rwsem_sleep_writer) /* # of writer sleeps */
LOCK_EVENT(rwsem_wake_reader) /* # of reader wakeups */
LOCK_EVENT(rwsem_wake_writer) /* # of writer wakeups */
+LOCK_EVENT(rwsem_opt_rlock) /* # of read locks opt-spin acquired */
LOCK_EVENT(rwsem_opt_wlock) /* # of write locks opt-spin acquired */
LOCK_EVENT(rwsem_opt_fail) /* # of failed opt-spinnings */
LOCK_EVENT(rwsem_rlock) /* # of read locks acquired */
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 0a29aac..015edd6 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -240,6 +240,30 @@ static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem,
#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
/*
+ * Try to acquire read lock before the reader is put on wait queue.
+ * Lock acquisition isn't allowed if the rwsem is locked or a writer handoff
+ * is ongoing.
+ */
+static inline bool rwsem_try_read_lock_unqueued(struct rw_semaphore *sem)
+{
+ long count = atomic_long_read(&sem->count);
+
+ if (RWSEM_COUNT_WLOCKED_OR_HANDOFF(count))
+ return false;
+
+ count = atomic_long_fetch_add_acquire(RWSEM_READER_BIAS, &sem->count);
+ if (!RWSEM_COUNT_WLOCKED_OR_HANDOFF(count)) {
+ rwsem_set_reader_owned(sem);
+ lockevent_inc(rwsem_opt_rlock);
+ return true;
+ }
+
+ /* Back out the change */
+ atomic_long_add(-RWSEM_READER_BIAS, &sem->count);
+ return false;
+}
+
+/*
* Try to acquire write lock before the writer has been put on wait queue.
*/
static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem,
@@ -291,8 +315,10 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
BUILD_BUG_ON(!rwsem_has_anonymous_owner(RWSEM_OWNER_UNKNOWN));
- if (need_resched())
+ if (need_resched()) {
+ lockevent_inc(rwsem_opt_fail);
return false;
+ }
rcu_read_lock();
owner = rwsem_get_owner(sem);
@@ -301,6 +327,7 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
owner_on_cpu(owner, sem);
}
rcu_read_unlock();
+ lockevent_cond_inc(rwsem_opt_fail, !ret);
return ret;
}
@@ -371,9 +398,6 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
preempt_disable();
/* sem->wait_lock should not be held when doing optimistic spinning */
- if (!rwsem_can_spin_on_owner(sem))
- goto done;
-
if (!osq_lock(&sem->osq))
goto done;
@@ -388,10 +412,11 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
/*
* Try to acquire the lock
*/
- if (rwsem_try_write_lock_unqueued(sem, wlock)) {
- taken = true;
+ taken = wlock ? rwsem_try_write_lock_unqueued(sem, wlock)
+ : rwsem_try_read_lock_unqueued(sem);
+
+ if (taken)
break;
- }
/*
* When there's no owner, we might have preempted between the
@@ -418,7 +443,13 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
return taken;
}
#else
-static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
+static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
+{
+ return false;
+}
+
+static inline bool rwsem_optimistic_spin(struct rw_semaphore *sem,
+ const long wlock)
{
return false;
}
@@ -444,6 +475,33 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
struct rwsem_waiter waiter;
DEFINE_WAKE_Q(wake_q);
+ if (!rwsem_can_spin_on_owner(sem))
+ goto queue;
+
+ /*
+ * Undo read bias from down_read() and do optimistic spinning.
+ */
+ atomic_long_add(-RWSEM_READER_BIAS, &sem->count);
+ adjustment = 0;
+ if (rwsem_optimistic_spin(sem, 0)) {
+ unsigned long flags;
+
+ /*
+ * Opportunistically wake up other readers in the wait queue.
+ * It has another chance of wakeup at unlock time.
+ */
+ if ((atomic_long_read(&sem->count) & RWSEM_FLAG_WAITERS) &&
+ raw_spin_trylock_irqsave(&sem->wait_lock, flags)) {
+ if (!list_empty(&sem->wait_list))
+ __rwsem_mark_wake(sem, RWSEM_WAKE_READ_OWNED,
+ &wake_q);
+ raw_spin_unlock_irqrestore(&sem->wait_lock, flags);
+ wake_up_q(&wake_q);
+ }
+ return sem;
+ }
+
+queue:
waiter.task = current;
waiter.type = RWSEM_WAITING_FOR_READ;
waiter.timeout = jiffies + RWSEM_WAIT_TIMEOUT;
@@ -456,7 +514,8 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
* immediately as its RWSEM_READER_BIAS has already been
* set in the count.
*/
- if (!(atomic_long_read(&sem->count) & RWSEM_WRITER_MASK)) {
+ if (adjustment &&
+ !(atomic_long_read(&sem->count) & RWSEM_WRITER_MASK)) {
raw_spin_unlock_irq(&sem->wait_lock);
rwsem_set_reader_owned(sem);
lockevent_inc(rwsem_rlock_fast);
@@ -543,7 +602,8 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
const long wlock = RWSEM_WRITER_LOCKED;
/* do optimistic spinning and steal lock if possible */
- if (rwsem_optimistic_spin(sem, wlock))
+ if (rwsem_can_spin_on_owner(sem) &&
+ rwsem_optimistic_spin(sem, wlock))
return sem;
/*
diff --git a/kernel/locking/rwsem-xadd.h b/kernel/locking/rwsem-xadd.h
index 1de6f1e..eb4ef36 100644
--- a/kernel/locking/rwsem-xadd.h
+++ b/kernel/locking/rwsem-xadd.h
@@ -109,9 +109,12 @@
RWSEM_FLAG_HANDOFF)
#define RWSEM_COUNT_LOCKED(c) ((c) & RWSEM_LOCK_MASK)
+#define RWSEM_COUNT_WLOCKED(c) ((c) & RWSEM_WRITER_MASK)
#define RWSEM_COUNT_HANDOFF(c) ((c) & RWSEM_FLAG_HANDOFF)
#define RWSEM_COUNT_LOCKED_OR_HANDOFF(c) \
((c) & (RWSEM_LOCK_MASK|RWSEM_FLAG_HANDOFF))
+#define RWSEM_COUNT_WLOCKED_OR_HANDOFF(c) \
+ ((c) & (RWSEM_WRITER_MASK | RWSEM_FLAG_HANDOFF))
/*
* Task structure pointer compression (64-bit only):
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 18/22] locking/rwsem: Make rwsem_spin_on_owner() return a tri-state value
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
This patch modifies rwsem_spin_on_owner() to return a tri-state value
to better reflect the state of lock holder which enables us to make a
better decision of what to do next.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 25 +++++++++++++++++++------
kernel/locking/rwsem-xadd.h | 5 +++++
2 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 21d462f..0a29aac 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -305,14 +305,24 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
}
/*
- * Return true only if we can still spin on the owner field of the rwsem.
+ * Return the folowing three values depending on the lock owner state.
+ * 1 when owner changes and no reader is detected yet.
+ * 0 when owner changes and the new owner may be a reader.
+ * -1 when optimistic spinning has to stop because either the owner stops
+ * running, is unknown, or its timeslice has been used up.
*/
-static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
+enum owner_state {
+ OWNER_SPINNABLE = 1,
+ OWNER_READER = 0,
+ OWNER_NONSPINNABLE = -1,
+};
+
+static noinline enum owner_state rwsem_spin_on_owner(struct rw_semaphore *sem)
{
struct task_struct *owner = rwsem_get_owner(sem);
if (!is_rwsem_owner_spinnable(owner))
- return false;
+ return OWNER_NONSPINNABLE;
rcu_read_lock();
/*
@@ -337,7 +347,7 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
*/
if (need_resched() || !owner_on_cpu(owner, sem)) {
rcu_read_unlock();
- return false;
+ return OWNER_NONSPINNABLE;
}
cpu_relax();
@@ -348,7 +358,10 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
* If there is a new owner or the owner is not set, we continue
* spinning.
*/
- return is_rwsem_owner_spinnable(rwsem_get_owner(sem));
+ owner = rwsem_get_owner(sem);
+ if (!is_rwsem_owner_spinnable(owner))
+ return OWNER_NONSPINNABLE;
+ return is_rwsem_owner_reader(owner) ? OWNER_READER : OWNER_SPINNABLE;
}
static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
@@ -371,7 +384,7 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
* 2) readers own the lock as we can't determine if they are
* actively running or not.
*/
- while (rwsem_spin_on_owner(sem)) {
+ while (rwsem_spin_on_owner(sem) == OWNER_SPINNABLE) {
/*
* Try to acquire the lock
*/
diff --git a/kernel/locking/rwsem-xadd.h b/kernel/locking/rwsem-xadd.h
index d54b5db..1de6f1e 100644
--- a/kernel/locking/rwsem-xadd.h
+++ b/kernel/locking/rwsem-xadd.h
@@ -200,6 +200,11 @@ static inline bool is_rwsem_owner_spinnable(struct task_struct *owner)
return !((unsigned long)owner & RWSEM_ANONYMOUSLY_OWNED);
}
+static inline bool is_rwsem_owner_reader(struct task_struct *owner)
+{
+ return (unsigned long)owner & RWSEM_READER_OWNED;
+}
+
/*
* Return true if the rwsem is owned by a reader.
*/
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 17/22] locking/rwsem: Recheck owner if it is not on cpu
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
After merging the owner value directly into the count field, it was
found that the number of failed optimistic spinning operations increased
significantly during the boot up process.
The cause of this increased failures was tracked down to the condition
that a lock holder might have just released the lock and gone to sleep
right after its owner value was fetched by a spinner. So the task might
have slept, but it was no longer the lock holder. The merging of owner
into count increases the chance this condition can happen.
To close this failure mode, we are now rechecking the owner value again
to see if it has been changed in case it is not on cpu.
On a 1-socket x86-64 system, the lock event counts before the patch
were:
rwsem_opt_fail=5847
rwsem_opt_wlock=7880
rwsem_wlock=5847
After the patch, the counts were:
rwsem_opt_fail=225
rwsem_opt_wlock=8541
rwsem_wlock=225
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 16dc7a1..21d462f 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -263,13 +263,25 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem,
}
}
-static inline bool owner_on_cpu(struct task_struct *owner)
+static inline bool owner_on_cpu(struct task_struct *owner,
+ struct rw_semaphore *sem)
{
/*
* As lock holder preemption issue, we both skip spinning if
* task is not on cpu or its cpu is preempted
*/
- return owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+ bool oncpu = owner->on_cpu && !vcpu_is_preempted(task_cpu(owner));
+
+ /*
+ * There is a slight chance that the lock holder might have
+ * just release the rwsem and gone to sleep right after we
+ * fetched the owner value. So we double-check the sem->owner
+ * field again to see if it has been changed. The sem->owner
+ * would have been cleared right before the lock was released.
+ */
+ if (!oncpu && (READ_ONCE(sem->owner) != owner))
+ return true; /* Assume the new owner is on cpu */
+ return oncpu;
}
static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
@@ -286,7 +298,7 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
owner = rwsem_get_owner(sem);
if (owner) {
ret = is_rwsem_owner_spinnable(owner) &&
- owner_on_cpu(owner);
+ owner_on_cpu(owner, sem);
}
rcu_read_unlock();
return ret;
@@ -323,7 +335,7 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
* abort spinning when need_resched or owner is not running or
* owner's cpu is preempted.
*/
- if (need_resched() || !owner_on_cpu(owner)) {
+ if (need_resched() || !owner_on_cpu(owner, sem)) {
rcu_read_unlock();
return false;
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 16/22] locking/rwsem: Remove redundant computation of writer lock word
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
On 64-bit architectures, each rwsem writer will have its unique lock
word for acquiring the lock. Right now, the writer code recomputes the
lock word every time it tries to acquire the lock. This is a waste of
time. The lock word is now cached and reused when it is needed.
On 32-bit architectures, the extra constant argument to
rwsem_try_write_lock() and rwsem_try_write_lock_unqueued() should be
optimized out by the compiler.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 0869fbf..16dc7a1 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -216,8 +216,8 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
* race conditions between checking the rwsem wait list and setting the
* sem->count accordingly.
*/
-static inline bool
-rwsem_try_write_lock(long count, struct rw_semaphore *sem, bool first)
+static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem,
+ const long wlock, bool first)
{
long new;
@@ -227,7 +227,7 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
if (!first && RWSEM_COUNT_HANDOFF(count))
return false;
- new = (count & ~RWSEM_FLAG_HANDOFF) + RWSEM_WRITER_LOCKED -
+ new = (count & ~RWSEM_FLAG_HANDOFF) + wlock -
(list_is_singular(&sem->wait_list) ? RWSEM_FLAG_WAITERS : 0);
if (atomic_long_cmpxchg_acquire(&sem->count, count, new) == count) {
@@ -242,7 +242,8 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
/*
* Try to acquire write lock before the writer has been put on wait queue.
*/
-static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
+static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem,
+ const long wlock)
{
long old, count = atomic_long_read(&sem->count);
@@ -251,7 +252,7 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
return false;
old = atomic_long_cmpxchg_acquire(&sem->count, count,
- count + RWSEM_WRITER_LOCKED);
+ count + wlock);
if (old == count) {
rwsem_set_owner(sem);
lockevent_inc(rwsem_opt_wlock);
@@ -338,7 +339,7 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
return is_rwsem_owner_spinnable(rwsem_get_owner(sem));
}
-static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
+static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
{
bool taken = false;
@@ -362,7 +363,7 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
/*
* Try to acquire the lock
*/
- if (rwsem_try_write_lock_unqueued(sem)) {
+ if (rwsem_try_write_lock_unqueued(sem, wlock)) {
taken = true;
break;
}
@@ -392,7 +393,7 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
return taken;
}
#else
-static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
+static bool rwsem_optimistic_spin(struct rw_semaphore *sem, const long wlock)
{
return false;
}
@@ -514,9 +515,10 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
struct rwsem_waiter waiter;
struct rw_semaphore *ret = sem;
DEFINE_WAKE_Q(wake_q);
+ const long wlock = RWSEM_WRITER_LOCKED;
/* do optimistic spinning and steal lock if possible */
- if (rwsem_optimistic_spin(sem))
+ if (rwsem_optimistic_spin(sem, wlock))
return sem;
/*
@@ -569,7 +571,7 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
/* wait until we successfully acquire the lock */
set_current_state(state);
while (true) {
- if (rwsem_try_write_lock(count, sem, first))
+ if (rwsem_try_write_lock(count, sem, wlock, first))
break;
raw_spin_unlock_irq(&sem->wait_lock);
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 15/22] locking/rwsem: Merge owner into count on x86-64
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
With separate count and owner, there are timing windows where the two
values are inconsistent. That can cause problem when trying to figure
out the exact state of the rwsem. For instance, a RT task will stop
optimistic spinning if the lock is acquired by a writer but the owner
field isn't set yet. That can be solved by combining the count and
owner together in a single atomic value.
On 32-bit architectures, there aren't enough bits to hold both.
64-bit architectures, however, can have enough bits to do that. For
x86-64, the physical address can use up to 52 bits. That is 4PB of
memory. That leaves 12 bits available for other use. The task structure
pointer is also aligned to the L1 cache size. That means another 6 bits
(64 bytes cacheline) will be available. Reserving 2 bits for status
flags, we will have 16 bits for the reader count. That can supports
up to (64k-1) readers.
The owner value will still be duplicated in the owner field for the
purpose of signalling that the task is in the process of acquiring or
releasing a rwsem.
This change is currently for x86-64 only. Other 64-bit architectures may
be enabled in the future if the need arises.
With a locking microbenchmark running on 5.0 based kernel, the total
locking rates (in kops/s) of the benchmark on a 4-socket 56-core
x86-64 system before and after the patch were as follows:
Before Patch After Patch
# of Threads wlock rlock wlock rlock
------------ ----- ----- ----- -----
1 29,085 30,179 27,892 29,514
2 7,341 14,084 6,240 14,304
4 7,393 14,246 5,216 11,754
8 7,139 13,860 5,400 11,308
16 6,650 15,773 5,744 15,405
This change does have an impact on both read and write lock performance.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 20 +++++++--
kernel/locking/rwsem-xadd.h | 105 +++++++++++++++++++++++++++++++++++++++-----
2 files changed, 110 insertions(+), 15 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 719d390..0869fbf 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -27,11 +27,11 @@
/*
* Guide to the rw_semaphore's count field.
*
- * When the RWSEM_WRITER_LOCKED bit in count is set, the lock is owned
- * by a writer.
+ * When any of the RWSEM_WRITER_MASK bits in count is set, the lock is
+ * owned by a writer.
*
* The lock is owned by readers when
- * (1) the RWSEM_WRITER_LOCKED isn't set in count,
+ * (1) none of the RWSEM_WRITER_MASK bits is set in count,
* (2) some of the reader bits are set in count, and
* (3) the owner field has RWSEM_READ_OWNED bit set.
*
@@ -47,6 +47,11 @@
void __init_rwsem(struct rw_semaphore *sem, const char *name,
struct lock_class_key *key)
{
+ /*
+ * We should support at least (4k-1) concurrent readers
+ */
+ BUILD_BUG_ON(sizeof(long) * 8 - RWSEM_READER_SHIFT < 12);
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
/*
* Make sure we are not reinitializing a held semaphore:
@@ -297,7 +302,14 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
return false;
rcu_read_lock();
- while (owner && (rwsem_get_owner(sem) == owner)) {
+ /*
+ * In case the owner task pointer is also stored in the count,
+ * checking the sem->owner value alone will give an early indication
+ * if the owner is about to release the lock (sem->owner cleared).
+ * This enables the spinner to move forward and do a trylock
+ * earlier.
+ */
+ while (owner && (READ_ONCE(sem->owner) == owner)) {
/*
* Ensure we emit the owner->on_cpu, dereference _after_
* checking sem->owner still matches owner, if that fails,
diff --git a/kernel/locking/rwsem-xadd.h b/kernel/locking/rwsem-xadd.h
index 277a134..d54b5db 100644
--- a/kernel/locking/rwsem-xadd.h
+++ b/kernel/locking/rwsem-xadd.h
@@ -37,25 +37,73 @@
#endif
/*
- * The definition of the atomic counter in the semaphore:
+ * With separate count and owner, there are timing windows where the two
+ * values are inconsistent. That can cause problem when trying to figure
+ * out the exact state of the rwsem. That can be solved by combining
+ * the count and owner together in a single atomic value.
*
- * Bit 0 - writer locked bit
- * Bit 1 - waiters present bit
- * Bit 2 - lock handoff bit
- * Bits 3-7 - reserved
- * Bits 8-X - 24-bit (32-bit) or 56-bit reader count
+ * On 64-bit architectures, the owner task structure pointer can be
+ * compressed and combined with reader count and other status flags.
+ * A simple compression method is to map the virtual address back to
+ * the physical address by subtracting PAGE_OFFSET. On 32-bit
+ * architectures, the long integer value just isn't big enough for
+ * combining owner and count. So they remain separate.
+ *
+ * For x86-64, the physical address can use up to 52 bits. That is 4PB
+ * of memory. That leaves 12 bits available for other use. The task
+ * structure pointer is also aligned to the L1 cache size. That means
+ * another 6 bits (64 bytes cacheline) will be available. Reserving
+ * 2 bits for status flags, we will have 16 bits for the reader count.
+ * That can supports up to (64k-1) readers.
+ *
+ * On x86-64, the bit definitions of the count are:
+ *
+ * Bit 0 - waiters present bit
+ * Bit 1 - lock handoff bit
+ * Bits 2-47 - compressed task structure pointer
+ * Bits 48-63 - 16-bit reader counts
+ *
+ * On other 64-bit architectures, the bit definitions are:
+ *
+ * Bit 0 - waiters present bit
+ * Bit 1 - lock handoff bit
+ * Bits 2-6 - reserved
+ * Bit 7 - writer lock bit
+ * Bits 8-63 - 56-bit reader counts
+ *
+ * On 32-bit architectures, the bit definitions of the count are:
+ *
+ * Bit 0 - waiters present bit
+ * Bit 1 - lock handoff bit
+ * Bits 2-6 - reserved
+ * Bit 7 - writer lock bit
+ * Bits 8-31 - 24-bit reader counts
*
* atomic_long_fetch_add() is used to obtain reader lock, whereas
* atomic_long_cmpxchg() will be used to obtain writer lock.
*/
-#define RWSEM_WRITER_LOCKED (1UL << 0)
-#define RWSEM_FLAG_WAITERS (1UL << 1)
-#define RWSEM_FLAG_HANDOFF (1UL << 2)
+#define RWSEM_FLAG_WAITERS (1UL << 0)
+#define RWSEM_FLAG_HANDOFF (1UL << 1)
+#ifdef CONFIG_X86_64
+
+#ifdef __PHYSICAL_MASK_SHIFT
+#define RWSEM_PA_MASK_SHIFT __PHYSICAL_MASK_SHIFT
+#else
+#define RWSEM_PA_MASK_SHIFT 52
+#endif
+#define RWSEM_READER_SHIFT (RWSEM_PA_MASK_SHIFT - L1_CACHE_SHIFT + 2)
+#define RWSEM_WRITER_MASK ((1UL << RWSEM_READER_SHIFT) - 4)
+#define RWSEM_WRITER_LOCKED rwsem_owner_count(current)
+
+#else /* CONFIG_X86_64 */
+#define RWSEM_WRITER_MASK (1UL << 7)
#define RWSEM_READER_SHIFT 8
+#define RWSEM_WRITER_LOCKED RWSEM_WRITER_MASK
+#endif /* CONFIG_X86_64 */
+
#define RWSEM_READER_BIAS (1UL << RWSEM_READER_SHIFT)
#define RWSEM_READER_MASK (~(RWSEM_READER_BIAS - 1))
-#define RWSEM_WRITER_MASK RWSEM_WRITER_LOCKED
#define RWSEM_LOCK_MASK (RWSEM_WRITER_MASK|RWSEM_READER_MASK)
#define RWSEM_READ_FAILED_MASK (RWSEM_WRITER_MASK|RWSEM_FLAG_WAITERS|\
RWSEM_FLAG_HANDOFF)
@@ -65,6 +113,21 @@
#define RWSEM_COUNT_LOCKED_OR_HANDOFF(c) \
((c) & (RWSEM_LOCK_MASK|RWSEM_FLAG_HANDOFF))
+/*
+ * Task structure pointer compression (64-bit only):
+ * (owner - PAGE_OFFSET) >> (L1_CACHE_SHIFT - 2)
+ */
+static inline unsigned long rwsem_owner_count(struct task_struct *owner)
+{
+ return ((unsigned long)owner - PAGE_OFFSET) >> (L1_CACHE_SHIFT - 2);
+}
+
+static inline unsigned long rwsem_count_owner(long count)
+{
+ return (((unsigned long)count & RWSEM_WRITER_MASK)
+ << (L1_CACHE_SHIFT - 2)) + PAGE_OFFSET;
+}
+
#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
/*
* All writes to owner are protected by WRITE_ONCE() to make sure that
@@ -72,7 +135,12 @@
* the owner value concurrently without lock. Read from owner, however,
* may not need READ_ONCE() as long as the pointer value is only used
* for comparison and isn't being dereferenced.
+ *
+ * On 32-bit architectures, the owner and count are separate. On 64-bit
+ * architectures, however, the writer task structure pointer is written
+ * to the count as well in addition to the owner field.
*/
+
static inline void rwsem_set_owner(struct rw_semaphore *sem)
{
WRITE_ONCE(sem->owner, current);
@@ -83,10 +151,22 @@ static inline void rwsem_clear_owner(struct rw_semaphore *sem)
WRITE_ONCE(sem->owner, NULL);
}
+#ifdef CONFIG_X86_64
+/*
+ * Get the owner value from count to have early access to the task structure.
+ */
+static inline struct task_struct *rwsem_get_owner(struct rw_semaphore *sem)
+{
+ return (struct task_struct *)
+ (rwsem_count_owner(atomic_long_read(&sem->count)) |
+ ((unsigned long)READ_ONCE(sem->owner) & 3));
+}
+#else /* !CONFIG_X86_64 */
static inline struct task_struct *rwsem_get_owner(struct rw_semaphore *sem)
{
return READ_ONCE(sem->owner);
}
+#endif /* CONFIG_X86_64 */
/*
* The task_struct pointer of the last owning reader will be left in
@@ -291,8 +371,11 @@ static inline void __up_write(struct rw_semaphore *sem)
long tmp;
DEBUG_RWSEMS_WARN_ON(sem->owner != current, sem);
+#ifdef CONFIG_X86_64
+ DEBUG_RWSEMS_WARN_ON(sem->owner != rwsem_get_owner(sem), sem);
+#endif
rwsem_clear_owner(sem);
- tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
+ tmp = atomic_long_fetch_and_release(~RWSEM_WRITER_MASK, &sem->count);
if (unlikely(tmp & RWSEM_FLAG_WAITERS))
rwsem_wake(sem, tmp);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 14/22] locking/rwsem: Add more rwsem owner access helpers
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
Before combining owner and count, we are adding two new helpers for
accessing the owner value in the rwsem.
1) struct task_struct *rwsem_get_owner(struct rw_semaphore *sem)
2) bool is_rwsem_reader_owned(struct rw_semaphore *sem)
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 11 ++++++-----
kernel/locking/rwsem-xadd.h | 32 ++++++++++++++++++++++++++------
kernel/locking/rwsem.c | 3 +--
3 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 5f74bae..719d390 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -277,7 +277,7 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
return false;
rcu_read_lock();
- owner = READ_ONCE(sem->owner);
+ owner = rwsem_get_owner(sem);
if (owner) {
ret = is_rwsem_owner_spinnable(owner) &&
owner_on_cpu(owner);
@@ -291,13 +291,13 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem)
*/
static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
{
- struct task_struct *owner = READ_ONCE(sem->owner);
+ struct task_struct *owner = rwsem_get_owner(sem);
if (!is_rwsem_owner_spinnable(owner))
return false;
rcu_read_lock();
- while (owner && (READ_ONCE(sem->owner) == owner)) {
+ while (owner && (rwsem_get_owner(sem) == owner)) {
/*
* Ensure we emit the owner->on_cpu, dereference _after_
* checking sem->owner still matches owner, if that fails,
@@ -323,7 +323,7 @@ static noinline bool rwsem_spin_on_owner(struct rw_semaphore *sem)
* If there is a new owner or the owner is not set, we continue
* spinning.
*/
- return is_rwsem_owner_spinnable(READ_ONCE(sem->owner));
+ return is_rwsem_owner_spinnable(rwsem_get_owner(sem));
}
static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
@@ -361,7 +361,8 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
* we're an RT task that will live-lock because we won't let
* the owner complete.
*/
- if (!sem->owner && (need_resched() || rt_task(current)))
+ if (!rwsem_get_owner(sem) &&
+ (need_resched() || rt_task(current)))
break;
/*
diff --git a/kernel/locking/rwsem-xadd.h b/kernel/locking/rwsem-xadd.h
index 6d4890d..277a134 100644
--- a/kernel/locking/rwsem-xadd.h
+++ b/kernel/locking/rwsem-xadd.h
@@ -83,6 +83,11 @@ static inline void rwsem_clear_owner(struct rw_semaphore *sem)
WRITE_ONCE(sem->owner, NULL);
}
+static inline struct task_struct *rwsem_get_owner(struct rw_semaphore *sem)
+{
+ return READ_ONCE(sem->owner);
+}
+
/*
* The task_struct pointer of the last owning reader will be left in
* the owner field.
@@ -116,6 +121,23 @@ static inline bool is_rwsem_owner_spinnable(struct task_struct *owner)
}
/*
+ * Return true if the rwsem is owned by a reader.
+ */
+static inline bool is_rwsem_reader_owned(struct rw_semaphore *sem)
+{
+#ifdef CONFIG_DEBUG_RWSEMS
+ /*
+ * Check the count to see if it is write-locked.
+ */
+ long count = atomic_long_read(&sem->count);
+
+ if (count & RWSEM_WRITER_MASK)
+ return false;
+#endif
+ return (unsigned long)sem->owner & RWSEM_READER_OWNED;
+}
+
+/*
* Return true if rwsem is owned by an anonymous writer or readers.
*/
static inline bool rwsem_has_anonymous_owner(struct task_struct *owner)
@@ -135,6 +157,7 @@ static inline void rwsem_clear_reader_owned(struct rw_semaphore *sem)
{
unsigned long val = (unsigned long)current | RWSEM_READER_OWNED
| RWSEM_ANONYMOUSLY_OWNED;
+
if (READ_ONCE(sem->owner) == (struct task_struct *)val)
cmpxchg_relaxed((unsigned long *)&sem->owner, val,
RWSEM_READER_OWNED | RWSEM_ANONYMOUSLY_OWNED);
@@ -181,8 +204,7 @@ static inline void __down_read(struct rw_semaphore *sem)
if (unlikely(atomic_long_fetch_add_acquire(RWSEM_READER_BIAS,
&sem->count) & RWSEM_READ_FAILED_MASK)) {
rwsem_down_read_failed(sem);
- DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner &
- RWSEM_READER_OWNED), sem);
+ DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
} else {
rwsem_set_reader_owned(sem);
}
@@ -194,8 +216,7 @@ static inline int __down_read_killable(struct rw_semaphore *sem)
&sem->count) & RWSEM_READ_FAILED_MASK)) {
if (IS_ERR(rwsem_down_read_failed_killable(sem)))
return -EINTR;
- DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner &
- RWSEM_READER_OWNED), sem);
+ DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
} else {
rwsem_set_reader_owned(sem);
}
@@ -254,8 +275,7 @@ static inline void __up_read(struct rw_semaphore *sem)
{
long tmp;
- DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner & RWSEM_READER_OWNED),
- sem);
+ DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
rwsem_clear_reader_owned(sem);
tmp = atomic_long_add_return_release(-RWSEM_READER_BIAS, &sem->count);
if (unlikely((tmp & (RWSEM_LOCK_MASK|RWSEM_FLAG_WAITERS))
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index bdfca7c..79fa6e4 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -203,8 +203,7 @@ int __sched down_write_killable_nested(struct rw_semaphore *sem, int subclass)
void up_read_non_owner(struct rw_semaphore *sem)
{
- DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner & RWSEM_READER_OWNED),
- sem);
+ DEBUG_RWSEMS_WARN_ON(!is_rwsem_reader_owned(sem), sem);
__up_read(sem);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 13/22] locking/rwsem: Remove rwsem_wake() wakeup optimization
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
With the commit 59aabfc7e959 ("locking/rwsem: Reduce spinlock contention
in wakeup after up_read()/up_write()"), the rwsem_wake() forgoes doing
a wakeup if the wait_lock cannot be directly acquired and an optimistic
spinning locker is present. This can help performance by avoiding
spinning on the wait_lock when it is contended.
With the later commit 133e89ef5ef3 ("locking/rwsem: Enable lockless
waiter wakeup(s)"), the performance advantage of the above optimization
diminishes as the average wait_lock hold time become much shorter.
By supporting rwsem lock handoff, we can no longer relies on the fact
that the presence of an optimistic spinning locker will ensure that the
lock will be acquired by a task soon. This can lead to missed wakeup
and application hang. So the commit 59aabfc7e959 ("locking/rwsem:
Reduce spinlock contention in wakeup after up_read()/up_write()")
will have to be reverted.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 74 ---------------------------------------------
1 file changed, 74 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 12b1d61..5f74bae 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -378,25 +378,11 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
lockevent_cond_inc(rwsem_opt_fail, !taken);
return taken;
}
-
-/*
- * Return true if the rwsem has active spinner
- */
-static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
-{
- return osq_is_locked(&sem->osq);
-}
-
#else
static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
{
return false;
}
-
-static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
-{
- return false;
-}
#endif
/*
@@ -662,67 +648,7 @@ struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem, long count)
unsigned long flags;
DEFINE_WAKE_Q(wake_q);
- /*
- * __rwsem_down_write_failed_common(sem)
- * rwsem_optimistic_spin(sem)
- * osq_unlock(sem->osq)
- * ...
- * atomic_long_add_return(&sem->count)
- *
- * - VS -
- *
- * __up_write()
- * if (atomic_long_sub_return_release(&sem->count) < 0)
- * rwsem_wake(sem)
- * osq_is_locked(&sem->osq)
- *
- * And __up_write() must observe !osq_is_locked() when it observes the
- * atomic_long_add_return() in order to not miss a wakeup.
- *
- * This boils down to:
- *
- * [S.rel] X = 1 [RmW] r0 = (Y += 0)
- * MB RMB
- * [RmW] Y += 1 [L] r1 = X
- *
- * exists (r0=1 /\ r1=0)
- */
- smp_rmb();
-
- /*
- * If a spinner is present and the handoff flag isn't set, it is
- * not necessary to do the wakeup.
- *
- * Try to do wakeup only if the trylock succeeds to minimize
- * spinlock contention which may introduce too much delay in the
- * unlock operation.
- *
- * spinning writer up_write/up_read caller
- * --------------- -----------------------
- * [S] osq_unlock() [L] osq
- * MB RMB
- * [RmW] rwsem_try_write_lock() [RmW] spin_trylock(wait_lock)
- *
- * Here, it is important to make sure that there won't be a missed
- * wakeup while the rwsem is free and the only spinning writer goes
- * to sleep without taking the rwsem. Even when the spinning writer
- * is just going to break out of the waiting loop, it will still do
- * a trylock in rwsem_down_write_failed() before sleeping. IOW, if
- * rwsem_has_spinner() is true, it will guarantee at least one
- * trylock attempt on the rwsem later on.
- */
- if (rwsem_has_spinner(sem) && !RWSEM_COUNT_HANDOFF(count)) {
- /*
- * The smp_rmb() here is to make sure that the spinner
- * state is consulted before reading the wait_lock.
- */
- smp_rmb();
- if (!raw_spin_trylock_irqsave(&sem->wait_lock, flags))
- return sem;
- goto locked;
- }
raw_spin_lock_irqsave(&sem->wait_lock, flags);
-locked:
if (!list_empty(&sem->wait_list))
__rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 12/22] locking/rwsem: Implement lock handoff to prevent lock starvation
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
Because of writer lock stealing, it is possible that a constant
stream of incoming writers will cause a waiting writer or reader to
wait indefinitely leading to lock starvation.
The mutex code has a lock handoff mechanism to prevent lock starvation.
This patch implements a similar lock handoff mechanism to disable
lock stealing and force lock handoff to the first waiter in the queue
after at least a 5ms waiting period. The waiting period is used to
avoid discouraging lock stealing too much to affect performance.
A rwsem microbenchmark was run for 8 seconds on a 4-socket 56-core
128-thread x86-64 system with a v5.0 based kernel and 112 write_lock
threads with 5us sleep critical section.
Before the patch, the min/mean/max numbers of locking operations for
the locking threads were 1/7,708/542,988. After the patch, the figures
became 6,031/7,267/9,476. It can be seen that the rwsem became much more
fair, though there was a slight drop of about 6% in the mean locking
operations done which was a tradeoff of having better fairness.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/lock_events_list.h | 2 +
kernel/locking/rwsem-xadd.c | 110 +++++++++++++++++++++++++++++++-------
kernel/locking/rwsem-xadd.h | 23 +++++---
3 files changed, 109 insertions(+), 26 deletions(-)
diff --git a/kernel/locking/lock_events_list.h b/kernel/locking/lock_events_list.h
index c33c5df..4cde507 100644
--- a/kernel/locking/lock_events_list.h
+++ b/kernel/locking/lock_events_list.h
@@ -62,6 +62,8 @@
LOCK_EVENT(rwsem_rlock) /* # of read locks acquired */
LOCK_EVENT(rwsem_rlock_fast) /* # of fast read locks acquired */
LOCK_EVENT(rwsem_rlock_fail) /* # of failed read lock acquisitions */
+LOCK_EVENT(rwsem_rlock_handoff) /* # of read lock handoffs */
LOCK_EVENT(rwsem_wlock) /* # of write locks acquired */
LOCK_EVENT(rwsem_wlock_fail) /* # of failed write lock acquisitions */
+LOCK_EVENT(rwsem_wlock_handoff) /* # of write lock handoffs */
#endif /* CONFIG_RWSEM_XCHGADD_ALGORITHM */
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 18a414e..12b1d61 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -74,6 +74,7 @@ struct rwsem_waiter {
struct list_head list;
struct task_struct *task;
enum rwsem_waiter_type type;
+ unsigned long timeout;
};
enum rwsem_wake_type {
@@ -83,6 +84,12 @@ enum rwsem_wake_type {
};
/*
+ * The minimum waiting time (5ms) in the wait queue before initiating the
+ * handoff protocol.
+ */
+#define RWSEM_WAIT_TIMEOUT ((HZ - 1)/200 + 1)
+
+/*
* handle the lock release when processes blocked on it that can now run
* - if we come here from up_xxxx(), then the RWSEM_FLAG_WAITERS bit must
* have been set.
@@ -132,6 +139,15 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
adjustment = RWSEM_READER_BIAS;
oldcount = atomic_long_fetch_add(adjustment, &sem->count);
if (unlikely(oldcount & RWSEM_WRITER_MASK)) {
+ /*
+ * Initiate handoff to reader, if applicable.
+ */
+ if (!(oldcount & RWSEM_FLAG_HANDOFF) &&
+ time_after(jiffies, waiter->timeout)) {
+ adjustment -= RWSEM_FLAG_HANDOFF;
+ lockevent_inc(rwsem_rlock_handoff);
+ }
+
atomic_long_sub(adjustment, &sem->count);
return;
}
@@ -180,6 +196,12 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
adjustment -= RWSEM_FLAG_WAITERS;
}
+ /*
+ * Clear the handoff flag
+ */
+ if (woken && RWSEM_COUNT_HANDOFF(atomic_long_read(&sem->count)))
+ adjustment -= RWSEM_FLAG_HANDOFF;
+
if (adjustment)
atomic_long_add(adjustment, &sem->count);
}
@@ -189,15 +211,19 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
* race conditions between checking the rwsem wait list and setting the
* sem->count accordingly.
*/
-static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem)
+static inline bool
+rwsem_try_write_lock(long count, struct rw_semaphore *sem, bool first)
{
long new;
if (RWSEM_COUNT_LOCKED(count))
return false;
- new = count + RWSEM_WRITER_LOCKED -
- (list_is_singular(&sem->wait_list) ? RWSEM_FLAG_WAITERS : 0);
+ if (!first && RWSEM_COUNT_HANDOFF(count))
+ return false;
+
+ new = (count & ~RWSEM_FLAG_HANDOFF) + RWSEM_WRITER_LOCKED -
+ (list_is_singular(&sem->wait_list) ? RWSEM_FLAG_WAITERS : 0);
if (atomic_long_cmpxchg_acquire(&sem->count, count, new) == count) {
rwsem_set_owner(sem);
@@ -216,7 +242,7 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
long old, count = atomic_long_read(&sem->count);
while (true) {
- if (RWSEM_COUNT_LOCKED(count))
+ if (RWSEM_COUNT_LOCKED_OR_HANDOFF(count))
return false;
old = atomic_long_cmpxchg_acquire(&sem->count, count,
@@ -374,6 +400,16 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
#endif
/*
+ * This is safe to be called without holding the wait_lock.
+ */
+static inline bool
+rwsem_waiter_is_first(struct rw_semaphore *sem, struct rwsem_waiter *waiter)
+{
+ return list_first_entry(&sem->wait_list, struct rwsem_waiter, list)
+ == waiter;
+}
+
+/*
* Wait for the read lock to be granted
*/
static inline struct rw_semaphore __sched *
@@ -385,6 +421,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
waiter.task = current;
waiter.type = RWSEM_WAITING_FOR_READ;
+ waiter.timeout = jiffies + RWSEM_WAIT_TIMEOUT;
raw_spin_lock_irq(&sem->wait_lock);
if (list_empty(&sem->wait_list)) {
@@ -441,8 +478,12 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
return sem;
out_nolock:
list_del(&waiter.list);
- if (list_empty(&sem->wait_list))
- atomic_long_add(-RWSEM_FLAG_WAITERS, &sem->count);
+ if (list_empty(&sem->wait_list)) {
+ int adjustment = -RWSEM_FLAG_WAITERS -
+ (atomic_long_read(&sem->count) & RWSEM_FLAG_HANDOFF);
+
+ atomic_long_add(adjustment, &sem->count);
+ }
raw_spin_unlock_irq(&sem->wait_lock);
__set_current_state(TASK_RUNNING);
lockevent_inc(rwsem_rlock_fail);
@@ -469,8 +510,8 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
static inline struct rw_semaphore *
__rwsem_down_write_failed_common(struct rw_semaphore *sem, int state)
{
- long count;
- bool waiting = true; /* any queued threads before us */
+ long count, adjustment;
+ bool first; /* First one in queue */
struct rwsem_waiter waiter;
struct rw_semaphore *ret = sem;
DEFINE_WAKE_Q(wake_q);
@@ -485,17 +526,17 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
*/
waiter.task = current;
waiter.type = RWSEM_WAITING_FOR_WRITE;
+ waiter.timeout = jiffies + RWSEM_WAIT_TIMEOUT;
raw_spin_lock_irq(&sem->wait_lock);
/* account for this before adding a new element to the list */
- if (list_empty(&sem->wait_list))
- waiting = false;
+ first = list_empty(&sem->wait_list);
list_add_tail(&waiter.list, &sem->wait_list);
/* we're now waiting on the lock, but no longer actively locking */
- if (waiting) {
+ if (!first) {
count = atomic_long_read(&sem->count);
/*
@@ -529,12 +570,13 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
/* wait until we successfully acquire the lock */
set_current_state(state);
while (true) {
- if (rwsem_try_write_lock(count, sem))
+ if (rwsem_try_write_lock(count, sem, first))
break;
+
raw_spin_unlock_irq(&sem->wait_lock);
/* Block until there are no active lockers. */
- do {
+ for (;;) {
if (signal_pending_state(state, current))
goto out_nolock;
@@ -542,7 +584,29 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
lockevent_inc(rwsem_sleep_writer);
set_current_state(state);
count = atomic_long_read(&sem->count);
- } while (RWSEM_COUNT_LOCKED(count));
+
+ if (!first)
+ first = rwsem_waiter_is_first(sem, &waiter);
+
+ if (!RWSEM_COUNT_LOCKED(count))
+ break;
+
+ if (first && !RWSEM_COUNT_HANDOFF(count) &&
+ time_after(jiffies, waiter.timeout)) {
+ atomic_long_or(RWSEM_FLAG_HANDOFF, &sem->count);
+ /*
+ * Make sure the handoff bit is seen by
+ * others before proceeding.
+ */
+ smp_mb__after_atomic();
+ lockevent_inc(rwsem_wlock_handoff);
+ /*
+ * Do a trylock after setting the handoff
+ * flag to avoid missed wakeup.
+ */
+ break;
+ }
+ }
raw_spin_lock_irq(&sem->wait_lock);
}
@@ -557,9 +621,15 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
__set_current_state(TASK_RUNNING);
raw_spin_lock_irq(&sem->wait_lock);
list_del(&waiter.list);
+ adjustment = 0;
if (list_empty(&sem->wait_list))
- atomic_long_add(-RWSEM_FLAG_WAITERS, &sem->count);
- else
+ adjustment -= RWSEM_FLAG_WAITERS;
+ if (first)
+ adjustment -= (atomic_long_read(&sem->count) &
+ RWSEM_FLAG_HANDOFF);
+ if (adjustment)
+ atomic_long_add(adjustment, &sem->count);
+ if (!list_empty(&sem->wait_list))
__rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
raw_spin_unlock_irq(&sem->wait_lock);
wake_up_q(&wake_q);
@@ -587,7 +657,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
* - up_read/up_write has decremented the active part of count if we come here
*/
__visible
-struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
+struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem, long count)
{
unsigned long flags;
DEFINE_WAKE_Q(wake_q);
@@ -620,7 +690,9 @@ struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
smp_rmb();
/*
- * If a spinner is present, it is not necessary to do the wakeup.
+ * If a spinner is present and the handoff flag isn't set, it is
+ * not necessary to do the wakeup.
+ *
* Try to do wakeup only if the trylock succeeds to minimize
* spinlock contention which may introduce too much delay in the
* unlock operation.
@@ -639,7 +711,7 @@ struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem)
* rwsem_has_spinner() is true, it will guarantee at least one
* trylock attempt on the rwsem later on.
*/
- if (rwsem_has_spinner(sem)) {
+ if (rwsem_has_spinner(sem) && !RWSEM_COUNT_HANDOFF(count)) {
/*
* The smp_rmb() here is to make sure that the spinner
* state is consulted before reading the wait_lock.
diff --git a/kernel/locking/rwsem-xadd.h b/kernel/locking/rwsem-xadd.h
index 1febd17..6d4890d 100644
--- a/kernel/locking/rwsem-xadd.h
+++ b/kernel/locking/rwsem-xadd.h
@@ -41,7 +41,8 @@
*
* Bit 0 - writer locked bit
* Bit 1 - waiters present bit
- * Bits 2-7 - reserved
+ * Bit 2 - lock handoff bit
+ * Bits 3-7 - reserved
* Bits 8-X - 24-bit (32-bit) or 56-bit reader count
*
* atomic_long_fetch_add() is used to obtain reader lock, whereas
@@ -49,14 +50,20 @@
*/
#define RWSEM_WRITER_LOCKED (1UL << 0)
#define RWSEM_FLAG_WAITERS (1UL << 1)
+#define RWSEM_FLAG_HANDOFF (1UL << 2)
+
#define RWSEM_READER_SHIFT 8
#define RWSEM_READER_BIAS (1UL << RWSEM_READER_SHIFT)
#define RWSEM_READER_MASK (~(RWSEM_READER_BIAS - 1))
#define RWSEM_WRITER_MASK RWSEM_WRITER_LOCKED
#define RWSEM_LOCK_MASK (RWSEM_WRITER_MASK|RWSEM_READER_MASK)
-#define RWSEM_READ_FAILED_MASK (RWSEM_WRITER_MASK|RWSEM_FLAG_WAITERS)
+#define RWSEM_READ_FAILED_MASK (RWSEM_WRITER_MASK|RWSEM_FLAG_WAITERS|\
+ RWSEM_FLAG_HANDOFF)
#define RWSEM_COUNT_LOCKED(c) ((c) & RWSEM_LOCK_MASK)
+#define RWSEM_COUNT_HANDOFF(c) ((c) & RWSEM_FLAG_HANDOFF)
+#define RWSEM_COUNT_LOCKED_OR_HANDOFF(c) \
+ ((c) & (RWSEM_LOCK_MASK|RWSEM_FLAG_HANDOFF))
#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
/*
@@ -163,7 +170,7 @@ static inline void rwsem_clear_reader_owned(struct rw_semaphore *sem)
extern struct rw_semaphore *rwsem_down_read_failed_killable(struct rw_semaphore *sem);
extern struct rw_semaphore *rwsem_down_write_failed(struct rw_semaphore *sem);
extern struct rw_semaphore *rwsem_down_write_failed_killable(struct rw_semaphore *sem);
-extern struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem);
+extern struct rw_semaphore *rwsem_wake(struct rw_semaphore *sem, long count);
extern struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem);
/*
@@ -253,7 +260,7 @@ static inline void __up_read(struct rw_semaphore *sem)
tmp = atomic_long_add_return_release(-RWSEM_READER_BIAS, &sem->count);
if (unlikely((tmp & (RWSEM_LOCK_MASK|RWSEM_FLAG_WAITERS))
== RWSEM_FLAG_WAITERS))
- rwsem_wake(sem);
+ rwsem_wake(sem, tmp);
}
/*
@@ -261,11 +268,13 @@ static inline void __up_read(struct rw_semaphore *sem)
*/
static inline void __up_write(struct rw_semaphore *sem)
{
+ long tmp;
+
DEBUG_RWSEMS_WARN_ON(sem->owner != current, sem);
rwsem_clear_owner(sem);
- if (unlikely(atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED,
- &sem->count) & RWSEM_FLAG_WAITERS))
- rwsem_wake(sem);
+ tmp = atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED, &sem->count);
+ if (unlikely(tmp & RWSEM_FLAG_WAITERS))
+ rwsem_wake(sem, tmp);
}
/*
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 11/22] locking/rwsem: Implement a new locking scheme
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
The current way of using various reader, writer and waiting biases
in the rwsem code are confusing and hard to understand. I have to
reread the rwsem count guide in the rwsem-xadd.c file from time to
time to remind myself how this whole thing works. It also makes the
rwsem code harder to be optimized.
To make rwsem more sane, a new locking scheme similar to the one in
qrwlock is now being used. The count is now a 32-bit atomic value
in all architectures. The current bit definitions are:
Bit 0 - writer locked bit
Bit 1 - waiters present bit
Bits 2-7 - reserved for future extension
Bits 8-X - reader count (24/56 bits)
Now the cmpxchg instruction is used to acquire the write lock. The
read lock is still acquired with xadd instruction, so there is no
change here. This scheme will allow up to 16M active readers which
should be more than enough. We can always use some more reserved bits
if necessary.
With that change, we can deterministically know if a rwsem has been
write-locked. Looking at the count alone, however, one cannot determine
for certain if a rwsem is owned by readers or not as the readers that
set the reader count bits may be in the process of backing out. So we
still need the reader-owned bit in the owner field to be sure.
With a locking microbenchmark running on 5.0 based kernel, the total
locking rates (in kops/s) of the benchmark on a 4-socket 56-core
x86-64 system before and after the patch were as follows:
Before Patch After Patch
# of Threads wlock rlock wlock rlock
------------ ----- ----- ----- -----
1 28,773 30,164 29,063 29,967
2 7,435 15,167 6,746 13,198
4 7,181 14,438 6,515 12,763
8 6,918 13,796 6,747 13,419
16 6,554 16,030 6,653 15,534
The locking rates of the benchmark on a Power9 system were as follows:
Before Patch After Patch
# of Threads wlock rlock wlock rlock
------------ ----- ----- ----- -----
1 11,527 12,378 11,527 12,195
2 6,318 15,116 7,690 15,065
4 3,957 16,812 4,495 16,884
8 3,617 16,496 3,993 16,588
The locking rates of the benchmark on a 2-socket ARM64 system were
as follows:
Before Patch After Patch
# of Threads wlock rlock wlock rlock
------------ ----- ----- ----- -----
1 23,703 23,208 23,923 23,167
2 6,206 11,996 6,756 13,268
4 6,098 12,343 6,928 15,064
8 6,007 13,152 7,117 15,335
For x86, the locking rate dropped somewhat at low contention level with
the patch. At high contention level, however, the performance recovered.
For both Power9 and ARM64, the patch caused no performance drop. In fact,
it improved a little bit after the patch.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.c | 145 +++++++++++++++-----------------------------
kernel/locking/rwsem-xadd.h | 85 +++++++++++++-------------
2 files changed, 89 insertions(+), 141 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index fff231a..18a414e 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -9,6 +9,8 @@
*
* Optimistic spinning by Tim Chen <tim.c.chen@intel.com>
* and Davidlohr Bueso <davidlohr@hp.com>. Based on mutexes.
+ *
+ * Rwsem count bit fields re-definition by Waiman Long <longman@redhat.com>.
*/
#include <linux/rwsem.h>
#include <linux/init.h>
@@ -23,52 +25,20 @@
#include "lock_events.h"
/*
- * Guide to the rw_semaphore's count field for common values.
- * (32-bit case illustrated, similar for 64-bit)
- *
- * 0x0000000X (1) X readers active or attempting lock, no writer waiting
- * X = #active_readers + #readers attempting to lock
- * (X*ACTIVE_BIAS)
- *
- * 0x00000000 rwsem is unlocked, and no one is waiting for the lock or
- * attempting to read lock or write lock.
- *
- * 0xffff000X (1) X readers active or attempting lock, with waiters for lock
- * X = #active readers + # readers attempting lock
- * (X*ACTIVE_BIAS + WAITING_BIAS)
- * (2) 1 writer attempting lock, no waiters for lock
- * X-1 = #active readers + #readers attempting lock
- * ((X-1)*ACTIVE_BIAS + ACTIVE_WRITE_BIAS)
- * (3) 1 writer active, no waiters for lock
- * X-1 = #active readers + #readers attempting lock
- * ((X-1)*ACTIVE_BIAS + ACTIVE_WRITE_BIAS)
- *
- * 0xffff0001 (1) 1 reader active or attempting lock, waiters for lock
- * (WAITING_BIAS + ACTIVE_BIAS)
- * (2) 1 writer active or attempting lock, no waiters for lock
- * (ACTIVE_WRITE_BIAS)
+ * Guide to the rw_semaphore's count field.
*
- * 0xffff0000 (1) There are writers or readers queued but none active
- * or in the process of attempting lock.
- * (WAITING_BIAS)
- * Note: writer can attempt to steal lock for this count by adding
- * ACTIVE_WRITE_BIAS in cmpxchg and checking the old count
+ * When the RWSEM_WRITER_LOCKED bit in count is set, the lock is owned
+ * by a writer.
*
- * 0xfffe0001 (1) 1 writer active, or attempting lock. Waiters on queue.
- * (ACTIVE_WRITE_BIAS + WAITING_BIAS)
- *
- * Note: Readers attempt to lock by adding ACTIVE_BIAS in down_read and checking
- * the count becomes more than 0 for successful lock acquisition,
- * i.e. the case where there are only readers or nobody has lock.
- * (1st and 2nd case above).
- *
- * Writers attempt to lock by adding ACTIVE_WRITE_BIAS in down_write and
- * checking the count becomes ACTIVE_WRITE_BIAS for successful lock
- * acquisition (i.e. nobody else has lock or attempts lock). If
- * unsuccessful, in rwsem_down_write_failed, we'll check to see if there
- * are only waiters but none active (5th case above), and attempt to
- * steal the lock.
+ * The lock is owned by readers when
+ * (1) the RWSEM_WRITER_LOCKED isn't set in count,
+ * (2) some of the reader bits are set in count, and
+ * (3) the owner field has RWSEM_READ_OWNED bit set.
*
+ * Having some reader bits set is not enough to guarantee a readers owned
+ * lock as the readers may be in the process of backing out from the count
+ * and a writer has just released the lock. So another writer may steal
+ * the lock immediately after that.
*/
/*
@@ -114,9 +84,8 @@ enum rwsem_wake_type {
/*
* handle the lock release when processes blocked on it that can now run
- * - if we come here from up_xxxx(), then:
- * - the 'active part' of count (&0x0000ffff) reached 0 (but may have changed)
- * - the 'waiting part' of count (&0xffff0000) is -ve (and will still be so)
+ * - if we come here from up_xxxx(), then the RWSEM_FLAG_WAITERS bit must
+ * have been set.
* - there must be someone on the queue
* - the wait_lock must be held by the caller
* - tasks are marked for wakeup, the caller must later invoke wake_up_q()
@@ -160,22 +129,11 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
* so we can bail out early if a writer stole the lock.
*/
if (wake_type != RWSEM_WAKE_READ_OWNED) {
- adjustment = RWSEM_ACTIVE_READ_BIAS;
- try_reader_grant:
+ adjustment = RWSEM_READER_BIAS;
oldcount = atomic_long_fetch_add(adjustment, &sem->count);
- if (unlikely(oldcount < RWSEM_WAITING_BIAS)) {
- /*
- * If the count is still less than RWSEM_WAITING_BIAS
- * after removing the adjustment, it is assumed that
- * a writer has stolen the lock. We have to undo our
- * reader grant.
- */
- if (atomic_long_add_return(-adjustment, &sem->count) <
- RWSEM_WAITING_BIAS)
- return;
-
- /* Last active locker left. Retry waking readers. */
- goto try_reader_grant;
+ if (unlikely(oldcount & RWSEM_WRITER_MASK)) {
+ atomic_long_sub(adjustment, &sem->count);
+ return;
}
/*
* Set it to reader-owned to give spinners an early
@@ -215,11 +173,11 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
wake_q_add_safe(wake_q, tsk);
}
- adjustment = woken * RWSEM_ACTIVE_READ_BIAS - adjustment;
+ adjustment = woken * RWSEM_READER_BIAS - adjustment;
lockevent_cond_inc(rwsem_wake_reader, woken);
if (list_empty(&sem->wait_list)) {
/* hit end of list above */
- adjustment -= RWSEM_WAITING_BIAS;
+ adjustment -= RWSEM_FLAG_WAITERS;
}
if (adjustment)
@@ -233,22 +191,15 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
*/
static inline bool rwsem_try_write_lock(long count, struct rw_semaphore *sem)
{
- /*
- * Avoid trying to acquire write lock if count isn't RWSEM_WAITING_BIAS.
- */
- if (count != RWSEM_WAITING_BIAS)
+ long new;
+
+ if (RWSEM_COUNT_LOCKED(count))
return false;
- /*
- * Acquire the lock by trying to set it to ACTIVE_WRITE_BIAS. If there
- * are other tasks on the wait list, we need to add on WAITING_BIAS.
- */
- count = list_is_singular(&sem->wait_list) ?
- RWSEM_ACTIVE_WRITE_BIAS :
- RWSEM_ACTIVE_WRITE_BIAS + RWSEM_WAITING_BIAS;
+ new = count + RWSEM_WRITER_LOCKED -
+ (list_is_singular(&sem->wait_list) ? RWSEM_FLAG_WAITERS : 0);
- if (atomic_long_cmpxchg_acquire(&sem->count, RWSEM_WAITING_BIAS, count)
- == RWSEM_WAITING_BIAS) {
+ if (atomic_long_cmpxchg_acquire(&sem->count, count, new) == count) {
rwsem_set_owner(sem);
return true;
}
@@ -265,11 +216,11 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
long old, count = atomic_long_read(&sem->count);
while (true) {
- if (!(count == 0 || count == RWSEM_WAITING_BIAS))
+ if (RWSEM_COUNT_LOCKED(count))
return false;
old = atomic_long_cmpxchg_acquire(&sem->count, count,
- count + RWSEM_ACTIVE_WRITE_BIAS);
+ count + RWSEM_WRITER_LOCKED);
if (old == count) {
rwsem_set_owner(sem);
lockevent_inc(rwsem_opt_wlock);
@@ -428,7 +379,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
static inline struct rw_semaphore __sched *
__rwsem_down_read_failed_common(struct rw_semaphore *sem, int state)
{
- long count, adjustment = -RWSEM_ACTIVE_READ_BIAS;
+ long count, adjustment = -RWSEM_READER_BIAS;
struct rwsem_waiter waiter;
DEFINE_WAKE_Q(wake_q);
@@ -440,16 +391,16 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
/*
* In case the wait queue is empty and the lock isn't owned
* by a writer, this reader can exit the slowpath and return
- * immediately as its RWSEM_ACTIVE_READ_BIAS has already
- * been set in the count.
+ * immediately as its RWSEM_READER_BIAS has already been
+ * set in the count.
*/
- if (atomic_long_read(&sem->count) >= 0) {
+ if (!(atomic_long_read(&sem->count) & RWSEM_WRITER_MASK)) {
raw_spin_unlock_irq(&sem->wait_lock);
rwsem_set_reader_owned(sem);
lockevent_inc(rwsem_rlock_fast);
return sem;
}
- adjustment += RWSEM_WAITING_BIAS;
+ adjustment += RWSEM_FLAG_WAITERS;
}
list_add_tail(&waiter.list, &sem->wait_list);
@@ -462,9 +413,8 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
* If there are no writers and we are first in the queue,
* wake our own waiter to join the existing active readers !
*/
- if (count == RWSEM_WAITING_BIAS ||
- (count > RWSEM_WAITING_BIAS &&
- adjustment != -RWSEM_ACTIVE_READ_BIAS))
+ if (!RWSEM_COUNT_LOCKED(count) ||
+ (!(count & RWSEM_WRITER_MASK) && (adjustment & RWSEM_FLAG_WAITERS)))
__rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
raw_spin_unlock_irq(&sem->wait_lock);
@@ -492,7 +442,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
out_nolock:
list_del(&waiter.list);
if (list_empty(&sem->wait_list))
- atomic_long_add(-RWSEM_WAITING_BIAS, &sem->count);
+ atomic_long_add(-RWSEM_FLAG_WAITERS, &sem->count);
raw_spin_unlock_irq(&sem->wait_lock);
__set_current_state(TASK_RUNNING);
lockevent_inc(rwsem_rlock_fail);
@@ -525,9 +475,6 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
struct rw_semaphore *ret = sem;
DEFINE_WAKE_Q(wake_q);
- /* undo write bias from down_write operation, stop active locking */
- count = atomic_long_sub_return(RWSEM_ACTIVE_WRITE_BIAS, &sem->count);
-
/* do optimistic spinning and steal lock if possible */
if (rwsem_optimistic_spin(sem))
return sem;
@@ -553,10 +500,12 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
/*
* If there were already threads queued before us and there are
- * no active writers, the lock must be read owned; so we try to
- * wake any read locks that were queued ahead of us.
+ * no active writers and some readers, the lock must be read
+ * owned; so we try to any read locks that were queued ahead
+ * of us.
*/
- if (count > RWSEM_WAITING_BIAS) {
+ if (!(count & RWSEM_WRITER_MASK) &&
+ (count & RWSEM_READER_MASK)) {
__rwsem_mark_wake(sem, RWSEM_WAKE_READERS, &wake_q);
/*
* The wakeup is normally called _after_ the wait_lock
@@ -573,8 +522,9 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
wake_q_init(&wake_q);
}
- } else
- count = atomic_long_add_return(RWSEM_WAITING_BIAS, &sem->count);
+ } else {
+ count = atomic_long_add_return(RWSEM_FLAG_WAITERS, &sem->count);
+ }
/* wait until we successfully acquire the lock */
set_current_state(state);
@@ -591,7 +541,8 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
schedule();
lockevent_inc(rwsem_sleep_writer);
set_current_state(state);
- } while ((count = atomic_long_read(&sem->count)) & RWSEM_ACTIVE_MASK);
+ count = atomic_long_read(&sem->count);
+ } while (RWSEM_COUNT_LOCKED(count));
raw_spin_lock_irq(&sem->wait_lock);
}
@@ -607,7 +558,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
raw_spin_lock_irq(&sem->wait_lock);
list_del(&waiter.list);
if (list_empty(&sem->wait_list))
- atomic_long_add(-RWSEM_WAITING_BIAS, &sem->count);
+ atomic_long_add(-RWSEM_FLAG_WAITERS, &sem->count);
else
__rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
raw_spin_unlock_irq(&sem->wait_lock);
diff --git a/kernel/locking/rwsem-xadd.h b/kernel/locking/rwsem-xadd.h
index 6d2478c..1febd17 100644
--- a/kernel/locking/rwsem-xadd.h
+++ b/kernel/locking/rwsem-xadd.h
@@ -37,24 +37,26 @@
#endif
/*
- * R/W semaphores originally for PPC using the stuff in lib/rwsem.c.
- * Adapted largely from include/asm-i386/rwsem.h
- * by Paul Mackerras <paulus@samba.org>.
- */
-
-/*
- * the semaphore definition
+ * The definition of the atomic counter in the semaphore:
+ *
+ * Bit 0 - writer locked bit
+ * Bit 1 - waiters present bit
+ * Bits 2-7 - reserved
+ * Bits 8-X - 24-bit (32-bit) or 56-bit reader count
+ *
+ * atomic_long_fetch_add() is used to obtain reader lock, whereas
+ * atomic_long_cmpxchg() will be used to obtain writer lock.
*/
-#ifdef CONFIG_64BIT
-# define RWSEM_ACTIVE_MASK 0xffffffffL
-#else
-# define RWSEM_ACTIVE_MASK 0x0000ffffL
-#endif
+#define RWSEM_WRITER_LOCKED (1UL << 0)
+#define RWSEM_FLAG_WAITERS (1UL << 1)
+#define RWSEM_READER_SHIFT 8
+#define RWSEM_READER_BIAS (1UL << RWSEM_READER_SHIFT)
+#define RWSEM_READER_MASK (~(RWSEM_READER_BIAS - 1))
+#define RWSEM_WRITER_MASK RWSEM_WRITER_LOCKED
+#define RWSEM_LOCK_MASK (RWSEM_WRITER_MASK|RWSEM_READER_MASK)
+#define RWSEM_READ_FAILED_MASK (RWSEM_WRITER_MASK|RWSEM_FLAG_WAITERS)
-#define RWSEM_ACTIVE_BIAS 0x00000001L
-#define RWSEM_WAITING_BIAS (-RWSEM_ACTIVE_MASK-1)
-#define RWSEM_ACTIVE_READ_BIAS RWSEM_ACTIVE_BIAS
-#define RWSEM_ACTIVE_WRITE_BIAS (RWSEM_WAITING_BIAS + RWSEM_ACTIVE_BIAS)
+#define RWSEM_COUNT_LOCKED(c) ((c) & RWSEM_LOCK_MASK)
#ifdef CONFIG_RWSEM_SPIN_ON_OWNER
/*
@@ -169,7 +171,8 @@ static inline void rwsem_clear_reader_owned(struct rw_semaphore *sem)
*/
static inline void __down_read(struct rw_semaphore *sem)
{
- if (unlikely(atomic_long_inc_return_acquire(&sem->count) <= 0)) {
+ if (unlikely(atomic_long_fetch_add_acquire(RWSEM_READER_BIAS,
+ &sem->count) & RWSEM_READ_FAILED_MASK)) {
rwsem_down_read_failed(sem);
DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner &
RWSEM_READER_OWNED), sem);
@@ -180,7 +183,8 @@ static inline void __down_read(struct rw_semaphore *sem)
static inline int __down_read_killable(struct rw_semaphore *sem)
{
- if (unlikely(atomic_long_inc_return_acquire(&sem->count) <= 0)) {
+ if (unlikely(atomic_long_fetch_add_acquire(RWSEM_READER_BIAS,
+ &sem->count) & RWSEM_READ_FAILED_MASK)) {
if (IS_ERR(rwsem_down_read_failed_killable(sem)))
return -EINTR;
DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner &
@@ -195,9 +199,10 @@ static inline int __down_read_trylock(struct rw_semaphore *sem)
{
long tmp;
- while ((tmp = atomic_long_read(&sem->count)) >= 0) {
+ while (!((tmp = atomic_long_read(&sem->count)) &
+ RWSEM_READ_FAILED_MASK)) {
if (tmp == atomic_long_cmpxchg_acquire(&sem->count, tmp,
- tmp + RWSEM_ACTIVE_READ_BIAS)) {
+ tmp + RWSEM_READER_BIAS)) {
rwsem_set_reader_owned(sem);
return 1;
}
@@ -210,22 +215,16 @@ static inline int __down_read_trylock(struct rw_semaphore *sem)
*/
static inline void __down_write(struct rw_semaphore *sem)
{
- long tmp;
-
- tmp = atomic_long_add_return_acquire(RWSEM_ACTIVE_WRITE_BIAS,
- &sem->count);
- if (unlikely(tmp != RWSEM_ACTIVE_WRITE_BIAS))
+ if (unlikely(atomic_long_cmpxchg_acquire(&sem->count, 0,
+ RWSEM_WRITER_LOCKED)))
rwsem_down_write_failed(sem);
rwsem_set_owner(sem);
}
static inline int __down_write_killable(struct rw_semaphore *sem)
{
- long tmp;
-
- tmp = atomic_long_add_return_acquire(RWSEM_ACTIVE_WRITE_BIAS,
- &sem->count);
- if (unlikely(tmp != RWSEM_ACTIVE_WRITE_BIAS))
+ if (unlikely(atomic_long_cmpxchg_acquire(&sem->count, 0,
+ RWSEM_WRITER_LOCKED)))
if (IS_ERR(rwsem_down_write_failed_killable(sem)))
return -EINTR;
rwsem_set_owner(sem);
@@ -234,15 +233,11 @@ static inline int __down_write_killable(struct rw_semaphore *sem)
static inline int __down_write_trylock(struct rw_semaphore *sem)
{
- long tmp;
-
- tmp = atomic_long_cmpxchg_acquire(&sem->count, RWSEM_UNLOCKED_VALUE,
- RWSEM_ACTIVE_WRITE_BIAS);
- if (tmp == RWSEM_UNLOCKED_VALUE) {
+ bool taken = !atomic_long_cmpxchg_acquire(&sem->count, 0,
+ RWSEM_WRITER_LOCKED);
+ if (taken)
rwsem_set_owner(sem);
- return true;
- }
- return false;
+ return taken;
}
/*
@@ -255,8 +250,9 @@ static inline void __up_read(struct rw_semaphore *sem)
DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner & RWSEM_READER_OWNED),
sem);
rwsem_clear_reader_owned(sem);
- tmp = atomic_long_dec_return_release(&sem->count);
- if (unlikely(tmp < -1 && (tmp & RWSEM_ACTIVE_MASK) == 0))
+ tmp = atomic_long_add_return_release(-RWSEM_READER_BIAS, &sem->count);
+ if (unlikely((tmp & (RWSEM_LOCK_MASK|RWSEM_FLAG_WAITERS))
+ == RWSEM_FLAG_WAITERS))
rwsem_wake(sem);
}
@@ -267,8 +263,8 @@ static inline void __up_write(struct rw_semaphore *sem)
{
DEBUG_RWSEMS_WARN_ON(sem->owner != current, sem);
rwsem_clear_owner(sem);
- if (unlikely(atomic_long_sub_return_release(RWSEM_ACTIVE_WRITE_BIAS,
- &sem->count) < 0))
+ if (unlikely(atomic_long_fetch_add_release(-RWSEM_WRITER_LOCKED,
+ &sem->count) & RWSEM_FLAG_WAITERS))
rwsem_wake(sem);
}
@@ -287,8 +283,9 @@ static inline void __downgrade_write(struct rw_semaphore *sem)
* write side. As such, rely on RELEASE semantics.
*/
DEBUG_RWSEMS_WARN_ON(sem->owner != current, sem);
- tmp = atomic_long_add_return_release(-RWSEM_WAITING_BIAS, &sem->count);
+ tmp = atomic_long_fetch_add_release(
+ -RWSEM_WRITER_LOCKED+RWSEM_READER_BIAS, &sem->count);
rwsem_set_reader_owned(sem);
- if (tmp < 0)
+ if (tmp & RWSEM_FLAG_WAITERS)
rwsem_downgrade_wake(sem);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 10/22] locking/rwsem: Enable lock event counting
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
Add lock event counting calls so that we can track the number of lock
events happening in the rwsem code.
With CONFIG_LOCK_EVENT_COUNTS on and booting a 1-socket 22-core
44-thread x86-64 system, the non-zero rwsem counts after system bootup
were as follows:
rwsem_opt_fail=113
rwsem_opt_wlock=13647
rwsem_rlock=176
rwsem_rlock_fast=10
rwsem_wake_reader=153
rwsem_wake_writer=139
rwsem_wlock=113
It can be seen that most of the lock acquisitions in the slowpath were
writer-locks in the optimistic spinning code path with no sleeping at
all. Only about 4% of locks were acquired after sleeping.
Signed-off-by: Waiman Long <longman@redhat.com>
---
arch/Kconfig | 2 +-
kernel/locking/lock_events_list.h | 17 +++++++++++++++++
kernel/locking/rwsem-xadd.c | 12 ++++++++++++
3 files changed, 30 insertions(+), 1 deletion(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index af147c2..7471791 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -891,7 +891,7 @@ config ARCH_USE_MEMREMAP_PROT
config LOCK_EVENT_COUNTS
bool "Locking event counts collection"
depends on DEBUG_FS
- depends on QUEUED_SPINLOCKS
+ depends on (QUEUED_SPINLOCKS || RWSEM_XCHGADD_ALGORITHM)
---help---
Enable light-weight counting of various locking related events
in the system with minimal performance impact. This reduces
diff --git a/kernel/locking/lock_events_list.h b/kernel/locking/lock_events_list.h
index 8b4d2e1..c33c5df 100644
--- a/kernel/locking/lock_events_list.h
+++ b/kernel/locking/lock_events_list.h
@@ -48,3 +48,20 @@
LOCK_EVENT(lock_use_node4) /* # of locking ops that use 4th percpu node */
LOCK_EVENT(lock_no_node) /* # of locking ops w/o using percpu node */
#endif /* CONFIG_QUEUED_SPINLOCKS */
+
+#ifdef CONFIG_RWSEM_XCHGADD_ALGORITHM
+/*
+ * Locking events for rwsem
+ */
+LOCK_EVENT(rwsem_sleep_reader) /* # of reader sleeps */
+LOCK_EVENT(rwsem_sleep_writer) /* # of writer sleeps */
+LOCK_EVENT(rwsem_wake_reader) /* # of reader wakeups */
+LOCK_EVENT(rwsem_wake_writer) /* # of writer wakeups */
+LOCK_EVENT(rwsem_opt_wlock) /* # of write locks opt-spin acquired */
+LOCK_EVENT(rwsem_opt_fail) /* # of failed opt-spinnings */
+LOCK_EVENT(rwsem_rlock) /* # of read locks acquired */
+LOCK_EVENT(rwsem_rlock_fast) /* # of fast read locks acquired */
+LOCK_EVENT(rwsem_rlock_fail) /* # of failed read lock acquisitions */
+LOCK_EVENT(rwsem_wlock) /* # of write locks acquired */
+LOCK_EVENT(rwsem_wlock_fail) /* # of failed write lock acquisitions */
+#endif /* CONFIG_RWSEM_XCHGADD_ALGORITHM */
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 62422a6..fff231a 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -20,6 +20,7 @@
#include <linux/osq_lock.h>
#include "rwsem-xadd.h"
+#include "lock_events.h"
/*
* Guide to the rw_semaphore's count field for common values.
@@ -147,6 +148,7 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
* will notice the queued writer.
*/
wake_q_add(wake_q, waiter->task);
+ lockevent_inc(rwsem_wake_writer);
}
return;
@@ -214,6 +216,7 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
}
adjustment = woken * RWSEM_ACTIVE_READ_BIAS - adjustment;
+ lockevent_cond_inc(rwsem_wake_reader, woken);
if (list_empty(&sem->wait_list)) {
/* hit end of list above */
adjustment -= RWSEM_WAITING_BIAS;
@@ -269,6 +272,7 @@ static inline bool rwsem_try_write_lock_unqueued(struct rw_semaphore *sem)
count + RWSEM_ACTIVE_WRITE_BIAS);
if (old == count) {
rwsem_set_owner(sem);
+ lockevent_inc(rwsem_opt_wlock);
return true;
}
@@ -394,6 +398,7 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem)
osq_unlock(&sem->osq);
done:
preempt_enable();
+ lockevent_cond_inc(rwsem_opt_fail, !taken);
return taken;
}
@@ -441,6 +446,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
if (atomic_long_read(&sem->count) >= 0) {
raw_spin_unlock_irq(&sem->wait_lock);
rwsem_set_reader_owned(sem);
+ lockevent_inc(rwsem_rlock_fast);
return sem;
}
adjustment += RWSEM_WAITING_BIAS;
@@ -477,9 +483,11 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
break;
}
schedule();
+ lockevent_inc(rwsem_sleep_reader);
}
__set_current_state(TASK_RUNNING);
+ lockevent_inc(rwsem_rlock);
return sem;
out_nolock:
list_del(&waiter.list);
@@ -487,6 +495,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
atomic_long_add(-RWSEM_WAITING_BIAS, &sem->count);
raw_spin_unlock_irq(&sem->wait_lock);
__set_current_state(TASK_RUNNING);
+ lockevent_inc(rwsem_rlock_fail);
return ERR_PTR(-EINTR);
}
@@ -580,6 +589,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
goto out_nolock;
schedule();
+ lockevent_inc(rwsem_sleep_writer);
set_current_state(state);
} while ((count = atomic_long_read(&sem->count)) & RWSEM_ACTIVE_MASK);
@@ -588,6 +598,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
__set_current_state(TASK_RUNNING);
list_del(&waiter.list);
raw_spin_unlock_irq(&sem->wait_lock);
+ lockevent_inc(rwsem_wlock);
return ret;
@@ -601,6 +612,7 @@ static inline bool rwsem_has_spinner(struct rw_semaphore *sem)
__rwsem_mark_wake(sem, RWSEM_WAKE_ANY, &wake_q);
raw_spin_unlock_irq(&sem->wait_lock);
wake_up_q(&wake_q);
+ lockevent_inc(rwsem_wlock_fail);
return ERR_PTR(-EINTR);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH-tip 09/22] locking/rwsem: Enhance DEBUG_RWSEMS_WARN_ON() macro
From: Waiman Long @ 2019-02-07 19:07 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Will Deacon, Thomas Gleixner
Cc: linux-arch, linux-xtensa, Davidlohr Bueso, linux-ia64, Tim Chen,
Arnd Bergmann, linux-sh, linux-hexagon, x86, H. Peter Anvin,
linux-kernel, Linus Torvalds, Borislav Petkov, linux-alpha,
sparclinux, Waiman Long, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1549566446-27967-1-git-send-email-longman@redhat.com>
Currently, the DEBUG_RWSEMS_WARN_ON() macro just dumps a stack trace
when the rwsem isn't in the right state. It does not show the actual
states of the rwsem. This may not be that helpful in the debugging
process.
Enhance the DEBUG_RWSEMS_WARN_ON() macro to also show the current
content of the rwsem count and owner fields to give more information
about what is wrong with the rwsem.
Signed-off-by: Waiman Long <longman@redhat.com>
---
kernel/locking/rwsem-xadd.h | 19 ++++++++++++-------
kernel/locking/rwsem.c | 5 +++--
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/kernel/locking/rwsem-xadd.h b/kernel/locking/rwsem-xadd.h
index 77151c3..6d2478c 100644
--- a/kernel/locking/rwsem-xadd.h
+++ b/kernel/locking/rwsem-xadd.h
@@ -27,9 +27,13 @@
#define RWSEM_ANONYMOUSLY_OWNED (1UL << 1)
#ifdef CONFIG_DEBUG_RWSEMS
-# define DEBUG_RWSEMS_WARN_ON(c) DEBUG_LOCKS_WARN_ON(c)
+# define DEBUG_RWSEMS_WARN_ON(c, sem) \
+ WARN_ONCE(c, "DEBUG_RWSEMS_WARN_ON(%s): count = 0x%lx, owner = 0x%lx, curr 0x%lx, list %sempty\n",\
+ #c, atomic_long_read(&(sem)->count), \
+ (long)((sem)->owner), (long)current, \
+ list_empty(&(sem)->wait_list) ? "" : "not ")
#else
-# define DEBUG_RWSEMS_WARN_ON(c)
+# define DEBUG_RWSEMS_WARN_ON(c, sem)
#endif
/*
@@ -168,7 +172,7 @@ static inline void __down_read(struct rw_semaphore *sem)
if (unlikely(atomic_long_inc_return_acquire(&sem->count) <= 0)) {
rwsem_down_read_failed(sem);
DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner &
- RWSEM_READER_OWNED));
+ RWSEM_READER_OWNED), sem);
} else {
rwsem_set_reader_owned(sem);
}
@@ -180,7 +184,7 @@ static inline int __down_read_killable(struct rw_semaphore *sem)
if (IS_ERR(rwsem_down_read_failed_killable(sem)))
return -EINTR;
DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner &
- RWSEM_READER_OWNED));
+ RWSEM_READER_OWNED), sem);
} else {
rwsem_set_reader_owned(sem);
}
@@ -248,7 +252,8 @@ static inline void __up_read(struct rw_semaphore *sem)
{
long tmp;
- DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner & RWSEM_READER_OWNED));
+ DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner & RWSEM_READER_OWNED),
+ sem);
rwsem_clear_reader_owned(sem);
tmp = atomic_long_dec_return_release(&sem->count);
if (unlikely(tmp < -1 && (tmp & RWSEM_ACTIVE_MASK) == 0))
@@ -260,7 +265,7 @@ static inline void __up_read(struct rw_semaphore *sem)
*/
static inline void __up_write(struct rw_semaphore *sem)
{
- DEBUG_RWSEMS_WARN_ON(sem->owner != current);
+ DEBUG_RWSEMS_WARN_ON(sem->owner != current, sem);
rwsem_clear_owner(sem);
if (unlikely(atomic_long_sub_return_release(RWSEM_ACTIVE_WRITE_BIAS,
&sem->count) < 0))
@@ -281,7 +286,7 @@ static inline void __downgrade_write(struct rw_semaphore *sem)
* read-locked region is ok to be re-ordered into the
* write side. As such, rely on RELEASE semantics.
*/
- DEBUG_RWSEMS_WARN_ON(sem->owner != current);
+ DEBUG_RWSEMS_WARN_ON(sem->owner != current, sem);
tmp = atomic_long_add_return_release(-RWSEM_WAITING_BIAS, &sem->count);
rwsem_set_reader_owned(sem);
if (tmp < 0)
diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 598fc7c..bdfca7c 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -17,7 +17,7 @@
#include "rwsem-xadd.h"
#else
#define __rwsem_set_reader_owned(a, b)
-#define DEBUG_RWSEMS_WARN_ON(a)
+#define DEBUG_RWSEMS_WARN_ON(a, b)
#endif
/*
@@ -203,7 +203,8 @@ int __sched down_write_killable_nested(struct rw_semaphore *sem, int subclass)
void up_read_non_owner(struct rw_semaphore *sem)
{
- DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner & RWSEM_READER_OWNED));
+ DEBUG_RWSEMS_WARN_ON(!((unsigned long)sem->owner & RWSEM_READER_OWNED),
+ sem);
__up_read(sem);
}
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox