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;
Date:   Tue, 24 May 2022 15:22:55 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Muchun Song <songmuchun@bytedance.com>
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,
        longman@redhat.com
Subject: Re: [PATCH v4 02/11] mm: memcontrol: introduce
 compact_folio_lruvec_lock_irqsave
Message-ID: <Yo0wj9OAPptmUoWM@cmpxchg.org>
References: <20220524060551.80037-1-songmuchun@bytedance.com>
 <20220524060551.80037-3-songmuchun@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220524060551.80037-3-songmuchun@bytedance.com>
Precedence: bulk

On Tue, May 24, 2022 at 02:05:42PM +0800, Muchun Song wrote:
> If we reuse the objcg APIs to charge LRU pages, the folio_memcg()
> can be changed when the LRU pages reparented. In this case, we need
> to acquire the new lruvec lock.
> 
>     lruvec = folio_lruvec(folio);
> 
>     // The page is reparented.
> 
>     compact_lock_irqsave(&lruvec->lru_lock, &flags, cc);
> 
>     // Acquired the wrong lruvec lock and need to retry.
> 
> But compact_lock_irqsave() only take lruvec lock as the parameter,
> we cannot aware this change. If it can take the page as parameter
> to acquire the lruvec lock. When the page memcg is changed, we can
> use the folio_memcg() detect whether we need to reacquire the new
> lruvec lock. So compact_lock_irqsave() is not suitable for us.
> Similar to folio_lruvec_lock_irqsave(), introduce
> compact_folio_lruvec_lock_irqsave() to acquire the lruvec lock in
> the compaction routine.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

This looks generally good to me.

It did raise the question how deferencing lruvec is safe before the
lock is acquired when reparenting can race. The answer is in the next
patch when you add the rcu_read_lock(). Since the patches aren't big,
it would probably be better to merge them.

> @@ -509,6 +509,29 @@ static bool compact_lock_irqsave(spinlock_t *lock, unsigned long *flags,
>  	return true;
>  }
>  
> +static struct lruvec *
> +compact_folio_lruvec_lock_irqsave(struct folio *folio, unsigned long *flags,
> +				  struct compact_control *cc)
> +{
> +	struct lruvec *lruvec;
> +
> +	lruvec = folio_lruvec(folio);
> +
> +	/* Track if the lock is contended in async mode */
> +	if (cc->mode == MIGRATE_ASYNC && !cc->contended) {
> +		if (spin_trylock_irqsave(&lruvec->lru_lock, *flags))
> +			goto out;
> +
> +		cc->contended = true;
> +	}
> +
> +	spin_lock_irqsave(&lruvec->lru_lock, *flags);

Can you implement this on top of the existing one?

	lruvec = folio_lruvec(folio);
	compact_lock_irqsave(&lruvec->lru_lock, flags);
	lruvec_memcg_debug(lruvec, folio);
	return lruvec;