Re: [PATCH 6/9] mm/z3fold: move decrement of pool->pages_nr into __release_z3fold_page()

From: Vitaly Wool
Date: Wed Mar 02 2022 - 03:32:18 EST

Next message: Yin Fengwei: "Re: [xfs] 6191cf3ad5: stress-ng.rename.ops_per_sec -73.5% regression"
Previous message: Michael S. Tsirkin: "Re: propagating vmgenid outward and upward"
Next in thread: David Laight: "RE: [PATCH 6/9] mm/z3fold: move decrement of pool->pages_nr into __release_z3fold_page()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Feb 21, 2022 at 6:17 AM David Laight <David.Laight@xxxxxxxxxx> wrote:
>
> From: Miaohe Lin <linmiaohe@xxxxxxxxxx>
> > Sent: 21 February 2022 02:53
> >
> > On 2022/2/20 0:33, David Laight wrote:
> > > From: Miaohe Lin
> > >> Sent: 19 February 2022 09:26
> > >>
> > >> The z3fold will always do atomic64_dec(&pool->pages_nr) when the
> > >> __release_z3fold_page() is called. Thus we can move decrement of
> > >> pool->pages_nr into __release_z3fold_page() to simplify the code.
> > >> Also we can reduce the size of z3fold.o ~1k.
> > >> Without this patch:
> > >> text data bss dec hex filename
> > >> 15444 1376 8 16828 41bc mm/z3fold.o
> > >> With this patch:
> > >> text data bss dec hex filename
> > >> 15044 1248 8 16300 3fac mm/z3fold.o
> > >
> > > I can't see anything obvious in this patch that would reduce the size much.
> > > OTOH there are some large functions that are pointlessly marked 'inline'.
> > > Maybe the compiler made a better choice?
> >
> > I think so too.
> >
> > > Although it isn't al all obvious why the 'data' size changes.
> >
> > I checked the header of z3fold.o. The size of .data is unchanged while
> > align is changed from 00003818 to 00003688. Maybe this is the reason
> > .data size changes.
>
> You are misreading the double line header.
> If is Offset that is changing, Align in 8 (as expected).
>
> It will be another section that gets added to the 'data' size
> reported by 'size'.
>
> >
> > Section Headers:
> > [Nr] Name Type Address Offset
> > Size EntSize Flags Link Info Align
> >
> > with this patch:
> > [ 3] .data PROGBITS 0000000000000000 00003688
> > 00000000000000c0 0000000000000000 WA 0 0 8
> >
> > without this patch:
> > [ 3] .data PROGBITS 0000000000000000 00003818
> > 00000000000000c0 0000000000000000 WA 0 0 8
> >
> > >
> > >> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
> > >> ---
> > >> mm/z3fold.c | 41 ++++++++++++-----------------------------
> > >> 1 file changed, 12 insertions(+), 29 deletions(-)
> > >>
> > >> diff --git a/mm/z3fold.c b/mm/z3fold.c
> > >> index adc0b3fa4906..18a697f6fe32 100644
> > >> --- a/mm/z3fold.c
> > >> +++ b/mm/z3fold.c
> > >> @@ -520,6 +520,8 @@ static void __release_z3fold_page(struct z3fold_header *zhdr, bool locked)
> > >> list_add(&zhdr->buddy, &pool->stale);
> > >> queue_work(pool->release_wq, &pool->work);
> > >> spin_unlock(&pool->stale_lock);
> > >> +
> > >> + atomic64_dec(&pool->pages_nr);
> > >
> > > Looks like you can move the decrement inside the lock.
> > > If you can do the same for the increment you can avoid the
> > > expensive locked bus cycle.
> > >
> >
> > atomic64_inc(&pool->pages_nr); is only done when init a new or reused z3fold_page.
> > There is no lock around. If we hold pool->lock there, this potential gain might be
> > nullified. Or am I miss something ?
>
> Atomic operations aren't magic.
> Atomic operations are (at best) one slow locked bus cycle.
> Acquiring a lock is the same.
> Releasing a lock might be cheaper, but is probably a locked bus cycle.
>
> So if you use state_lock to protect pages_nr then you lose an atomic
> operation for the decrement and gain one (for the unlock) in the increment.
> That is even or maybe a slight gain.
> OTOH a 64bit atomic is a PITA on some 32bit systems.
> (In fact any atomic is a PITA on sparc32.)

It's actually *stale_lock* and it's very misleading to use it for this.
I would actually like to keep atomics but I have no problem with
making it 32-bit for 32-bit systems. Would that work for you guys?

~Vitaly

> Actually does this even need to be 64bit, should it just be 'long'.
> That will mean that any 'read' just needs a simple single memory read.
>
> I've just looked at the code.
> Some of the one line wrapper functions don't make the code any
> easier to read.
> There is no point having inline wrappers to acquire locks if you
> only use them some of the time.
>
> David
>
>
> >
> > Many thanks for your review and reply.
> >
> > > David
> > >
> > >> }
> > >>
> > >> static void release_z3fold_page(struct kref *ref)
> > >> @@ -737,13 +739,9 @@ static struct z3fold_header *compact_single_buddy(struct z3fold_header *zhdr)
> > >> return new_zhdr;
> > >>
> > >> out_fail:
> > >> - if (new_zhdr) {
> > >> - if (kref_put(&new_zhdr->refcount, release_z3fold_page_locked))
> > >> - atomic64_dec(&pool->pages_nr);
> > >> - else {
> > >> - add_to_unbuddied(pool, new_zhdr);
> > >> - z3fold_page_unlock(new_zhdr);
> > >> - }
> > >> + if (new_zhdr && !kref_put(&new_zhdr->refcount, release_z3fold_page_locked)) {
> > >> + add_to_unbuddied(pool, new_zhdr);
> > >> + z3fold_page_unlock(new_zhdr);
> > >> }
> > >> return NULL;
> > >>
> > >> @@ -816,10 +814,8 @@ static void do_compact_page(struct z3fold_header *zhdr, bool locked)
> > >> list_del_init(&zhdr->buddy);
> > >> spin_unlock(&pool->lock);
> > >>
> > >> - if (kref_put(&zhdr->refcount, release_z3fold_page_locked)) {
> > >> - atomic64_dec(&pool->pages_nr);
> > >> + if (kref_put(&zhdr->refcount, release_z3fold_page_locked))
> > >> return;
> > >> - }
> > >>
> > >> if (test_bit(PAGE_STALE, &page->private) ||
> > >> test_and_set_bit(PAGE_CLAIMED, &page->private)) {
> > >> @@ -829,9 +825,7 @@ static void do_compact_page(struct z3fold_header *zhdr, bool locked)
> > >>
> > >> if (!zhdr->foreign_handles && buddy_single(zhdr) &&
> > >> zhdr->mapped_count == 0 && compact_single_buddy(zhdr)) {
> > >> - if (kref_put(&zhdr->refcount, release_z3fold_page_locked))
> > >> - atomic64_dec(&pool->pages_nr);
> > >> - else {
> > >> + if (!kref_put(&zhdr->refcount, release_z3fold_page_locked)) {
> > >> clear_bit(PAGE_CLAIMED, &page->private);
> > >> z3fold_page_unlock(zhdr);
> > >> }
> > >> @@ -1089,10 +1083,8 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> > >> if (zhdr) {
> > >> bud = get_free_buddy(zhdr, chunks);
> > >> if (bud == HEADLESS) {
> > >> - if (kref_put(&zhdr->refcount,
> > >> + if (!kref_put(&zhdr->refcount,
> > >> release_z3fold_page_locked))
> > >> - atomic64_dec(&pool->pages_nr);
> > >> - else
> > >> z3fold_page_unlock(zhdr);
> > >> pr_err("No free chunks in unbuddied\n");
> > >> WARN_ON(1);
> > >> @@ -1239,10 +1231,8 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
> > >>
> > >> if (!page_claimed)
> > >> free_handle(handle, zhdr);
> > >> - if (kref_put(&zhdr->refcount, release_z3fold_page_locked_list)) {
> > >> - atomic64_dec(&pool->pages_nr);
> > >> + if (kref_put(&zhdr->refcount, release_z3fold_page_locked_list))
> > >> return;
> > >> - }
> > >> if (page_claimed) {
> > >> /* the page has not been claimed by us */
> > >> put_z3fold_header(zhdr);
> > >> @@ -1353,9 +1343,7 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int
> > retries)
> > >> break;
> > >> }
> > >> if (!z3fold_page_trylock(zhdr)) {
> > >> - if (kref_put(&zhdr->refcount,
> > >> - release_z3fold_page))
> > >> - atomic64_dec(&pool->pages_nr);
> > >> + kref_put(&zhdr->refcount, release_z3fold_page);
> > >> zhdr = NULL;
> > >> continue; /* can't evict at this point */
> > >> }
> > >> @@ -1366,10 +1354,8 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int
> > retries)
> > >> */
> > >> if (zhdr->foreign_handles ||
> > >> test_and_set_bit(PAGE_CLAIMED, &page->private)) {
> > >> - if (kref_put(&zhdr->refcount,
> > >> + if (!kref_put(&zhdr->refcount,
> > >> release_z3fold_page_locked))
> > >> - atomic64_dec(&pool->pages_nr);
> > >> - else
> > >> z3fold_page_unlock(zhdr);
> > >> zhdr = NULL;
> > >> continue; /* can't evict such page */
> > >> @@ -1447,7 +1433,6 @@ static int z3fold_reclaim_page(struct z3fold_pool *pool, unsigned int
> > retries)
> > >> if (kref_put(&zhdr->refcount,
> > >> release_z3fold_page_locked)) {
> > >> kmem_cache_free(pool->c_handle, slots);
> > >> - atomic64_dec(&pool->pages_nr);
> > >> return 0;
> > >> }
> > >> /*
> > >> @@ -1669,10 +1654,8 @@ static void z3fold_page_putback(struct page *page)
> > >> if (!list_empty(&zhdr->buddy))
> > >> list_del_init(&zhdr->buddy);
> > >> INIT_LIST_HEAD(&page->lru);
> > >> - if (kref_put(&zhdr->refcount, release_z3fold_page_locked)) {
> > >> - atomic64_dec(&pool->pages_nr);
> > >> + if (kref_put(&zhdr->refcount, release_z3fold_page_locked))
> > >> return;
> > >> - }
> > >> spin_lock(&pool->lock);
> > >> list_add(&page->lru, &pool->lru);
> > >> spin_unlock(&pool->lock);
> > >> --
> > >> 2.23.0
> > >
> > > -
> > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > > Registration No: 1397386 (Wales)
> > >
> > > .
> > >
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

Next message: Yin Fengwei: "Re: [xfs] 6191cf3ad5: stress-ng.rename.ops_per_sec -73.5% regression"
Previous message: Michael S. Tsirkin: "Re: propagating vmgenid outward and upward"
Next in thread: David Laight: "RE: [PATCH 6/9] mm/z3fold: move decrement of pool->pages_nr into __release_z3fold_page()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]