| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20230214180324.71585-1-sj@kernel.org>
Date: Tue, 14 Feb 2023 18:03:24 +0000
From: SeongJae Park <sj@...nel.org>
To: Baolin Wang <baolin.wang@...ux.alibaba.com>
Cc: akpm@...ux-foundation.org, torvalds@...ux-foundation.org,
sj@...nel.org, hannes@...xchg.org, mhocko@...nel.org,
roman.gushchin@...ux.dev, shakeelb@...gle.com,
muchun.song@...ux.dev, naoya.horiguchi@....com,
linmiaohe@...wei.com, david@...hat.com, osalvador@...e.de,
mike.kravetz@...cle.com, willy@...radead.org,
damon@...ts.linux.dev, cgroups@...r.kernel.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] mm: hugetlb: change to return bool for isolate_hugetlb()
On Tue, 14 Feb 2023 21:59:31 +0800 Baolin Wang <baolin.wang@...ux.alibaba.com> wrote:
> Now the isolate_hugetlb() only returns 0 or -EBUSY, and most users did not
> care about the negative value, thus we can convert the isolate_hugetlb()
> to return a boolean value to make code more clear when checking the
> hugetlb isolation state. Moreover converts 2 users which will consider
> the negative value returned by isolate_hugetlb().
>
> No functional changes intended.
>
> Signed-off-by: Baolin Wang <baolin.wang@...ux.alibaba.com>
> ---
> include/linux/hugetlb.h | 6 +++---
> mm/hugetlb.c | 12 ++++++++----
> mm/memory-failure.c | 2 +-
> mm/mempolicy.c | 2 +-
> mm/migrate.c | 2 +-
> 5 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index df6dd624ccfe..5f5e4177b2e0 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -171,7 +171,7 @@ bool hugetlb_reserve_pages(struct inode *inode, long from, long to,
> vm_flags_t vm_flags);
> long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
> long freed);
> -int isolate_hugetlb(struct folio *folio, struct list_head *list);
> +bool isolate_hugetlb(struct folio *folio, struct list_head *list);
> int get_hwpoison_hugetlb_folio(struct folio *folio, bool *hugetlb, bool unpoison);
> int get_huge_page_for_hwpoison(unsigned long pfn, int flags,
> bool *migratable_cleared);
> @@ -413,9 +413,9 @@ static inline pte_t *huge_pte_offset(struct mm_struct *mm, unsigned long addr,
> return NULL;
> }
>
> -static inline int isolate_hugetlb(struct folio *folio, struct list_head *list)
> +static inline bool isolate_hugetlb(struct folio *folio, struct list_head *list)
> {
> - return -EBUSY;
> + return false;
> }
>
> static inline int get_hwpoison_hugetlb_folio(struct folio *folio, bool *hugetlb, bool unpoison)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 3a01a9dbf445..75097e3abc18 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -2932,6 +2932,10 @@ static int alloc_and_dissolve_hugetlb_folio(struct hstate *h,
> spin_unlock_irq(&hugetlb_lock);
> ret = isolate_hugetlb(old_folio, list);
> spin_lock_irq(&hugetlb_lock);
> + if (!ret)
> + ret = -EBUSY;
> + else
> + ret = 0;
This would work, but 'ret' is not 'bool' but 'int'. How about below?
ret = isolate_hugetlb(old_folio, list) ? 0 : -EBUSY;
> goto free_new;
> } else if (!folio_test_hugetlb_freed(old_folio)) {
> /*
> @@ -3005,7 +3009,7 @@ int isolate_or_dissolve_huge_page(struct page *page, struct list_head *list)
> if (hstate_is_gigantic(h))
> return -ENOMEM;
>
> - if (folio_ref_count(folio) && !isolate_hugetlb(folio, list))
> + if (folio_ref_count(folio) && isolate_hugetlb(folio, list))
> ret = 0;
> else if (!folio_ref_count(folio))
> ret = alloc_and_dissolve_hugetlb_folio(h, folio, list);
> @@ -7251,15 +7255,15 @@ __weak unsigned long hugetlb_mask_last_page(struct hstate *h)
> * These functions are overwritable if your architecture needs its own
> * behavior.
> */
> -int isolate_hugetlb(struct folio *folio, struct list_head *list)
> +bool isolate_hugetlb(struct folio *folio, struct list_head *list)
> {
> - int ret = 0;
> + bool ret = true;
>
> spin_lock_irq(&hugetlb_lock);
> if (!folio_test_hugetlb(folio) ||
> !folio_test_hugetlb_migratable(folio) ||
> !folio_try_get(folio)) {
> - ret = -EBUSY;
> + ret = false;
> goto unlock;
> }
> folio_clear_hugetlb_migratable(folio);
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index e504362fdb23..8604753bc644 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2508,7 +2508,7 @@ static bool isolate_page(struct page *page, struct list_head *pagelist)
> bool isolated = false;
>
> if (PageHuge(page)) {
> - isolated = !isolate_hugetlb(page_folio(page), pagelist);
> + isolated = isolate_hugetlb(page_folio(page), pagelist);
> } else {
> bool lru = !__PageMovable(page);
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 2751bc3310fd..a256a241fd1d 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -609,7 +609,7 @@ static int queue_folios_hugetlb(pte_t *pte, unsigned long hmask,
> if (flags & (MPOL_MF_MOVE_ALL) ||
> (flags & MPOL_MF_MOVE && folio_estimated_sharers(folio) == 1 &&
> !hugetlb_pmd_shared(pte))) {
> - if (isolate_hugetlb(folio, qp->pagelist) &&
> + if (!isolate_hugetlb(folio, qp->pagelist) &&
> (flags & MPOL_MF_STRICT))
> /*
> * Failed to isolate folio but allow migrating pages
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 53010a142e7f..c5136fa48638 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -2128,7 +2128,7 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
> if (PageHead(page)) {
> err = isolate_hugetlb(page_folio(page), pagelist);
> if (!err)
> - err = 1;
> + err = -EBUSY;
Again, I think this is confusing. 'err' is 'bool', not 'int'.
Thanks,
SJ
> }
> } else {
> struct page *head;
> --
> 2.27.0
>
Powered by blists - more mailing lists