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.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 AF290C4708A for ; Thu, 27 May 2021 12:31:26 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 656AF613AA for ; Thu, 27 May 2021 12:31:24 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 656AF613AA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C3FC46B0036; Thu, 27 May 2021 08:31:23 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C169A6B006E; Thu, 27 May 2021 08:31:23 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A90216B0070; Thu, 27 May 2021 08:31:23 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0159.hostedemail.com [216.40.44.159]) by kanga.kvack.org (Postfix) with ESMTP id 715C26B0036 for ; Thu, 27 May 2021 08:31:23 -0400 (EDT) Received: from smtpin11.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 13AE2181AF5CA for ; Thu, 27 May 2021 12:31:23 +0000 (UTC) X-FDA: 78186946446.11.B25B399 Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by imf09.hostedemail.com (Postfix) with ESMTP id 973D86000EC5 for ; Thu, 27 May 2021 12:31:17 +0000 (UTC) IronPort-SDR: V7FmpMDx4ljI5EEG/AGKYpBGYhb2VErO6yaz5355v3mQ8WMsUAH63O7d5WvpYaLgzvvuEJkzJr b2STO7GrzX6g== X-IronPort-AV: E=McAfee;i="6200,9189,9996"; a="190091786" X-IronPort-AV: E=Sophos;i="5.82,334,1613462400"; d="scan'208";a="190091786" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 May 2021 05:31:18 -0700 IronPort-SDR: jmoxIXcIfQT0DREIICnlWr7O6I0fUXakADm4PXnDMea8QXGI/mXODP6Ov4mQl/qKfPzDPoZiBZ ZlfEy3ZEuihw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.82,334,1613462400"; d="scan'208";a="443545354" Received: from shbuild999.sh.intel.com (HELO localhost) ([10.239.147.94]) by orsmga008.jf.intel.com with ESMTP; 27 May 2021 05:31:14 -0700 Date: Thu, 27 May 2021 20:31:13 +0800 From: Feng Tang To: Michal Hocko 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: <20210527123113.GB7743@shbuild999.sh.intel.com> 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: User-Agent: Mutt/1.5.24 (2015-08-30) X-Rspamd-Queue-Id: 973D86000EC5 Authentication-Results: imf09.hostedemail.com; dkim=none; dmarc=fail reason="No valid SPF, No valid DKIM" header.from=intel.com (policy=none); spf=none (imf09.hostedemail.com: domain of feng.tang@intel.com has no SPF policy when checking 134.134.136.126) smtp.mailfrom=feng.tang@intel.com X-Rspamd-Server: rspam04 X-Stat-Signature: 7jiae7dzd7w3phk5mi8qk3nx7sxxwgae X-HE-Tag: 1622118677-323657 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 Thu, May 27, 2021 at 09:39:49AM +0200, Michal Hocko wrote: > 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 ;) Yes. And in last verion which uses macro to unify the code: https://lore.kernel.org/lkml/1621499404-67756-3-git-send-email-feng.tang@intel.com/ it does save some lines :) mm/mempolicy.c | 46 +++++++++++++++++++--------------------------- 1 file changed, 19 insertions(+), 27 deletions(-) Thanks, Feng > > > > 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