From: sashiko-bot@kernel.org
To: "Wei Li" <liwei1412@hotmail.com>
Cc: linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH 3/6] rv: Show the mixed enable state of containers
Date: Wed, 04 Nov 2026 09:37:14 +0000 [thread overview]
Message-ID: <sashiko-outbox-196972@kernel.org> (raw)
In-Reply-To: <CY8PR19MB70099F2263C408361053B295B7682@CY8PR19MB7009.namprd19.prod.outlook.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rv: stale parent container state on re-enable error in rv_update_single()
--
commit 2db3de084bc5c2af773f028e5905ec5c314ba488
Author: Wei Li <liwei391@huawei.com>
rv: Show the mixed enable state of containers
This commit converts struct rv_monitor::enabled into enum rv_monitor_state
so container monitors can report a mixed ("X") enable state when only a
subset of their nested monitors are enabled. It updates rv_enable_single()
and rv_disable_single() to refresh the parent container state.
> diff --git a/kernel/trace/rv/rv.c b/kernel/trace/rv/rv.c
> index 4e689d4590aee..7846e4092b9c4 100644
> --- a/kernel/trace/rv/rv.c
> +++ b/kernel/trace/rv/rv.c
[ ... ]
> @@ -269,10 +272,22 @@ static int __rv_disable_monitor(struct rv_monitor *mon, bool sync)
>
> static void rv_disable_single(struct rv_monitor *mon)
> {
> + struct rv_monitor *cntr = mon->parent;
> + struct rv_monitor *p = cntr;
> +
> __rv_disable_monitor(mon, true);
> + if (!cntr)
> + return;
> + cntr->enabled = 0;
> + list_for_each_entry_continue(p, &rv_monitors_list, list) {
> + if (p->parent != cntr)
> + break;
> + if (p->enabled)
> + cntr->enabled = 2;
> + }
> }
>
> -static int rv_enable_single(struct rv_monitor *mon)
> +static int __rv_enable_monitor(struct rv_monitor *mon)
> {
> int retval;
>
> @@ -289,6 +304,27 @@ static int rv_enable_single(struct rv_monitor *mon)
> return retval;
> }
>
> +static int rv_enable_single(struct rv_monitor *mon)
> +{
> + struct rv_monitor *cntr = mon->parent;
> + struct rv_monitor *p = cntr;
> + int retval;
> +
> + retval = __rv_enable_monitor(mon);
> +
> + if (!retval && cntr) {
> + cntr->enabled = 1;
> + list_for_each_entry_continue(p, &rv_monitors_list, list) {
> + if (p->parent != cntr)
> + break;
> + if (!p->enabled)
> + cntr->enabled = 2;
> + }
> + }
> +
> + return retval;
> +}
> +
> static int rv_update_single(struct rv_monitor *mon, rvmon_update_fn_t update, void *data)
> {
> int retval = 0;
> @@ -297,7 +333,7 @@ static int rv_update_single(struct rv_monitor *mon, rvmon_update_fn_t update, vo
> enabled = __rv_disable_monitor(mon, true);
> update(mon, data);
> if (enabled)
> - retval = rv_enable_single(mon);
> + retval = __rv_enable_monitor(mon);
[Severity: Medium]
Can this leave mon->parent->enabled stale if re-enabling a nested monitor
fails in rv_update_single()?
When updating a single nested monitor (for example via
monitor_reactors_write() -> monitor_swap_reactors() -> rv_update_monitor() ->
rv_update_single()), __rv_disable_monitor() sets mon->enabled = 0 without
updating mon->parent->enabled.
If the subsequent __rv_enable_monitor(mon) call fails (such as
enable_nomiss() failing in init_storage(false)), mon remains disabled while
mon->parent->enabled still holds its previous enabled or mixed state.
Also, this isn't a bug, but would it be cleaner to use a shared helper with
the enum rv_monitor_state constants instead of open-coding the parent state
loops with raw 0, 1, and 2 literals in rv_disable_single() and
rv_enable_single()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/CY8PR19MB70098BF493A3B67418C7B9ABB7682@CY8PR19MB7009.namprd19.prod.outlook.com?part=3
next prev parent reply other threads:[~2026-11-04 9:37 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-04 9:21 [PATCH 0/6] rv: Expose and preserve nested monitor states Wei Li
2026-11-04 9:21 ` [PATCH 1/6] rv: Mark diverged child reactors with a star Wei Li
2026-11-04 9:21 ` [PATCH 2/6] rv: Update monitors through a quiesce helper Wei Li
2026-11-04 9:32 ` sashiko-bot
2026-11-04 9:21 ` [PATCH 3/6] rv: Show the mixed enable state of containers Wei Li
2026-11-04 9:37 ` sashiko-bot [this message]
2026-11-04 9:21 ` [PATCH 4/6] Documentation/rv: Document the enable and reactor states Wei Li
2026-11-04 9:21 ` [PATCH 5/6] tools/rv: Parse the mixed enable state of containers Wei Li
2026-11-04 9:39 ` sashiko-bot
2026-11-04 9:21 ` [PATCH 6/6] rv: Do not send the trailing NUL to userspace Wei Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-196972@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=linux-trace-kernel@vger.kernel.org \
--cc=liwei1412@hotmail.com \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.