| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20240802071817.47081-1-21cnbao@gmail.com>
Date: Fri, 2 Aug 2024 19:18:17 +1200
From: Barry Song <21cnbao@...il.com>
To: ying.huang@...el.com
Cc: akpm@...ux-foundation.org,
baolin.wang@...ux.alibaba.com,
chrisl@...nel.org,
david@...hat.com,
hannes@...xchg.org,
hughd@...gle.com,
kaleshsingh@...gle.com,
kasong@...cent.com,
linux-kernel@...r.kernel.org,
linux-mm@...ck.org,
mhocko@...e.com,
minchan@...nel.org,
nphamcs@...il.com,
ryan.roberts@....com,
senozhatsky@...omium.org,
shakeel.butt@...ux.dev,
shy828301@...il.com,
surenb@...gle.com,
v-songbaohua@...o.com,
willy@...radead.org,
xiang@...nel.org,
yosryahmed@...gle.com
Subject: Re: [PATCH 1/1] mm: swap: add nr argument in swapcache_prepare and swapcache_clear to support large folios
On Fri, Aug 2, 2024 at 1:50 PM Huang, Ying <ying.huang@...el.com> wrote:
> >
> > Right. I believe the change below can help improve readability and also
> > clarify the swap_count_continued() case.
> >
> > diff --git a/mm/swapfile.c b/mm/swapfile.c
> > index 2fa29bdec171..75a89ce18edc 100644
> > --- a/mm/swapfile.c
> > +++ b/mm/swapfile.c
> > @@ -3538,6 +3538,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> >
> > offset = swp_offset(entry);
> > VM_WARN_ON(nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER);
> > + VM_WARN_ON(usage == 1 && nr > 1);
> > ci = lock_cluster_or_swap_info(p, offset);
> >
> > err = 0;
> > @@ -3564,17 +3565,9 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> > err = -EEXIST;
> > else /* no users remaining */
> > err = -ENOENT;
> > -
> > } else if (count || has_cache) {
> > -
> > - if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX)
> > - continue;
> > - else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX)
> > + if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX)
> > err = -EINVAL;
> > - else if (swap_count_continued(p, offset + i, count))
> > - continue;
> > - else
> > - err = -ENOMEM;
> > } else
> > err = -ENOENT; /* unused swap entry */
> >
> > @@ -3591,8 +3584,12 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
> > has_cache = SWAP_HAS_CACHE;
> > else if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX)
> > count += usage;
> > - else
> > + else if (swap_count_continued(p, offset + i, count))
> > count = COUNT_CONTINUED;
> > + else {
> > + err = -ENOMEM;
> > + goto unlock_out;
> > + }
> >
> > WRITE_ONCE(p->swap_map[offset + i], count | has_cache);
> > }
> >
> > This makes the two iterations become:
> >
> > for (i = 0; i < nr; i++) {
> > count = p->swap_map[offset + i];
> >
> > /*
> > * swapin_readahead() doesn't check if a swap entry is valid, so the
> > * swap entry could be SWAP_MAP_BAD. Check here with lock held.
> > */
> > if (unlikely(swap_count(count) == SWAP_MAP_BAD)) {
> > err = -ENOENT;
> > goto unlock_out;
> > }
> >
> > has_cache = count & SWAP_HAS_CACHE;
> > count &= ~SWAP_HAS_CACHE;
> >
> > if (usage == SWAP_HAS_CACHE) {
> > /* set SWAP_HAS_CACHE if there is no cache and entry is used */
>
> The comments doen't apply now, we don't "set" here.
>
> > if (!has_cache && count)
> > continue;
> > else if (has_cache) /* someone else added cache */
> > err = -EEXIST;
> > else /* no users remaining */
> > err = -ENOENT;
> > } else if (count || has_cache) {
> > if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX)
> > err = -EINVAL;
> > } else
> > err = -ENOENT; /* unused swap entry */
>
> It seems that this can be simplified to:
>
> if (!count && !has_cache) {
> err = -ENOENT;
> } else if (usage == SWAP_HAS_CACHE) {
> if (has_cache)
> err = -EEXIST;
> } else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) {
> err = -EINVAL;
> }
>
> > if (err)
> > goto unlock_out;
> > }
> >
> > for (i = 0; i < nr; i++) {
> > count = p->swap_map[offset + i];
> > has_cache = count & SWAP_HAS_CACHE;
> > count &= ~SWAP_HAS_CACHE;
> >
> > if (usage == SWAP_HAS_CACHE)
> > has_cache = SWAP_HAS_CACHE;
> > else if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX)
> > count += usage;
> > else if (swap_count_continued(p, offset + i, count))
> > count = COUNT_CONTINUED;
> > else {
>
> Better to add some comments here,
>
> /*
> * Don't need to rollback changes, because if
> * usage == 1, there must be nr == 1.
> */
>
> > err = -ENOMEM;
> > goto unlock_out;
> > }
> >
> > WRITE_ONCE(p->swap_map[offset + i], count | has_cache);
> > }
> >
> > Ying, do you feel more satisfied with the version above
> > compared to the code in mm-unstable?
>
> This looks good to me except some minor comments above. Thanks!
Thanks very much, Ying.
Hi Andrew,
Could you please help squash the below change?
>From 17cbd696ecd37bc199b6e87c0c837d1a1ae9ac8d Mon Sep 17 00:00:00 2001
From: Barry Song <v-songbaohua@...o.com>
Date: Fri, 2 Aug 2024 17:52:43 +1200
Subject: [PATCH] mm: clarify swap_count_continued and improve readability for
__swap_duplicate
when usage=1 and swapcount is very large, the situation can become quite
complex. The first entry might succeed with swap_count_continued(),
but the second entry may require extending to an additional continued
page. Rolling back these changes can be extremely challenging. Therefore,
anyone using usage==1 for batched __swap_duplicate() operations should
proceed with caution.
Additionally, we have moved swap_count_continued() to the second iteration
to enhance readability, as this function may modify data.
Suggested-by: "Huang, Ying" <ying.huang@...el.com>
Signed-off-by: Barry Song <v-songbaohua@...o.com>
---
mm/swapfile.c | 38 +++++++++++++++++---------------------
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index d9cf31b04db3..ea023fc25d08 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -3540,6 +3540,7 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
offset = swp_offset(entry);
VM_WARN_ON(nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER);
+ VM_WARN_ON(usage == 1 && nr > 1);
ci = lock_cluster_or_swap_info(p, offset);
err = 0;
@@ -3558,27 +3559,14 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
has_cache = count & SWAP_HAS_CACHE;
count &= ~SWAP_HAS_CACHE;
- if (usage == SWAP_HAS_CACHE) {
- /* set SWAP_HAS_CACHE if there is no cache and entry is used */
- if (!has_cache && count)
- continue;
- else if (has_cache) /* someone else added cache */
+ if (!count && !has_cache) {
+ err = -ENOENT;
+ } else if (usage == SWAP_HAS_CACHE) {
+ if (has_cache)
err = -EEXIST;
- else /* no users remaining */
- err = -ENOENT;
-
- } else if (count || has_cache) {
-
- if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX)
- continue;
- else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX)
- err = -EINVAL;
- else if (swap_count_continued(p, offset + i, count))
- continue;
- else
- err = -ENOMEM;
- } else
- err = -ENOENT; /* unused swap entry */
+ } else if ((count & ~COUNT_CONTINUED) > SWAP_MAP_MAX) {
+ err = -EINVAL;
+ }
if (err)
goto unlock_out;
@@ -3593,8 +3581,16 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage, int nr)
has_cache = SWAP_HAS_CACHE;
else if ((count & ~COUNT_CONTINUED) < SWAP_MAP_MAX)
count += usage;
- else
+ else if (swap_count_continued(p, offset + i, count))
count = COUNT_CONTINUED;
+ else {
+ /*
+ * Don't need to rollback changes, because if
+ * usage == 1, there must be nr == 1.
+ */
+ err = -ENOMEM;
+ goto unlock_out;
+ }
WRITE_ONCE(p->swap_map[offset + i], count | has_cache);
}
--
2.34.1
> >> >> >> >> >> Best Regards,
> >> >> >> >> >> Huang, Ying
> >> >> >> >> >
> >> >> >
> >
Thanks
Barry
Powered by blists - more mailing lists