| Message ID | 20220610135229.182859-4-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/demotion: Memory tiers and demotion | expand |
On Fri, 2022-06-10 at 19:22 +0530, Aneesh Kumar K.V wrote: > With CONFIG_MIGRATION disabled return EINVAL on write. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > mm/memory-tiers.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > index 9c6b40d7e0bf..c3123a457d90 100644 > --- a/mm/memory-tiers.c > +++ b/mm/memory-tiers.c > @@ -105,6 +105,9 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj, > { > ssize_t ret; > > > + if (!IS_ENABLED(CONFIG_MIGRATION)) > + return -EINVAL; > + How about enclose numa_demotion_enabled_xxx related code with CONFIG_MIGRATION? > ret = kstrtobool(buf, &numa_demotion_enabled); > if (ret) > return ret; Best Regards, Huang, Ying
On 6/13/22 8:56 AM, Ying Huang wrote: > On Fri, 2022-06-10 at 19:22 +0530, Aneesh Kumar K.V wrote: >> With CONFIG_MIGRATION disabled return EINVAL on write. >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> mm/memory-tiers.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c >> index 9c6b40d7e0bf..c3123a457d90 100644 >> --- a/mm/memory-tiers.c >> +++ b/mm/memory-tiers.c >> @@ -105,6 +105,9 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj, >> { >> ssize_t ret; >> >> >> + if (!IS_ENABLED(CONFIG_MIGRATION)) >> + return -EINVAL; >> + > > How about enclose numa_demotion_enabled_xxx related code with CONFIG_MIGRATION? > IIUC there is a desire to use IS_ENABLED() in the kernel instead of #ifdef since that helps in more compile time checks. Because there are no dead codes during compile now with IS_ENABLED(). W.r.t leaving the sysfs file visible even when CONFIG_MIGRATION is disabled, I was thinking it gives better visibility into numa_demotion status. I could switch to hide numa_demotion file if that is desirable. >> ret = kstrtobool(buf, &numa_demotion_enabled); >> if (ret) >> return ret; > -aneesh
On Mon, 2022-06-13 at 09:05 +0530, Aneesh Kumar K V wrote: > On 6/13/22 8:56 AM, Ying Huang wrote: > > On Fri, 2022-06-10 at 19:22 +0530, Aneesh Kumar K.V wrote: > > > With CONFIG_MIGRATION disabled return EINVAL on write. > > > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > > > --- > > > mm/memory-tiers.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > > > index 9c6b40d7e0bf..c3123a457d90 100644 > > > --- a/mm/memory-tiers.c > > > +++ b/mm/memory-tiers.c > > > @@ -105,6 +105,9 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj, > > > { > > > ssize_t ret; > > > > > > > > > > > > + if (!IS_ENABLED(CONFIG_MIGRATION)) > > > + return -EINVAL; > > > + > > > > How about enclose numa_demotion_enabled_xxx related code with CONFIG_MIGRATION? > > > > IIUC there is a desire to use IS_ENABLED() in the kernel instead of > #ifdef since that helps in more compile time checks. Because there are > no dead codes during compile now with IS_ENABLED(). IS_ENABLED() is used to reduce usage of "#ifdef" in ".c" file, especially inside a function. We have good build test coverage with 0Day now. To avoid code size inflate, it's better to use #ifdef CONFIG_MIGRATION. > W.r.t leaving the sysfs file visible even when CONFIG_MIGRATION is > disabled, I was thinking it gives better visibility into numa_demotion > status. I could switch to hide numa_demotion file if that is desirable. > > > > ret = kstrtobool(buf, &numa_demotion_enabled); > > > if (ret) > > > return ret; > > Best Regards, Huang, Ying
On 6/13/22 11:03 AM, Ying Huang wrote: > On Mon, 2022-06-13 at 09:05 +0530, Aneesh Kumar K V wrote: >> On 6/13/22 8:56 AM, Ying Huang wrote: >>> On Fri, 2022-06-10 at 19:22 +0530, Aneesh Kumar K.V wrote: >>>> With CONFIG_MIGRATION disabled return EINVAL on write. >>>> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>>> --- >>>> mm/memory-tiers.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c >>>> index 9c6b40d7e0bf..c3123a457d90 100644 >>>> --- a/mm/memory-tiers.c >>>> +++ b/mm/memory-tiers.c >>>> @@ -105,6 +105,9 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj, >>>> { >>>> ssize_t ret; >>>> >>>> >>>> >>>> + if (!IS_ENABLED(CONFIG_MIGRATION)) >>>> + return -EINVAL; >>>> + >>> >>> How about enclose numa_demotion_enabled_xxx related code with CONFIG_MIGRATION? >>> >> >> IIUC there is a desire to use IS_ENABLED() in the kernel instead of >> #ifdef since that helps in more compile time checks. Because there are >> no dead codes during compile now with IS_ENABLED(). > > IS_ENABLED() is used to reduce usage of "#ifdef" in ".c" file, > especially inside a function. We have good build test coverage with > 0Day now. > > To avoid code size inflate, it's better to use #ifdef CONFIG_MIGRATION. > For a diff like below I am finding IS_ENABLED better. size memory-tiers.o.isenabled memory-tiers.o text data bss dec hex filename 4776 989 5 5770 168a memory-tiers.o.isenabled 5257 990 5 6252 186c memory-tiers.o modified mm/memory-tiers.c @@ -710,12 +710,11 @@ static int __meminit migrate_on_reclaim_callback(struct notifier_block *self, static void __init migrate_on_reclaim_init(void) { - - if (IS_ENABLED(CONFIG_MIGRATION)) { +#ifdef CONFIG_MIGRATION node_demotion = kcalloc(MAX_NUMNODES, sizeof(struct demotion_nodes), GFP_KERNEL); WARN_ON(!node_demotion); - } +#endif hotplug_memory_notifier(migrate_on_reclaim_callback, 100); } @@ -844,14 +843,19 @@ static ssize_t numa_demotion_enabled_show(struct kobject *kobj, numa_demotion_enabled ? "true" : "false"); } +#ifdef CONFIG_MIGRATION static ssize_t numa_demotion_enabled_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - ssize_t ret; - - if (!IS_ENABLED(CONFIG_MIGRATION)) - return -EINVAL; + return -EINVAL; +} +#else +static ssize_t numa_demotion_enabled_store(struct kobject *kobj, + struct kobj_attribute *attr, + const char *buf, size_t count) +{ + ssize_t ret; ret = kstrtobool(buf, &numa_demotion_enabled); if (ret) @@ -859,6 +863,7 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj, return count; } +#endif static struct kobj_attribute numa_demotion_enabled_attr = __ATTR(demotion_enabled, 0644, numa_demotion_enabled_show, I also find that #ifdef config not easier to the eyes. If there is a large code that we can end up #ifdef out, then it might be worth it. IIUC, we might want to keep the establish_migration target to find top_tier rank and lower_tier mask. Once we do that only thing that we could comment out is the node_demotion sysfs creation and I was considering to keep that even if migration is disabled with a write to the file returning EINVAL. I could switch that if you strongly feel that we should hide node_demotion sysfs file. -aneesh
On Mon, 2022-06-13 at 11:18 +0530, Aneesh Kumar K V wrote: > On 6/13/22 11:03 AM, Ying Huang wrote: > > On Mon, 2022-06-13 at 09:05 +0530, Aneesh Kumar K V wrote: > > > On 6/13/22 8:56 AM, Ying Huang wrote: > > > > On Fri, 2022-06-10 at 19:22 +0530, Aneesh Kumar K.V wrote: > > > > > With CONFIG_MIGRATION disabled return EINVAL on write. > > > > > > > > > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > > > > > --- > > > > > mm/memory-tiers.c | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > > > > > index 9c6b40d7e0bf..c3123a457d90 100644 > > > > > --- a/mm/memory-tiers.c > > > > > +++ b/mm/memory-tiers.c > > > > > @@ -105,6 +105,9 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj, > > > > > { > > > > > ssize_t ret; > > > > > > > > > > > > > > > > > > > > > > > > > + if (!IS_ENABLED(CONFIG_MIGRATION)) > > > > > + return -EINVAL; > > > > > + > > > > > > > > How about enclose numa_demotion_enabled_xxx related code with CONFIG_MIGRATION? > > > > > > > > > > IIUC there is a desire to use IS_ENABLED() in the kernel instead of > > > #ifdef since that helps in more compile time checks. Because there are > > > no dead codes during compile now with IS_ENABLED(). > > > > IS_ENABLED() is used to reduce usage of "#ifdef" in ".c" file, > > especially inside a function. We have good build test coverage with > > 0Day now. > > > > To avoid code size inflate, it's better to use #ifdef CONFIG_MIGRATION. > > > > For a diff like below I am finding IS_ENABLED better. > > size memory-tiers.o.isenabled memory-tiers.o > text data bss dec hex filename > 4776 989 5 5770 168a memory-tiers.o.isenabled > 5257 990 5 6252 186c memory-tiers.o > > > modified mm/memory-tiers.c > @@ -710,12 +710,11 @@ static int __meminit > migrate_on_reclaim_callback(struct notifier_block *self, > > static void __init migrate_on_reclaim_init(void) > { > - > - if (IS_ENABLED(CONFIG_MIGRATION)) { > +#ifdef CONFIG_MIGRATION > node_demotion = kcalloc(MAX_NUMNODES, sizeof(struct demotion_nodes), > GFP_KERNEL); > WARN_ON(!node_demotion); > - } > +#endif > hotplug_memory_notifier(migrate_on_reclaim_callback, 100); > } > > @@ -844,14 +843,19 @@ static ssize_t numa_demotion_enabled_show(struct > kobject *kobj, > numa_demotion_enabled ? "true" : "false"); > } > > +#ifdef CONFIG_MIGRATION > static ssize_t numa_demotion_enabled_store(struct kobject *kobj, > struct kobj_attribute *attr, > const char *buf, size_t count) > { > - ssize_t ret; > - > - if (!IS_ENABLED(CONFIG_MIGRATION)) > - return -EINVAL; > + return -EINVAL; > +} > +#else > +static ssize_t numa_demotion_enabled_store(struct kobject *kobj, > + struct kobj_attribute *attr, > + const char *buf, size_t count) > +{ > + ssize_t ret; > > ret = kstrtobool(buf, &numa_demotion_enabled); > if (ret) > @@ -859,6 +863,7 @@ static ssize_t numa_demotion_enabled_store(struct > kobject *kobj, > > return count; > } > +#endif > > static struct kobj_attribute numa_demotion_enabled_attr = > __ATTR(demotion_enabled, 0644, numa_demotion_enabled_show, > > I also find that #ifdef config not easier to the eyes. If there is a > large code that we can end up #ifdef out, then it might be worth it. > IIUC, we might want to keep the establish_migration target to find > top_tier rank and lower_tier mask. Once we do that only thing that we > could comment out is the node_demotion sysfs creation and I was > considering to keep that even if migration is disabled with a write to > the file returning EINVAL. I could switch that if you strongly feel that > we should hide node_demotion sysfs file. Per my understanding, we can enclose most code about demoting/promoting inside CONFIG_MIGRATION, including numa/demotion_enabled sysfs interface. In this way, the code size can be reduced. Best Regards, Huang, Ying
diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c index 9c6b40d7e0bf..c3123a457d90 100644 --- a/mm/memory-tiers.c +++ b/mm/memory-tiers.c @@ -105,6 +105,9 @@ static ssize_t numa_demotion_enabled_store(struct kobject *kobj, { ssize_t ret; + if (!IS_ENABLED(CONFIG_MIGRATION)) + return -EINVAL; + ret = kstrtobool(buf, &numa_demotion_enabled); if (ret) return ret;
With CONFIG_MIGRATION disabled return EINVAL on write. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- mm/memory-tiers.c | 3 +++ 1 file changed, 3 insertions(+)