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, 5 Jan 2023 10:48:12 -0800
From:   Mike Kravetz <mike.kravetz@oracle.com>
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Muchun Song <songmuchun@bytedance.com>,
        Nadav Amit <nadav.amit@gmail.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        David Hildenbrand <david@redhat.com>,
        James Houghton <jthoughton@google.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH 2/3] mm/mprotect: Use long for page accountings and retval
Message-ID: <Y7cbbLAZIxcL77+f@monkey>
References: <20230104225207.1066932-1-peterx@redhat.com>
 <20230104225207.1066932-3-peterx@redhat.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230104225207.1066932-3-peterx@redhat.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?v0wsM0LxSFNHIOhaXVpJryb00C8zUffcTeEQY6oOrmGBBqd+Jo0uSA9pFctl?=
 =?us-ascii?Q?EQokVY+hB9rOuCs5AxEzcLzSFPfMn5Q1Em9gEw26wI3xazmVne1LI3HhpaoW?=
 =?us-ascii?Q?iunECBsSigF6vfGQpQvaSD2xiLLgyhQTpZBMXvUSGz8kruWD/w/rzxWCe/NZ?=
 =?us-ascii?Q?MGGOWWgmQ5kx9ym/0t/mjpK9yPGn1lAlHl5I6Dqh/ToSF49Md/6+oHsJ8PC0?=
 =?us-ascii?Q?E0NO7p84+ufdFM7jDsefT+iA9c2rmGgMHh/9pDegJg+GzlNDRMfFswC2end+?=
 =?us-ascii?Q?zWPVyomdtvIgEaSiKo2IK03Pf1a3D/6wNSpDkErBJCy/zbrr1fbIvM8mWTuT?=
 =?us-ascii?Q?Wx0KyWvHkcuBeM0sznK/TwCbwU5LL8vI24i5dGzYI0sc7vwwVpgr5Oxlhi2E?=
 =?us-ascii?Q?hx3lR0gTnOfPTHmJ4qvZCzJnj1htwgaxSTU2v+kArFB1PtI1nynMzdEKOw86?=
 =?us-ascii?Q?xI/VcYKg8oR23NLFo6qAIi4f9snT10fmbhpDDHtD4kmvEIi2/QjnSeNY/wb3?=
 =?us-ascii?Q?RV4/ycKPA5A4FCRaz0Luhc2Cb22vnowyH+1dvVtz3w+rRKq83I+tCJCihCEy?=
 =?us-ascii?Q?13umbdJzknuTAEYJaXvuN0BcMH2bd3kYwqkOiv/6AQkF8ZoqNjYd6ez7jTxg?=
 =?us-ascii?Q?SZm7zJcLZCI+ufj+FP+9I4BUBruxFqJZHJJcLzsFNm8v8gBR+ZVWyM6l/b/Q?=
 =?us-ascii?Q?lskCeMcTpP6MqKKGLs21SEEmMbxPMPrSAcsRoFqFuL38us1ket0X3upDszVk?=
 =?us-ascii?Q?H4xWSiLwG970TCl6wF1D6i0qipiH+mIpLVDzH4OJWyda5jQPo7MqkbZbCrTt?=
 =?us-ascii?Q?XN96OflEG47eMkpu0ZkyRIObcJo+2fRbt6juEbu1Jwrj58dBLlaQ9A7IDuvP?=
 =?us-ascii?Q?+S13ZssuZxSz4yuH5/RkzPSih2hmgj/eKIHONMc0EwjPYXlz7JPbY5sykmj1?=
 =?us-ascii?Q?tBAbSq6FsQhJyE3ApEThPVSoSYgwKKXSIdt2Na4lMrCh0r4oOG4qVxmVpSic?=
 =?us-ascii?Q?KVe+hFjc9zB/kveC/JUIuwzklJgcQcP3ZXaOhFjjiARuMXRGBhmlZJzeaIw8?=
 =?us-ascii?Q?acfKXhNpqMwp0ezXKk5C7cZf1aVd6D9egMoUHEniHpSphVwUVujQETHoeK4k?=
 =?us-ascii?Q?iB/hNDl4IuzMiBBBh777qZ41V01p924v1ryUr4vFA7icl/02erDhA3oQTIt6?=
 =?us-ascii?Q?kXMK4U242zdcgXTsgC5RmRtDIavQNun/bSbZFwY+thPftgsIdewhzeqjPSwM?=
 =?us-ascii?Q?m7vQzqbXqA8ugJf9ka42NXM9eJyCV/hThni/0R2qzizAh4ZFlihP3KECk1G3?=
 =?us-ascii?Q?ofNsxZ3azi5QhPWe+PuGF0ZdURbPNLWxf1VsecPILWbwe95EwU5GKFQEDWzb?=
 =?us-ascii?Q?Vo0OWNShkD083ltS/iieTAJYb96Q63YoeFx0pLtI70umhpCFV+ZJSXaqGLQF?=
 =?us-ascii?Q?QQrs5cx64IYIiZ2RNzjcUfMY3HtUMVxa97QaWsk0Kb9LOIGmk5v9GPxoO731?=
 =?us-ascii?Q?DT5cccd70MOBS7RULMzL1W4gus0Ybg7iXvjQqb0miUi1ZwnjhuLjQ9cdV0if?=
 =?us-ascii?Q?neguV5xZJbWrfp1RpzAXV+9fiu7MOyYHoexf2ZR2u4zj8NZzRb++BiEnf6Cf?=
 =?us-ascii?Q?6g=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?mGqpWNr6FqX+0AWjzRQHaY67oAglPrsvGB/BA2scqxQDCbNNrtX7xcrWyXFq?=
 =?us-ascii?Q?VkZmF8XdqlChQfY93WL3mwpSBRJfEZqs4DrhVucY6zkZ9M+0sfhMd5CDXV4v?=
 =?us-ascii?Q?802xID1MNaz5NqPUNJMs2aNvYwvN+Q4xiFhOVn+PV1xMMCOQFpfWgGRP5/K/?=
 =?us-ascii?Q?7Ug84uVn5un71lckFHNwSMJFIqKfoG3uSrUfBsjEaqHrJGZc9s/wNn2UFIg8?=
 =?us-ascii?Q?SnGddgmJLv6N9zpfFdUNS9vqwvXtzoPt1ZeUZYn4TfkK1hggzPZxzd/8nsjZ?=
 =?us-ascii?Q?bU7eGd0A/GRZX9EXtdxicnoUbE5ljzRoFuljSdhxsGrqEYkH6hUmnr4BhBRK?=
 =?us-ascii?Q?oOwSWpHSwYzAsmAQ+bVszX+aQp2iVP5Erwcl/qLgE6fdfP6A+Rk471fMHubc?=
 =?us-ascii?Q?qS+PFAMzSq3cdLyh2YterDgjkaSOMeI6VNF1YJD8vA8IXXu7pOvi85gY0DuS?=
 =?us-ascii?Q?XkITz4QUiV13O05S6L3BAyXgyK5Mu/Y3gD3LhYuCmIcXYV8OKMtcWa1Q/en4?=
 =?us-ascii?Q?RhYWo5aQq1vg0wZbkLlr4YeMaV/Gqh4YIlqT9PBHH6P1E0J1MXwu9bRL3akc?=
 =?us-ascii?Q?O0qUGwdUWmWs6qqvWUAQFgZcG+9rsqiCGc+dgOapFqn1JXqFbURSz3ZemMOV?=
 =?us-ascii?Q?jOZQcP5w+LdJa9+6nj6vLJLkkgkvF2H53k0pusSto1XHkaRtyOFZ271xXztc?=
 =?us-ascii?Q?5HL+XVQ7H8AmOznkfBOg7OzYWSMhq1IG+RRj+l+19VR65IkUd5kgL8K/M1Mv?=
 =?us-ascii?Q?uYuMfF0XRHGCO4Hq1REsPOx5nvTUHeECUxRtsRArqXZFLFLrLLEPsqRRMCGm?=
 =?us-ascii?Q?3y/GMQqUmXFsOC8ItvYyhT3lsf8vsASiY+zqKKM8Fbh1zlrwNHok33Z+e5O5?=
 =?us-ascii?Q?ZdhcZRq4YFg9o5f5CzH+c0WjKyNufXblvAJuapPOYlS7FGsCDAwjunx2Jzdo?=
 =?us-ascii?Q?0OiAIIaMLTZw9ILJVLSXvWxsktJHdlF/QqeCBP8LT78xsXED9Yydu9p/Kure?=
 =?us-ascii?Q?WxYUSrAmg6izBFIN+NBMkOaqz6NRm7Lg4+owH+3Jr2/ZbsU=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d225651b-ab79-4b6b-8642-08daef4d679b
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jan 2023 18:48:15.7674
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: P3Vmyl8Mw/osal6pNdVVp2dm7x+ZGzKXx2mG9CcZUR/9cuGPXM/9x+s2Sy3cYsLvL+yFlyB/VP9rXnORy33lZQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4687
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.923,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2023-01-05_10,2023-01-05_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0 spamscore=0
 mlxlogscore=999 malwarescore=0 phishscore=0 adultscore=0 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2212070000
 definitions=main-2301050148
X-Proofpoint-GUID: O8nkGx2J7z_En8a4uylZ18z4PI8oV3uK
X-Proofpoint-ORIG-GUID: O8nkGx2J7z_En8a4uylZ18z4PI8oV3uK
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 01/04/23 17:52, Peter Xu wrote:
> Switch to use type "long" for page accountings and retval across the whole
> procedure of change_protection().
> 
> The change should have shrinked the possible maximum page number to be half
> comparing to previous (ULONG_MAX / 2), but it shouldn't overflow on any
> system either because the maximum possible pages touched by change
> protection should be ULONG_MAX / PAGE_SIZE.
> 
> Two reasons to switch from "unsigned long" to "long":
> 
>   1. It suites better on count_vm_numa_events(), whose 2nd parameter takes
>      a long type.
> 
>   2. It paves way for returning negative (error) values in the future.
> 
> Currently the only caller that consumes this retval is change_prot_numa(),
> where the unsigned long was converted to an int.  Since at it, touching up
> the numa code to also take a long, so it'll avoid any possible overflow too
> during the int-size convertion.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/linux/hugetlb.h |  4 ++--
>  include/linux/mm.h      |  2 +-
>  mm/hugetlb.c            |  4 ++--
>  mm/mempolicy.c          |  2 +-
>  mm/mprotect.c           | 26 +++++++++++++-------------
>  5 files changed, 19 insertions(+), 19 deletions(-)

Acked-by: Mike Kravetz <mike.kravetz@oracle.com>

> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index b6b10101bea7..e3aa336df900 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -248,7 +248,7 @@ void hugetlb_vma_lock_release(struct kref *kref);
>  
>  int pmd_huge(pmd_t pmd);
>  int pud_huge(pud_t pud);
> -unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
> +long hugetlb_change_protection(struct vm_area_struct *vma,
>  		unsigned long address, unsigned long end, pgprot_t newprot,
>  		unsigned long cp_flags);
>  
> @@ -437,7 +437,7 @@ static inline void move_hugetlb_state(struct folio *old_folio,
>  {
>  }
>  
> -static inline unsigned long hugetlb_change_protection(
> +static inline long hugetlb_change_protection(
>  			struct vm_area_struct *vma, unsigned long address,
>  			unsigned long end, pgprot_t newprot,
>  			unsigned long cp_flags)
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c37f9330f14e..86fe17e6ded7 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2132,7 +2132,7 @@ static inline bool vma_wants_manual_pte_write_upgrade(struct vm_area_struct *vma
>  }
>  bool can_change_pte_writable(struct vm_area_struct *vma, unsigned long addr,
>  			     pte_t pte);
> -extern unsigned long change_protection(struct mmu_gather *tlb,
> +extern long change_protection(struct mmu_gather *tlb,
>  			      struct vm_area_struct *vma, unsigned long start,
>  			      unsigned long end, unsigned long cp_flags);
>  extern int mprotect_fixup(struct mmu_gather *tlb, struct vm_area_struct *vma,
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 017d9159cddf..84bc665c7c86 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6613,7 +6613,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
>  	return i ? i : err;
>  }
>  
> -unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
> +long hugetlb_change_protection(struct vm_area_struct *vma,
>  		unsigned long address, unsigned long end,
>  		pgprot_t newprot, unsigned long cp_flags)
>  {
> @@ -6622,7 +6622,7 @@ unsigned long hugetlb_change_protection(struct vm_area_struct *vma,
>  	pte_t *ptep;
>  	pte_t pte;
>  	struct hstate *h = hstate_vma(vma);
> -	unsigned long pages = 0, psize = huge_page_size(h);
> +	long pages = 0, psize = huge_page_size(h);

Small nit,
psize is passed to routines as an unsigned long argument.  Arithmetic
should always be correct, but I am not sure if some of the static
checkers may complain.
-- 
Mike Kravetz

>  	bool shared_pmd = false;
>  	struct mmu_notifier_range range;
>  	unsigned long last_addr_mask;