From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 99760C47089 for ; Thu, 27 May 2021 07:39:53 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3D8CB613AB for ; Thu, 27 May 2021 07:39:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3D8CB613AB Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 967F76B0036; Thu, 27 May 2021 03:39:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8F0E66B006E; Thu, 27 May 2021 03:39:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 76B836B0070; Thu, 27 May 2021 03:39:52 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0152.hostedemail.com [216.40.44.152]) by kanga.kvack.org (Postfix) with ESMTP id 3B7DA6B0036 for ; Thu, 27 May 2021 03:39:52 -0400 (EDT) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id C3D168249980 for ; Thu, 27 May 2021 07:39:51 +0000 (UTC) X-FDA: 78186211782.21.D54FA8A Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf26.hostedemail.com (Postfix) with ESMTP id 6E77940002F1 for ; Thu, 27 May 2021 07:39:47 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1622101190; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ysXBzYpFMZ/j0HqnWY8w72Bm/ehnFceNL/rYPSTk6WI=; b=CPxznGk9sU8MRdsMlJGqkmEKZ6Lx9jRRvt2blsPxx9IRr/KgnYX2/Q50lPoABV46qOx9yv pin9N25i4XFHbSxii7XDKu3EHu2YaUgIlg7dCBPMZG75NJEz86wzjQQN+cXe8dQkdYzkb2 X8LfYrHBORZMJ6onkLDreb70QTv8eoc= Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 18CF6AC46; Thu, 27 May 2021 07:39:50 +0000 (UTC) Date: Thu, 27 May 2021 09:39:49 +0200 From: Michal Hocko To: Feng Tang Cc: linux-mm@kvack.org, Andrew Morton , David Rientjes , Dave Hansen , Ben Widawsky , linux-kernel@vger.kernel.org, Andrea Arcangeli , Mel Gorman , Mike Kravetz , Randy Dunlap , Vlastimil Babka , Andi Kleen , Dan Williams , ying.huang@intel.com Subject: Re: [PATCH v1 2/4] mm/mempolicy: unify the preprocessing for mbind and set_mempolicy Message-ID: References: <1622005302-23027-1-git-send-email-feng.tang@intel.com> <1622005302-23027-3-git-send-email-feng.tang@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1622005302-23027-3-git-send-email-feng.tang@intel.com> Authentication-Results: imf26.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=CPxznGk9; dmarc=pass (policy=quarantine) header.from=suse.com; spf=pass (imf26.hostedemail.com: domain of mhocko@suse.com designates 195.135.220.15 as permitted sender) smtp.mailfrom=mhocko@suse.com X-Stat-Signature: qsjw9ph3wz1yg9zb7pyaz5yzcdj45aho X-Rspamd-Queue-Id: 6E77940002F1 X-Rspamd-Server: rspam02 X-HE-Tag: 1622101187-863632 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed 26-05-21 13:01:40, Feng Tang wrote: > Currently the kernel_mbind() and kernel_set_mempolicy() do almost > the same operation for parameter sanity check and preprocessing. > > Add a helper function to unify the code to reduce the redundancy, > and make it easier for changing the pre-processing code in future. > > [thanks to David Rientjes for suggesting using helper function > instead of macro] I appreciate removing the code duplication but I am not really convinced this is an improvement. You are conflating two things. One is the mpol flags checking and node mask copying. While abstracting the first one makes sense to me the later is already a single line of code that makes your helper unnecessarily complex. So I would go with sanitize_mpol_flags and put a flags handling there and leave get_nodes alone. > Signed-off-by: Feng Tang > --- > mm/mempolicy.c | 43 ++++++++++++++++++++++++------------------- > 1 file changed, 24 insertions(+), 19 deletions(-) Funny how removing code duplication adds more code than it removes ;) > > diff --git a/mm/mempolicy.c b/mm/mempolicy.c > index 1964cca..2830bb8 100644 > --- a/mm/mempolicy.c > +++ b/mm/mempolicy.c > @@ -1460,6 +1460,20 @@ static int copy_nodes_to_user(unsigned long __user *mask, unsigned long maxnode, > return copy_to_user(mask, nodes_addr(*nodes), copy) ? -EFAULT : 0; > } > > +static inline int mpol_pre_process(int *mode, const unsigned long __user *nmask, unsigned long maxnode, nodemask_t *nodes, unsigned short *flags) > +{ > + int ret; > + > + *flags = *mode & MPOL_MODE_FLAGS; > + *mode &= ~MPOL_MODE_FLAGS; > + if ((unsigned int)(*mode) >= MPOL_MAX) > + return -EINVAL; > + if ((*flags & MPOL_F_STATIC_NODES) && (*flags & MPOL_F_RELATIVE_NODES)) > + return -EINVAL; > + ret = get_nodes(nodes, nmask, maxnode); > + return ret; > +} > + > static long kernel_mbind(unsigned long start, unsigned long len, > unsigned long mode, const unsigned long __user *nmask, > unsigned long maxnode, unsigned int flags) > @@ -1467,19 +1481,14 @@ static long kernel_mbind(unsigned long start, unsigned long len, > nodemask_t nodes; > int err; > unsigned short mode_flags; > + int lmode = mode; > > - start = untagged_addr(start); > - mode_flags = mode & MPOL_MODE_FLAGS; > - mode &= ~MPOL_MODE_FLAGS; > - if (mode >= MPOL_MAX) > - return -EINVAL; > - if ((mode_flags & MPOL_F_STATIC_NODES) && > - (mode_flags & MPOL_F_RELATIVE_NODES)) > - return -EINVAL; > - err = get_nodes(&nodes, nmask, maxnode); > + err = mpol_pre_process(&lmode, nmask, maxnode, &nodes, &mode_flags); > if (err) > return err; > - return do_mbind(start, len, mode, mode_flags, &nodes, flags); > + > + start = untagged_addr(start); > + return do_mbind(start, len, lmode, mode_flags, &nodes, flags); > } > > SYSCALL_DEFINE6(mbind, unsigned long, start, unsigned long, len, > @@ -1495,18 +1504,14 @@ static long kernel_set_mempolicy(int mode, const unsigned long __user *nmask, > { > int err; > nodemask_t nodes; > - unsigned short flags; > + unsigned short mode_flags; > + int lmode = mode; > > - flags = mode & MPOL_MODE_FLAGS; > - mode &= ~MPOL_MODE_FLAGS; > - if ((unsigned int)mode >= MPOL_MAX) > - return -EINVAL; > - if ((flags & MPOL_F_STATIC_NODES) && (flags & MPOL_F_RELATIVE_NODES)) > - return -EINVAL; > - err = get_nodes(&nodes, nmask, maxnode); > + err = mpol_pre_process(&lmode, nmask, maxnode, &nodes, &mode_flags); > if (err) > return err; > - return do_set_mempolicy(mode, flags, &nodes); > + > + return do_set_mempolicy(lmode, mode_flags, &nodes); > } > > SYSCALL_DEFINE3(set_mempolicy, int, mode, const unsigned long __user *, nmask, > -- > 2.7.4 -- Michal Hocko SUSE Labs