Received-SPF: pass (google.com: domain of linux-kernel+bounces-48984-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <7f19b4d69ff20efe8260a174c7866b4819532b1f.camel@linux.intel.com>
Subject: Re: [PATCH v2] mm: swap: async free swap slot cache entries
From: Tim Chen <tim.c.chen@linux.intel.com>
To: "Huang, Ying" <ying.huang@intel.com>, Chris Li <chrisl@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org,
  linux-mm@kvack.org, Wei =?UTF-8?Q?Xu=EF=BF=BC?= <weixugc@google.com>, Yu
 =?UTF-8?Q?Zhao=EF=BF=BC?= <yuzhao@google.com>, Greg Thelen
 <gthelen@google.com>, Chun-Tse Shao <ctshao@google.com>, Suren
 =?UTF-8?Q?Baghdasaryan=EF=BF=BC?= <surenb@google.com>,  Yosry
 =?UTF-8?Q?Ahmed=EF=BF=BC?= <yosryahmed@google.com>, Brain Geffon
 <bgeffon@google.com>, Minchan Kim <minchan@kernel.org>, Michal Hocko
 <mhocko@suse.com>, Mel Gorman <mgorman@techsingularity.net>, Nhat Pham
 <nphamcs@gmail.com>, Johannes Weiner <hannes@cmpxchg.org>, Kairui Song
 <kasong@tencent.com>, Zhongkun He <hezhongkun.hzk@bytedance.com>, Kemeng
 Shi <shikemeng@huaweicloud.com>,  Barry Song <v-songbaohua@oppo.com>
Date: Thu, 01 Feb 2024 15:20:58 -0800
In-Reply-To: <87sf2ceoks.fsf@yhuang6-desk2.ccr.corp.intel.com>
References: <20240131-async-free-v2-1-525f03e07184@kernel.org>
	 <87sf2ceoks.fsf@yhuang6-desk2.ccr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
Precedence: bulk
MIME-Version: 1.0

On Thu, 2024-02-01 at 13:33 +0800, Huang, Ying wrote:
> Chris Li <chrisl@kernel.org> writes:
>=20
> >=20
> > Changes in v2:
> > - Add description of the impact of time changing suggest by Ying.
> > - Remove create_workqueue() and use schedule_work()
> > - Link to v1: https://lore.kernel.org/r/20231221-async-free-v1-1-94b277=
992cb0@kernel.org
> > ---
> >  include/linux/swap_slots.h |  1 +
> >  mm/swap_slots.c            | 29 +++++++++++++++++++++--------
> >  2 files changed, 22 insertions(+), 8 deletions(-)
> >=20
> > diff --git a/include/linux/swap_slots.h b/include/linux/swap_slots.h
> > index 15adfb8c813a..67bc8fa30d63 100644
> > --- a/include/linux/swap_slots.h
> > +++ b/include/linux/swap_slots.h
> > @@ -19,6 +19,7 @@ struct swap_slots_cache {
> >  	spinlock_t	free_lock;  /* protects slots_ret, n_ret */
> >  	swp_entry_t	*slots_ret;
> >  	int		n_ret;
> > +	struct work_struct async_free;
> >  };
> > =20
> >  void disable_swap_slots_cache_lock(void);
> > diff --git a/mm/swap_slots.c b/mm/swap_slots.c
> > index 0bec1f705f8e..71d344564e55 100644
> > --- a/mm/swap_slots.c
> > +++ b/mm/swap_slots.c
> > @@ -44,6 +44,7 @@ static DEFINE_MUTEX(swap_slots_cache_mutex);
> >  static DEFINE_MUTEX(swap_slots_cache_enable_mutex);
> > =20
> >  static void __drain_swap_slots_cache(unsigned int type);
> > +static void swapcache_async_free_entries(struct work_struct *data);
> > =20
> >  #define use_swap_slot_cache (swap_slot_cache_active && swap_slot_cache=
_enabled)
> >  #define SLOTS_CACHE 0x1
> > @@ -149,6 +150,7 @@ static int alloc_swap_slot_cache(unsigned int cpu)
> >  		spin_lock_init(&cache->free_lock);
> >  		cache->lock_initialized =3D true;
> >  	}
> > +	INIT_WORK(&cache->async_free, swapcache_async_free_entries);
> >  	cache->nr =3D 0;
> >  	cache->cur =3D 0;
> >  	cache->n_ret =3D 0;
> > @@ -269,6 +271,20 @@ static int refill_swap_slots_cache(struct swap_slo=
ts_cache *cache)
> >  	return cache->nr;
> >  }
> > =20
> > +static void swapcache_async_free_entries(struct work_struct *data)
> > +{
> > +	struct swap_slots_cache *cache;
> > +
> > +	cache =3D container_of(data, struct swap_slots_cache, async_free);
> > +	spin_lock_irq(&cache->free_lock);
> > +	/* Swap slots cache may be deactivated before acquiring lock */
> > +	if (cache->slots_ret) {
> > +		swapcache_free_entries(cache->slots_ret, cache->n_ret);
> > +		cache->n_ret =3D 0;
> > +	}
> > +	spin_unlock_irq(&cache->free_lock);
> > +}
> > +
> >  void free_swap_slot(swp_entry_t entry)
> >  {
> >  	struct swap_slots_cache *cache;
> > @@ -282,17 +298,14 @@ void free_swap_slot(swp_entry_t entry)
> >  			goto direct_free;
> >  		}
> >  		if (cache->n_ret >=3D SWAP_SLOTS_CACHE_SIZE) {
> > -			/*
> > -			 * Return slots to global pool.
> > -			 * The current swap_map value is SWAP_HAS_CACHE.
> > -			 * Set it to 0 to indicate it is available for
> > -			 * allocation in global pool
> > -			 */
> > -			swapcache_free_entries(cache->slots_ret, cache->n_ret);
> > -			cache->n_ret =3D 0;
> > +			spin_unlock_irq(&cache->free_lock);
> > +			schedule_work(&cache->async_free);
> > +			goto direct_free;
> >  		}
> >  		cache->slots_ret[cache->n_ret++] =3D entry;
> >  		spin_unlock_irq(&cache->free_lock);
> > +		if (cache->n_ret >=3D SWAP_SLOTS_CACHE_SIZE)
> > +			schedule_work(&cache->async_free);


I have some concerns about the current patch with the change above.
We could hit the direct_free path very often.

By delaying the freeing of entries in the return
cache, we have to do more freeing of swap entry one at a time. When
we try to free an entry, we can find the return cache still full, waiting t=
o be freed.

So we have fewer batch free of swap entries, resulting in an increase in
number of sis->lock acquisitions overall. This could have the
effect of reducing swap throughput overall when swap is under heavy
operations and sis->lock is contended.

Tim

>=20

> >  	} else {
> >  direct_free:
> >  		swapcache_free_entries(&entry, 1);
> >=20
> > ---
> > base-commit: eacce8189e28717da6f44ee492b7404c636ae0de
> > change-id: 20231216-async-free-bef392015432
> >=20
> > Best regards,
>=20