| Message ID | 20220908081932.77370-1-xhao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/damon/sysfs: change few functions execute order | expand |
Hi Xin, On Thu, 8 Sep 2022 16:19:32 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > These nr_{schemes,regions,contexts,kdamonds}_store() functions are both call checkpatch complains as below: WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #10: These nr_{schemes,regions,contexts,kdamonds}_store() functions are both call > kstrtoint() to get relative values from sysfs interface, if it return an > error, there get kobject instance would be meaningless through 'container_of'. I was thinking the compiler could do that kind of optimization on its own, so I preferred to make the code shorter. This change makes the code slightly longer, while the benefit of the change is unclear. I'd like to keep it as is unless we get some clear benefit of this? Thanks, SJ > > Signed-off-by: Xin Hao <xhao@linux.alibaba.com> > --- > mm/damon/sysfs.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c > index 88f208ff235d..54fdfcdbb3e4 100644 > --- a/mm/damon/sysfs.c > +++ b/mm/damon/sysfs.c > @@ -1031,8 +1031,7 @@ static ssize_t nr_schemes_show(struct kobject *kobj, > static ssize_t nr_schemes_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_schemes *schemes = container_of(kobj, > - struct damon_sysfs_schemes, kobj); > + struct damon_sysfs_schemes *schemes; > int nr, err = kstrtoint(buf, 0, &nr); > > if (err) > @@ -1040,6 +1039,8 @@ static ssize_t nr_schemes_store(struct kobject *kobj, > if (nr < 0) > return -EINVAL; > > + schemes = container_of(kobj, struct damon_sysfs_schemes, kobj); > + > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_schemes_add_dirs(schemes, nr); > @@ -1237,8 +1238,7 @@ static ssize_t nr_regions_show(struct kobject *kobj, > static ssize_t nr_regions_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_regions *regions = container_of(kobj, > - struct damon_sysfs_regions, kobj); > + struct damon_sysfs_regions *regions; > int nr, err = kstrtoint(buf, 0, &nr); > > if (err) > @@ -1246,6 +1246,8 @@ static ssize_t nr_regions_store(struct kobject *kobj, > if (nr < 0) > return -EINVAL; > > + regions = container_of(kobj, struct damon_sysfs_regions, kobj); > + > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_regions_add_dirs(regions, nr); > @@ -1440,8 +1442,7 @@ static ssize_t nr_targets_show(struct kobject *kobj, > static ssize_t nr_targets_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_targets *targets = container_of(kobj, > - struct damon_sysfs_targets, kobj); > + struct damon_sysfs_targets *targets; > int nr, err = kstrtoint(buf, 0, &nr); > > if (err) > @@ -1449,6 +1450,8 @@ static ssize_t nr_targets_store(struct kobject *kobj, > if (nr < 0) > return -EINVAL; > > + targets = container_of(kobj, struct damon_sysfs_targets, kobj); > + > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_targets_add_dirs(targets, nr); > @@ -1962,8 +1965,7 @@ static ssize_t nr_contexts_show(struct kobject *kobj, > static ssize_t nr_contexts_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_contexts *contexts = container_of(kobj, > - struct damon_sysfs_contexts, kobj); > + struct damon_sysfs_contexts *contexts; > int nr, err; > > err = kstrtoint(buf, 0, &nr); > @@ -1973,6 +1975,7 @@ static ssize_t nr_contexts_store(struct kobject *kobj, > if (nr < 0 || 1 < nr) > return -EINVAL; > > + contexts = container_of(kobj, struct damon_sysfs_contexts, kobj); > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_contexts_add_dirs(contexts, nr); > @@ -2741,8 +2744,7 @@ static ssize_t nr_kdamonds_show(struct kobject *kobj, > static ssize_t nr_kdamonds_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_kdamonds *kdamonds = container_of(kobj, > - struct damon_sysfs_kdamonds, kobj); > + struct damon_sysfs_kdamonds *kdamonds; > int nr, err; > > err = kstrtoint(buf, 0, &nr); > @@ -2751,6 +2753,8 @@ static ssize_t nr_kdamonds_store(struct kobject *kobj, > if (nr < 0) > return -EINVAL; > > + kdamonds = container_of(kobj, struct damon_sysfs_kdamonds, kobj); > + > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_kdamonds_add_dirs(kdamonds, nr); > -- > 2.31.0
On Thu, 8 Sep 2022 18:49:47 +0000 SeongJae Park <sj@kernel.org> wrote: > Hi Xin, > > On Thu, 8 Sep 2022 16:19:32 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > > > These nr_{schemes,regions,contexts,kdamonds}_store() functions are both call > > checkpatch complains as below: > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > #10: > These nr_{schemes,regions,contexts,kdamonds}_store() functions are both call I fix that sort of thing. A lot ;) > > kstrtoint() to get relative values from sysfs interface, if it return an > > error, there get kobject instance would be meaningless through 'container_of'. > > I was thinking the compiler could do that kind of optimization on its own, so I > preferred to make the code shorter. Yes, the compiler will do this. > This change makes the code slightly longer, while the benefit of the change is > unclear. I'd like to keep it as is unless we get some clear benefit of this? I find it a readability improvement. Puts the initialization in a more appropriate place and avoids those nasty tricks to prevent unreasonable line lengths.
On Thu, 8 Sep 2022 15:10:42 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Thu, 8 Sep 2022 18:49:47 +0000 SeongJae Park <sj@kernel.org> wrote: > > > Hi Xin, > > > > On Thu, 8 Sep 2022 16:19:32 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > > > > > These nr_{schemes,regions,contexts,kdamonds}_store() functions are both call > > > > checkpatch complains as below: > > > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > > #10: > > These nr_{schemes,regions,contexts,kdamonds}_store() functions are both call > > I fix that sort of thing. A lot ;) Appreciate your efforts always. If you need any help from my side, please let me know at any time. > > > > kstrtoint() to get relative values from sysfs interface, if it return an > > > error, there get kobject instance would be meaningless through 'container_of'. > > > > I was thinking the compiler could do that kind of optimization on its own, so I > > preferred to make the code shorter. > > Yes, the compiler will do this. > > > This change makes the code slightly longer, while the benefit of the change is > > unclear. I'd like to keep it as is unless we get some clear benefit of this? > > I find it a readability improvement. Puts the initialization in a more > appropriate place and avoids those nasty tricks to prevent unreasonable > line lengths. Agreed. Reviewed-by: SeongJae Park <sj@kernel.org> Thanks, SJ
On Thu, 8 Sep 2022 22:16:53 +0000 SeongJae Park <sj@kernel.org> wrote: > Reviewed-by: SeongJae Park <sj@kernel.org> Cool. I rewrote the changelog significantly: From: Xin Hao <xhao@linux.alibaba.com> Subject: mm/damon/sysfs: change few functions execute order Date: Thu, 8 Sep 2022 16:19:32 +0800 There's no need to run container_of() as early as we do. The compiler figures this out, but the resulting code is more readable. Link: https://lkml.kernel.org/r/20220908081932.77370-1-xhao@linux.alibaba.com Signed-off-by: Xin Hao <xhao@linux.alibaba.com> Reviewed-by: SeongJae Park <sj@kernel.org> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- mm/damon/sysfs.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) --- a/mm/damon/sysfs.c~mm-damon-sysfs-change-few-functions-execute-order +++ a/mm/damon/sysfs.c @@ -1031,8 +1031,7 @@ static ssize_t nr_schemes_show(struct ko static ssize_t nr_schemes_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_schemes *schemes = container_of(kobj, - struct damon_sysfs_schemes, kobj); + struct damon_sysfs_schemes *schemes; int nr, err = kstrtoint(buf, 0, &nr); if (err) @@ -1040,6 +1039,8 @@ static ssize_t nr_schemes_store(struct k if (nr < 0) return -EINVAL; + schemes = container_of(kobj, struct damon_sysfs_schemes, kobj); + if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_schemes_add_dirs(schemes, nr); @@ -1237,8 +1238,7 @@ static ssize_t nr_regions_show(struct ko static ssize_t nr_regions_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_regions *regions = container_of(kobj, - struct damon_sysfs_regions, kobj); + struct damon_sysfs_regions *regions; int nr, err = kstrtoint(buf, 0, &nr); if (err) @@ -1246,6 +1246,8 @@ static ssize_t nr_regions_store(struct k if (nr < 0) return -EINVAL; + regions = container_of(kobj, struct damon_sysfs_regions, kobj); + if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_regions_add_dirs(regions, nr); @@ -1440,8 +1442,7 @@ static ssize_t nr_targets_show(struct ko static ssize_t nr_targets_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_targets *targets = container_of(kobj, - struct damon_sysfs_targets, kobj); + struct damon_sysfs_targets *targets; int nr, err = kstrtoint(buf, 0, &nr); if (err) @@ -1449,6 +1450,8 @@ static ssize_t nr_targets_store(struct k if (nr < 0) return -EINVAL; + targets = container_of(kobj, struct damon_sysfs_targets, kobj); + if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_targets_add_dirs(targets, nr); @@ -1962,8 +1965,7 @@ static ssize_t nr_contexts_show(struct k static ssize_t nr_contexts_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_contexts *contexts = container_of(kobj, - struct damon_sysfs_contexts, kobj); + struct damon_sysfs_contexts *contexts; int nr, err; err = kstrtoint(buf, 0, &nr); @@ -1973,6 +1975,7 @@ static ssize_t nr_contexts_store(struct if (nr < 0 || 1 < nr) return -EINVAL; + contexts = container_of(kobj, struct damon_sysfs_contexts, kobj); if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_contexts_add_dirs(contexts, nr); @@ -2737,8 +2740,7 @@ static ssize_t nr_kdamonds_show(struct k static ssize_t nr_kdamonds_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_kdamonds *kdamonds = container_of(kobj, - struct damon_sysfs_kdamonds, kobj); + struct damon_sysfs_kdamonds *kdamonds; int nr, err; err = kstrtoint(buf, 0, &nr); @@ -2747,6 +2749,8 @@ static ssize_t nr_kdamonds_store(struct if (nr < 0) return -EINVAL; + kdamonds = container_of(kobj, struct damon_sysfs_kdamonds, kobj); + if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_kdamonds_add_dirs(kdamonds, nr);
On Fri, 9 Sep 2022 15:44:34 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Thu, 8 Sep 2022 22:16:53 +0000 SeongJae Park <sj@kernel.org> wrote: > > > Reviewed-by: SeongJae Park <sj@kernel.org> > > Cool. I rewrote the changelog significantly: > > From: Xin Hao <xhao@linux.alibaba.com> > Subject: mm/damon/sysfs: change few functions execute order > Date: Thu, 8 Sep 2022 16:19:32 +0800 > > There's no need to run container_of() as early as we do. > > The compiler figures this out, but the resulting code is more readable. Looks very nice to me. Thank you, Andrew! Thanks, SJ [...]
在 2022/9/10 上午6:44, Andrew Morton 写道: > On Thu, 8 Sep 2022 22:16:53 +0000 SeongJae Park <sj@kernel.org> wrote: > >> Reviewed-by: SeongJae Park <sj@kernel.org> > Cool. I rewrote the changelog significantly: Thanks for all of your hard work and detailed review ! > > From: Xin Hao <xhao@linux.alibaba.com> > Subject: mm/damon/sysfs: change few functions execute order > Date: Thu, 8 Sep 2022 16:19:32 +0800 > > There's no need to run container_of() as early as we do. > > The compiler figures this out, but the resulting code is more readable. > > Link: https://lkml.kernel.org/r/20220908081932.77370-1-xhao@linux.alibaba.com > Signed-off-by: Xin Hao <xhao@linux.alibaba.com> > Reviewed-by: SeongJae Park <sj@kernel.org> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > --- > > mm/damon/sysfs.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > --- a/mm/damon/sysfs.c~mm-damon-sysfs-change-few-functions-execute-order > +++ a/mm/damon/sysfs.c > @@ -1031,8 +1031,7 @@ static ssize_t nr_schemes_show(struct ko > static ssize_t nr_schemes_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_schemes *schemes = container_of(kobj, > - struct damon_sysfs_schemes, kobj); > + struct damon_sysfs_schemes *schemes; > int nr, err = kstrtoint(buf, 0, &nr); > > if (err) > @@ -1040,6 +1039,8 @@ static ssize_t nr_schemes_store(struct k > if (nr < 0) > return -EINVAL; > > + schemes = container_of(kobj, struct damon_sysfs_schemes, kobj); > + > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_schemes_add_dirs(schemes, nr); > @@ -1237,8 +1238,7 @@ static ssize_t nr_regions_show(struct ko > static ssize_t nr_regions_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_regions *regions = container_of(kobj, > - struct damon_sysfs_regions, kobj); > + struct damon_sysfs_regions *regions; > int nr, err = kstrtoint(buf, 0, &nr); > > if (err) > @@ -1246,6 +1246,8 @@ static ssize_t nr_regions_store(struct k > if (nr < 0) > return -EINVAL; > > + regions = container_of(kobj, struct damon_sysfs_regions, kobj); > + > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_regions_add_dirs(regions, nr); > @@ -1440,8 +1442,7 @@ static ssize_t nr_targets_show(struct ko > static ssize_t nr_targets_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_targets *targets = container_of(kobj, > - struct damon_sysfs_targets, kobj); > + struct damon_sysfs_targets *targets; > int nr, err = kstrtoint(buf, 0, &nr); > > if (err) > @@ -1449,6 +1450,8 @@ static ssize_t nr_targets_store(struct k > if (nr < 0) > return -EINVAL; > > + targets = container_of(kobj, struct damon_sysfs_targets, kobj); > + > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_targets_add_dirs(targets, nr); > @@ -1962,8 +1965,7 @@ static ssize_t nr_contexts_show(struct k > static ssize_t nr_contexts_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_contexts *contexts = container_of(kobj, > - struct damon_sysfs_contexts, kobj); > + struct damon_sysfs_contexts *contexts; > int nr, err; > > err = kstrtoint(buf, 0, &nr); > @@ -1973,6 +1975,7 @@ static ssize_t nr_contexts_store(struct > if (nr < 0 || 1 < nr) > return -EINVAL; > > + contexts = container_of(kobj, struct damon_sysfs_contexts, kobj); > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_contexts_add_dirs(contexts, nr); > @@ -2737,8 +2740,7 @@ static ssize_t nr_kdamonds_show(struct k > static ssize_t nr_kdamonds_store(struct kobject *kobj, > struct kobj_attribute *attr, const char *buf, size_t count) > { > - struct damon_sysfs_kdamonds *kdamonds = container_of(kobj, > - struct damon_sysfs_kdamonds, kobj); > + struct damon_sysfs_kdamonds *kdamonds; > int nr, err; > > err = kstrtoint(buf, 0, &nr); > @@ -2747,6 +2749,8 @@ static ssize_t nr_kdamonds_store(struct > if (nr < 0) > return -EINVAL; > > + kdamonds = container_of(kobj, struct damon_sysfs_kdamonds, kobj); > + > if (!mutex_trylock(&damon_sysfs_lock)) > return -EBUSY; > err = damon_sysfs_kdamonds_add_dirs(kdamonds, nr); > _
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c index 88f208ff235d..54fdfcdbb3e4 100644 --- a/mm/damon/sysfs.c +++ b/mm/damon/sysfs.c @@ -1031,8 +1031,7 @@ static ssize_t nr_schemes_show(struct kobject *kobj, static ssize_t nr_schemes_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_schemes *schemes = container_of(kobj, - struct damon_sysfs_schemes, kobj); + struct damon_sysfs_schemes *schemes; int nr, err = kstrtoint(buf, 0, &nr); if (err) @@ -1040,6 +1039,8 @@ static ssize_t nr_schemes_store(struct kobject *kobj, if (nr < 0) return -EINVAL; + schemes = container_of(kobj, struct damon_sysfs_schemes, kobj); + if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_schemes_add_dirs(schemes, nr); @@ -1237,8 +1238,7 @@ static ssize_t nr_regions_show(struct kobject *kobj, static ssize_t nr_regions_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_regions *regions = container_of(kobj, - struct damon_sysfs_regions, kobj); + struct damon_sysfs_regions *regions; int nr, err = kstrtoint(buf, 0, &nr); if (err) @@ -1246,6 +1246,8 @@ static ssize_t nr_regions_store(struct kobject *kobj, if (nr < 0) return -EINVAL; + regions = container_of(kobj, struct damon_sysfs_regions, kobj); + if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_regions_add_dirs(regions, nr); @@ -1440,8 +1442,7 @@ static ssize_t nr_targets_show(struct kobject *kobj, static ssize_t nr_targets_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_targets *targets = container_of(kobj, - struct damon_sysfs_targets, kobj); + struct damon_sysfs_targets *targets; int nr, err = kstrtoint(buf, 0, &nr); if (err) @@ -1449,6 +1450,8 @@ static ssize_t nr_targets_store(struct kobject *kobj, if (nr < 0) return -EINVAL; + targets = container_of(kobj, struct damon_sysfs_targets, kobj); + if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_targets_add_dirs(targets, nr); @@ -1962,8 +1965,7 @@ static ssize_t nr_contexts_show(struct kobject *kobj, static ssize_t nr_contexts_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_contexts *contexts = container_of(kobj, - struct damon_sysfs_contexts, kobj); + struct damon_sysfs_contexts *contexts; int nr, err; err = kstrtoint(buf, 0, &nr); @@ -1973,6 +1975,7 @@ static ssize_t nr_contexts_store(struct kobject *kobj, if (nr < 0 || 1 < nr) return -EINVAL; + contexts = container_of(kobj, struct damon_sysfs_contexts, kobj); if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_contexts_add_dirs(contexts, nr); @@ -2741,8 +2744,7 @@ static ssize_t nr_kdamonds_show(struct kobject *kobj, static ssize_t nr_kdamonds_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t count) { - struct damon_sysfs_kdamonds *kdamonds = container_of(kobj, - struct damon_sysfs_kdamonds, kobj); + struct damon_sysfs_kdamonds *kdamonds; int nr, err; err = kstrtoint(buf, 0, &nr); @@ -2751,6 +2753,8 @@ static ssize_t nr_kdamonds_store(struct kobject *kobj, if (nr < 0) return -EINVAL; + kdamonds = container_of(kobj, struct damon_sysfs_kdamonds, kobj); + if (!mutex_trylock(&damon_sysfs_lock)) return -EBUSY; err = damon_sysfs_kdamonds_add_dirs(kdamonds, nr);
These nr_{schemes,regions,contexts,kdamonds}_store() functions are both call kstrtoint() to get relative values from sysfs interface, if it return an error, there get kobject instance would be meaningless through 'container_of'. Signed-off-by: Xin Hao <xhao@linux.alibaba.com> --- mm/damon/sysfs.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)