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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 453D2C433F5 for ; Thu, 26 May 2022 20:17:34 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BE3A68D0003; Thu, 26 May 2022 16:17:33 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B93148D0001; Thu, 26 May 2022 16:17:33 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A313C8D0003; Thu, 26 May 2022 16:17:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 95C188D0001 for ; Thu, 26 May 2022 16:17:33 -0400 (EDT) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 5841F70B for ; Thu, 26 May 2022 20:17:33 +0000 (UTC) X-FDA: 79509004386.16.34045DF Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by imf13.hostedemail.com (Postfix) with ESMTP id C646F2003E for ; Thu, 26 May 2022 20:17:03 +0000 (UTC) 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 X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: C646F2003E X-Rspam-User: X-Stat-Signature: i79st87nayc9ddn7ozxok8udkyg6m3bi Authentication-Results: imf13.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=UVRNItfu; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf13.hostedemail.com: domain of longman@redhat.com has no SPF policy when checking 170.10.129.124) smtp.mailfrom=longman@redhat.com X-HE-Tag: 1653596223-378342 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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