Messages in this thread | | | Subject | Re: [PATCH v2] mm: Fix checking unmapped holes for mbind | From | Li Xinhai <> | Date | Thu, 31 Oct 2019 20:32:01 +0800 |
| |
On 2019/10/31 17:06, Naoya Horiguchi wrote: > On Thu, Oct 31, 2019 at 11:15:09AM +0800, Li Xinhai wrote: >> On 2019-10-29 at 17:56 Li Xinhai wrote: >>> queue_pages_range() will check for unmapped holes besides queue pages for >>> migration. The rules for checking unmapped holes are: >>> 1 Unmapped holes at any part of the specified range should be reported as >>> EFAULT if mbind() for none MPOL_DEFAULT cases; >>> 2 Unmapped holes at any part of the specified range should be ignored if >>> mbind() for MPOL_DEFAULT case; >>> Note that the second rule is the current implementation, but it seems >>> conflicts the Linux API definition. >>> >>> queue_pages_test_walk() is fixed by introduce new fields in struct >>> queue_pages which help to check: >>> 1 holes at head and tail side of specified range; >>> 2 the whole range is in a hole; >>> >>> Besides, queue_pages_test_walk() must update previous vma record no matter >>> the current vma should be considered for queue pages or not. >>> >> >> More details about current issue (which breaks the mbind() API definition): >> 1. In queue_pages_test_walk() >> checking on (!vma->vm_next && vma->vm_end < end) would never success, >> because 'end' passed from walk_page_test() make sure "end <= vma->vm_end". so hole >> beyond the last vma can't be detected. >> >> 2. queue_pages_test_walk() only called for vma, and 'start', 'end' parameters are guaranteed >> within vma. Then, if the range starting or ending in an unmapped hole, >> queue_pages_test_walk() don't have chance to be called to check. In other words, the >> current code can detect this case (range span over hole): >> [ vma ][ hole ][ vma] >> [ range ] >> but cant not detect these case : >> [ vma ][ hole ][ vma] >> [ range ] >> [ vma ][ hole ][ vma ] >> [ range ] > > IIRC, page table walker (walk_page_range()) should be designed to > handle these range inputs by separating into sub-ranges by vma > boundaries like below (with your notation): > > [ vma ][ hole ][ vma ] > [ ][ ][ ] // for your 1st case > [ ][ ] // for your 2nd case > [ ][ ] // for your 3rd case > Yes, pagewalk works exactly as your description.
> And I found that .pte_hole is undefined in queue_pages_walk_ops. > So I'm guessing now that that's why hole regions are ignored and > the definition of EFAULT behavior in manpage is violated. > So providing proper .pte_hole callback could be another approach > for this issue which might fit better to the design. Using the .pte_hole() can be one option, we may stop detecting the unmapped holes when .pte_hole() been first called for a hole which is outside vma. But, .pte_hole() would be called many times during the walking, because it is called for holes inside and outside VMA. .test_walk() is called once for one vma, and not called for other situation, so better for cost.
> IOW, queue_pages_test_walk() might not the right place to handle > hole regions by definition. I guess the original design was to avoid walking those vma twice, one for test holes and one for queue pages. Maybe we can find better choice?
> > Thanks, > Naoya Horiguchi > >> >> 3. a checking in mbind_range() try to recover if the hole is in head side, but can't >> recover if hole is in tail side of range. >> >> - Xinhai >> >>> Fixes: 9d8cebd4bcd7 ("mm: fix mbind vma merge problem") >>> Fixes: 6f4576e3687b ("mempolicy: apply page table walker on queue_pages_range()") >>> Fixes: 48684a65b4e3 ("mm: pagewalk: fix misbehavior of walk_page_range for vma(VM_PFNMAP)") >>> Signed-off-by: Li Xinhai <lixinhai.li@gmail.com> >>> --- >>> Changes in v2: >>> - Fix the unmapped holes checking in queue_pages_test_walk() instead of >>> mbind_rnage(). >>> >>> mm/mempolicy.c | 44 +++++++++++++++++++++++++++++--------------- >>> 1 file changed, 29 insertions(+), 15 deletions(-) >>> >>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c >>> index 4ae967bcf954..24087dfa4dcd 100644 >>> --- a/mm/mempolicy.c >>> +++ b/mm/mempolicy.c >>> @@ -411,6 +411,9 @@ struct queue_pages { >>> unsigned long flags; >>> nodemask_t *nmask; >>> struct vm_area_struct *prev; >>> + unsigned long start; >>> + unsigned long end; >>> + int in_hole; >>> }; >>> >>> /* >>> @@ -618,28 +621,31 @@ static int queue_pages_test_walk(unsigned long start, unsigned long end, >>> unsigned long endvma = vma->vm_end; >>> unsigned long flags = qp->flags; >>> >>> - /* >>> - * Need check MPOL_MF_STRICT to return -EIO if possible >>> - * regardless of vma_migratable >>> - */ >>> - if (!vma_migratable(vma) && >>> - !(flags & MPOL_MF_STRICT)) >>> - return 1; >>> - >>> + /* range check first */ >>> if (endvma > end) >>> endvma = end; >>> - if (vma->vm_start > start) >>> - start = vma->vm_start; >>> + BUG_ON((vma->vm_start > start) || (vma->vm_end < end)); >>> >>> + qp->in_hole = 0; >>> if (!(flags & MPOL_MF_DISCONTIG_OK)) { >>> - if (!vma->vm_next && vma->vm_end < end) >>> + if ((!vma->vm_next && vma->vm_end < qp->end) || >>> + (vma->vm_next && qp->end < vma->vm_next->vm_start)) >>> return -EFAULT; >>> - if (qp->prev && qp->prev->vm_end < vma->vm_start) >>> + if ((qp->prev && qp->prev->vm_end < vma->vm_start) || >>> + (!qp->prev && qp->start < vma->vm_start)) >>> return -EFAULT; >>> } >>> >>> qp->prev = vma; >>> >>> + /* >>> + * Need check MPOL_MF_STRICT to return -EIO if possible >>> + * regardless of vma_migratable >>> + */ >>> + if (!vma_migratable(vma) && >>> + !(flags & MPOL_MF_STRICT)) >>> + return 1; >>> + >>> if (flags & MPOL_MF_LAZY) { >>> /* Similar to task_numa_work, skip inaccessible VMAs */ >>> if (!is_vm_hugetlb_page(vma) && >>> @@ -679,14 +685,23 @@ queue_pages_range(struct mm_struct *mm, unsigned long start, unsigned long end, >>> nodemask_t *nodes, unsigned long flags, >>> struct list_head *pagelist) >>> { >>> + int err; >>> struct queue_pages qp = { >>> .pagelist = pagelist, >>> .flags = flags, >>> .nmask = nodes, >>> .prev = NULL, >>> + .start = start, >>> + .end = end, >>> + .in_hole = 1, >>> }; >>> >>> - return walk_page_range(mm, start, end, &queue_pages_walk_ops, &qp); >>> + err = walk_page_range(mm, start, end, &queue_pages_walk_ops, &qp); >>> + /* whole range in unmapped hole */ >>> + if (qp->in_hole && !(flags & MPOL_MF_DISCONTIG_OK)) >>> + err = -EFAULT; >>> + >>> + return err; >>> } >>> >>> /* >>> @@ -738,8 +753,7 @@ static int mbind_range(struct mm_struct *mm, unsigned long start, >>> unsigned long vmend; >>> >>> vma = find_vma(mm, start); >>> - if (!vma || vma->vm_start > start) >>> - return -EFAULT; >>> + BUG_ON(!vma); >>> >>> prev = vma->vm_prev; >>> if (start > vma->vm_start) >>> -- >>> 2.22.0
| |