linux-kernel - Re: [PATCH 3/3] memory hotplug: use unified logic for is_removable and offline

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: <20100907131612.GA23617@tiehlicka.suse.cz>
Date:	Tue, 7 Sep 2010 15:16:12 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	Hiroyuki Kamezawa <kamezawa.hiroyuki@...il.com>
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

On Mon 06-09-10 22:30:43, Hiroyuki Kamezawa wrote:
> 2010/9/6 Michal Hocko <mhocko@...e.cz>:
> > On Mon 06-09-10 14:47:16, KAMEZAWA Hiroyuki wrote:
[...]
> >> 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().

Sounds good as well.

[...]
> >> +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. */

Sure, but I think that if you explicitely mention that the lock is not
held intentionaly then it would be good to provide some reasonining.

> 
> 
> > [...]
> >> ? ? ? /* 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.

It is not, but then it should be defined under CONFIG_HOTREMOVE without
#else part, shoudln't it?

> 
> Thanks,
> -Kame

Thanks!
-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic
--
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/