linux-kernel - Re: [PATCH v3] mm/page_alloc: add a helper function to check page before alloc/free

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 9 Sep 2015 15:39:54 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Yaowei Bai <bywxiaobai@....com>, akpm@...ux-foundation.org,
	mgorman@...e.de, mhocko@...nel.org, js1304@...il.com,
	hannes@...xchg.org, alexander.h.duyck@...hat.com,
	sasha.levin@...cle.com
Cc:	linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] mm/page_alloc: add a helper function to check page
 before alloc/free

On 09/08/2015 09:19 PM, Vlastimil Babka wrote:
> bloat-o-meter looks favorably with my gcc, although there shouldn't be a real
> reason for it, as the inlining didn't change:
>
> add/remove: 1/1 grow/shrink: 1/1 up/down: 285/-336 (-51)
> function                                     old     new   delta
> bad_page                                       -     276    +276
> get_page_from_freelist                      2521    2530      +9
> free_pages_prepare                           745     667     -78
> bad_page.part                                258       -    -258
>
> With that,
>
> Acked-by: Vlastimil Babka <vbabka@...e.cz>

BTW, why do we do all these checks in non-DEBUG_VM builds? Are they so 
often hit nowadays? Shouldn't we check just for hwpoison in the 
non-debug case?

Alternatively, I've considered creating a fast inline pre-check that 
calls a non-inline check-with-report:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0c9c82a..cff92f8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -707,7 +707,20 @@ static inline void __free_one_page(struct page *page,
  	zone->free_area[order].nr_free++;
  }

-static inline int check_one_page(struct page *page, unsigned long 
bad_flags)
+static inline int check_one_page_fast(struct page *page, unsigned long
+		bad_flags)
+{
+	return (page_mapcount(page)
+			|| page->mapping != NULL
+			|| atomic_read(&page->_count) != 0
+			|| page->flags & bad_flags
+#ifdef CONFIG_MEMCG
+			|| page->mem_cgroup
+#endif
+			);
+}
+
+static noinline int check_one_page(struct page *page, unsigned long 
bad_flags)
  {
  	const char *bad_reason = NULL;

@@ -743,9 +756,12 @@ static inline int free_pages_check(struct page *page)
  {
  	int ret = 0;

-	ret = check_one_page(page, PAGE_FLAGS_CHECK_AT_FREE);
-	if (ret)
-		return ret;
+	ret = check_one_page_fast(page, PAGE_FLAGS_CHECK_AT_FREE);
+	if (ret) {
+		ret = check_one_page(page, PAGE_FLAGS_CHECK_AT_FREE);
+		if (ret)
+			return ret;
+	}

  	page_cpupid_reset_last(page);
  	if (page->flags & PAGE_FLAGS_CHECK_AT_PREP)
@@ -1304,7 +1320,9 @@ static inline void expand(struct zone *zone, 
struct page *page,
   */
  static inline int check_new_page(struct page *page)
  {
-	return check_one_page(page, PAGE_FLAGS_CHECK_AT_PREP);
+	if (check_one_page_fast(page, PAGE_FLAGS_CHECK_AT_PREP | __PG_HWPOISON))
+		return check_one_page(page, PAGE_FLAGS_CHECK_AT_PREP);
+	return 0;
  }

  static int prep_new_page(struct page *page, unsigned int order, gfp_t 
gfp_flags,

---

That shrinks the fast paths nicely:

add/remove: 1/1 grow/shrink: 0/2 up/down: 480/-498 (-18)
function                                     old     new   delta
check_one_page                                 -     480    +480
get_page_from_freelist                      2530    2458     -72
free_pages_prepare                           667     517    -150
bad_page                                     276       -    -276

On top of that, the number of branches in the fast paths can be reduced 
if we use arithmetic OR to avoid the short-circuit boolean evaluation:

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index cff92f8..e8b42ba 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -710,12 +710,12 @@ static inline void __free_one_page(struct page *page,
  static inline int check_one_page_fast(struct page *page, unsigned long
  		bad_flags)
  {
-	return (page_mapcount(page)
-			|| page->mapping != NULL
-			|| atomic_read(&page->_count) != 0
-			|| page->flags & bad_flags
+	return ((unsigned long) page_mapcount(page)
+			| (unsigned long) page->mapping
+			| (unsigned long) atomic_read(&page->_count)
+			| (page->flags & bad_flags)
  #ifdef CONFIG_MEMCG
-			|| page->mem_cgroup
+			| (unsigned long) page->mem_cgroup
  #endif
  			);
  }

That further reduces the fast paths, not much in bytes, but importantly 
in branches:

add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-51 (-51)
function                                     old     new   delta
get_page_from_freelist                      2458    2443     -15
free_pages_prepare                           517     481     -36

But I can understand it's rather hackish, and maybe some architectures 
won't be happy with the extra unsigned long arithmetics. Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/