[PATCH 0/2] mm: KSM: fix data types

[PATCH 0/2] mm: KSM: fix data types

[Zhansaya Bagdauletkyzy]

Change data types of some variables as there were a few discrepancies
between the types and actual values that were stored.

Zhansaya Bagdauletkyzy (2):
  mm: KSM: fix ksm_run data type
  mm: KSM: fix data type

 mm/ksm.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH 1/2] mm: KSM: fix ksm_run data type

[Zhansaya Bagdauletkyzy]

ksm_run is declared as unsigned long but in run_store(), it is converted
to unsigned int. Change its type to unsigned int.

Signed-off-by: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com>
---
 mm/ksm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 3fa9bc8a67cf..057d0c245bf4 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -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,
-- 
2.25.1

Re: [PATCH 1/2] mm: KSM: fix ksm_run data type

[Pavel Tatashin]

On Thu, Jul 15, 2021 at 2:01 PM Zhansaya Bagdauletkyzy
<zhansayabagdaulet@gmail.com> wrote:
>
> ksm_run is declared as unsigned long but in run_store(), it is converted
> to unsigned int. Change its type to unsigned int.
>
> Signed-off-by: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com>
> ---
>  mm/ksm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 3fa9bc8a67cf..057d0c245bf4 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -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);

Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>

Re: [PATCH 1/2] mm: KSM: fix ksm_run data type

[Matthew Wilcox]

On Fri, Jul 16, 2021 at 12:01:01AM +0600, Zhansaya Bagdauletkyzy wrote:
> +++ b/mm/ksm.c
> @@ -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;

Should this be an enum instead?

Re: [PATCH 1/2] mm: KSM: fix ksm_run data type

[Pavel Tatashin]

On Thu, Jul 15, 2021 at 2:18 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Fri, Jul 16, 2021 at 12:01:01AM +0600, Zhansaya Bagdauletkyzy wrote:
> > +++ b/mm/ksm.c
> > @@ -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;
>
> Should this be an enum instead?

I think "unsigned int" is OK here, as it is exposed as uint to users:
Documentation/ABI/testing/sysfs-kernel-mm-ksm

/sys/kernel/mm/ksm/run

run: write 0 to disable ksm, read 0 while ksm is disabled.

- write 1 to run ksm, read 1 while ksm is running.
- write 2 to disable ksm and unmerge all its pages.

Pasha

Re: [PATCH 1/2] mm: KSM: fix ksm_run data type

[Matthew Wilcox]

On Thu, Jul 15, 2021 at 02:21:21PM -0400, Pavel Tatashin wrote:
> On Thu, Jul 15, 2021 at 2:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Fri, Jul 16, 2021 at 12:01:01AM +0600, Zhansaya Bagdauletkyzy wrote:
> > > +++ b/mm/ksm.c
> > > @@ -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;
> >
> > Should this be an enum instead?
> 
> I think "unsigned int" is OK here, as it is exposed as uint to users:
> Documentation/ABI/testing/sysfs-kernel-mm-ksm
> 
> /sys/kernel/mm/ksm/run
> 
> run: write 0 to disable ksm, read 0 while ksm is disabled.
> 
> - write 1 to run ksm, read 1 while ksm is running.
> - write 2 to disable ksm and unmerge all its pages.

The document is out of date then as it does not mention 'offline'.

Also, why does the call to kstrtouint() specify base 10?  If it is a
bitmap, then permitting 0x [1] is more natural.  I would expect to see
base 0 there.

[1] or even 0b, although I see that _parse_integer_fixup_radix does not
support the 0b notation.

Re: [PATCH 1/2] mm: KSM: fix ksm_run data type

[Pavel Tatashin]

On Thu, Jul 15, 2021 at 2:30 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Jul 15, 2021 at 02:21:21PM -0400, Pavel Tatashin wrote:
> > On Thu, Jul 15, 2021 at 2:18 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Fri, Jul 16, 2021 at 12:01:01AM +0600, Zhansaya Bagdauletkyzy wrote:
> > > > +++ b/mm/ksm.c
> > > > @@ -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;
> > >
> > > Should this be an enum instead?
> >
> > I think "unsigned int" is OK here, as it is exposed as uint to users:
> > Documentation/ABI/testing/sysfs-kernel-mm-ksm
> >
> > /sys/kernel/mm/ksm/run
> >
> > run: write 0 to disable ksm, read 0 while ksm is disabled.
> >
> > - write 1 to run ksm, read 1 while ksm is running.
> > - write 2 to disable ksm and unmerge all its pages.
>
> The document is out of date then as it does not mention 'offline'.

The offline mode cannot be set externally.

In run_store()
   if (flags > KSM_RUN_UNMERGE)
      return -EINVAL;

>
> Also, why does the call to kstrtouint() specify base 10?  If it is a
> bitmap, then permitting 0x [1] is more natural.  I would expect to see
> base 0 there.

Users can only write 0, 1, or 2, it is not a bitmap from the user's
perspective as the user cannot write: '3' . But, I think it is
somewhat weird that ksm_run is used as a bitmap internally with
KSM_RUN_OFFLINE = 4.

Imo, KSM_RUN_OFFLINE should be placed in a separate boolean from "ksm_run".

>
> [1] or even 0b, although I see that _parse_integer_fixup_radix does not
> support the 0b notation.

Re: [PATCH 1/2] mm: KSM: fix ksm_run data type

[Pavel Tatashin]

> > > /sys/kernel/mm/ksm/run
> > >
> > > run: write 0 to disable ksm, read 0 while ksm is disabled.
> > >
> > > - write 1 to run ksm, read 1 while ksm is running.
> > > - write 2 to disable ksm and unmerge all its pages.
> >
> > The document is out of date then as it does not mention 'offline'.
>
> The offline mode cannot be set externally.
>
> In run_store()
>    if (flags > KSM_RUN_UNMERGE)
>       return -EINVAL;

However, I see that KSM_RUN_OFFLINE is visible via run_show(), so yes
doc should be updated. And, yes it becomes a bitfield from the user
perspective.

>
> >
> > Also, why does the call to kstrtouint() specify base 10?  If it is a
> > bitmap, then permitting 0x [1] is more natural.  I would expect to see
> > base 0 there.
>
> Users can only write 0, 1, or 2, it is not a bitmap from the user's
> perspective as the user cannot write: '3' . But, I think it is
> somewhat weird that ksm_run is used as a bitmap internally with
> KSM_RUN_OFFLINE = 4.
>
> Imo, KSM_RUN_OFFLINE should be placed in a separate boolean from "ksm_run".
>
> >
> > [1] or even 0b, although I see that _parse_integer_fixup_radix does not
> > support the 0b notation.

[PATCH 2/2] mm: KSM: fix data type

[Zhansaya Bagdauletkyzy]

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 the variable type to unsigned int.

Signed-off-by: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com>
---
 mm/ksm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/mm/ksm.c b/mm/ksm.c
index 057d0c245bf4..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;
@@ -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

Re: [PATCH 2/2] mm: KSM: fix data type

[Pavel Tatashin]

On Thu, Jul 15, 2021 at 2:01 PM Zhansaya Bagdauletkyzy
<zhansayabagdaulet@gmail.com> wrote:
>
> 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 the variable type to unsigned int.
>
> Signed-off-by: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com>
> ---
>  mm/ksm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 057d0c245bf4..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;
> @@ -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;

LGTM, but I would merge the two patches together. They both update
types of sysfs tunnables in the same file.

Reviewed-by: Pavel Tatashin <pasha.tatashin@soleen.com>


> --
> 2.25.1
>

Re: [PATCH 2/2] mm: KSM: fix data type

[Zhansaya Bagdauletkyzy]

On Thu, Jul 15, 2021 at 02:10:33PM -0400, Pavel Tatashin wrote:
> On Thu, Jul 15, 2021 at 2:01 PM Zhansaya Bagdauletkyzy
> <zhansayabagdaulet@gmail.com> wrote:
> >
> > 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 the variable type to unsigned int.
> >
> > Signed-off-by: Zhansaya Bagdauletkyzy <zhansayabagdaulet@gmail.com>
> > ---
> >  mm/ksm.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/ksm.c b/mm/ksm.c
> > index 057d0c245bf4..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;
> > @@ -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;
> 
> LGTM, but I would merge the two patches together. They both update
> types of sysfs tunnables in the same file.

Ok, I'll send v2 as a single patch.

Thanks,
Zhansaya