damon_lru_sort_apply_parameters() allocates a new DAMON context, stages
user-specified DAMON parameters on it, and commits to running DAMON
context at once, using damon_commit_ctx(). The code is, however,
directly updating the monitoring attributes of the running context. This
doesn't cause a real user problem but apparently this is an
unintentional mistake that can cause code review confusions and future
real problems. Fix the wrong use of the parameter context.
Fixes: a30969436428 ("mm/damon/lru_sort: use damon_commit_ctx()")
Signed-off-by: SeongJae Park <sj@kernel.org>
---
mm/damon/lru_sort.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 14d31009c09e..ab6173a646bd 100644
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
@@ -219,7 +219,7 @@ static int damon_lru_sort_apply_parameters(void)
goto out;
}
- err = damon_set_attrs(ctx, &damon_lru_sort_mon_attrs);
+ err = damon_set_attrs(param_ctx, &damon_lru_sort_mon_attrs);
if (err)
goto out;
--
2.39.5On Sun, 14 Sep 2025 18:58:04 -0700 SeongJae Park <sj@kernel.org> wrote:
> damon_lru_sort_apply_parameters() allocates a new DAMON context, stages
> user-specified DAMON parameters on it, and commits to running DAMON
> context at once, using damon_commit_ctx(). The code is, however,
> directly updating the monitoring attributes of the running context. This
> doesn't cause a real user problem but apparently this is an
> unintentional mistake that can cause code review confusions and future
> real problems. Fix the wrong use of the parameter context.
Hi SJ,
Thank you for the patch! I am a little bit confused by the behavior in
damon_lru_sort_apply_parameters. I was hoping that you could help me understand : -)
In particular, I think that this patch fixes two possible user visible errors.
My understanding is that we want to make changes to the param_ctx first,
validate the changes, and commit these changes to the global ctx struct at the
end. In the middle in the errors, we can abort the operation without committing,
and ctx will remain unchanged.
So to me, it does seem like the current code could lead to some visible effects
from the user's perspective (error-handling case).
Also, I am a bit confused by how the commit is currently called. We have
err = damon_commit_ctx(ctx, param_ctx), where the first argument is the
destination and the second argument is the source. There is a bit of a mismatch
because in the current code we have the following:
+------------------------------------------------+
| ctx param_ctx |
+------------------------------------------------+
| New &damon_lru_sort_mon_attrs |
| New scheme |
| attrs overwritten to NULL <-- |
| scheme rewritten to new scheme <-- |
+------------------------------------------------+
So in particular, the global ctx will never have the correct attrs pre-patch,
since it will always be rewritten by param_ctx, which never had its attrs
initialized.
I hope this makes sense : -) All of this is just to say that this patch does
more than just improve review confusions -- I think there at least two errors
that this fixes for the user. So perhaps a more descriptive commit will be
helpful in the future, since we are also adding a fixes tag?
Thank you again for the patch, SJ! Feel free to add:
Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>
Have a great day!
Joshua
> Fixes: a30969436428 ("mm/damon/lru_sort: use damon_commit_ctx()")
> Signed-off-by: SeongJae Park <sj@kernel.org>
> ---
> mm/damon/lru_sort.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 14d31009c09e..ab6173a646bd 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -219,7 +219,7 @@ static int damon_lru_sort_apply_parameters(void)
> goto out;
> }
>
> - err = damon_set_attrs(ctx, &damon_lru_sort_mon_attrs);
> + err = damon_set_attrs(param_ctx, &damon_lru_sort_mon_attrs);
> if (err)
> goto out;
>
> --
> 2.39.5
Sent using hkml (https://github.com/sjp38/hackermail)
On Mon, 15 Sep 2025 08:05:47 -0700 Joshua Hahn <joshua.hahnjy@gmail.com> wrote: > On Sun, 14 Sep 2025 18:58:04 -0700 SeongJae Park <sj@kernel.org> wrote: > > > damon_lru_sort_apply_parameters() allocates a new DAMON context, stages > > user-specified DAMON parameters on it, and commits to running DAMON > > context at once, using damon_commit_ctx(). The code is, however, > > directly updating the monitoring attributes of the running context. This > > doesn't cause a real user problem but apparently this is an > > unintentional mistake that can cause code review confusions and future > > real problems. Fix the wrong use of the parameter context. > > Hi SJ, > > Thank you for the patch! I am a little bit confused by the behavior in > damon_lru_sort_apply_parameters. I was hoping that you could help me understand : -) Thank you for sharing this Joshua! > In particular, I think that this patch fixes two possible user visible errors. > > My understanding is that we want to make changes to the param_ctx first, > validate the changes, and commit these changes to the global ctx struct at the > end. In the middle in the errors, we can abort the operation without committing, > and ctx will remain unchanged. > > So to me, it does seem like the current code could lead to some visible effects > from the user's perspective (error-handling case). damon_set_attrs() has its own parameters validation. If the validation fails, it returns an error. damon_commit_ctx()'s internal validation for damon_attrs parameters also depend on the validsation of damon_set_attrs(). If the given parameter is invalid, damon_set_attrs() will return an error without committing the change, so there should be no user-visible behavioral difference. > > Also, I am a bit confused by how the commit is currently called. We have > err = damon_commit_ctx(ctx, param_ctx), where the first argument is the > destination and the second argument is the source. There is a bit of a mismatch > because in the current code we have the following: > > +------------------------------------------------+ > | ctx param_ctx | > +------------------------------------------------+ > | New &damon_lru_sort_mon_attrs | > | New scheme | > | attrs overwritten to NULL <-- | > | scheme rewritten to new scheme <-- | > +------------------------------------------------+ > > So in particular, the global ctx will never have the correct attrs pre-patch, > since it will always be rewritten by param_ctx, which never had its attrs > initialized. > > I hope this makes sense : -) All of this is just to say that this patch does > more than just improve review confusions -- I think there at least two errors > that this fixes for the user. So perhaps a more descriptive commit will be > helpful in the future, since we are also adding a fixes tag? Makes sense, thank you for finding this Joshua! I will send v2 of this patch with updated commit message! > > Thank you again for the patch, SJ! Feel free to add: > Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com> Thank you! Thanks, SJ [...]
On Mon, 15 Sep 2025 11:43:06 -0700 SeongJae Park <sj@kernel.org> wrote: > On Mon, 15 Sep 2025 08:05:47 -0700 Joshua Hahn <joshua.hahnjy@gmail.com> wrote: > > > On Sun, 14 Sep 2025 18:58:04 -0700 SeongJae Park <sj@kernel.org> wrote: > > > > > damon_lru_sort_apply_parameters() allocates a new DAMON context, stages > > > user-specified DAMON parameters on it, and commits to running DAMON > > > context at once, using damon_commit_ctx(). The code is, however, > > > directly updating the monitoring attributes of the running context. This > > > doesn't cause a real user problem but apparently this is an > > > unintentional mistake that can cause code review confusions and future > > > real problems. Fix the wrong use of the parameter context. [...] > > So in particular, the global ctx will never have the correct attrs pre-patch, > > since it will always be rewritten by param_ctx, which never had its attrs > > initialized. [...] And it measn the monitoring attributes prameters of DAMON_LRU_SORT are not working at all. I think this deserves to be a hotfix. I will send v2 of this separately, cc-ing stable@. Thanks, SJ [...]
© 2016 - 2025 Red Hat, Inc.