Re: [PATCH] mm/damon/sysfs: Remove misleading todo comment in nid_show()

Re: [PATCH] mm/damon/sysfs: Remove misleading todo comment in nid_show()

[SeongJae Park]

Hello Swaraj,

On Tue, 21 Oct 2025 02:17:13 +0000 Swaraj Gaikwad <swarajgaikwad1925@gmail.com> wrote:

> The TODO comment in nid_show() suggested returning an error if the goal was
> not using nid. However, this comment was found to be inaccurate and misleading.
> This patch removes the TODO comment without changing any existing behavior.

checkpatch.pl complains as below.

    WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?)
    #10:
    not using nid. However, this comment was found to be inaccurate and misleading.

> 
> This change follows feedback from SJ who pointed out that wiring-order

s/wiring/writing/

Also, giving a pointer to the previous discussion could be a good practice, in
my opinion.  E.g.,

   This change follows feedback from SJ who pointed out [1] that ...
   [...]
   [1] https://lore.kernel.org/20251020151315.66260-1-sj@kernel.org

> independence is expected and the function should simply show the last set value.
> 
> No functional code changes were made.
> 
> Tested with KUnit:
> - Built kernel with KUnit and DAMON sysfs tests enabled.
> - Executed KUnit tests:
>   ./kunit.py run --kunitconfig ./mm/mm/damon/tests/.kunitconfig

Not an important thing, but...  if you were doing this on the root of
unmodified linux tree, the command you executed would be,

./tools/testing/kunit/kunit.py run --kunitconfig ./mm/damon/tests

I guess it was just a simple copy-and-pasta mistake and I don't really mind,
though.

> - All 25 tests passed, including damon_sysfs_test_add_targets.
> 
> Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
> Suggested-by: SeongJae Park <sj@kernel.org>

Other than the abovely mentioned trivial things, so nice patch, thank you!  For
another version of this patch that the above trivial things are fixed, please
feel free to add below.

Reviewed-by: SeongJae Park <sj@kernel.org>


Thanks,
SJ

[...]

[PATCH] mm/damon/sysfs: Remove misleading todo comment in nid_show()

[Swaraj Gaikwad]

The TODO comment in nid_show() suggested returning an error if the goal was
not using nid. However, this comment was found to be inaccurate and misleading.
This patch removes the TODO comment without changing any existing behavior.

This change follows feedback from SJ who pointed out that wiring-order
independence is expected and the function should simply show the last set value.

No functional code changes were made.

Tested with KUnit:
- Built kernel with KUnit and DAMON sysfs tests enabled.
- Executed KUnit tests:
  ./kunit.py run --kunitconfig ./mm/mm/damon/tests/.kunitconfig
- All 25 tests passed, including damon_sysfs_test_add_targets.

Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
Suggested-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/sysfs-schemes.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 6536f16006c9..760279092b4f 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -1112,7 +1112,6 @@ static ssize_t nid_show(struct kobject *kobj,
 	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
 			damos_sysfs_quota_goal, kobj);

-	/* todo: return error if the goal is not using nid */

 	return sysfs_emit(buf, "%d\n", goal->nid);
 }

base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
--
2.51.0

[PATCH] mm/damon/sysfs: Remove misleading todo comment in nid_show()

[Swaraj Gaikwad]

The TODO comment in nid_show() suggested returning an error if the goal was
not using nid. However, this comment was found to be inaccurate and
misleading.This patch removes the TODO comment without changing any
existing behavior.

This change follows feedback from SJ who pointed out [1] that wiring-order
independence is expected and the function should simply show the last
set value. and [2] checkpatch.pl complain about number of chars per line

No functional code changes were made.

Tested with KUnit:
- Built kernel with KUnit and DAMON sysfs tests enabled.
- Executed KUnit tests:
  ./tools/testing/kunit/kunit.py run --kunitconfig ./mm/damon/tests/
- All 25 tests passed, including damon_sysfs_test_add_targets.

Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
Suggested-by: SeongJae Park <sj@kernel.org>

[1] https://lore.kernel.org/lkml/20251020151315.66260-1-sj@kernel.org/
[2] https://lore.kernel.org/lkml/20251021010847.68473-1-sj@kernel.org/
---
 mm/damon/sysfs-schemes.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
index 6536f16006c9..760279092b4f 100644
--- a/mm/damon/sysfs-schemes.c
+++ b/mm/damon/sysfs-schemes.c
@@ -1112,7 +1112,6 @@ static ssize_t nid_show(struct kobject *kobj,
 	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
 			damos_sysfs_quota_goal, kobj);

-	/* todo: return error if the goal is not using nid */

 	return sysfs_emit(buf, "%d\n", goal->nid);
 }

base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
--
2.51.0

Re: [PATCH] mm/damon/sysfs: Remove misleading todo comment in nid_show()

[SeongJae Park]

On Tue, 21 Oct 2025 21:53:24 +0000 Swaraj Gaikwad <swarajgaikwad1925@gmail.com> wrote:

> The TODO comment in nid_show() suggested returning an error if the goal was
> not using nid. However, this comment was found to be inaccurate and
> misleading.This patch removes the TODO comment without changing any
> existing behavior.
> 
> This change follows feedback from SJ who pointed out [1] that wiring-order
> independence is expected and the function should simply show the last
> set value. and [2] checkpatch.pl complain about number of chars per line

This is another revision of your previous patch [1], right?  Thank you for
fixing the things I commented on.

That said, marking the fact that this is a new revision of other one on
subject, for example, setting the subject prefix as "[PATCH v2]", and adding
changelog from the previous version in commentary section could help review.
Please consider doing so from your next patch.  Please refer to the related
docs [2,3] for more details about that.

> 
> No functional code changes were made.
> 
> Tested with KUnit:
> - Built kernel with KUnit and DAMON sysfs tests enabled.
> - Executed KUnit tests:
>   ./tools/testing/kunit/kunit.py run --kunitconfig ./mm/damon/tests/
> - All 25 tests passed, including damon_sysfs_test_add_targets.
> 
> Signed-off-by: Swaraj Gaikwad <swarajgaikwad1925@gmail.com>
> Suggested-by: SeongJae Park <sj@kernel.org>

Reviewed-by: SeongJae Park <sj@kernel.org>

> 
> [1] https://lore.kernel.org/lkml/20251020151315.66260-1-sj@kernel.org/
> [2] https://lore.kernel.org/lkml/20251021010847.68473-1-sj@kernel.org/

And you should have put the above two lines before the above Signed-off-by:
line.  Please consider doing so from the next time.  Again, read the document
[1] for more details.

I found Andrew picked this patch on mm tree, adjusting the above two lines
properly [4].  So no new revision would be required.  But I'm noting this for
your next work.

> ---
>  mm/damon/sysfs-schemes.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/mm/damon/sysfs-schemes.c b/mm/damon/sysfs-schemes.c
> index 6536f16006c9..760279092b4f 100644
> --- a/mm/damon/sysfs-schemes.c
> +++ b/mm/damon/sysfs-schemes.c
> @@ -1112,7 +1112,6 @@ static ssize_t nid_show(struct kobject *kobj,
>  	struct damos_sysfs_quota_goal *goal = container_of(kobj, struct
>  			damos_sysfs_quota_goal, kobj);
> 
> -	/* todo: return error if the goal is not using nid */
> 
>  	return sysfs_emit(buf, "%d\n", goal->nid);
>  }

And this results in the code having two empty lines.  Just one single empty
line would be enough.  But that's too trivial, so I wouldn't request a new
revision to you, or in-queue direct fix to Andrew.

I only hope my trivial comments to help your future patches be easier to review
and more smoothly be merged :)

So, thank you for this nice patch.

[1] https://lore.kernel.org/20251021021712.59017-2-swarajgaikwad1925@gmail.com
[2] https://docs.kernel.org/process/submitting-patches.html#subject-line
[3] https://docs.kernel.org/process/submitting-patches.html#commentary
[4] https://lore.kernel.org/20251021183613.C4A6DC4CEF7@smtp.kernel.org


Thanks,
SJ

[...]