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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox