Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 28 Aug 2021 19:19:46 +0300
From:   Cyrill Gorcunov <gorcunov@gmail.com>
To:     Suren Baghdasaryan <surenb@google.com>
Cc:     akpm@linux-foundation.org, ccross@google.com,
        sumit.semwal@linaro.org, mhocko@suse.com, dave.hansen@intel.com,
        keescook@chromium.org, willy@infradead.org,
        kirill.shutemov@linux.intel.com, vbabka@suse.cz,
        hannes@cmpxchg.org, corbet@lwn.net, viro@zeniv.linux.org.uk,
        rdunlap@infradead.org, kaleshsingh@google.com, peterx@redhat.com,
        rppt@kernel.org, peterz@infradead.org, catalin.marinas@arm.com,
        vincenzo.frascino@arm.com, chinwen.chang@mediatek.com,
        axelrasmussen@google.com, aarcange@redhat.com, jannh@google.com,
        apopple@nvidia.com, jhubbard@nvidia.com, yuzhao@google.com,
        will@kernel.org, fenghua.yu@intel.com, thunder.leizhen@huawei.com,
        hughd@google.com, feng.tang@intel.com, jgg@ziepe.ca, guro@fb.com,
        tglx@linutronix.de, krisman@collabora.com, chris.hyser@oracle.com,
        pcc@google.com, ebiederm@xmission.com, axboe@kernel.dk,
        legion@kernel.org, eb@emlix.com, songmuchun@bytedance.com,
        viresh.kumar@linaro.org, thomascedeno@google.com,
        sashal@kernel.org, cxfcosmos@gmail.com, linux@rasmusvillemoes.dk,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-mm@kvack.org,
        kernel-team@android.com, Pekka Enberg <penberg@kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Oleg Nesterov <oleg@redhat.com>,
        Jan Glauber <jan.glauber@gmail.com>,
        John Stultz <john.stultz@linaro.org>,
        Rob Landley <rob@landley.net>,
        "Serge E. Hallyn" <serge.hallyn@ubuntu.com>,
        David Rientjes <rientjes@google.com>,
        Rik van Riel <riel@redhat.com>, Mel Gorman <mgorman@suse.de>,
        Michel Lespinasse <walken@google.com>,
        Tang Chen <tangchen@cn.fujitsu.com>, Robin Holt <holt@sgi.com>,
        Shaohua Li <shli@fusionio.com>,
        Sasha Levin <sasha.levin@oracle.com>,
        Minchan Kim <minchan@kernel.org>
Subject: Re: [PATCH v8 1/3] mm: rearrange madvise code to allow for reuse
Message-ID: <YSpiIrQKs5RUccYk@grain>
References: <20210827191858.2037087-1-surenb@google.com>
 <20210827191858.2037087-2-surenb@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210827191858.2037087-2-surenb@google.com>
User-Agent: Mutt/2.0.7 (2021-05-04)
Precedence: bulk

On Fri, Aug 27, 2021 at 12:18:56PM -0700, Suren Baghdasaryan wrote:
...
>  
> +/*
> + * Apply an madvise behavior to a region of a vma.  madvise_update_vma
> + * will handle splitting a vm area into separate areas, each area with its own
> + * behavior.
> + */
> +static int madvise_vma_behavior(struct vm_area_struct *vma,
> +				struct vm_area_struct **prev,
> +				unsigned long start, unsigned long end,
> +				unsigned long behavior)
> +{
> +	int error = 0;


Hi Suren! A nitpick -- this variable is never used with default value
so I think we could drop assignment here.
...
> +	case MADV_DONTFORK:
> +		new_flags |= VM_DONTCOPY;
> +		break;
> +	case MADV_DOFORK:
> +		if (vma->vm_flags & VM_IO) {
> +			error = -EINVAL;

We can exit early here, without jumping to the end of the function, right?

> +			goto out;
> +		}
> +		new_flags &= ~VM_DONTCOPY;
> +		break;
> +	case MADV_WIPEONFORK:
> +		/* MADV_WIPEONFORK is only supported on anonymous memory. */
> +		if (vma->vm_file || vma->vm_flags & VM_SHARED) {
> +			error = -EINVAL;

And here too.

> +			goto out;
> +		}
> +		new_flags |= VM_WIPEONFORK;
> +		break;
> +	case MADV_KEEPONFORK:
> +		new_flags &= ~VM_WIPEONFORK;
> +		break;
> +	case MADV_DONTDUMP:
> +		new_flags |= VM_DONTDUMP;
> +		break;
> +	case MADV_DODUMP:
> +		if (!is_vm_hugetlb_page(vma) && new_flags & VM_SPECIAL) {
> +			error = -EINVAL;

Same.

> +			goto out;
> +		}
> +		new_flags &= ~VM_DONTDUMP;
> +		break;
> +	case MADV_MERGEABLE:
> +	case MADV_UNMERGEABLE:
> +		error = ksm_madvise(vma, start, end, behavior, &new_flags);
> +		if (error)
> +			goto out;
> +		break;
> +	case MADV_HUGEPAGE:
> +	case MADV_NOHUGEPAGE:
> +		error = hugepage_madvise(vma, &new_flags, behavior);
> +		if (error)
> +			goto out;
> +		break;
> +	}
> +
> +	error = madvise_update_vma(vma, prev, start, end, new_flags);
> +
> +out:

I suppose we better keep the former comment on why we maps ENOMEM to EAGAIN?

	Cyrill