Re: [PATCH 2/2] Make is_mem_section_removable more conformable with offlining code

[Michal Hocko <mhocko@suse.cz>]

On Fri 03-09-10 18:13:27, KAMEZAWA Hiroyuki wrote:
> On Fri, 3 Sep 2010 10:25:58 +0200
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Fri 03-09-10 12:14:52, KAMEZAWA Hiroyuki wrote:
> > [...]
[...]
> > Cannot ZONE_MOVABLE contain different MIGRATE_types?
> > 
> never.

Then I am terribly missing something. Zone contains free lists for
different MIGRATE_TYPES, doesn't it? Pages allocated from those free
lists keep the migration type of the list, right?

ZONE_MOVABLE just says whether it makes sense to move pages in that zone
at all, right?

> 
> > > +
> > > +	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))
> > 
> > Why do you check page_count as well? PageBuddy has alwyas count==0,
> > right?
> > 
> 
> But PageBuddy() flag is considered to be valid only when page_count()==0.
> This is for safe handling.

OK. I don't see that documented anywhere but it makes sense. Anyway
there are some places which don't do this test (e.g.
isolate_freepages_block, suitable_migration_target, etc.).

> 
> 
> > > +				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. But if
> > > +		 * a PG_reserved page is _used_ (at boot), page_count > 1.
> > > +		 * But...is there PG_reserved && page_count(page)==0 page ?
> > 
> > Can we have PG_reserved && PG_lru? 
> 
> I think never.
> 
> > I also quite don't understand the comment. 
> 
> There an issue that "remove an memory section which includes memory hole".
> Then,
> 
>    a page used by bootmem .... PG_reserved.
>    a page of memory hole  .... PG_reserved.
> 
> We need to call page_is_ram() or some for handling this mess.

OK, I see.

> 
> 
> > At this place we are sure that the page is valid and neither
> > free nor LRU.
> > 
[...]
> > > +bool is_pageblock_removable(struct page *page)
> > > +{
> > > +	struct zone *zone = page_zone(page);
> > > +	unsigned long flags;
> > > +	int num;
> > > +
> > > +	spin_lock_irqsave(&zone->lock, flags);
> > > +	num = __count_unmovable_pages(zone, page);
> > > +	spin_unlock_irqrestore(&zone->lock, flags);
> > 
> > Isn't this a problem? The function is triggered from userspace by sysfs
> > (0444 file) and holds the lock for pageblock_nr_pages. So someone can
> > simply read the file and block the zone->lock preventing/delaying
> > allocations for the rest of the system.
> > 
> But we need to take this. Maybe no panic you'll see even if no-lock.

Yes, I think that this can only lead to a false possitive in sysfs
interface. Isolating code holds the lock.

Thanks
-- 
Michal Hocko
L3 team 
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>