Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 21 Jun 2023 22:17:16 +0900
From:   Sergey Senozhatsky <senozhatsky@chromium.org>
To:     Alexey Romanov <AVRomanov@sberdevices.ru>,
        Minchan Kim <minchan@kernel.org>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        kernel <kernel@sberdevices.ru>
Subject: Re: [PATCH v1 1/2] zsmalloc: add allocated objects counter for
 subpage
Message-ID: <20230621131716.GC2934656@google.com>
References: <20230619143506.45253-1-avromanov@sberdevices.ru>
 <20230619143506.45253-2-avromanov@sberdevices.ru>
 <20230620103629.GA42985@google.com>
 <20230620111635.gztldehfzvuzkdnj@cab-wsm-0029881>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230620111635.gztldehfzvuzkdnj@cab-wsm-0029881>
Precedence: bulk

On (23/06/20 11:16), Alexey Romanov wrote:
> If sizeof(unsigned int) >= 32 bits the this will be enough for us. 
> Of course, in rare cases this will not be the case. But it seems that
> zram and kernel already has similiar places. For example, if page size
> is 256 Kb and sizeof(unsigned int) = 16 bits (2 byte), zram will not
> wotk on such system, because we can't store offset. But such case is
> very rare, most systems have unsigned int over 32 bits. 
> 
> Therefore, I think that my idea is still applicable, we just need to
> change the counter type. What do you think?

My gut feeling is that we better avoid mixing in architecture specific
magic into generic code. It works fine until it doesn't. May be Minchan
will have a different opinion tho.

There can be other ways to avoid linear scan of empty sub-pages. For
instance, something like below probably can cover less cases than your
patch 0002, but on the other hand is rather generic, trivial and doesn't
contain any assumptions on the architecture specifics.

(composed/edited in mail client, so likely is broken, but outlines
 the idea)

====================================================================

mm/zsmalloc: do not scan empty zspages

We already stop zspage migration when we detect that target
zspage has no space left for any new objects. There is
one more thing we can do in order to avoid doing useless
work: stop scanning for allocated objects in sub-pages when
we have migrated the last inuse object from the zspage in
question.

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 02f7f414aade..2875152e6497 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1263,6 +1263,11 @@ static bool zspage_full(struct size_class *class, struct zspage *zspage)
        return get_zspage_inuse(zspage) == class->objs_per_zspage;
 }
 
+static bool zspage_empty(struct zspage *zspage)
+{
+	return get_zspage_inuse(zspage) == 0;
+}
+
 /**
  * zs_lookup_class_index() - Returns index of the zsmalloc &size_class
  * that hold objects of the provided size.
@@ -1787,6 +1792,10 @@ static void migrate_zspage(struct zs_pool *pool, struct size_class *class,
 		obj_idx++;
 		record_obj(handle, free_obj);
 		obj_free(class->size, used_obj, NULL);
+
+		/* Stop if there are no more objects to migrate */
+		if (zspage_empty(get_zspage(s_page)))
+			break;
 	}