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:   Fri, 16 Jul 2021 13:50:10 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com>
cc:     akpm@linux-foundation.org, tyhicks@linux.microsoft.com,
        pasha.tatashin@soleen.com, willy@infradead.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mm: KSM: fix data types
In-Reply-To: <20210716055800.GA255851@asus>
Message-ID: <181bd7fd-dcb6-3dea-305c-5764603d7d45@google.com>
References: <20210716055800.GA255851@asus>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 16 Jul 2021, Zhansaya Bagdauletkyzy wrote:

> Change data types of ksm_run and ksm_stable_node_chains_prune_millisecs
> as there were a few discrepancies between the types and actual values
> that were stored:
> 1) ksm_run is declared as unsigned long but in run_store(), it is
> converted to unsigned int. Change its type to unsigned int.

NAK to that part.

Compiling with CONFIG_MEMORY_HOTREMOVE=y would show why.

Search for where ksm_run is used: in one place there's
	wait_on_bit(&ksm_run, ilog2(KSM_RUN_OFFLINE), TASK_UNINTERRUPTIBLE);
and wait_on_bit() operates on unsigned long *word.

Before that wait while offlining, yes, ksm_run used to be an unsigned int.

But there's nothing to be fixed by changing it to unsigned int now.
Save four bytes?  Better look elsewhere if you want to save four bytes.

> 2) ksm_stable_node_chains_prune_millisecs is declared as int, but in
> stable__node_chains_prune_millisecs_store(), it can store values up to
> UINT_MAX. Change its type to unsigned int.

Ack to that part.

> 
> Signed-off-by: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com>
> ---
> v1 -> v2:
> - merge two patches into one

Sorry, that didn't work out so well.

Hugh

> 
>  mm/ksm.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 3fa9bc8a67cf..2e4bd7662e52 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -259,7 +259,7 @@ static unsigned long ksm_stable_node_chains;
>  static unsigned long ksm_stable_node_dups;
>  
>  /* Delay in pruning stale stable_node_dups in the stable_node_chains */
> -static int ksm_stable_node_chains_prune_millisecs = 2000;
> +static unsigned int ksm_stable_node_chains_prune_millisecs = 2000;
>  
>  /* Maximum number of page slots sharing a stable node */
>  static int ksm_max_page_sharing = 256;
> @@ -289,7 +289,7 @@ static int ksm_nr_node_ids = 1;
>  #define KSM_RUN_MERGE	1
>  #define KSM_RUN_UNMERGE	2
>  #define KSM_RUN_OFFLINE	4
> -static unsigned long ksm_run = KSM_RUN_STOP;
> +static unsigned int ksm_run = KSM_RUN_STOP;
>  static void wait_while_offlining(void);
>  
>  static DECLARE_WAIT_QUEUE_HEAD(ksm_thread_wait);
> @@ -2874,7 +2874,7 @@ KSM_ATTR(pages_to_scan);
>  static ssize_t run_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			char *buf)
>  {
> -	return sysfs_emit(buf, "%lu\n", ksm_run);
> +	return sysfs_emit(buf, "%u\n", ksm_run);
>  }
>  
>  static ssize_t run_store(struct kobject *kobj, struct kobj_attribute *attr,
> @@ -3105,11 +3105,11 @@ stable_node_chains_prune_millisecs_store(struct kobject *kobj,
>  					 struct kobj_attribute *attr,
>  					 const char *buf, size_t count)
>  {
> -	unsigned long msecs;
> +	unsigned int msecs;
>  	int err;
>  
> -	err = kstrtoul(buf, 10, &msecs);
> -	if (err || msecs > UINT_MAX)
> +	err = kstrtouint(buf, 10, &msecs);
> +	if (err)
>  		return -EINVAL;
>  
>  	ksm_stable_node_chains_prune_millisecs = msecs;
> -- 
> 2.25.1