Hi,
This series contains a few patches to simplify the code, remove unneeded
return value, fix obsolete comment and so on. More details can be found
in the respective changelogs. Thanks!
Miaohe Lin (9):
mm/z3fold: declare z3fold_mount with __init
mm/z3fold: remove obsolete comment in z3fold_alloc
mm/z3fold: minor clean up for z3fold_free
mm/z3fold: remove unneeded page_mapcount_reset and ClearPagePrivate
mm/z3fold: remove confusing local variable l reassignment
mm/z3fold: move decrement of pool->pages_nr into
__release_z3fold_page()
mm/z3fold: remove redundant list_del_init of zhdr->buddy in
z3fold_free
mm/z3fold: remove unneeded PAGE_HEADLESS check in free_handle()
mm/z3fold: remove unneeded return value of z3fold_compact_page()
mm/z3fold.c | 78 +++++++++++++++--------------------------------------
1 file changed, 22 insertions(+), 56 deletions(-)
--
2.23.0
The highmem pages are supported since commit f1549cb5ab2b ("mm/z3fold.c:
allow __GFP_HIGHMEM in z3fold_alloc"). Remove the residual comment.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/z3fold.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index e86aafea6599..87689f50f709 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -1064,9 +1064,6 @@ static void z3fold_destroy_pool(struct z3fold_pool *pool)
* performed first. If no suitable free region is found, then a new page is
* allocated and added to the pool to satisfy the request.
*
- * gfp should not set __GFP_HIGHMEM as highmem pages cannot be used
- * as z3fold pool pages.
- *
* Return: 0 if success and handle is set, otherwise -EINVAL if the size or
* gfp arguments are invalid or -ENOMEM if the pool was unable to allocate
* a new page.
--
2.23.0
Use put_z3fold_header() to pair with get_z3fold_header. Also fix the wrong
comments. Minor readability improvement.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/z3fold.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index 87689f50f709..eb89271aea83 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -1187,9 +1187,9 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
* @handle: handle associated with the allocation returned by z3fold_alloc()
*
* In the case that the z3fold page in which the allocation resides is under
- * reclaim, as indicated by the PG_reclaim flag being set, this function
- * only sets the first|last_chunks to 0. The page is actually freed
- * once both buddies are evicted (see z3fold_reclaim_page() below).
+ * reclaim, as indicated by the PAGE_CLAIMED flag being set, this function
+ * only sets the first|middle|last_chunks to 0. The page is actually freed
+ * once all buddies are evicted (see z3fold_reclaim_page() below).
*/
static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
{
@@ -1247,7 +1247,7 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
}
if (page_claimed) {
/* the page has not been claimed by us */
- z3fold_page_unlock(zhdr);
+ put_z3fold_header(zhdr);
return;
}
if (test_and_set_bit(NEEDS_COMPACTING, &page->private)) {
--
2.23.0
Page->page_type and PagePrivate are not used in z3fold. We should remove
these confusing unneeded operations. The z3fold do these here is due to
referring to zsmalloc's migration code which does need these operations.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/z3fold.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index eb89271aea83..2f848ea45b4d 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -420,7 +420,6 @@ static void free_z3fold_page(struct page *page, bool headless)
__ClearPageMovable(page);
unlock_page(page);
}
- ClearPagePrivate(page);
__free_page(page);
}
@@ -1635,7 +1634,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
INIT_LIST_HEAD(&new_zhdr->buddy);
new_mapping = page_mapping(page);
__ClearPageMovable(page);
- ClearPagePrivate(page);
get_page(newpage);
z3fold_page_lock(new_zhdr);
@@ -1655,7 +1653,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
queue_work_on(new_zhdr->cpu, pool->compact_wq, &new_zhdr->work);
- page_mapcount_reset(page);
clear_bit(PAGE_CLAIMED, &page->private);
put_page(page);
return 0;
--
2.23.0
The only caller z3fold_free() never calls free_handle() in PAGE_HEADLESS
case. Remove this unneeded check.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/z3fold.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index 867c590df027..83b5a3514427 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -297,9 +297,6 @@ static inline void free_handle(unsigned long handle, struct z3fold_header *zhdr)
int i;
bool is_free;
- if (handle & (1 << PAGE_HEADLESS))
- return;
-
if (WARN_ON(*(unsigned long *)handle == 0))
return;
--
2.23.0
The local variable l holds the address of unbuddied[i] which won't change
after we take the pool lock. Remove it to avoid confusion.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/z3fold.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index 2f848ea45b4d..adc0b3fa4906 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -876,7 +876,6 @@ static inline struct z3fold_header *__z3fold_alloc(struct z3fold_pool *pool,
/* Re-check under lock. */
spin_lock(&pool->lock);
- l = &unbuddied[i];
if (unlikely(zhdr != list_first_entry(READ_ONCE(l),
struct z3fold_header, buddy)) ||
!z3fold_page_trylock(zhdr)) {
--
2.23.0
The do_compact_page will do list_del_init(&zhdr->buddy) for us. Remove this
extra one to save some possible cpu cycles.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/z3fold.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index 18a697f6fe32..867c590df027 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -1244,9 +1244,6 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
return;
}
if (zhdr->cpu < 0 || !cpu_online(zhdr->cpu)) {
- spin_lock(&pool->lock);
- list_del_init(&zhdr->buddy);
- spin_unlock(&pool->lock);
zhdr->cpu = -1;
kref_get(&zhdr->refcount);
clear_bit(PAGE_CLAIMED, &page->private);
--
2.23.0
z3fold_mount is only called during init. So we should declare it
with __init.
Signed-off-by: Miaohe Lin <[email protected]>
---
mm/z3fold.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/z3fold.c b/mm/z3fold.c
index b3c0577b8095..e86aafea6599 100644
--- a/mm/z3fold.c
+++ b/mm/z3fold.c
@@ -345,7 +345,7 @@ static struct file_system_type z3fold_fs = {
};
static struct vfsmount *z3fold_mnt;
-static int z3fold_mount(void)
+static int __init z3fold_mount(void)
{
int ret = 0;
--
2.23.0
On 2022/2/19 17:25, Miaohe Lin wrote:
> Hi,
> This series contains a few patches to simplify the code, remove unneeded
> return value, fix obsolete comment and so on. More details can be found
> in the respective changelogs. Thanks!
>
Friendly ping in case this is forgotten. :)
> Miaohe Lin (9):
> mm/z3fold: declare z3fold_mount with __init
> mm/z3fold: remove obsolete comment in z3fold_alloc
> mm/z3fold: minor clean up for z3fold_free
> mm/z3fold: remove unneeded page_mapcount_reset and ClearPagePrivate
> mm/z3fold: remove confusing local variable l reassignment
> mm/z3fold: move decrement of pool->pages_nr into
> __release_z3fold_page()
> mm/z3fold: remove redundant list_del_init of zhdr->buddy in
> z3fold_free
> mm/z3fold: remove unneeded PAGE_HEADLESS check in free_handle()
> mm/z3fold: remove unneeded return value of z3fold_compact_page()
>
> mm/z3fold.c | 78 +++++++++++++++--------------------------------------
> 1 file changed, 22 insertions(+), 56 deletions(-)
>
On Tue, 1 Mar 2022 21:03:37 +0800 Miaohe Lin <[email protected]> wrote:
> On 2022/2/19 17:25, Miaohe Lin wrote:
> > Hi,
> > This series contains a few patches to simplify the code, remove unneeded
> > return value, fix obsolete comment and so on. More details can be found
> > in the respective changelogs. Thanks!
> >
>
> Friendly ping in case this is forgotten. :)
I was expecting updates resulting from the [6/9] discussion?
On 2022/3/2 1:36, Andrew Morton wrote:
> On Tue, 1 Mar 2022 21:03:37 +0800 Miaohe Lin <[email protected]> wrote:
>
>> On 2022/2/19 17:25, Miaohe Lin wrote:
>>> Hi,
>>> This series contains a few patches to simplify the code, remove unneeded
>>> return value, fix obsolete comment and so on. More details can be found
>>> in the respective changelogs. Thanks!
>>>
>>
>> Friendly ping in case this is forgotten. :)
>
> I was expecting updates resulting from the [6/9] discussion?
I think that could be another separate patch to reduce the atomic operation overhead.
And I have some z3fold bugfix patches pending to sent. They might can be sent together
if such separate patch is prepared at that point.
Many thanks. :)
> .
>
On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <[email protected]> wrote:
>
> Use put_z3fold_header() to pair with get_z3fold_header. Also fix the wrong
> comments. Minor readability improvement.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Vitaly Wool <[email protected]>
> ---
> mm/z3fold.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 87689f50f709..eb89271aea83 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -1187,9 +1187,9 @@ static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> * @handle: handle associated with the allocation returned by z3fold_alloc()
> *
> * In the case that the z3fold page in which the allocation resides is under
> - * reclaim, as indicated by the PG_reclaim flag being set, this function
> - * only sets the first|last_chunks to 0. The page is actually freed
> - * once both buddies are evicted (see z3fold_reclaim_page() below).
> + * reclaim, as indicated by the PAGE_CLAIMED flag being set, this function
> + * only sets the first|middle|last_chunks to 0. The page is actually freed
> + * once all buddies are evicted (see z3fold_reclaim_page() below).
> */
> static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
> {
> @@ -1247,7 +1247,7 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
> }
> if (page_claimed) {
> /* the page has not been claimed by us */
> - z3fold_page_unlock(zhdr);
> + put_z3fold_header(zhdr);
> return;
> }
> if (test_and_set_bit(NEEDS_COMPACTING, &page->private)) {
> --
> 2.23.0
>
On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <[email protected]> wrote:
>
> The local variable l holds the address of unbuddied[i] which won't change
> after we take the pool lock. Remove it to avoid confusion.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Vitaly Wool <[email protected]>
> ---
> mm/z3fold.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 2f848ea45b4d..adc0b3fa4906 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -876,7 +876,6 @@ static inline struct z3fold_header *__z3fold_alloc(struct z3fold_pool *pool,
>
> /* Re-check under lock. */
> spin_lock(&pool->lock);
> - l = &unbuddied[i];
> if (unlikely(zhdr != list_first_entry(READ_ONCE(l),
> struct z3fold_header, buddy)) ||
> !z3fold_page_trylock(zhdr)) {
> --
> 2.23.0
>
On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <[email protected]> wrote:
>
> The highmem pages are supported since commit f1549cb5ab2b ("mm/z3fold.c:
> allow __GFP_HIGHMEM in z3fold_alloc"). Remove the residual comment.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Vitaly Wool <[email protected]>
> ---
> mm/z3fold.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index e86aafea6599..87689f50f709 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -1064,9 +1064,6 @@ static void z3fold_destroy_pool(struct z3fold_pool *pool)
> * performed first. If no suitable free region is found, then a new page is
> * allocated and added to the pool to satisfy the request.
> *
> - * gfp should not set __GFP_HIGHMEM as highmem pages cannot be used
> - * as z3fold pool pages.
> - *
> * Return: 0 if success and handle is set, otherwise -EINVAL if the size or
> * gfp arguments are invalid or -ENOMEM if the pool was unable to allocate
> * a new page.
> --
> 2.23.0
>
On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <[email protected]> wrote:
>
> z3fold_mount is only called during init. So we should declare it
> with __init.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Vitaly Wool <[email protected]>
> ---
> mm/z3fold.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index b3c0577b8095..e86aafea6599 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -345,7 +345,7 @@ static struct file_system_type z3fold_fs = {
> };
>
> static struct vfsmount *z3fold_mnt;
> -static int z3fold_mount(void)
> +static int __init z3fold_mount(void)
> {
> int ret = 0;
>
> --
> 2.23.0
>
On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <[email protected]> wrote:
>
> Page->page_type and PagePrivate are not used in z3fold. We should remove
> these confusing unneeded operations. The z3fold do these here is due to
> referring to zsmalloc's migration code which does need these operations.
Absolutely, thanks for pointing this out.
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Vitaly Wool <[email protected]>
> ---
> mm/z3fold.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index eb89271aea83..2f848ea45b4d 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -420,7 +420,6 @@ static void free_z3fold_page(struct page *page, bool headless)
> __ClearPageMovable(page);
> unlock_page(page);
> }
> - ClearPagePrivate(page);
> __free_page(page);
> }
>
> @@ -1635,7 +1634,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
> INIT_LIST_HEAD(&new_zhdr->buddy);
> new_mapping = page_mapping(page);
> __ClearPageMovable(page);
> - ClearPagePrivate(page);
>
> get_page(newpage);
> z3fold_page_lock(new_zhdr);
> @@ -1655,7 +1653,6 @@ static int z3fold_page_migrate(struct address_space *mapping, struct page *newpa
>
> queue_work_on(new_zhdr->cpu, pool->compact_wq, &new_zhdr->work);
>
> - page_mapcount_reset(page);
> clear_bit(PAGE_CLAIMED, &page->private);
> put_page(page);
> return 0;
> --
> 2.23.0
>
On Sat, Feb 19, 2022 at 10:26 AM Miaohe Lin <[email protected]> wrote:
>
> The do_compact_page will do list_del_init(&zhdr->buddy) for us. Remove this
> extra one to save some possible cpu cycles.
>
> Signed-off-by: Miaohe Lin <[email protected]>
Reviewed-by: Vitaly Wool <[email protected]>
> ---
> mm/z3fold.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/mm/z3fold.c b/mm/z3fold.c
> index 18a697f6fe32..867c590df027 100644
> --- a/mm/z3fold.c
> +++ b/mm/z3fold.c
> @@ -1244,9 +1244,6 @@ static void z3fold_free(struct z3fold_pool *pool, unsigned long handle)
> return;
> }
> if (zhdr->cpu < 0 || !cpu_online(zhdr->cpu)) {
> - spin_lock(&pool->lock);
> - list_del_init(&zhdr->buddy);
> - spin_unlock(&pool->lock);
> zhdr->cpu = -1;
> kref_get(&zhdr->refcount);
> clear_bit(PAGE_CLAIMED, &page->private);
> --
> 2.23.0
>