RE: [PATCH RFC V6 24/24] tcg: Defer TB flush for 'lazy realized' vCPUs on first region alloc

[Salil Mehta via <qemu-devel@nongnu.org>]

Hi Richard,

Sorry for the delay in reply. 

> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Thursday, October 2, 2025 4:41 PM
> 
> On 10/2/25 05:27, Salil Mehta wrote:
> > Hi Richard,
> >
> > Thanks for the reply. Please find my response inline.
> >
> > Cheers.
> >
> >> From: qemu-devel-bounces+salil.mehta=huawei.com@nongnu.org
> <qemu-
> >> devel-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of
> Richard
> >> Henderson
> >> Sent: Wednesday, October 1, 2025 10:34 PM
> >> To: salil.mehta@opnsrc.net; qemu-devel@nongnu.org; qemu-
> >> arm@nongnu.org; mst@redhat.com
> >> Subject: Re: [PATCH RFC V6 24/24] tcg: Defer TB flush for 'lazy
> >> realized' vCPUs on first region alloc
> >>
> >> On 9/30/25 18:01, salil.mehta@opnsrc.net wrote:
> >>> From: Salil Mehta <salil.mehta@huawei.com>
> >>>
> >>> The TCG code cache is split into regions shared by vCPUs under MTTCG.
> >>> For cold-boot (early realized) vCPUs, regions are sized/allocated
> >>> during
> >> bring-up.
> >>> However, when a vCPU is *lazy_realized* (administratively "disabled"
> >>> at boot and realized later on demand), its TCGContext may fail the
> >>> very first code region allocation if the shared TB cache is
> >>> saturated by already-running vCPUs.
> >>>
> >>> Flushing the TB cache is the right remediation, but `tb_flush()`
> >>> must be performed from the safe execution context
> >> (cpu_exec_loop()/tb_gen_code()).
> >>> This patch wires a deferred flush:
> >>>
> >>>     * In `tcg_region_initial_alloc__locked()`, treat an initial allocation
> >>>       failure for a lazily realized vCPU as non-fatal: set `s->tbflush_pend`
> >>>       and return.
> >>>
> >>>     * In `tcg_tb_alloc()`, if `s->tbflush_pend` is observed, clear it and
> >>>       return NULL so the caller performs a synchronous `tb_flush()` and
> then
> >>>       retries allocation.
> >>>
> >>> This avoids hangs observed when a newly realized vCPU cannot obtain
> >>> its first region under TB-cache pressure, while keeping the flush at
> >>> a safe
> >> point.
> >>>
> >>> No change for cold-boot vCPUs and when accel ops is KVM.
> >>>
> >>> In earlier series, this patch was with below named,
> >>> 'tcg: Update tcg_register_thread() leg to handle region alloc for
> >>> hotplugged
> >> vCPU'
> >>
> >>
> >> I don't see why you need two different booleans for this.
> >
> >
> > I can see your point. Maybe I can move `s->tbflush_pend`  to 'CPUState'
> instead?
> >
> >
> >> It seems to me that you could create the cpu in a state for which the
> >> first call to
> >> tcg_tb_alloc() sees highwater state, and everything after that
> >> happens per usual allocating a new region, and possibly flushing the full
> buffer.
> >
> >
> > Correct. but with a distinction that highwater state is relevant to a
> > TCGContext and the regions are allocated from a common pool 'Code
> Generation Buffer'.
> > 'code_gen_highwater' is use to detect whether current context needs
> > more region allocation for the dynamic translation to continue. This
> > is a different condition than what we are encountering; which is the
> > worst case condition that the entire code generation buffer is
> > saturated and cannot even allocate a single free TCG region successfully.
> 
> I think you misunderstand "and everything after that happens per usual".
> 
> When allocating a tb, if a cpu finds that it's current region is full, then it tries
> to allocate another region.  If that is not successful, then we flush the entire
> code_gen_buffer and try again.
> 
> Thus tbflush_pend is exactly equivalent to setting
> 
>      s->code_gen_ptr > s->code_gen_highwater.
> 
> As far as lazy_realized...  The utility of the assert under these conditions may
> be called into question; we could just remove it.


I understand your point. I'll remove the 'tbflush_pend' flag  and directly use
'code_gen_highwater = NULL' so that we hit the highwater condition early
when the TCG threads gets lazily realized. And yes, we might have to either
remove or conditionally bypass the assert(). Will dig further and validate. 

Many thanks for this optimization!

Best regards
Salil.


> 
> 
> r~