Re: [PATCH 2/2] mm/mempolicy: Skip walking HUGETLB vma if MPOL_MF_STRICT is specified alone

[Yang Shi <yang.shi@linux.alibaba.com>]

On 1/14/20 11:36 PM, Li Xinhai wrote:
> On 2020-01-15 at 13:23 Yang Shi wrote:
>>
>> On 1/14/20 8:28 PM, Mike Kravetz wrote:
>>> On 1/14/20 5:24 PM, Yang Shi wrote:
>>>> On 1/14/20 5:07 PM, Mike Kravetz wrote:
>>>>> On 1/14/20 6:09 AM, Li Xinhai wrote:
>>>>>> Add cc to
>>>>>> Yang Shi <yang.shi@linux.alibaba.com>
>>>>>> Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>>>>> , who has been worked on this part
>>>>>>
>>>>>> On 2020-01-14 at 17:16 Li Xinhai wrote:
>>>>>>> Checking MPOL_MF_STRICT is ignored for HUGETLB vma according to mbind man
>>>>>>> page:
>>>>>>>
>>>>>>> Notes
>>>>>>> MPOL_MF_STRICT is ignored on huge page mappings.
>>>>>>>
>>>>>>> If MPOL_MF_STRICT is specified alone without any MOVE flag, we should
>>>>>>> indicate, from test_walk, that walking this vma should be skipped even if
>>>>>>> there are misplaced pages.
>>>>>>>
>>>>>>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>>>>>>> Cc: Michal Hocko <mhocko@suse.com>
>>>>>>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>>>>> I do not necessarily disagree with the change.  However, this has made me
>>>>> question a couple things:
>>>>> 1) Why does the man page say MPOL_MF_STRICT is ignored on huge page mappings?
>>>>>        - Is that leftover from the the days when huge page migration was not
>>>>>          supported?
>>>>>        - Is it just because huge page migration is more likely to fail than
>>>>>          base page migration.
>>>>> 2) Does the mbind code function properly when unable to migrate a huge page
>>>>>        MPOL_MF_STRICT is set?  A quick look at the code looks like it returns
>>>>>        EIO.
> for question (2),
> look into queue_pages_hugetlb(), the misplaced page would not
> cause -EIO reported, for both STRICT set alone and STRICT set with MOVE*;
> that means STRICT been effectively ignored during isolation phase.
>
> In unmap and move phase, -EIO is reported if failed to move page and
> STRICT is set.
>
>>>> I don't know the answer about question #1 I didn't dig into the history. The queue_pages_hugetlb() returns 0 unconditionally, I think this is what "MPOL_MF_STRICT is ignored on huge page mappings" means in code.
>>>>
>>>> It would return -EIO for base pages or THP as what the manpage describes.
>>>>
>>> I was thinking about a migration failure after isolation.  This block of
>>> code in do_mbind() after queue_pages_range() and mbind_range().
>>>
>>>            if (!err) {
>>>                    int nr_failed = 0;
>>>
>>>                    if (!list_empty(&pagelist)) {
>>>                            WARN_ON_ONCE(flags & MPOL_MF_LAZY);
>>>                            nr_failed = migrate_pages(&pagelist, new_page, NULL,
>>>                                    start, MIGRATE_SYNC, MR_MEMPOLICY_MBIND);
>>>                            if (nr_failed)
>>>                                    putback_movable_pages(&pagelist);
>>>                    }
>>>
>>>                    if ((ret > 0) || (nr_failed && (flags & MPOL_MF_STRICT)))
>>>                            err = -EIO;
>> Hmm.. I agree this part in man page does look ambiguous. We may assume
>> "MPOL_MF_STRICT is ignored on huge page mappings." implies if
>> MPOL_MF_STRICT is specified alone? If MOVE flag is specified it should
>> return -EIO if some pages could not be moved as what the man page describes.
>>
> It looks to me that current code has no feasible way to ignore STRICT
> flag for hugetlb page when failure happen in  unmap&move phase,
> because mbind is about to handle multiple vma(i.e., hugetlb vma mixed with
> other vma) in one call.

Yes, if you have both MPOL_MF_STRICT and MPOL_MF_MOVE_{ALL} specified, 
so I speculate the condition is MPOL_MF_STRICT is specified alone, but 
the man page doesn't elaborate this. This is also what the code does. 
But, I'm not sure if my understanding is correct or not.

>
>> I don't know what the intention was at the first place. We may have to
>> dig into the history.
>>
> >