Re: [PATCH V15 05/14] monitor: track IOThread users with QOM paths

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangckid@gmail.com> writes:

> On Mon, Sep 7, 2026 at 8:44 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Zhang Chen <zhangckid@gmail.com> writes:
>>
>> > Monitors that require an IOThread share the internal mon_iothread.
>> > Account for each monitor explicitly in IOThread holder tracking, using
>> > the monitor's canonical QOM path to distinguish holders.
>> >
>> > Acquire the shared monitor IOThread AioContext once and store the
>> > IOThread and canonical QOM path so the holder can be released after the
>> > monitor has been removed from the QOM tree. Reuse the stored context for
>> > later operations.
>> >
>> > Release the holder from monitor_finalize(), after resources associated
>> > with the AioContext have been destroyed. This pairs the reference taken
>> > in complete() with instance finalization and keeps the IOThread alive
>> > throughout monitor teardown.
>> >
>> > The internal mon_iothread remains hidden from query-iothreads, as before.
>> > Management applications therefore cannot use it to determine which
>> > monitors are using the IOThread. Nevertheless, track the monitor
>> > holders so that they follow the same explicit holder and
>> > IOThread reference lifecycle as other AioContext users.
>> >
>> > Signed-off-by: Zhang Chen <zhangckid@gmail.com>
>> > ---
>> >  monitor/monitor-internal.h |  5 +++++
>> >  monitor/monitor.c          | 26 +++++++++++++++++++++-----
>> >  monitor/qmp.c              |  5 +++--
>> >  3 files changed, 29 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
>> > index 822a66d06d..1d5aeae5e6 100644
>> > --- a/monitor/monitor-internal.h
>> > +++ b/monitor/monitor-internal.h
>> > @@ -77,6 +77,11 @@ struct Monitor {
>> >      GString *outbuf;
>> >      guint out_watch;
>> >      int mux_out;
>> > +
>> > +    /* iothread context and holder identity */
>> > +    IOThread *iothread;
>> > +    char *iothread_qom_path;
>> > +    AioContext *ctx;
>> >  };
>> >
>> >  struct MonitorQMPClass {
>> > diff --git a/monitor/monitor.c b/monitor/monitor.c
>> > index 2654b81890..cc7f6785b5 100644
>> > --- a/monitor/monitor.c
>> > +++ b/monitor/monitor.c
>> > @@ -93,6 +93,16 @@ static void monitor_finalize(Object *obj)
>> >      qemu_chr_fe_deinit(&mon->chr, false);
>> >      g_string_free(mon->outbuf, true);
>> >      qemu_mutex_destroy(&mon->mon_lock);
>> > +
>> > +    if (mon->iothread) {
>>
>> As we'll see in monitor_complete() below, mon->iothread is mon_iothread
>> if monitor_requires_iothread(mon), else null.  Which means
>> mon->mon_iothread is non-null if and only if
>> monitor_requires_iothread(mon).
>>
>> Here, you check whether mon->mon_iothread is non-null to find out
>> whether @mon uses the monitor I/O thread, and you read the monitor I/O
>> thread from mon->mon_iothread.
>>
>> Elsewhere in this file, we check monitor_requires_iothread(mon) and you
>> read mon_iothread, e.g. in monitor_cancel_out_watch()
>>
>>            if (monitor_requires_iothread(mon)) {
>>                ctx = iothread_get_g_main_context(mon_iothread);
>>            }
>>
>> I don't like this inconsistency.  Two ways to avoid it:
>>
>> 1. Always use monitor_requires_iothread(mon) to check and mon_iothread
>> to read.  Drop struct Monitor member iothread.
>>
>> 2. Always use mon_iothread, except where we initialize it in
>> monitor_complete().
>
> Just to clarify: by option 2, do you mean using mon->iothread
> consistently for per-monitor checks and accesses, except in
> monitor_complete(), where monitor_requires_iothread() determines
> whether to initialize it?

Yes.  I meant to write 2. Always use mon->iothread, but mistyped.