| Message ID | 20240204131543.1469661-1-hezhongkun.hzk@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/z3fold:remove unneeded spinlock in z3fold_alloc | expand |
On Sun, Feb 04, 2024 at 09:15:43PM +0800, Zhongkun He wrote: > The spinklock in z3fold_alloc() is used to protect page->lru, > but now it was removed in commit 'e774a7bc7f0ad', so remove > the spinlock too. The pool->lock clearly protects things other than page->lru. I'm not saying that you're wrong, but your changelog doesn't make a strong case that you're right, either ;) Please CC Vitaly (scripts/get_maintainer.pl) on these patches.
On Thu, Feb 8, 2024 at 11:34 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Sun, Feb 04, 2024 at 09:15:43PM +0800, Zhongkun He wrote: > > The spinklock in z3fold_alloc() is used to protect page->lru, > > but now it was removed in commit 'e774a7bc7f0ad', so remove > > the spinlock too. > > The pool->lock clearly protects things other than page->lru. Yes, it protects pool unbuddied lists now. I mean the following page-lru is removed in z3fold_alloc: headless: spin_lock(&pool->lock); - /* Add/move z3fold page to beginning of LRU */ - if (!list_empty(&page->lru)) - list_del(&page->lru); - - list_add(&page->lru, &pool->lru); - *handle = encode_handle(zhdr, bud); spin_unlock(&pool->lock); if (bud != HEADLESS) z3fold_page_unlock(zhdr); For more detail please see the commit 'e774a7bc7f0ad'. > > I'm not saying that you're wrong, but your changelog doesn't make a > strong case that you're right, either ;) > Hi Johannes, I am confused by the annotation : The __encode_handle() annotation show: * Pool lock should be held as this function accesses first_num. but I did not find any use cases where first_num was protected by pool lock. Furthermore, encode_handle() is protected by z3fold_page_lock() as I can see. > Please CC Vitaly (scripts/get_maintainer.pl) on these patches. Got it, thanks.
On Sun, 4 Feb 2024 21:15:43 +0800 Zhongkun He <hezhongkun.hzk@bytedance.com> wrote: > The spinklock in z3fold_alloc() is used to protect page->lru, > but now it was removed in commit 'e774a7bc7f0ad', so remove > the spinlock too. > I think I'll drop this patch and "mm/z3fold: remove unneeded spinlock" pending some more clarity and agreement on whether these are safe. Thanks.
> > I think I'll drop this patch and "mm/z3fold: remove unneeded spinlock" > pending some more clarity and agreement on whether these are > safe. Thanks. Hi Andrew. Per my understanding, these patches are safe and I hope they can be reviewed by some experts. Thanks.
diff --git a/mm/z3fold.c b/mm/z3fold.c index 7f608c0667f3..58946cacbfbb 100644 --- a/mm/z3fold.c +++ b/mm/z3fold.c @@ -1068,9 +1068,7 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp, add_to_unbuddied(pool, zhdr); headless: - spin_lock(&pool->lock); *handle = encode_handle(zhdr, bud); - spin_unlock(&pool->lock); if (bud != HEADLESS) z3fold_page_unlock(zhdr);
The spinklock in z3fold_alloc() is used to protect page->lru, but now it was removed in commit 'e774a7bc7f0ad', so remove the spinlock too. Signed-off-by: Zhongkun He <hezhongkun.hzk@bytedance.com> --- mm/z3fold.c | 2 -- 1 file changed, 2 deletions(-)