Linux Virtualization - Jun 2016 - [PATCH v6v3 02/12] mm: migrate: support non-lru movable page migration

On 05/31/2016 05:31 AM, Minchan Kim wrote:
> @@ -791,6 +921,7 @@ static int __unmap_and_move(struct page *page, struct
page *newpage,
>  	int rc = -EAGAIN;
>  	int page_was_mapped = 0;
>  	struct anon_vma *anon_vma = NULL;
> +	bool is_lru = !__PageMovable(page);
>  
>  	if (!trylock_page(page)) {
>  		if (!force || mode == MIGRATE_ASYNC)
> @@ -871,6 +1002,11 @@ static int __unmap_and_move(struct page *page, struct
page *newpage,
>  		goto out_unlock_both;
>  	}
>  
> +	if (unlikely(!is_lru)) {
> +		rc = move_to_new_page(newpage, page, mode);
> +		goto out_unlock_both;
> +	}
> +
Hello Minchan,

I might be missing something here but does this implementation support the
scenario where these non LRU pages owned by the driver mapped as PTE into
process page table ? Because the "goto out_unlock_both" statement
above
skips all the PTE unmap, putting a migration PTE and removing the migration
PTE steps.

Regards
Anshuman

Hi,

On Mon, Jun 13, 2016 at 03:08:19PM +0530, Anshuman Khandual
wrote:
> On 05/31/2016 05:31 AM, Minchan Kim wrote:
> > @@ -791,6 +921,7 @@ static int __unmap_and_move(struct page *page,
struct page *newpage,
> >  	int rc = -EAGAIN;
> >  	int page_was_mapped = 0;
> >  	struct anon_vma *anon_vma = NULL;
> > +	bool is_lru = !__PageMovable(page);
> >  
> >  	if (!trylock_page(page)) {
> >  		if (!force || mode == MIGRATE_ASYNC)
> > @@ -871,6 +1002,11 @@ static int __unmap_and_move(struct page *page,
struct page *newpage,
> >  		goto out_unlock_both;
> >  	}
> >  
> > +	if (unlikely(!is_lru)) {
> > +		rc = move_to_new_page(newpage, page, mode);
> > +		goto out_unlock_both;
> > +	}
> > +
> 
> Hello Minchan,
> 
> I might be missing something here but does this implementation support the
> scenario where these non LRU pages owned by the driver mapped as PTE into
> process page table ? Because the "goto out_unlock_both" statement
above
> skips all the PTE unmap, putting a migration PTE and removing the migration
> PTE steps.
You're right. Unfortunately, it doesn't support right now but surely,
it's my TODO after landing this work.

Could you share your usecase?

It would be helpful for merging when I wll send patchset.

Thanks!

On 06/15/2016 08:02 AM, Minchan Kim wrote:
> Hi,
> 
> On Mon, Jun 13, 2016 at 03:08:19PM +0530, Anshuman Khandual wrote:
>> > On 05/31/2016 05:31 AM, Minchan Kim wrote:
>>> > > @@ -791,6 +921,7 @@ static int __unmap_and_move(struct
page *page, struct page *newpage,
>>> > >  	int rc = -EAGAIN;
>>> > >  	int page_was_mapped = 0;
>>> > >  	struct anon_vma *anon_vma = NULL;
>>> > > +	bool is_lru = !__PageMovable(page);
>>> > >  
>>> > >  	if (!trylock_page(page)) {
>>> > >  		if (!force || mode == MIGRATE_ASYNC)
>>> > > @@ -871,6 +1002,11 @@ static int __unmap_and_move(struct
page *page, struct page *newpage,
>>> > >  		goto out_unlock_both;
>>> > >  	}
>>> > >  
>>> > > +	if (unlikely(!is_lru)) {
>>> > > +		rc = move_to_new_page(newpage, page, mode);
>>> > > +		goto out_unlock_both;
>>> > > +	}
>>> > > +
>> > 
>> > Hello Minchan,
>> > 
>> > I might be missing something here but does this implementation
support the
>> > scenario where these non LRU pages owned by the driver mapped as
PTE into
>> > process page table ? Because the "goto out_unlock_both"
statement above
>> > skips all the PTE unmap, putting a migration PTE and removing the
migration
>> > PTE steps.
> You're right. Unfortunately, it doesn't support right now but
surely,
> it's my TODO after landing this work.
> 
> Could you share your usecase?
Sure.

My driver has privately managed non LRU pages which gets mapped into user space
process page table through f_ops->mmap() and vmops->fault() which then
updates
the file RMAP (page->mapping->i_mmap) through page_add_file_rmap(page).
One thing
to note here is that the page->mapping eventually points to struct
address_space
(file->f_mapping) which belongs to the character device file (created using
mknod)
which we are using for establishing the mmap() regions in the user space.

Now as per this new framework, all the page's are to be made
__SetPageMovable before
passing the list down to migrate_pages(). Now __SetPageMovable() takes *new*
struct
address_space as an argument and replaces the existing page->mapping. Now
thats the
problem, we have lost all our connection to the existing file RMAP information.
This
stands as a problem when we try to migrate these non LRU pages which are PTE
mapped.
The rmap_walk_file() never finds them in the VMA, skips all the migrate PTE
steps and
then the migration eventually fails.

Seems like assigning a new struct address_space to the page through
__SetPageMovable()
is the source of the problem. Can it take the existing (file->f_mapping) as
an argument
in there ? Sure, but then can we override file system generic ->isolate(),
->putback(),
->migratepages() functions ? I dont think so. I am sure, there must be some
work around
to fix this problem for the driver. But we need to rethink this framework from
supporting
these mapped non LRU pages point of view.

I might be missing something here, feel free to point out.

- Anshuman