PATCH v6v2 02/12] mm: migrate: support non-lru movable page migration
On 05/30/2016 06:25 PM, Minchan Kim wrote:
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -81,6 +81,39 @@ static inline bool migrate_async_suitable(int
>>> migratetype)
>>>
>>> #ifdef CONFIG_COMPACTION
>>>
>>> +int PageMovable(struct page *page)
>>> +{
>>> + struct address_space *mapping;
>>> +
>>> + VM_BUG_ON_PAGE(!PageLocked(page), page);
>>> + if (!__PageMovable(page))
>>> + return 0;
>>> +
>>> + mapping = page_mapping(page);
>>> + if (mapping && mapping->a_ops && mapping->a_ops->isolate_page)
>>> + return 1;
>>> +
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(PageMovable);
>>> +
>>> +void __SetPageMovable(struct page *page, struct address_space *mapping)
>>> +{
>>> + VM_BUG_ON_PAGE(!PageLocked(page), page);
>>> + VM_BUG_ON_PAGE((unsigned long)mapping & PAGE_MAPPING_MOVABLE, page);
>>> + page->mapping = (void *)((unsigned long)mapping | PAGE_MAPPING_MOVABLE);
>>> +}
>>> +EXPORT_SYMBOL(__SetPageMovable);
>>> +
>>> +void __ClearPageMovable(struct page *page)
>>> +{
>>> + VM_BUG_ON_PAGE(!PageLocked(page), page);
>>> + VM_BUG_ON_PAGE(!PageMovable(page), page);
>>> + page->mapping = (void *)((unsigned long)page->mapping &
>>> + PAGE_MAPPING_MOVABLE);
>>> +}
>>> +EXPORT_SYMBOL(__ClearPageMovable);
>>
>> The second confusing thing is that the function is named
>> __ClearPageMovable(), but what it really clears is the mapping
>> pointer,
>> which is not at all the opposite of what __SetPageMovable() does.
>>
>> I know it's explained in the documentation, but it also deserves a
>> comment here so it doesn't confuse everyone who looks at it.
>> Even better would be a less confusing name for the function, but I
>> can't offer one right now.
>
> To me, __ClearPageMovable naming is suitable for user POV.
> It effectively makes the page unmovable. The confusion is just caused
> by the implementation and I don't prefer exported API depends on the
> implementation. So I want to add just comment.
>
> I didn't add comment above the function because I don't want to export
> internal implementation to the user. I think they don't need to know it.
>
> index a7df2ae71f2a..d1d2063b4fd9 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -108,6 +108,11 @@ void __ClearPageMovable(struct page *page)
> {
> VM_BUG_ON_PAGE(!PageLocked(page), page);
> VM_BUG_ON_PAGE(!PageMovable(page), page);
> + /*
> +* Clear registered address_space val with keeping
> PAGE_MAPPING_MOVABLE
> +* flag so that VM can catch up released page by driver after
> isolation.
> +* With it, VM migration doesn't try to put it back.
> +*/
> page->mapping = (void *)((unsigned long)page->mapping &
> PAGE_MAPPING_MOVABLE);
OK, that's fine!
>>
>>> diff --git a/mm/util.c b/mm/util.c
>>> index 917e0e3d0f8e..b756ee36f7f0 100644
>>> --- a/mm/util.c
>>> +++ b/mm/util.c
>>> @@ -399,10 +399,12 @@ struct address_space *page_mapping(struct page *page)
>>> }
>>>
>>> mapping = page->mapping;
>>
>> I'd probably use READ_ONCE() here to be safe. Not all callers are
>> under page lock?
>
> I don't understand. Yeah, all caller are not under page lock but at least,
> new user of movable pages should call it under page_lock.
> Yeah, I will write the rule down in document.
> In this case, what kinds of problem do you see?
After more thinking, probably none. It wouldn't prevent any extra races.
Sorry for the noise.
PATCH v6v2 02/12] mm: migrate: support non-lru movable page migration
On Mon, May 30, 2016 at 11:36:07AM +0200, Vlastimil Babka wrote:
> On 05/30/2016 03:39 AM, Minchan Kim wrote:
> >After isolation, VM calls migratepage of driver with isolated page.
> >The function of migratepage is to move content of the old page to new page
> >and set up fields of struct page newpage. Keep in mind that you should
> >clear PG_movable of oldpage via __ClearPageMovable under page_lock if you
> >migrated the oldpage successfully and returns 0.
>
> This "clear PG_movable" is one of the reasons I was confused about
> what __ClearPageMovable() really does. There's no actual
> "PG_movable" page flag and the function doesn't clear even the
> actual mapping flag :) Also same thing in the Documentation/ part.
>
> Something like "... you should indicate to the VM that the oldpage
> is no longer movable via __ClearPageMovable() ..."?
It's better. I will change it.
>
> >--- a/mm/compaction.c
> >+++ b/mm/compaction.c
> >@@ -81,6 +81,39 @@ static inline bool migrate_async_suitable(int migratetype)
> >
> > #ifdef CONFIG_COMPACTION
> >
> >+int PageMovable(struct page *page)
> >+{
> >+struct address_space *mapping;
> >+
> >+VM_BUG_ON_PAGE(!PageLocked(page), page);
> >+if (!__PageMovable(page))
> >+return 0;
> >+
> >+mapping = page_mapping(page);
> >+if (mapping && mapping->a_ops && mapping->a_ops->isolate_page)
> >+return 1;
> >+
> >+return 0;
> >+}
> >+EXPORT_SYMBOL(PageMovable);
> >+
> >+void __SetPageMovable(struct page *page, struct address_space *mapping)
> >+{
> >+VM_BUG_ON_PAGE(!PageLocked(page), page);
> >+VM_BUG_ON_PAGE((unsigned long)mapping & PAGE_MAPPING_MOVABLE, page);
> >+page->mapping = (void *)((unsigned long)mapping | PAGE_MAPPING_MOVABLE);
> >+}
> >+EXPORT_SYMBOL(__SetPageMovable);
> >+
> >+void __ClearPageMovable(struct page *page)
> >+{
> >+VM_BUG_ON_PAGE(!PageLocked(page), page);
> >+VM_BUG_ON_PAGE(!PageMovable(page), page);
> >+page->mapping = (void *)((unsigned long)page->mapping &
> >+PAGE_MAPPING_MOVABLE);
> >+}
> >+EXPORT_SYMBOL(__ClearPageMovable);
>
> The second confusing thing is that the function is named
> __ClearPageMovable(), but what it really clears is the mapping
> pointer,
> which is not at all the opposite of what __SetPageMovable() does.
>
> I know it's explained in the documentation, but it also deserves a
> comment here so it doesn't confuse everyone who looks at it.
> Even better would be a less confusing name for the function, but I
> can't offer one right now.
To me, __ClearPageMovable naming is suitable for user POV.
It effectively makes the page unmovable. The confusion is just caused
by the implementation and I don't prefer exported API depends on the
implementation. So I want to add just comment.
I didn't add comment above the function because I don't want to export
internal implementation to the user. I think they don't need to know it.
index a7df2ae71f2a..d1d2063b4fd9 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -108,6 +108,11 @@ void __ClearPageMovable(struct page *page)
{
VM_BUG_ON_PAGE(!PageLocked(page), page);
VM_BUG_ON_PAGE(!PageMovable(page), page);
+ /*
+* Clear registered address_space val with keeping PAGE_MAPPING_MOVABLE
+* flag so that VM can catch up released page by driver after isolation.
+* With it, VM migration doesn't try to put it back.
+*/
page->mapping = (void *)((unsigned long)page->mapping &
PAGE_MAPPING_MOVABLE);
>
> >diff --git a/mm/util.c b/mm/util.c
> >index 917e0e3d0f8e..b756ee36f7f0 100644
> >--- a/mm/util.c
> >+++ b/mm/util.c
> >@@ -399,10 +399,12 @@ struct address_space *page_mapping(struct page *page)
> > }
> >
> > mapping = page->mapping;
>
> I'd probably use READ_ONCE() here to be safe. Not all callers are
> under page lock?
I don't understand. Yeah, all caller are not under page lock but at least,
new user of movable pages should call it under page_lock.
Yeah, I will write the rule down in document.
In this case, what kinds of problem do you see?
>
> >-if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
> >+if ((unsigned long)mapping & PAGE_MAPPING_ANON)
> > return NULL;
> >-return mapping;
> >+
> >+return (void *)((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
> > }
> >+EXPORT_SYMBOL(page_mapping);
> >
> > /* Slow path of page_mapcount() for compound pages */
> > int __page_mapcount(struct page *page)
> >
>
PATCH v6v2 02/12] mm: migrate: support non-lru movable page migration
On 05/30/2016 03:39 AM, Minchan Kim wrote:
> After isolation, VM calls migratepage of driver with isolated page.
> The function of migratepage is to move content of the old page to new page
> and set up fields of struct page newpage. Keep in mind that you should
> clear PG_movable of oldpage via __ClearPageMovable under page_lock if you
> migrated the oldpage successfully and returns 0.
This "clear PG_movable" is one of the reasons I was confused about what
__ClearPageMovable() really does. There's no actual "PG_movable" page
flag and the function doesn't clear even the actual mapping flag :) Also
same thing in the Documentation/ part.
Something like "... you should indicate to the VM that the oldpage is no
longer movable via __ClearPageMovable() ..."?
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -81,6 +81,39 @@ static inline bool migrate_async_suitable(int migratetype)
>
> #ifdef CONFIG_COMPACTION
>
> +int PageMovable(struct page *page)
> +{
> + struct address_space *mapping;
> +
> + VM_BUG_ON_PAGE(!PageLocked(page), page);
> + if (!__PageMovable(page))
> + return 0;
> +
> + mapping = page_mapping(page);
> + if (mapping && mapping->a_ops && mapping->a_ops->isolate_page)
> + return 1;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(PageMovable);
> +
> +void __SetPageMovable(struct page *page, struct address_space *mapping)
> +{
> + VM_BUG_ON_PAGE(!PageLocked(page), page);
> + VM_BUG_ON_PAGE((unsigned long)mapping & PAGE_MAPPING_MOVABLE, page);
> + page->mapping = (void *)((unsigned long)mapping | PAGE_MAPPING_MOVABLE);
> +}
> +EXPORT_SYMBOL(__SetPageMovable);
> +
> +void __ClearPageMovable(struct page *page)
> +{
> + VM_BUG_ON_PAGE(!PageLocked(page), page);
> + VM_BUG_ON_PAGE(!PageMovable(page), page);
> + page->mapping = (void *)((unsigned long)page->mapping &
> + PAGE_MAPPING_MOVABLE);
> +}
> +EXPORT_SYMBOL(__ClearPageMovable);
The second confusing thing is that the function is named
__ClearPageMovable(), but what it really clears is the mapping pointer,
which is not at all the opposite of what __SetPageMovable() does.
I know it's explained in the documentation, but it also deserves a
comment here so it doesn't confuse everyone who looks at it.
Even better would be a less confusing name for the function, but I can't
offer one right now.
> diff --git a/mm/util.c b/mm/util.c
> index 917e0e3d0f8e..b756ee36f7f0 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -399,10 +399,12 @@ struct address_space *page_mapping(struct page *page)
> }
>
> mapping = page->mapping;
I'd probably use READ_ONCE() here to be safe. Not all callers are under
page lock?
> - if ((unsigned long)mapping & PAGE_MAPPING_FLAGS)
> + if ((unsigned long)mapping & PAGE_MAPPING_ANON)
> return NULL;
> - return mapping;
> +
> + return (void *)((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
> }
> +EXPORT_SYMBOL(page_mapping);
>
> /* Slow path of page_mapcount() for compound pages */
> int __page_mapcount(struct page *page)
>
PATCH v6v2 02/12] mm: migrate: support non-lru movable page migration
Per Vlastimil's review comment, Vlastimil, I updated based on your comment. Please review this. If everything is done, I will send v7 rebased on recent mmotm. Thanks for the review! >From ad4157e98651a2d18fd0a4ae90d1d9f609aab314 Mon Sep 17 00:00:00 2001 From: Minchan Kim Date: Fri, 8 Apr 2016 10:34:49 +0900 Subject: [PATCH v6r2] mm: migrate: support non-lru movable page migration We have allowed migration for only LRU pages until now and it was enough to make high-order pages. But recently, embedded system(e.g., webOS, android) uses lots of non-movable pages(e.g., zram, GPU memory) so we have seen several reports about troubles of small high-order allocation. For fixing the problem, there were several efforts (e,g,. enhance compaction algorithm, SLUB fallback to 0-order page, reserved memory, vmalloc and so on) but if there are lots of non-movable pages in system, their solutions are void in the long run. So, this patch is to support facility to change non-movable pages with movable. For the feature, this patch introduces functions related to migration to address_space_operations as well as some page flags. If a driver want to make own pages movable, it should define three functions which are function pointers of struct address_space_operations. 1. bool (*isolate_page) (struct page *page, isolate_mode_t mode); What VM expects on isolate_page function of driver is to return *true* if driver isolates page successfully. On returing true, VM marks the page as PG_isolated so concurrent isolation in several CPUs skip the page for isolation. If a driver cannot isolate the page, it should return *false*. Once page is successfully isolated, VM uses page.lru fields so driver shouldn't expect to preserve values in that fields. 2. int (*migratepage) (struct address_space *mapping, struct page *newpage, struct page *oldpage, enum migrate_mode); After isolation, VM calls migratepage of driver with isolated page. The function of migratepage is to move content of the old page to new page and set up fields of struct page newpage. Keep in mind that you should clear PG_movable of oldpage via __ClearPageMovable under page_lock if you migrated the oldpage successfully and returns 0. If driver cannot migrate the page at the moment, driver can return -EAGAIN. On -EAGAIN, VM will retry page migration in a short time because VM interprets -EAGAIN as "temporal migration failure". On returning any error except -EAGAIN, VM will give up the page migration without retrying in this time. Driver shouldn't touch page.lru field VM using in the functions. 3. void (*putback_page)(struct page *); If migration fails on isolated page, VM should return the isolated page to the driver so VM calls driver's putback_page with migration failed page. In this function, driver should put the isolated page back to the own data structure. 4. non-lru movable page flags There are two page flags for supporting non-lru movable page. * PG_movable Driver should use the below function to make page movable under page_lock. void __SetPageMovable(struct page *page, struct address_space *mapping) It needs argument of address_space for registering migration family functions which will be called by VM. Exactly speaking, PG_movable is not a real flag of struct page. Rather than, VM reuses page->mapping's lower bits to represent it. #define PAGE_MAPPING_MOVABLE 0x2 page->mapping = page->mapping | PAGE_MAPPING_MOVABLE; so driver shouldn't access page->mapping directly. Instead, driver should use page_mapping which mask off the low two bits of page->mapping so it can get right struct address_space. For testing of non-lru movable page, VM supports __PageMovable function. However, it doesn't guarantee to identify non-lru movable page because page->mapping field is unified with other variables in struct page. As well, if driver releases the page after isolation by VM, page->mapping doesn't have stable value although it has PAGE_MAPPING_MOVABLE (Look at __ClearPageMovable). But __PageMovable is cheap to catch whether page is LRU or non-lru movable once the page has been isolated. Because LRU pages never can have PAGE_MAPPING_MOVABLE in page->mapping. It is also good for just peeking to test non-lru movable pages before more expensive checking with lock_page in pfn scanning to select victim. For guaranteeing non-lru movable page, VM provides PageMovable function. Unlike __PageMovable, PageMovable functions validates page->mapping and mapping->a_ops->isolate_page under lock_page. The lock_page prevents sudden destroying of page->mapping. Driver using __SetPageMovable should clear the flag via __ClearMovablePage under page_lock before the releasing the page. * PG_isolated To prevent concurrent isolation among several CPUs, VM marks isolated page as PG_isolated under lock_page. So if a CPU encounters PG_isolated non-lru movable page, it can skip it. Driver doesn't need to manipulate the flag because VM will set/c
