From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id C9F94C433EF for ; Thu, 26 May 2022 20:17:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1343906AbiEZURh (ORCPT ); Thu, 26 May 2022 16:17:37 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60272 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232295AbiEZURe (ORCPT ); Thu, 26 May 2022 16:17:34 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 08B04C964A for ; Thu, 26 May 2022 13:17:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1653596252; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KcZz6yVLgZ62yrFoGOM5DElgQzD6OwyWHQ72tERIC7w=; b=UVRNItfurIuaE/1CqJJ31GnavcHM+FXhc6x9K9qjlj+geHOoRxldxNr/U84hr+NyyJG4UA RYeuGSzBNLLN02Yp6/6Up1P6Trz98DxdBMGjkPXRrnsTQcJ/jwOl4GKGEgeUVean6vlJd7 9e7LlPmxq1jVpOyfRSouGlUo57ldeeE= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-511-dYebSbIfM4yLfnBJjQQ3eg-1; Thu, 26 May 2022 16:17:28 -0400 X-MC-Unique: dYebSbIfM4yLfnBJjQQ3eg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B89FF185A7A4; Thu, 26 May 2022 20:17:27 +0000 (UTC) Received: from [10.22.8.143] (unknown [10.22.8.143]) by smtp.corp.redhat.com (Postfix) with ESMTP id 389B12026D64; Thu, 26 May 2022 20:17:27 +0000 (UTC) Message-ID: <9fe57cf7-9d21-3f91-ef27-e046b426c219@redhat.com> Date: Thu, 26 May 2022 16:17:27 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH v4 03/11] mm: memcontrol: make lruvec lock safe when LRU pages are reparented Content-Language: en-US To: Muchun Song , Johannes Weiner Cc: mhocko@kernel.org, roman.gushchin@linux.dev, shakeelb@google.com, cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, duanxiongchun@bytedance.com References: <20220524060551.80037-1-songmuchun@bytedance.com> <20220524060551.80037-4-songmuchun@bytedance.com> From: Waiman Long In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 5/25/22 11:38, Muchun Song wrote: > On Wed, May 25, 2022 at 10:48:54AM -0400, Johannes Weiner wrote: >> On Wed, May 25, 2022 at 09:03:59PM +0800, Muchun Song wrote: >>> On Wed, May 25, 2022 at 08:30:15AM -0400, Johannes Weiner wrote: >>>> On Wed, May 25, 2022 at 05:53:30PM +0800, Muchun Song wrote: >>>>> On Tue, May 24, 2022 at 03:27:20PM -0400, Johannes Weiner wrote: >>>>>> On Tue, May 24, 2022 at 02:05:43PM +0800, Muchun Song wrote: >>>>>>> @@ -1230,10 +1213,23 @@ void lruvec_memcg_debug(struct lruvec *lruvec, struct folio *folio) >>>>>>> */ >>>>>>> struct lruvec *folio_lruvec_lock(struct folio *folio) >>>>>>> { >>>>>>> - struct lruvec *lruvec = folio_lruvec(folio); >>>>>>> + struct lruvec *lruvec; >>>>>>> >>>>>>> + rcu_read_lock(); >>>>>>> +retry: >>>>>>> + lruvec = folio_lruvec(folio); >>>>>>> spin_lock(&lruvec->lru_lock); >>>>>>> - lruvec_memcg_debug(lruvec, folio); >>>>>>> + >>>>>>> + if (unlikely(lruvec_memcg(lruvec) != folio_memcg(folio))) { >>>>>>> + spin_unlock(&lruvec->lru_lock); >>>>>>> + goto retry; >>>>>>> + } >>>>>>> + >>>>>>> + /* >>>>>>> + * Preemption is disabled in the internal of spin_lock, which can serve >>>>>>> + * as RCU read-side critical sections. >>>>>>> + */ >>>>>>> + rcu_read_unlock(); >>>>>> The code looks right to me, but I don't understand the comment: why do >>>>>> we care that the rcu read-side continues? With the lru_lock held, >>>>>> reparenting is on hold and the lruvec cannot be rcu-freed anyway, no? >>>>>> >>>>> Right. We could hold rcu read lock until end of reparting. So you mean >>>>> we do rcu_read_unlock in folio_lruvec_lock()? >>>> The comment seems to suggest that disabling preemption is what keeps >>>> the lruvec alive. But it's the lru_lock that keeps it alive. The >>>> cgroup destruction path tries to take the lru_lock long before it even >>>> gets to synchronize_rcu(). Once you hold the lru_lock, having an >>>> implied read-side critical section as well doesn't seem to matter. >>>> >>> Well, I thought that spinlocks have implicit read-side critical sections >>> because it disables preemption (I learned from the comments above >>> synchronize_rcu() that says interrupts, preemption, or softirqs have been >>> disabled also serve as RCU read-side critical sections). So I have a >>> question: is it still true in a PREEMPT_RT kernel (I am not familiar with >>> this)? >> Yes, but you're missing my point. >> >>>> Should the comment be deleted? >>> I think we could remove the comments. If the above question is false, seems >>> like we should continue holding rcu read lock. >> It's true. >> > Thanks for your answer. > >> But assume it's false for a second. Why would you need to continue >> holding it? What would it protect? The lruvec would be pinned by the >> spinlock even if it DIDN'T imply an RCU lock, right? >> >> So I don't understand the point of the comment. If the implied RCU >> lock is protecting something not covered by the bare spinlock itself, >> it should be added to the comment. Otherwise, the comment should go. >> > Got it. Thanks for your nice explanation. I'll remove > the comment here. Note that there is a similar comment in patch 6 which may have to be removed as well. Cheers, Longman