On 4/6/21 7:05 PM, Miaohe Lin wrote: > Hi: > On 2021/4/7 8:53, Mike Kravetz wrote: >> On 4/2/21 2:32 AM, Miaohe Lin wrote: >>> It's guaranteed that the vma is associated with a resv_map, i.e. either >>> VM_MAYSHARE or HPAGE_RESV_OWNER, when the code reaches here or we would >>> have returned via !resv check above. So ret must be less than 0 in the >>> 'else' case. Simplify the return code to make this clear. >> >> I believe we still neeed that ternary operator in the return statement. >> Why? >> >> There are two basic types of mappings to be concerned with: >> shared and private. >> For private mappings, a task can 'own' the mapping as indicated by >> HPAGE_RESV_OWNER. Or, it may not own the mapping. The most common way >> to create a non-owner private mapping is to have a task with a private >> mapping fork. The parent process will have HPAGE_RESV_OWNER set, the >> child process will not. The idea is that since the child has a COW copy >> of the mapping it should not consume reservations made by the parent. > > The child process will not have HPAGE_RESV_OWNER set because at fork time, we do: > /* > * Clear hugetlb-related page reserves for children. This only > * affects MAP_PRIVATE mappings. Faults generated by the child > * are not guaranteed to succeed, even if read-only > */ > if (is_vm_hugetlb_page(tmp)) > reset_vma_resv_huge_pages(tmp); > i.e. we have vma->vm_private_data = (void *)0; for child process and vma_resv_map() will > return NULL in this case. > Or am I missed something? > >> Only the parent (HPAGE_RESV_OWNER) is allowed to consume the >> reservations. >> Hope that makens sense? >> >>> >>> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx> >>> --- >>> mm/hugetlb.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>> index a03a50b7c410..b7864abded3d 100644 >>> --- a/mm/hugetlb.c >>> +++ b/mm/hugetlb.c >>> @@ -2183,7 +2183,7 @@ static long __vma_reservation_common(struct hstate *h, >>> return 1; >>> } >>> else >> >> This else also handles the case !HPAGE_RESV_OWNER. In this case, we > > IMO, for the case !HPAGE_RESV_OWNER, we won't reach here. What do you think? > I think you are correct. However, if this is true we should be able to simply the code even further. There is no need to check for HPAGE_RESV_OWNER because we know it must be set. Correct? If so, the code could look something like: if (vma->vm_flags & VM_MAYSHARE) return ret; /* We know private mapping with HPAGE_RESV_OWNER */ * ... * * Add that existing comment */ if (ret > 0) return 0; if (ret == 0) return 1; return ret; -- Mike Kravetz
