Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ccaf5e8e-3457-a2cf-b6eb-794cbf1b46f5@bytedance.com>
Date:   Tue, 11 Apr 2023 22:08:01 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.9.1
Subject: Re: [PATCH] mm: slub: annotate kmem_cache_node->list_lock as
 raw_spinlock
Content-Language: en-US
To:     Vlastimil Babka <vbabka@suse.cz>, 42.hyeyoo@gmail.com,
        akpm@linux-foundation.org, roman.gushchin@linux.dev,
        iamjoonsoo.kim@lge.com, rientjes@google.com, penberg@kernel.org,
        cl@linux.com
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Zhao Gongyi <zhaogongyi@bytedance.com>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        RCU <rcu@vger.kernel.org>,
        "Paul E . McKenney" <paulmck@kernel.org>
References: <20230411130854.46795-1-zhengqi.arch@bytedance.com>
 <c6ea3b17-a89c-6f66-5c86-967f1da601b4@suse.cz>
From:   Qi Zheng <zhengqi.arch@bytedance.com>
In-Reply-To: <c6ea3b17-a89c-6f66-5c86-967f1da601b4@suse.cz>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2023/4/11 21:40, Vlastimil Babka wrote:
> On 4/11/23 15:08, Qi Zheng wrote:
>> The list_lock can be held in the critical section of
>> raw_spinlock, and then lockdep will complain about it
>> like below:
>>
>>   =============================
>>   [ BUG: Invalid wait context ]
>>   6.3.0-rc6-next-20230411 #7 Not tainted
>>   -----------------------------
>>   swapper/0/1 is trying to lock:
>>   ffff888100055418 (&n->list_lock){....}-{3:3}, at: ___slab_alloc+0x73d/0x1330
>>   other info that might help us debug this:
>>   context-{5:5}
>>   2 locks held by swapper/0/1:
>>    #0: ffffffff824e8160 (rcu_tasks.cbs_gbl_lock){....}-{2:2}, at: cblist_init_generic+0x22/0x2d0
>>    #1: ffff888136bede50 (&ACCESS_PRIVATE(rtpcp, lock)){....}-{2:2}, at: cblist_init_generic+0x232/0x2d0
>>   stack backtrace:
>>   CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.3.0-rc6-next-20230411 #7
>>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014
>>   Call Trace:
>>    <TASK>
>>    dump_stack_lvl+0x77/0xc0
>>    __lock_acquire+0xa65/0x2950
>>    ? arch_stack_walk+0x65/0xf0
>>    ? arch_stack_walk+0x65/0xf0
>>    ? unwind_next_frame+0x602/0x8d0
>>    lock_acquire+0xe0/0x300
>>    ? ___slab_alloc+0x73d/0x1330
>>    ? find_usage_forwards+0x39/0x50
>>    ? check_irq_usage+0x162/0xa70
>>    ? __bfs+0x10c/0x2c0
>>    _raw_spin_lock_irqsave+0x4f/0x90
>>    ? ___slab_alloc+0x73d/0x1330
>>    ___slab_alloc+0x73d/0x1330
>>    ? fill_pool+0x16b/0x2a0
>>    ? look_up_lock_class+0x5d/0x160
>>    ? register_lock_class+0x48/0x500
>>    ? __lock_acquire+0xabc/0x2950
>>    ? fill_pool+0x16b/0x2a0
>>    kmem_cache_alloc+0x358/0x3b0
>>    ? __lock_acquire+0xabc/0x2950
>>    fill_pool+0x16b/0x2a0
>>    ? __debug_object_init+0x292/0x560
>>    ? lock_acquire+0xe0/0x300
>>    ? cblist_init_generic+0x232/0x2d0
>>    __debug_object_init+0x2c/0x560
>>    cblist_init_generic+0x147/0x2d0
>>    rcu_init_tasks_generic+0x15/0x190
>>    kernel_init_freeable+0x6e/0x3e0
>>    ? rest_init+0x1e0/0x1e0
>>    kernel_init+0x1b/0x1d0
>>    ? rest_init+0x1e0/0x1e0
>>    ret_from_fork+0x1f/0x30
>>    </TASK>
>>
>> The fill_pool() can only be called in the !PREEMPT_RT kernel
>> or in the preemptible context of the PREEMPT_RT kernel, so
>> the above warning is not a real issue, but it's better to
>> annotate kmem_cache_node->list_lock as raw_spinlock to get
>> rid of such issue.
> 
> + CC some RT and RCU people

Thanks.

> 
> AFAIK raw_spinlock is not just an annotation, but on RT it changes the
> implementation from preemptible mutex to actual spin lock, so it would be

Yeah.

> rather unfortunate to do that for a spurious warning. Can it be somehow
> fixed in a better way?

It's indeed unfortunate for the warning in the commit message. But
functions like kmem_cache_alloc(GFP_ATOMIC) may indeed be called
in the critical section of raw_spinlock or in the hardirq context, which
will cause problem in the PREEMPT_RT kernel. So I still think it is
reasonable to convert kmem_cache_node->list_lock to raw_spinlock type.

In addition, there are many fix patches for this kind of warning in the
git log, so I also think there should be a general and better solution. :)

> 

-- 
Thanks,
Qi