On 11/20/2012 07:38 AM, Andrew Morton wrote:
> On Mon, 19 Nov 2012 00:07:26 +0800
> Jiang Liu <liu...@gmail.com> wrote:
>
>> Currently a zone's present_pages is calcuated as below, which is
>> inaccurate and may cause trouble to memory hotplug.
>> spanned_pages - absent_pages - memmap_pages - dma_reserve.
>>
>> During fixing bugs caused by inaccurate zone->present_pages, we found
>> zone->present_pages has been abused. The field zone->present_pages
>> may have different meanings in different contexts:
>> 1) pages existing in a zone.
>> 2) pages managed by the buddy system.
>>
>> For more discussions about the issue, please refer to:
>> http://lkml.org/lkml/2012/11/5/866
>> https://patchwork.kernel.org/patch/1346751/
>>
>> This patchset tries to introduce a new field named "managed_pages" to
>> struct zone, which counts "pages managed by the buddy system". And
>> revert zone->present_pages to count "physical pages existing in a zone",
>> which also keep in consistence with pgdat->node_present_pages.
>>
>> We will set an initial value for zone->managed_pages in function
>> free_area_init_core() and will be adjusted later if the initial value is
>> inaccurate.
>>
>> For DMA/normal zones, the initial value is set to:
>> (spanned_pages - absent_pages - memmap_pages - dma_reserve)
>> Later zone->managed_pages will be adjusted to the accurate value when
>> the bootmem allocator frees all free pages to the buddy system in
>> function free_all_bootmem_node() and free_all_bootmem().
>>
>> The bootmem allocator doesn't touch highmem pages, so highmem zones'
>> managed_pages is set to the accurate value "spanned_pages - absent_pages"
>> in function free_area_init_core() and won't be updated anymore.
>>
>> This patch also adds a new field "managed_pages" to /proc/zoneinfo
>> and sysrq showmem.
>
> hoo boy, what a mess we made. I'd like to merge these patches and get
> them into -next for some testing, but -next has stopped for a couple of
> weeks. Oh well, let's see what can be done.
Hi Andrew,
Really sorry for the delay. Within last a few weeks, I could only
find after work hours or weekends for programming:(
>> --- a/include/linux/mmzone.h
>> +++ b/include/linux/mmzone.h
>> @@ -480,6 +480,7 @@ struct zone {
>> */
>> unsigned long spanned_pages; /* total size, including holes
>> */
>> unsigned long present_pages; /* amount of memory (excluding
>> holes) */
>> + unsigned long managed_pages; /* pages managed by the Buddy */
>
> Can you please add a nice big comment over these three fields which
> fully describes what they do and the relationship between them?
> Basically that stuff that's in the changelog.
>
> Also, the existing comment tells us that spanned_pages and
> present_pages are protected by span_seqlock but has not been updated to
> describe the locking (if any) for managed_pages.
How about this?
/*
* spanned_pages is the total pages spanned by the zone, including
* holes, which is calcualted as:
* spanned_pages = zone_end_pfn - zone_start_pfn;
*
* present_pages is physical pages existing within the zone, which
* is calculated as:
* present_pages = spanned_pages - absent_pages(pags in holes);
*
* managed_pages is present pages managed by the buddy system, which
* is calculated as (reserved_pages includes pages allocated by the
* bootmem allocator):
* managed_pages = present_pages - reserved_pages;
*
* So present_pages may be used by memory hotplug or memory power
* management logic to figure out unmanaged pages by checking
* (present_pages - managed_pages). And managed_pages should be used
* by page allocator and vm scanner to calculate all kinds of watermarks
* and thresholds.
*
* Lock Rules:
*
* zone_start_pfn, spanned_pages are protected by span_seqlock.
* It is a seqlock because it has to be read outside of zone->lock,
* and it is done in the main allocator path. But, it is written
* quite infrequently.
*
* The span_seq lock is declared along with zone->lock because it is
* frequently read in proximity to zone->lock. It's good to
* give them a chance of being in the same cacheline.
*
* Writing access to present_pages and managed_pages at runtime should
* be protected by lock_memory_hotplug()/unlock_memory_hotplug().
* Any reader who can't tolerant drift of present_pages and
* managed_pages should hold memory hotplug lock to get a stable value.
*/
unsigned long spanned_pages;
unsigned long present_pages;
unsigned long managed_pages;
>
>> /*
>> * rarely used fields:
>> diff --git a/mm/bootmem.c b/mm/bootmem.c
>> index f468185..a813e5b 100644
>> --- a/mm/bootmem.c
>> +++ b/mm/bootmem.c
>> @@ -229,6 +229,15 @@ static unsigned long __init
>> free_all_bootmem_core(bootmem_data_t *bdata)
>> return count;
>> }
>>
>> +static void reset_node_lowmem_managed_pages(pg_data_t *pgdat)
>> +{
>> + struct zone *z;
>> +
>> + for (z = pgdat->node_zones; z < pgdat->node_zones + MAX_NR_ZONES; z++)
>> + if (!is_highmem(z))
>
> Needs a comment explaining why we skip the highmem zone, please.
How about this?
/*
* In free_area_init_core(), highmem zone's managed_pages is set to
* present_pages, and bootmem allocator doesn't allocate from highmem
* zones. So there's no need to recalculate managed_pages because all
* highmem pages will be managed by the buddy system. Here highmem
* zone also includes highmem movable zone.
*/
>> + z->managed_pages = 0;
>> +}
>> +
>>
>> ...
>>
>> @@ -106,6 +106,7 @@ static void get_page_bootmem(unsigned long info, struct
>> page *page,
>> void __ref put_page_bootmem(struct page *page)
>> {
>> unsigned long type;
>> + static DEFINE_MUTEX(ppb_lock);
>>
>> type = (unsigned long) page->lru.next;
>> BUG_ON(type < MEMORY_HOTPLUG_MIN_BOOTMEM_TYPE ||
>> @@ -115,7 +116,9 @@ void __ref put_page_bootmem(struct page *page)
>> ClearPagePrivate(page);
>> set_page_private(page, 0);
>> INIT_LIST_HEAD(&page->lru);
>> + mutex_lock(&ppb_lock);
>> __free_pages_bootmem(page, 0);
>> + mutex_unlock(&ppb_lock);
>
> The mutex is odd. Nothing in the changelog, no code comment.
> __free_pages_bootmem() is called from a lot of places but only this one
> has locking. I'm madly guessing that the lock is here to handle two or
> more concurrent memory hotpluggings, but I shouldn't need to guess!!
Actually I'm a little hesitate whether we should add a lock here.
All callers of __free_pages_bootmem() other than put_page_bootmem() should
only be used at startup time. And currently the only caller of
put_page_bootmem()
has already been protected by pgdat_resize_lock(pgdat, &flags). So there's
no real need for lock, just defensive.
I'm not sure which is the best solution here.
1) add a comments into __free_pages_bootmem() to state that the caller should
serialize themselves.
2) Use a dedicated lock to serialize updates to zone->managed_pages, this need
modifications to page_alloc.c and memory_hotplug.c.
3) The above solution to serialize in put_page_bootmem().
What's your suggestions here?
Thanks
Gerry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
