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 <[email protected]>
---
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
Hi Xin,
On Thu, 8 Sep 2022 16:19:32 +0800 Xin Hao <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> wrote:
> Hi Xin,
>
> On Thu, 8 Sep 2022 16:19:32 +0800 Xin Hao <[email protected]> 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 <[email protected]> wrote:
> On Thu, 8 Sep 2022 18:49:47 +0000 SeongJae Park <[email protected]> wrote:
>
> > Hi Xin,
> >
> > On Thu, 8 Sep 2022 16:19:32 +0800 Xin Hao <[email protected]> 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 <[email protected]>
Thanks,
SJ
On Thu, 8 Sep 2022 22:16:53 +0000 SeongJae Park <[email protected]> wrote:
> Reviewed-by: SeongJae Park <[email protected]>
Cool. I rewrote the changelog significantly:
From: Xin Hao <[email protected]>
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/[email protected]
Signed-off-by: Xin Hao <[email protected]>
Reviewed-by: SeongJae Park <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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 <[email protected]> wrote:
> On Thu, 8 Sep 2022 22:16:53 +0000 SeongJae Park <[email protected]> wrote:
>
> > Reviewed-by: SeongJae Park <[email protected]>
>
> Cool. I rewrote the changelog significantly:
>
> From: Xin Hao <[email protected]>
> 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 <[email protected]> wrote:
>
>> Reviewed-by: SeongJae Park <[email protected]>
> Cool. I rewrote the changelog significantly:
Thanks for all of your hard work and detailed review !
>
> From: Xin Hao <[email protected]>
> 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/[email protected]
> Signed-off-by: Xin Hao <[email protected]>
> Reviewed-by: SeongJae Park <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> 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);
> _