From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lj1-f170.google.com (mail-lj1-f170.google.com [209.85.208.170]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4A7EA1B21B8 for ; Fri, 3 Jan 2025 08:07:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.170 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735891648; cv=none; b=OIOFNZdpHO5Z+KG9Dv83wHq/ajQVzXhRzXD0FMnJ0j9/DqrHGBDQvUkChBeVdPSUglb3aFfbJkTB4Z6p+iowthShgjLAsLdo0f071ZL3S1IR6HLdOmsbPCN+qTbfiJfqzw1pkRjpfWPb6Pidju3dnK2sB2dmPPgSFJApw5bFq4M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735891648; c=relaxed/simple; bh=6fTbEcMfkFtJW7W3MeebDn00aHfaaJACOvtPL4gYyds=; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject: To:Cc:Content-Type; b=LvxzyF1SRSJyNAy5Q4xCCT4dizrB1jJUzSSVReJ+BAWLSd0x6U8GsttTkm8gM24ymwwAalZ9EmJ7FwnrC5GtytWZPOqWwGHXd82A3+Vo6p5BqEGsny7rnfDR3YiBkn50rSt4n6mr1Bud63YVEk1+gaJBI4eLVXnXuvSYAmK2+5g= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=Yzck2adU; arc=none smtp.client-ip=209.85.208.170 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Yzck2adU" Received: by mail-lj1-f170.google.com with SMTP id 38308e7fff4ca-2ffd6af012eso166920351fa.2 for ; Fri, 03 Jan 2025 00:07:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1735891644; x=1736496444; darn=vger.kernel.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=hZLTArBw1/+FylqQRi3iKQavt8iTMYzpPuCs1DTqFLY=; b=Yzck2adUh3MMw0jfjKQzDRqqBQ5xUocUs6bJsxMS98RWT5pLj/qfXxBKIWZaIEH7GR pO8KTInevVUlyRuBFq6vYFv35lqweXUEQ1WV/qa6GP+tzPo5Iy/2x4qkDwpq7rQir2Ic oLiXbL0JX3nnYf5EZ8XXUeqqM7XfFtY3qaesQ7hJY56CD47r/n3F5MizlnK7Ev6e9kyJ Mt9xLrwPK7UsKq9ToBuymP+jw2cqNaAnJPKsX5gKGHL72KIRvt+TP13A3Ro3mVJuZWpU WWZcKP6D7NVU8Nvr6paX7cjv6o6Vb8Uqv3UO2adNvFbT2Q1nuVIQTBlqYsApdbNgj7or cW8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735891644; x=1736496444; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=hZLTArBw1/+FylqQRi3iKQavt8iTMYzpPuCs1DTqFLY=; b=gEkmDtrk+jVjOsXrVG3OgXe4Q0JFjJpR1sej6U1FYIFqa/OV74PBDn/N+NjpaT5+1p V3urXO9fmGZgw7amSwhUhNH49aOMFQdzp9otybtjxDm65nu4it6fmfZV44bd7hEl7qoh LskD7+hO8/xkcn7dIPb2uyOOcyHYZTYHpRzXj/JnrmcznBr6k49YhL4Xz8pbvLe/swcD l8dDejqRazQhguclbfLU2Whp/wwR3oFUxn1IaKnLyAQJ5uO+WuGOnOHTY1HOXKhC+qPX 97BJTYe024niktEKXcaPTYIgV1TeZID7d06jhznB+yEvx1In82lTLCcOsEBmIobBcVNo 1XoQ== X-Forwarded-Encrypted: i=1; AJvYcCWZdsRF7SJ/lC0szb/TQCp2tExjsdT9e2NKfUvi9sxt+MfyH5sSsdmCmEPAtFo7wYQh8jdGcx5iOov+Bek=@vger.kernel.org X-Gm-Message-State: AOJu0Yyr3kDYyKm2lnTo+5DCQnjM745vDkRN54QAhSzCPV82hNGGBE8n 0RYhtm93GHAu7cOgApdZ0SyHHUv+kKNozbWf6H4LAwcI3WtFwCS62gZuAtXMnnWWKP9scR2pvwF npb++v02pvOAgX/+uU528Q3udc4k= X-Gm-Gg: ASbGncsUrRIhMjw762U1fGnBSBBsrV5wDZuT1ZippYcY+LGRSbg5/kqOCQgzfThbHyp VP0FMYssbBdrSLw3vzpBTb/mGiaEQJaqEDf8V X-Google-Smtp-Source: AGHT+IEMP3ZnKpbq9a+Y2Qbv+rEisacNJcO89zxahEGXIURQ6iicibbw/sgNPQHslmIKoyEZrt4o4jcpBRtAJG5I6+8= X-Received: by 2002:a05:651c:511:b0:302:25ef:813e with SMTP id 38308e7fff4ca-30468608eaamr164131501fa.32.1735891643699; Fri, 03 Jan 2025 00:07:23 -0800 (PST) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 References: <20241230174621.61185-1-ryncsn@gmail.com> <20241230174621.61185-7-ryncsn@gmail.com> In-Reply-To: From: Kairui Song Date: Fri, 3 Jan 2025 16:07:07 +0800 Message-ID: Subject: Re: [PATCH v3 06/13] mm, swap: clean up plist removal and adding To: Baoquan He Cc: linux-mm@kvack.org, Andrew Morton , Chris Li , Barry Song , Ryan Roberts , Hugh Dickins , Yosry Ahmed , "Huang, Ying" , Nhat Pham , Johannes Weiner , Kalesh Singh , linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Jan 2, 2025 at 4:59=E2=80=AFPM Baoquan He wrote: > > Hi Kairui, > > On 12/31/24 at 01:46am, Kairui Song wrote: > ......snip... > > diff --git a/mm/swapfile.c b/mm/swapfile.c > > index 7963a0c646a4..e6e58cfb5178 100644 > > --- a/mm/swapfile.c > > +++ b/mm/swapfile.c > > @@ -128,6 +128,26 @@ static inline unsigned char swap_count(unsigned ch= ar ent) > > return ent & ~SWAP_HAS_CACHE; /* may include COUNT_CONTINUED fl= ag */ > > } > > I am reading swap code, while at it, I am going through this patchset > too. Have some nitpick, please see below inline comments. Thanks! > > > > +/* > > + * Use the second highest bit of inuse_pages counter as the indicator > > + * of if one swap device is on the available plist, so the atomic can > ~~ redundant? > > + * still be updated arithmetic while having special data embedded. > ~~~~~~~~~~ typo, arithmetically? > > + * > > + * inuse_pages counter is the only thing indicating if a device should > > + * be on avail_lists or not (except swapon / swapoff). By embedding th= e > > + * on-list bit in the atomic counter, updates no longer need any lock > ~~~ off-list? Ah, right, some typos, will fix these. > > + * to check the list status. > > + * > > + * This bit will be set if the device is not on the plist and not > > + * usable, will be cleared if the device is on the plist. > > + */ > > +#define SWAP_USAGE_OFFLIST_BIT (1UL << (BITS_PER_TYPE(atomic_t) - 2)) > > +#define SWAP_USAGE_COUNTER_MASK (~SWAP_USAGE_OFFLIST_BIT) > > +static long swap_usage_in_pages(struct swap_info_struct *si) > > +{ > > + return atomic_long_read(&si->inuse_pages) & SWAP_USAGE_COUNTER_MA= SK; > > +} > > + > > /* Reclaim the swap entry anyway if possible */ > > #define TTRS_ANYWAY 0x1 > > /* > > @@ -717,7 +737,7 @@ static void swap_reclaim_full_clusters(struct swap_= info_struct *si, bool force) > > int nr_reclaim; > > > > if (force) > > - to_scan =3D si->inuse_pages / SWAPFILE_CLUSTER; > > + to_scan =3D swap_usage_in_pages(si) / SWAPFILE_CLUSTER; > > > > while (!list_empty(&si->full_clusters)) { > > ci =3D list_first_entry(&si->full_clusters, struct swap_c= luster_info, list); > > @@ -872,42 +892,128 @@ static unsigned long cluster_alloc_swap_entry(st= ruct swap_info_struct *si, int o > > return found; > > } > > > > -static void __del_from_avail_list(struct swap_info_struct *si) > > +/* SWAP_USAGE_OFFLIST_BIT can only be cleared by this helper. */ > Seems it just says the opposite. The off-list bit is set in > this function. Right, the comments are opposite... will fix them. > > +static void del_from_avail_list(struct swap_info_struct *si, bool swap= off) > > { > > int nid; > > + unsigned long pages; > > + > > + spin_lock(&swap_avail_lock); > > + > > + if (swapoff) { > > + /* > > + * Forcefully remove it. Clear the SWP_WRITEOK flags for > > + * swapoff here so it's synchronized by both si->lock and > > + * swap_avail_lock, to ensure the result can be seen by > > + * add_to_avail_list. > > + */ > > + lockdep_assert_held(&si->lock); > > + si->flags &=3D ~SWP_WRITEOK; > > + atomic_long_or(SWAP_USAGE_OFFLIST_BIT, &si->inuse_pages); > > + } else { > > + /* > > + * If not called by swapoff, take it off-list only if it'= s > > + * full and SWAP_USAGE_OFFLIST_BIT is not set (strictly > > + * si->inuse_pages =3D=3D pages), any concurrent slot fre= eing, > > + * or device already removed from plist by someone else > > + * will make this return false. > > + */ > > + pages =3D si->pages; > > + if (!atomic_long_try_cmpxchg(&si->inuse_pages, &pages, > > + pages | SWAP_USAGE_OFFLIST_B= IT)) > > + goto skip; > > + } > > > > - assert_spin_locked(&si->lock); > > for_each_node(nid) > > plist_del(&si->avail_lists[nid], &swap_avail_heads[nid]); > > + > > +skip: > > + spin_unlock(&swap_avail_lock); > > } > > > > -static void del_from_avail_list(struct swap_info_struct *si) > > +/* SWAP_USAGE_OFFLIST_BIT can only be set by this helper. */ > > Ditto. > > > +static void add_to_avail_list(struct swap_info_struct *si, bool swapon= ) > > { > > + int nid; > > + long val; > > + unsigned long pages; > > + > > spin_lock(&swap_avail_lock); > > - __del_from_avail_list(si); > > + > > + /* Corresponding to SWP_WRITEOK clearing in del_from_avail_list *= / > > + if (swapon) { > > + lockdep_assert_held(&si->lock); > > + si->flags |=3D SWP_WRITEOK; > > + } else { > > + if (!(READ_ONCE(si->flags) & SWP_WRITEOK)) > > + goto skip; > > + } > > + > > + if (!(atomic_long_read(&si->inuse_pages) & SWAP_USAGE_OFFLIST_BIT= )) > > + goto skip; > > + > > + val =3D atomic_long_fetch_and_relaxed(~SWAP_USAGE_OFFLIST_BIT, &s= i->inuse_pages); > > + > > + /* > > + * When device is full and device is on the plist, only one updat= er will > > + * see (inuse_pages =3D=3D si->pages) and will call del_from_avai= l_list. If > > + * that updater happen to be here, just skip adding. > > + */ > > + pages =3D si->pages; > > + if (val =3D=3D pages) { > > + /* Just like the cmpxchg in del_from_avail_list */ > > + if (atomic_long_try_cmpxchg(&si->inuse_pages, &pages, > > + pages | SWAP_USAGE_OFFLIST_BI= T)) > > + goto skip; > > + } > > + > > + for_each_node(nid) > > + plist_add(&si->avail_lists[nid], &swap_avail_heads[nid]); > > + > > +skip: > > spin_unlock(&swap_avail_lock); > > } > > > > -static void swap_range_alloc(struct swap_info_struct *si, > > - unsigned int nr_entries) > > +/* > > + * swap_usage_add / swap_usage_sub of each slot are serialized by ci->= lock > > Not sure if swap_inuse_add()/swap_inuse_sub() or swap_inuse_cnt_add/sub() > is better, because it mixes with the usage of si->swap_map[offset]. > Anyway, not strong opinion. > > > + * within each cluster, so the total contribution to the global counte= r should > > + * always be positive and cannot exceed the total number of usable slo= ts. > > + */ > > +static bool swap_usage_add(struct swap_info_struct *si, unsigned int n= r_entries) > > { > > - WRITE_ONCE(si->inuse_pages, si->inuse_pages + nr_entries); > > - if (si->inuse_pages =3D=3D si->pages) { > > - del_from_avail_list(si); > > + long val =3D atomic_long_add_return_relaxed(nr_entries, &si->inus= e_pages); > > > > - if (si->cluster_info && vm_swap_full()) > > - schedule_work(&si->reclaim_work); > > + /* > > + * If device is full, and SWAP_USAGE_OFFLIST_BIT is not set, > > + * remove it from the plist. > > + */ > > + if (unlikely(val =3D=3D si->pages)) { > > + del_from_avail_list(si, false); > > + return true; > > } > > + > > + return false; > > } > > > > -static void add_to_avail_list(struct swap_info_struct *si) > > +static void swap_usage_sub(struct swap_info_struct *si, unsigned int n= r_entries) > > { > > - int nid; > > + long val =3D atomic_long_sub_return_relaxed(nr_entries, &si->inus= e_pages); > > > > - spin_lock(&swap_avail_lock); > > - for_each_node(nid) > > - plist_add(&si->avail_lists[nid], &swap_avail_heads[nid]); > > - spin_unlock(&swap_avail_lock); > > + /* > > + * If device is not full, and SWAP_USAGE_OFFLIST_BIT is set, > > + * remove it from the plist. > > + */ > > + if (unlikely(val & SWAP_USAGE_OFFLIST_BIT)) > > + add_to_avail_list(si, false); > > +} > > + > > +static void swap_range_alloc(struct swap_info_struct *si, > > + unsigned int nr_entries) > > +{ > > + if (swap_usage_add(si, nr_entries)) { > > + if (si->cluster_info && vm_swap_full()) > > We may not need check si->cluster_info here since it always exists now. Good catch, it can be dropped indeed as an optimization, one previous patch in this series is supposed to drop them all, I think I forgot this one. > > > + schedule_work(&si->reclaim_work); > > + } > > } > > > > static void swap_range_free(struct swap_info_struct *si, unsigned long= offset, > > @@ -925,8 +1031,6 @@ static void swap_range_free(struct swap_info_struc= t *si, unsigned long offset, > > for (i =3D 0; i < nr_entries; i++) > > clear_bit(offset + i, si->zeromap); > > > > - if (si->inuse_pages =3D=3D si->pages) > > - add_to_avail_list(si); > > if (si->flags & SWP_BLKDEV) > > swap_slot_free_notify =3D > > si->bdev->bd_disk->fops->swap_slot_free_notify; > > @@ -946,7 +1050,7 @@ static void swap_range_free(struct swap_info_struc= t *si, unsigned long offset, > > */ > > smp_wmb(); > > atomic_long_add(nr_entries, &nr_swap_pages); > > - WRITE_ONCE(si->inuse_pages, si->inuse_pages - nr_entries); > > + swap_usage_sub(si, nr_entries); > > } > > > > static int cluster_alloc_swap(struct swap_info_struct *si, > > @@ -1036,19 +1140,6 @@ int get_swap_pages(int n_goal, swp_entry_t swp_e= ntries[], int entry_order) > > plist_requeue(&si->avail_lists[node], &swap_avail_heads[n= ode]); > > spin_unlock(&swap_avail_lock); > > spin_lock(&si->lock); > > - if ((si->inuse_pages =3D=3D si->pages) || !(si->flags & S= WP_WRITEOK)) { > > - spin_lock(&swap_avail_lock); > > - if (plist_node_empty(&si->avail_lists[node])) { > > - spin_unlock(&si->lock); > > - goto nextsi; > > - } > > - WARN(!(si->flags & SWP_WRITEOK), > > - "swap_info %d in list but !SWP_WRITEOK\n", > > - si->type); > > - __del_from_avail_list(si); > > - spin_unlock(&si->lock); > > - goto nextsi; > > - } > > n_ret =3D scan_swap_map_slots(si, SWAP_HAS_CACHE, > > n_goal, swp_entries, order); > > spin_unlock(&si->lock); > > @@ -1057,7 +1148,6 @@ int get_swap_pages(int n_goal, swp_entry_t swp_en= tries[], int entry_order) > > cond_resched(); > > > > spin_lock(&swap_avail_lock); > > -nextsi: > > /* > > * if we got here, it's likely that si was almost full be= fore, > > * and since scan_swap_map_slots() can drop the si->lock, > > @@ -1789,7 +1879,7 @@ unsigned int count_swap_pages(int type, int free) > > if (sis->flags & SWP_WRITEOK) { > > n =3D sis->pages; > > if (free) > > - n -=3D sis->inuse_pages; > > + n -=3D swap_usage_in_pages(sis); > > } > > spin_unlock(&sis->lock); > > } > > @@ -2124,7 +2214,7 @@ static int try_to_unuse(unsigned int type) > > swp_entry_t entry; > > unsigned int i; > > > > - if (!READ_ONCE(si->inuse_pages)) > > + if (!swap_usage_in_pages(si)) > > goto success; > > > > retry: > > @@ -2137,7 +2227,7 @@ static int try_to_unuse(unsigned int type) > > > > spin_lock(&mmlist_lock); > > p =3D &init_mm.mmlist; > > - while (READ_ONCE(si->inuse_pages) && > > + while (swap_usage_in_pages(si) && > > !signal_pending(current) && > > (p =3D p->next) !=3D &init_mm.mmlist) { > > > > @@ -2165,7 +2255,7 @@ static int try_to_unuse(unsigned int type) > > mmput(prev_mm); > > > > i =3D 0; > > - while (READ_ONCE(si->inuse_pages) && > > + while (swap_usage_in_pages(si) && > > !signal_pending(current) && > > (i =3D find_next_to_unuse(si, i)) !=3D 0) { > > > > @@ -2200,7 +2290,7 @@ static int try_to_unuse(unsigned int type) > > * folio_alloc_swap(), temporarily hiding that swap. It's easy > > * and robust (though cpu-intensive) just to keep retrying. > > */ > > - if (READ_ONCE(si->inuse_pages)) { > > + if (swap_usage_in_pages(si)) { > > if (!signal_pending(current)) > > goto retry; > > return -EINTR; > > @@ -2209,7 +2299,7 @@ static int try_to_unuse(unsigned int type) > > success: > > /* > > * Make sure that further cleanups after try_to_unuse() returns h= appen > > - * after swap_range_free() reduces si->inuse_pages to 0. > > + * after swap_range_free() reduces inuse_pages to 0. > > Here, I personally think the original si->inuse_pages may be better. I updated this comment to avoid people from mis-using it directly, anyway it's a trivial comment, can keep it unchanged.