Re: [PATCH V2 2/7] rcu: cleanup rcu_preempt_deferred_qs()

[Lai Jiangshan <laijs@linux.alibaba.com>]

On 2019/11/11 10:32 下午, Paul E. McKenney wrote:
> On Mon, Nov 04, 2019 at 11:19:11PM -0800, Paul E. McKenney wrote:
>> On Tue, Nov 05, 2019 at 10:09:15AM +0800, Lai Jiangshan wrote:
>>> On 2019/11/4 10:55 下午, Paul E. McKenney wrote:
>>>> On Sun, Nov 03, 2019 at 01:01:21PM +0800, Lai Jiangshan wrote:
>>>>>
>>>>>
>>>>> On 2019/11/3 10:01 上午, Boqun Feng wrote:
>>>>>> Hi Jiangshan,
>>>>>>
>>>>>>
>>>>>> I haven't checked the correctness of this patch carefully, but..
>>>>>>
>>>>>>
>>>>>> On Sat, Nov 02, 2019 at 12:45:54PM +0000, Lai Jiangshan wrote:
>>>>>>> Don't need to set ->rcu_read_lock_nesting negative, irq-protected
>>>>>>> rcu_preempt_deferred_qs_irqrestore() doesn't expect
>>>>>>> ->rcu_read_lock_nesting to be negative to work, it even
>>>>>>> doesn't access to ->rcu_read_lock_nesting any more.
>>>>>>
>>>>>> rcu_preempt_deferred_qs_irqrestore() will report RCU qs, and may
>>>>>> eventually call swake_up() or its friends to wake up, say, the gp
>>>>>> kthread, and the wake up functions could go into the scheduler code
>>>>>> path which might have RCU read-side critical section in it, IOW,
>>>>>> accessing ->rcu_read_lock_nesting.
>>>>>
>>>>> Sure, thank you for pointing it out.
>>>>>
>>>>> I should rewrite the changelog in next round. Like this:
>>>>>
>>>>> rcu: cleanup rcu_preempt_deferred_qs()
>>>>>
>>>>> IRQ-protected rcu_preempt_deferred_qs_irqrestore() itself doesn't
>>>>> expect ->rcu_read_lock_nesting to be negative to work.
>>>>>
>>>>> There might be RCU read-side critical section in it (from wakeup()
>>>>> or so), 1711d15bf5ef(rcu: Clear ->rcu_read_unlock_special only once)
>>>>> will ensure that ->rcu_read_unlock_special is zero and these RCU
>>>>> read-side critical sections will not call rcu_read_unlock_special().
>>>>>
>>>>> Thanks
>>>>> Lai
>>>>>
>>>>> ===
>>>>> PS: Were 1711d15bf5ef(rcu: Clear ->rcu_read_unlock_special only once)
>>>>> not applied earlier, it will be protected by previous patch (patch1)
>>>>> in this series
>>>>> "rcu: use preempt_count to test whether scheduler locks is held"
>>>>> when rcu_read_unlock_special() is called.
>>>>
>>>> This one in -rcu, you mean?
>>>>
>>>> 5c5d9065e4eb ("rcu: Clear ->rcu_read_unlock_special only once")
>>>
>>> Yes, but the commit ID is floating in the tree.
>>
>> Indeed, that part of -rcu is subject to rebase, and will continue
>> to be until about v5.5-rc5 or thereabouts.
>>
>> https://mirrors.edge.kernel.org/pub/linux/kernel/people/paulmck/rcutodo.html
>>
>> My testing of your full stack should be complete by this coming Sunday
>> morning, Pacific Time.
> 
> And you will be happy to hear that it ran the full time without errors.
> 
> Good show!!!
> 
> My next step is to look much more carefully at the remaining patches,
> checking my first impressions.  This will take a few days.
> 

Hi, All

I'm still asking for more comments.

By now, I have received some precious comments, mainly due to my
stupid naming mistakes and a misleading changelog. I should have
updated all these with a new series patches. But I hope I
can polish more in the new patchset with more suggestions from
valuable comments, especially in x86,scheduler,percpu and rcu
areas.

I'm very obliged to hear anything.

Thanks
Lai