Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Mar 2022 13:48:05 +0100
From:   Michal Hocko <mhocko@suse.com>
To:     Charan Teja Kalla <quic_charante@quicinc.com>
Cc:     akpm@linux-foundation.org, minchan@kernel.org, surenb@google.com,
        vbabka@suse.cz, rientjes@google.com, nadav.amit@gmail.com,
        edgararriaga@google.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] Revert "mm: madvise: skip unmapped vma holes passed
 to process_madvise"
Message-ID: <Yjxoha4PLlKjEayl@dhcp22.suse.cz>
References: <cover.1648046642.git.quic_charante@quicinc.com>
 <e73da1304a88b6a8a11907045117cccf4c2b8374.1648046642.git.quic_charante@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e73da1304a88b6a8a11907045117cccf4c2b8374.1648046642.git.quic_charante@quicinc.com>
Precedence: bulk

On Wed 23-03-22 20:54:09, Charan Teja Kalla wrote:
> This reverts commit 08095d6310a7 ("mm: madvise: skip unmapped vma holes
> passed to process_madvise") as process_madvise() fails to return exact
> processed bytes at other cases too. As an example: if the
> process_madvise() hits mlocked pages after processing some initial bytes
> passed in [start, end), it just returns EINVAL though some bytes are
> processed. Thus making an exception only for ENOMEM is partially fixing
> the problem of returning the proper advised bytes.
> 
> Thus revert this patch and return proper bytes advised, if there any,
> for all the error types in the following patch.

I do agree with the revert. I am not sure the above really is a proper
justification though. 08095d6310a7 was changing one (arguably) dubious
semantic by another one without a proper justification and wider
consensus which I would expect from a patch which changes an existing
semantic. Not to mention it being marked for stable tree.

But let's not nit pick on that now. Let's send this revert ASAP and use
some more time to discuss the semantic and whether any change is really
required.

> Signed-off-by: Charan Teja Kalla <quic_charante@quicinc.com>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/madvise.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 39b712f..0d8fd17 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1433,16 +1433,9 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
>  
>  	while (iov_iter_count(&iter)) {
>  		iovec = iov_iter_iovec(&iter);
> -		/*
> -		 * do_madvise returns ENOMEM if unmapped holes are present
> -		 * in the passed VMA. process_madvise() is expected to skip
> -		 * unmapped holes passed to it in the 'struct iovec' list
> -		 * and not fail because of them. Thus treat -ENOMEM return
> -		 * from do_madvise as valid and continue processing.
> -		 */
>  		ret = do_madvise(mm, (unsigned long)iovec.iov_base,
>  					iovec.iov_len, behavior);
> -		if (ret < 0 && ret != -ENOMEM)
> +		if (ret < 0)
>  			break;
>  		iov_iter_advance(&iter, iovec.iov_len);
>  	}
> -- 
> 2.7.4

-- 
Michal Hocko
SUSE Labs