| 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: <AANLkTikOi6BqXs2wiLetFP9OgYtXD+vbC+Ez8a7z0dcU@mail.gmail.com>
Date: Mon, 6 Sep 2010 22:30:43 +0900
From: Hiroyuki Kamezawa <kamezawa.hiroyuki@...il.com>
To: Michal Hocko <mhocko@...e.cz>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
"linux-mm@...ck.org" <linux-mm@...ck.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
fengguang.wu@...el.com,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
Mel Gorman <mel@....ul.ie>, andi.kleen@...el.com,
Dave Hansen <dave@...ux.vnet.ibm.com>
Subject: Re: [PATCH 3/3] memory hotplug: use unified logic for is_removable
and offline_pages
2010/9/6 Michal Hocko <mhocko@...e.cz>:
> On Mon 06-09-10 14:47:16, KAMEZAWA Hiroyuki wrote:
>>
>> Now, sysfs interface of memory hotplug shows whether the section is
>> removable or not. But it checks only migrateype of pages and doesn't
>> check details of cluster of pages.
>>
>> Next, memory hotplug's set_migratetype_isolate() has the same kind
>> of check, too. But the migrate-type is just a "hint" and the pageblock
>> can contain several types of pages if fragmentation is very heavy.
>>
>> To get precise information, we need to check
>> - the pageblock only contains free pages or LRU pages.
>>
>> This patch adds the function __count_unmovable_pages() and makes
>> above 2 checks to use the same logic. This will improve user experience
>> of memory hotplug because sysfs interface tells accurate information.
>>
>> Note:
>> it may be better to check MIGRATE_UNMOVABLE for making failure case quick.
>>
>> Changelog: 2010/09/06
>> - added comments.
>> - removed zone->lock.
>> - changed the name of the function to be is_pageblock_removable_async().
>> because I removed the zone->lock.
>
> wouldn't be __is_pageblock_removable a better name? _async suffix is
> usually used for asynchronous operations and this is just a function
> withtout locks.
>
rename as _is_pagebloc_removable_nolock().
>>
>> Reported-by: Michal Hocko <mhocko@...e.cz>
>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
>> ---
>> include/linux/memory_hotplug.h | 1
>> mm/memory_hotplug.c | 15 -------
>> mm/page_alloc.c | 77 ++++++++++++++++++++++++++++++-----------
>> 3 files changed, 60 insertions(+), 33 deletions(-)
>>
>> Index: kametest/mm/page_alloc.c
>> ===================================================================
>> --- kametest.orig/mm/page_alloc.c
>> +++ kametest/mm/page_alloc.c
>> @@ -5274,11 +5274,61 @@ void set_pageblock_flags_group(struct pa
>> * page allocater never alloc memory from ISOLATE block.
>> */
>>
>
> Can we add a comment on the locking? Something like:
> Caller should hold zone->lock if he needs consistent results.
>
Hmm. ok.
>> +static int __count_immobile_pages(struct zone *zone, struct page *page)
>> +{
>> + unsigned long pfn, iter, found;
>> + /*
>> + * For avoiding noise data, lru_add_drain_all() should be called
>> + * If ZONE_MOVABLE, the zone never contains immobile pages
>> + */
>> + if (zone_idx(zone) == ZONE_MOVABLE)
>> + return 0;
>> +
>> + pfn = page_to_pfn(page);
>> + for (found = 0, iter = 0; iter < pageblock_nr_pages; iter++) {
>> + unsigned long check = pfn + iter;
>> +
>> + if (!pfn_valid_within(check)) {
>> + iter++;
>> + continue;
>> + }
>> + page = pfn_to_page(check);
>> + if (!page_count(page)) {
>> + if (PageBuddy(page))
>> + iter += (1 << page_order(page)) - 1;
>> + continue;
>> + }
>> + if (!PageLRU(page))
>> + found++;
>> + /*
>> + * If the page is not RAM, page_count()should be 0.
>> + * we don't need more check. This is an _used_ not-movable page.
>> + *
>> + * The problematic thing here is PG_reserved pages. PG_reserved
>> + * is set to both of a memory hole page and a _used_ kernel
>> + * page at boot.
>> + */
>> + }
>> + return found;
>> +}
>> +
>> +bool is_pageblock_removable_async(struct page *page)
>> +{
>> + struct zone *zone = page_zone(page);
>> + unsigned long flags;
>> + int num;
>> + /* Don't take zone->lock interntionally. */
>
> Could you add the reason?
> Don't take zone-> lock intentionally because we are called from the
> userspace (sysfs interface).
>
I don't like to assume caller context which will limit the callers.
/* holding zone->lock or not is caller's job. */
> [...]
>> /* All pageblocks in the memory block are likely to be hot-removable */
>> Index: kametest/include/linux/memory_hotplug.h
>> ===================================================================
>> --- kametest.orig/include/linux/memory_hotplug.h
>> +++ kametest/include/linux/memory_hotplug.h
>> @@ -69,6 +69,7 @@ extern void online_page(struct page *pag
>> /* VM interface that may be used by firmware interface */
>> extern int online_pages(unsigned long, unsigned long);
>> extern void __offline_isolated_pages(unsigned long, unsigned long);
>
> #ifdef CONFIG_HOTREMOVE
>
>> +extern bool is_pageblock_removable_async(struct page *page);
>
> #else
> #define is_pageblock_removable_async(p) 0
> #endif
> ?
Is this function is called even if HOTREMOVE is off ?
If so, the caller is buggy. I'll check tomorrow.
Thanks,
-Kame
--
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/
Powered by blists - more mailing lists