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: <56BB3E61.50707@suse.cz>
Date:	Wed, 10 Feb 2016 14:42:57 +0100
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Joonsoo Kim <js1304@...il.com>, Aaron Lu <aaron.lu@...el.com>,
	Mel Gorman <mgorman@...e.de>, Rik van Riel <riel@...hat.com>,
	David Rientjes <rientjes@...gle.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux Memory Management List <linux-mm@...ck.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH v2 3/3] mm/compaction: speed up pageblock_pfn_to_page()
 when zone is contiguous

On 02/09/2016 09:53 PM, Andrew Morton wrote:
> On Tue, 9 Feb 2016 18:58:32 +0100 Vlastimil Babka <vbabka@...e.cz> wrote:
> 
>> On 02/05/2016 05:11 PM, Joonsoo Kim wrote:
>>> Yeah, it seems wrong to me. :)
>>> Here goes fix.
>>
>> Doesn't apply for me, even after fixing the most obvious line wraps.
>> Seems like the version in mmotm is still your original patch and
>> Andrew's hotfix?
> 
> Yes, that patch was hopelessly mailer-mangled.  I painstakingly fixed
> it up and generated the incremental:

Thanks a lot. My review of the final patch also involved pain (due to
the cold, not the patch!).

You can take my Acked-by, but I also find the definitions of
set_zone_contiguous/clear_zone_contiguous() "in the header of the
consumer" (hotplug) somewhat unusual. It works, but e.g. mm/internal.h
would be more expected.

Then there's this:

> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -509,6 +509,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn,
>  	int start_sec, end_sec;
>  	struct vmem_altmap *altmap;
>  
> +	clear_zone_contiguous(zone);
> +
>  	/* during initialize mem_map, align hot-added range to section */
>  	start_sec = pfn_to_section_nr(phys_start_pfn);
>  	end_sec = pfn_to_section_nr(phys_start_pfn + nr_pages - 1);
> @@ -540,6 +542,8 @@ int __ref __add_pages(int nid, struct zone *zone, unsigned long phys_start_pfn,
>  	}
>  	vmemmap_populate_print_last();
>  
> +	set_zone_contiguous(zone);
> +
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(__add_pages);

Between the clear and set, __add_pages() might return with -EINVAL,
leaving the flag cleared potentially forever. Not critical, probably
rare, but it should be possible to avoid this by moving the clear below
the altmap check?

Thanks,
Vlastimil

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ