Re: [PATCH v5 00/18] locking/rwsem: Rwsem rearchitecture part 2

[Waiman Long]

On 04/19/2019 11:00 AM, Waiman Long wrote:
> On 04/19/2019 08:49 AM, Ingo Molnar wrote:
>> * Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>>
>>> * Waiman Long <longman@xxxxxxxxxx> wrote:
>>>
>>>> On 04/18/2019 07:46 PM, Waiman Long wrote:
>>>>>  v5:
>>>>>   - Drop v4 patch 1 as it is merged into tip's locking/core branch.
>>>>>   - Integrate the 2 followup patches into the series. The first
>>>>>     follow-up patch is broken into 2 pieces. The first piece comes in
>>>>>     before the "Enable readers spinning on writer" and the 2nd piece
>>>>>     is merged into the "Enable time-based spinning on reader-owned
>>>>>     rwsem" patch. The 2nd followup patch is added after that.
>>>>>   - Add a new patch to make all wake_up_q() calls after dropping
>>>>>     wait_lock as suggested by PeterZ.
>>>>>   - Incorporate numerouos suggestions by PeterZ and Davidlohr.
>>>> This patchset is still being reviewed by Peter . The purpose of this
>>>> series is mainly to sync up the version that Peter has and the ones that
>>>> I am working on incorporating his feedback. Further changes may still be
>>>> needed.
>>>>
>>>> I run an overall performance test on this new patchset and present the
>>>> data in this cover letter. However, I haven't run performance tests for
>>>> individual patches. So the performance data listed in some of the
>>>> patches may be stale.
>>> Just for those who'd like to follow the scope of changes, find below the 
>>> v4->v5 interdiff. v5 is now included in tip:WIP.locking/core, and also 
>>> merged into tip:master. (But not propagated towards linux-next yet.)
>> Hm, I'm experiencing early boot hangs with v5, on defconfig-ish x86-64 
>> kernels:
>>
>> [    0.153940] rcu: Hierarchical RCU implementation.
>> [    0.154289] rcu: 	RCU restricting CPUs from NR_CPUS=128 to nr_cpu_ids=17.
>> [    0.154829] rcu: RCU calculated value of scheduler-enlistment delay is 100 jiffies.
>> [    0.155390] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=17
>>
>>
>> I bisected it back to the v5 version of this patch:
>>
>>   2fd5f60fa4c3: locking/rwsem: Merge owner into count on x86-64
>>
>> I'm moving -tip back to -v4 meanwhile.
>>
>> Thanks,
>>
>> 	Ingo
> Sorry about that. Will look into that problem.
>
> -Longman
>
Yes, there is a bug in that patch. The following change should fix it:

diff --git a/kernel/locking/rwsem.c b/kernel/locking/rwsem.c
index 19d8fbd50d17..857dff330f9b 100644
--- a/kernel/locking/rwsem.c
+++ b/kernel/locking/rwsem.c
@@ -198,9 +198,15 @@
Â/*
 * Task structure pointer compression (64-bit only):
 * (owner - PAGE_OFFSET) >> (L1_CACHE_SHIFT - 2)
+ *
+ * However, init_task may lie outside of the linearly mapped physical
+ * to virtual memory range and so has to be handled separately.
 */
Âstatic inline unsigned long rwsem_owner_count(struct task_struct *owner)
Â{
+ÂÂÂÂÂÂ if (unlikely(owner == &init_task))
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return RWSEM_WRITER_MASK;
+
ÂÂÂÂÂÂÂ return ((unsigned long)owner - PAGE_OFFSET) >> (L1_CACHE_SHIFT - 2);
Â}

@@ -208,6 +214,9 @@ static inline unsigned long rwsem_count_owner(long
count)
Â{
ÂÂÂÂÂÂÂ unsigned long writer = (unsigned long)count & RWSEM_WRITER_MASK;

+ÂÂÂÂÂÂ if (unlikely(writer == RWSEM_WRITER_MASK))
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ return (unsigned long)&init_task;
+
ÂÂÂÂÂÂÂ return writer ? (writer << (L1_CACHE_SHIFT - 2)) + PAGE_OFFSET : 0;
Â}