| Message ID | 343394260f599d940cacc37f1dcc0309239ae220.1626371112.git.zhansayabagdaulet@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: KSM: fix data types | expand |
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>
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?
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
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.
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.
> > > /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.
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,
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(-)