From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl0-f71.google.com (mail-pl0-f71.google.com [209.85.160.71]) by kanga.kvack.org (Postfix) with ESMTP id 4F3716B0005 for ; Mon, 6 Aug 2018 12:53:39 -0400 (EDT) Received: by mail-pl0-f71.google.com with SMTP id h4-v6so8748898pll.4 for ; Mon, 06 Aug 2018 09:53:39 -0700 (PDT) Received: from out30-131.freemail.mail.aliyun.com (out30-131.freemail.mail.aliyun.com. [115.124.30.131]) by mx.google.com with ESMTPS id 63-v6si15174644pfg.67.2018.08.06.09.53.37 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Aug 2018 09:53:37 -0700 (PDT) Subject: Re: [RFC v6 PATCH 1/2] mm: refactor do_munmap() to extract the common part References: <1532628614-111702-1-git-send-email-yang.shi@linux.alibaba.com> <1532628614-111702-2-git-send-email-yang.shi@linux.alibaba.com> <20180803085335.GH27245@dhcp22.suse.cz> <7b84088a-4e49-ed7c-e750-7aba5cc17f11@linux.alibaba.com> <20180806132657.GB22858@dhcp22.suse.cz> From: Yang Shi Message-ID: <6af33f8f-042a-888f-2dad-6023fa5533f0@linux.alibaba.com> Date: Mon, 6 Aug 2018 09:53:13 -0700 MIME-Version: 1.0 In-Reply-To: <20180806132657.GB22858@dhcp22.suse.cz> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko Cc: willy@infradead.org, ldufour@linux.vnet.ibm.com, kirill@shutemov.name, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org On 8/6/18 6:26 AM, Michal Hocko wrote: > On Fri 03-08-18 13:47:19, Yang Shi wrote: >> >> On 8/3/18 1:53 AM, Michal Hocko wrote: >>> On Fri 27-07-18 02:10:13, Yang Shi wrote: >>>> Introduces three new helper functions: >>>> * munmap_addr_sanity() >>>> * munmap_lookup_vma() >>>> * munmap_mlock_vma() >>>> >>>> They will be used by do_munmap() and the new do_munmap with zapping >>>> large mapping early in the later patch. >>>> >>>> There is no functional change, just code refactor. >>>> >>>> Reviewed-by: Laurent Dufour >>>> Signed-off-by: Yang Shi >>>> --- >>>> mm/mmap.c | 120 ++++++++++++++++++++++++++++++++++++++++++-------------------- >>>> 1 file changed, 82 insertions(+), 38 deletions(-) >>>> >>>> diff --git a/mm/mmap.c b/mm/mmap.c >>>> index d1eb87e..2504094 100644 >>>> --- a/mm/mmap.c >>>> +++ b/mm/mmap.c >>>> @@ -2686,34 +2686,44 @@ int split_vma(struct mm_struct *mm, struct vm_area_struct *vma, >>>> return __split_vma(mm, vma, addr, new_below); >>>> } >>>> -/* Munmap is split into 2 main parts -- this part which finds >>>> - * what needs doing, and the areas themselves, which do the >>>> - * work. This now handles partial unmappings. >>>> - * Jeremy Fitzhardinge >>>> - */ >>>> -int do_munmap(struct mm_struct *mm, unsigned long start, size_t len, >>>> - struct list_head *uf) >>>> +static inline bool munmap_addr_sanity(unsigned long start, size_t len) >>> munmap_check_addr? Btw. why does this need to have munmap prefix at all? >>> This is a general address space check. >> Just because I extracted this from do_munmap, no special consideration. It >> is definitely ok to use another name. >> >>>> { >>>> - unsigned long end; >>>> - struct vm_area_struct *vma, *prev, *last; >>>> - >>>> if ((offset_in_page(start)) || start > TASK_SIZE || len > TASK_SIZE-start) >>>> - return -EINVAL; >>>> + return false; >>>> - len = PAGE_ALIGN(len); >>>> - if (len == 0) >>>> - return -EINVAL; >>>> + if (PAGE_ALIGN(len) == 0) >>>> + return false; >>>> + >>>> + return true; >>>> +} >>>> + >>>> +/* >>>> + * munmap_lookup_vma: find the first overlap vma and split overlap vmas. >>>> + * @mm: mm_struct >>>> + * @vma: the first overlapping vma >>>> + * @prev: vma's prev >>>> + * @start: start address >>>> + * @end: end address >>> This really doesn't help me to understand how to use the function. >>> Why do we need both prev and vma etc... >> prev will be used by unmap_region later. > But what does it stand for? Why cannot you take prev from the returned > vma? In other words, if somebody reads this documentation how does he > know what the prev is supposed to be used for? > >>>> + * >>>> + * returns 1 if successful, 0 or errno otherwise >>> This is a really weird calling convention. So what does 0 tell? /me >>> checks the code. Ohh, it is nothing to do. Why cannot you simply return >>> the vma. NULL implies nothing to do, ERR_PTR on error. >> A couple of reasons why it is implemented as so: >> >> A A A * do_munmap returns 0 for both success and no suitable vma >> >> A A A * Since prev is needed by finding the start vma, and prev will be used >> by unmap_region later too, so I just thought it would look clean to have one >> function to return both start vma and prev. In this way, we can share as >> much as possible common code. >> >> A A A * In this way, we just need return 0, 1 or error no just as same as what >> do_munmap does currently. Then we know what is failure case exactly to just >> bail out right away. >> >> Actually, I tried the same approach as you suggested, but it had two >> problems: >> >> A A A * If it returns the start vma, we have to re-find its prev later, but >> the prev has been found during finding start vma. And, duplicate the code in >> do_munmap_zap_rlock. It sounds not that ideal. >> >> A A A * If it returns prev, it might be null (start vma is the first vma). We >> can't tell if null is a failure or success case > Even if you need to return both vma and prev then it would be better to > simply return vma directly than having this -errno, 0 or 1 return > semantic. OK, I will try to refactor the code. Thanks, Yang