lkml.org 
[lkml]   [2010]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@suse.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


\
 
 \ /
  Last update: 2010-09-07 15:19    [W:0.057 / U:19.532 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site