lkml.org 
[lkml]   [2020]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 3/3] mm, page_alloc: reduce static keys in prep_new_page()
Date
On 10/27/20 12:05 PM, Vlastimil Babka wrote:
> On 10/27/20 10:10 AM, David Hildenbrand wrote:
>> On 26.10.20 18:33, Vlastimil Babka wrote:
>>> prep_new_page() will always zero a new page (regardless of __GFP_ZERO) when
>>> init_on_alloc is enabled, but will also always skip zeroing if the page was
>>> already zeroed on free by init_on_free or page poisoning.
>>>
>>> The latter check implemented by free_pages_prezeroed() can involve two
>>> different static keys. As prep_new_page() is really a hot path, let's introduce
>>> a single static key free_pages_not_prezeroed for this purpose and initialize it
>>> in init_mem_debugging().
>>
>> Is this actually observable in practice? This smells like
>> micro-optimization to me.
>>
>> Also, I thought the whole reason for static keys is to have basically no
>> overhead at runtime, so I wonder if replacing two static key checks by a
>> single one actually makes *some* difference.
>
> You're right, the difference seems to be just a single NOP. The static key
> infrastructure seems to be working really well.
> (At least the asm inspection made me realize that kernel_poison_pages() is
> called unconditionally and the static key is checked inside, not inline so I'll
> be amending patch 2...)
>
> Initially I thought I would be reducing 3 keys to 1 in this patch, but I got the
> code wrong. So unless others think it's a readability improvements, we can drop
> this patch.
>
> Or we can also reconsider this whole optimization. If the point is to be
> paranoid and enable both init_on_free and init_on_alloc, should we trust that
> nobody wrote something after the clearing on free via use-after-free? :) Kees/Alex?

More thoughts...

PAGE_POISONING_NO_SANITY skips the check on "unpoisoning" whether poison was
corrupted
PAGE_POISONING_ZERO uses zero instead of 0xAA as poison pattern

the point of enabling both of these seems to be moot now that init_on_free
exists, as that zeroes pages that are being freed, without checking on alloc
that they are still zeroed.

What if only one is enabled?
- PAGE_POISONING_NO_SANITY without PAGE_POISONING_ZERO - we poison with the 0xAA
pattern but nobody checks it, so does it give us anything over init_on_free
writing zeroes? I don't think so?

- PAGE_POISONING_ZERO without PAGE_POISONING_NO_SANITY - we use zeroes (like
init_on_free) but also check that it wasn't corrupted. We save some time on
writing zeroes again on alloc, but the check is still expensive. And writing
0xAA would possibly detect more corruptions than writing zero (a stray write of
NULL is more likely to happen than of 0xAA?).

So my conclusion:
- We can remove PAGE_POISONING_NO_SANITY because it only makes sense with
PAGE_POISONING_ZERO, and we can use init_on_free instead

- We can also probably remove PAGE_POISONING_ZERO, because if we want to do the
unpoisoning sanity check, then we also most likely want the 0xAA pattern and not
zero.

Thoughts?

\
 
 \ /
  Last update: 2020-10-27 14:32    [W:0.087 / U:1.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site