diff mbox series

mm/z3fold: remove unneeded spinlock

Message ID 20240204125404.2112384-1-hezhongkun.hzk@bytedance.com
State New
Headers
Series mm/z3fold: remove unneeded spinlock |

Commit Message

Zhongkun He Feb. 4, 2024, 12:54 p.m. UTC 
  There is no need to use spinlock in this section, so
remove it.

Signed-off-by: Zhongkun He <hezhongkun.hzk@bytedance.com>
---
 mm/z3fold.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Matthew Wilcox Feb. 4, 2024, 6:46 p.m. UTC | #1 
On Sun, Feb 04, 2024 at 08:54:04PM +0800, Zhongkun He wrote:
> There is no need to use spinlock in this section, so
> remove it.

I don't know this code at all, but the idiom is (relatively) common.
It waits until anybody _currently_ holding the lock has released it.

That would, eg, make it safe to free the 'pool' memory.

> -	spin_lock(&pool->lock);
> -	spin_unlock(&pool->lock);
Zhongkun He Feb. 5, 2024, 1:08 a.m. UTC | #2 
On Mon, Feb 5, 2024 at 2:46 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Feb 04, 2024 at 08:54:04PM +0800, Zhongkun He wrote:
> > There is no need to use spinlock in this section, so
> > remove it.
>
> I don't know this code at all, but the idiom is (relatively) common.
> It waits until anybody _currently_ holding the lock has released it.
>
> That would, eg, make it safe to free the 'pool' memory.
>
> > -     spin_lock(&pool->lock);
> > -     spin_unlock(&pool->lock);
>

no,  please see the commit 'e774a7bc7f0adb'.

        spin_lock(&pool->lock);
-       if (!list_empty(&page->lru))
-               list_del_init(&page->lru);
        spin_unlock(&pool->lock);

The original purpose of this lock was to protect  page->lru,
which was removed now, so the spinlock is unnecessary.

Thanks for your time.
Johannes Weiner Feb. 8, 2024, 3:29 a.m. UTC | #3 
On Mon, Feb 05, 2024 at 09:08:05AM +0800, Zhongkun He wrote:
> On Mon, Feb 5, 2024 at 2:46 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Sun, Feb 04, 2024 at 08:54:04PM +0800, Zhongkun He wrote:
> > > There is no need to use spinlock in this section, so
> > > remove it.
> >
> > I don't know this code at all, but the idiom is (relatively) common.
> > It waits until anybody _currently_ holding the lock has released it.
> >
> > That would, eg, make it safe to free the 'pool' memory.
> >
> > > -     spin_lock(&pool->lock);
> > > -     spin_unlock(&pool->lock);
> >
> 
> no,  please see the commit 'e774a7bc7f0adb'.
> 
>         spin_lock(&pool->lock);
> -       if (!list_empty(&page->lru))
> -               list_del_init(&page->lru);
>         spin_unlock(&pool->lock);
> 
> The original purpose of this lock was to protect  page->lru,
> which was removed now, so the spinlock is unnecessary.

But pool->lock protects other stuff too? This doesn't rule out that
there is some other ordering dependency on cycling the lock before
freeing the entry. The person who would know best is the maintainer of
this code, Vitaly. Let's CC him.
Zhongkun He Feb. 8, 2024, 5:39 p.m. UTC | #4 
On Thu, Feb 8, 2024 at 11:29 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Mon, Feb 05, 2024 at 09:08:05AM +0800, Zhongkun He wrote:
> > On Mon, Feb 5, 2024 at 2:46 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Sun, Feb 04, 2024 at 08:54:04PM +0800, Zhongkun He wrote:
> > > > There is no need to use spinlock in this section, so
> > > > remove it.
> > >
> > > I don't know this code at all, but the idiom is (relatively) common.
> > > It waits until anybody _currently_ holding the lock has released it.
> > >
> > > That would, eg, make it safe to free the 'pool' memory.
> > >
> > > > -     spin_lock(&pool->lock);
> > > > -     spin_unlock(&pool->lock);
> > >
> >
> > no,  please see the commit 'e774a7bc7f0adb'.
> >
> >         spin_lock(&pool->lock);
> > -       if (!list_empty(&page->lru))
> > -               list_del_init(&page->lru);
> >         spin_unlock(&pool->lock);
> >
> > The original purpose of this lock was to protect  page->lru,
> > which was removed now, so the spinlock is unnecessary.
>
> But pool->lock protects other stuff too? This doesn't rule out that
> there is some other ordering dependency on cycling the lock before
> freeing the entry. The person who would know best is the maintainer of
> this code, Vitaly. Let's CC him.

Thank you for your reply and look forward to hearing from Vitaly.
diff mbox series

Patch

diff --git a/mm/z3fold.c b/mm/z3fold.c
index 7c76b396b74c..7f608c0667f3 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -442,8 +442,6 @@  static void __release_z3fold_page(struct z3fold_header *zhdr, bool locked)
 	WARN_ON(!list_empty(&zhdr->buddy));
 	set_bit(PAGE_STALE, &page->private);
 	clear_bit(NEEDS_COMPACTING, &page->private);
-	spin_lock(&pool->lock);
-	spin_unlock(&pool->lock);
 
 	if (locked)
 		z3fold_page_unlock(zhdr);