linux-kernel - Re: [mm PATCH v2 4/6] mm: Do not set reserved flag for hotplug memory

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]

Message-ID: <0b7001f8-3653-d100-4218-3d9b06ff8b3a@linux.intel.com>
Date:   Thu, 11 Oct 2018 16:22:30 -0700
From:   Alexander Duyck <alexander.h.duyck@...ux.intel.com>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     Linux MM <linux-mm@...ck.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Pasha Tatashin <pavel.tatashin@...rosoft.com>,
        Michal Hocko <mhocko@...e.com>,
        Dave Jiang <dave.jiang@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Matthew Wilcox <willy@...radead.org>,
        David Miller <davem@...emloft.net>,
        Zhang Yi <yi.z.zhang@...ux.intel.com>, khalid.aziz@...cle.com,
        rppt@...ux.vnet.ibm.com, Vlastimil Babka <vbabka@...e.cz>,
        sparclinux@...r.kernel.org,
        Laurent Dufour <ldufour@...ux.vnet.ibm.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Ingo Molnar <mingo@...nel.org>,
        "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [mm PATCH v2 4/6] mm: Do not set reserved flag for hotplug memory

On 10/11/2018 3:58 PM, Dan Williams wrote:
> On Thu, Oct 11, 2018 at 3:18 PM Alexander Duyck
> <alexander.h.duyck@...ux.intel.com> wrote:
>>
>> The general suspicion at this point is that the setting of the reserved bit
>> is not really needed for hotplug memory. In addition the setting of this
>> bit results in issues for DAX in that it is not possible to assign the
>> region to KVM if the reserved bit is set in each page.
>>
>> For now we can try just not setting the bit since we suspect it isn't
>> adding value in setting it. If at a later time we find that it is needed we
>> can come back through and re-add it for the hotplug paths.
>>
>> Suggested-by: Michael Hocko <mhocko@...e.com>
>> Reported-by: Dan Williams <dan.j.williams@...el.com>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@...ux.intel.com>
>> ---
>>   mm/page_alloc.c |   11 -----------
>>   1 file changed, 11 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 3603d5444865..e435223e2ddb 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -5571,8 +5571,6 @@ void __meminit memmap_init_zone(unsigned long size, int nid, unsigned long zone,
>>
>>                  page = pfn_to_page(pfn);
>>                  __init_single_page(page, pfn, zone, nid);
>> -               if (context == MEMMAP_HOTPLUG)
>> -                       __SetPageReserved(page);
> 
> At a minimum I think we need to do this before removing PageReserved,
> to make sure zone_device pages are not tracked in the hibernation
> image.
> 
> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
> index 3d37c279c090..c0613137d726 100644
> --- a/kernel/power/snapshot.c
> +++ b/kernel/power/snapshot.c
> @@ -1285,6 +1285,9 @@ static struct page *saveable_page(struct zone
> *zone, unsigned long pfn)
>          if (swsusp_page_is_forbidden(page) || swsusp_page_is_free(page))
>                  return NULL;
> 
> +       if (is_zone_device_page(page))
> +               return NULL;
> +
>          if (PageReserved(page)
>              && (!kernel_page_present(page) || pfn_is_nosave(pfn)))
>                  return NULL;
> 

Yeah, I am pretty sure I broke hotplug in general with this too since it 
seems like it checks for the reserved flag before bringing a range 
online in online_pages_range.

I think I will drop this patch and go back to what I had before. There 
was a slight performance gain to be had for not setting the bit at all, 
but I think we are probably looking at yet another patch set if we want 
to go through and drop the need for the reserved bit to be set.

- Alex