RE: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace
Hi Peter,
> From: Peter Maydell
> Sent: Tuesday, May 7, 2024 10:03 AM
>
> On Tue, 7 May 2024 at 01:11, Salil Mehta wrote:
> >
> > > From: Peter Maydell
> > > Sent: Monday, May 6, 2024 10:29 AM
> > > To: Salil Mehta
> > >
> > > On Mon, 6 May 2024 at 10:06, Salil Mehta
> > > wrote:
> > > >
> > > > Hi Peter,
> > > >
> > > > Thanks for the review.
> > > >
> > > > > From: Peter Maydell When do we
> > > need to > > destroy a single address space in this way that means
> > > we need to > > keep a count of how many ASes the CPU currently has?
> > > The commit > > message talks about the case when we unrealize the
> > > whole CPU > > object, but in that situation you can just throw away
> > > all the ASes > > at once (eg by calling some > >
> > > cpu_destroy_address_spaces() function from
> > > cpu_common_unrealizefn()).
> > > >
> > > >
> > > > Yes, maybe, we can destroy all at once from common leg as well.
> > > I'd > prefer this to be done from the arch specific function for
> > > ARM to > maintain the clarity & symmetry of initialization and >
> > > un-initialization legs. For now, all of these address space
> > > destruction is happening in context to the arm_cpu_unrealizefn().
> > > >
> > > > It’s a kind of trade-off between little more code and clarity but
> > > I'm > open to further suggestions.
> > > >
> > > >
> > > > >
> > > > > Also, if we're leaking stuff here by failing to destroy it, is
> > > that > > a problem for existing CPU types like x86 that we can already
> hotplug?
> > > >
> > > > No we are not. We are taking care of these in the ARM arch
> > > specific > legs within functions arm_cpu_(un)realizefn().
> > >
> > > How can you be taking care of *x86* CPU types in the Arm unrealize?
> >
> >
> > Sorry, yes, I missed to reply that clearly. There was indeed a leak
> > with x86 reported by Phillipe/David last year. In fact, Phillipe floated a
> patch last year for this.
> > I thought it was fixed already as part of cpu_common_unrealize() but I
> > just checked and realized that the below proposed changed still isn’t
> > part of the mainline
> >
> > https://lore.kernel.org/qemu-devel/20230918160257.30127-9-
> philmd@linar
> > o.org/
>
> That seems like the right way to clean these up -- Philippe, do you want to
> fish that bugfix out of your big patchset and submit it separately ?
>
> > I can definitely add a common CPU AddressSpace destruction leg as part
> > of this patch if in case arch specific CPU unrealize does not cleans
> > up its CPU AddressSpace?
>
> Arch-specific CPU unrealize shouldn't need to do anything -- if we fix this
> similarly to Philippe's patch above then that will do the cleanup required.
> Handling this kind of cleanup in common code is more reliable because it
> doesn't require every target-arch maintainer to remember it needs to be
> done, plus it's less code.
Agreed but It is a trade-off between 'lines of code' and 'clarity of the code'.
Ideally, if someone is adding a initialization part one must consciously verify
the locations where these will get deallocated/deinited as well so I do not
think there is an escape from that kind of maintenance part.
For arch like x86, where we are doing default AddressSpace allocation from
common realize, it is more obvious to symmetrically destroy from common
leg but for any other architecture including ARM where many types of CPU
AddressSpace are getting allocated it would not be obvious to find its
destruction leg buried somewhere in the common code.
Hence, I would suggest to make release of the AddressSpace from common
unrealization part optional which by default it is in the code excerpt from the
patch:
https://lore.kernel.org/qemu-devel/20230918160257.30127-9-phi...@linaro.org/
+/* Destroy CPU address space */
+for (unsigned idx = 0; idx < cpu->num_ases; idx++) {
+cpu_address_space_destroy(cpu, idx);
+}
cpu->num_ases will be 0 in case Arch specific code has already destroyed it.
That said we will still need some modification in the address space destruction
function to gracefully return in case that CPU AddressSpace was already
destroyed or does not exist. This will keep both options open.
Thanks
Salil.
>
> thanks
> -- PMM
Re: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace
On Tue, 7 May 2024 at 01:11, Salil Mehta wrote: > > > From: Peter Maydell > > Sent: Monday, May 6, 2024 10:29 AM > > To: Salil Mehta > > > > On Mon, 6 May 2024 at 10:06, Salil Mehta > > wrote: > > > > > > Hi Peter, > > > > > > Thanks for the review. > > > > > > > From: Peter Maydell When do we need to > > > > destroy a single address space in this way that means we need to > > > > keep a count of how many ASes the CPU currently has? The commit > > > > message talks about the case when we unrealize the whole CPU > > > > object, but in that situation you can just throw away all the ASes > > > > at once (eg by calling some > > > > cpu_destroy_address_spaces() function from > > cpu_common_unrealizefn()). > > > > > > > > > Yes, maybe, we can destroy all at once from common leg as well. I'd > > > prefer this to be done from the arch specific function for ARM to > > > maintain the clarity & symmetry of initialization and > > > un-initialization legs. For now, all of these address space destruction > > is > > happening in context to the arm_cpu_unrealizefn(). > > > > > > It’s a kind of trade-off between little more code and clarity but I'm > > > open to further suggestions. > > > > > > > > > > > > > > Also, if we're leaking stuff here by failing to destroy it, is that > > > > a problem for existing CPU types like x86 that we can already hotplug? > > > > > > No we are not. We are taking care of these in the ARM arch specific > > > legs within functions arm_cpu_(un)realizefn(). > > > > How can you be taking care of *x86* CPU types in the Arm unrealize? > > > Sorry, yes, I missed to reply that clearly. There was indeed a leak with x86 > reported > by Phillipe/David last year. In fact, Phillipe floated a patch last year for > this. > I thought it was fixed already as part of cpu_common_unrealize() but I just > checked and realized that the below proposed changed still isn’t part of the > mainline > > https://lore.kernel.org/qemu-devel/20230918160257.30127-9-phi...@linaro.org/ That seems like the right way to clean these up -- Philippe, do you want to fish that bugfix out of your big patchset and submit it separately ? > I can definitely add a common CPU AddressSpace destruction leg as part of this > patch if in case arch specific CPU unrealize does not cleans up its CPU > AddressSpace? Arch-specific CPU unrealize shouldn't need to do anything -- if we fix this similarly to Philippe's patch above then that will do the cleanup required. Handling this kind of cleanup in common code is more reliable because it doesn't require every target-arch maintainer to remember it needs to be done, plus it's less code. thanks -- PMM
RE: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace
> From: Peter Maydell > Sent: Monday, May 6, 2024 10:29 AM > To: Salil Mehta > > On Mon, 6 May 2024 at 10:06, Salil Mehta > wrote: > > > > Hi Peter, > > > > Thanks for the review. > > > > > From: Peter Maydell When do we need to > > > destroy a single address space in this way that means we need to > > > keep a count of how many ASes the CPU currently has? The commit > > > message talks about the case when we unrealize the whole CPU > > > object, but in that situation you can just throw away all the ASes > > > at once (eg by calling some > > > cpu_destroy_address_spaces() function from > cpu_common_unrealizefn()). > > > > > > Yes, maybe, we can destroy all at once from common leg as well. I'd > > prefer this to be done from the arch specific function for ARM to > > maintain the clarity & symmetry of initialization and > > un-initialization legs. For now, all of these address space destruction is > happening in context to the arm_cpu_unrealizefn(). > > > > It’s a kind of trade-off between little more code and clarity but I'm > > open to further suggestions. > > > > > > > > > > Also, if we're leaking stuff here by failing to destroy it, is that > > > a problem for existing CPU types like x86 that we can already hotplug? > > > > No we are not. We are taking care of these in the ARM arch specific > > legs within functions arm_cpu_(un)realizefn(). > > How can you be taking care of *x86* CPU types in the Arm unrealize? Sorry, yes, I missed to reply that clearly. There was indeed a leak with x86 reported by Phillipe/David last year. In fact, Phillipe floated a patch last year for this. I thought it was fixed already as part of cpu_common_unrealize() but I just checked and realized that the below proposed changed still isn’t part of the mainline https://lore.kernel.org/qemu-devel/20230918160257.30127-9-phi...@linaro.org/ I can definitely add a common CPU AddressSpace destruction leg as part of this patch if in case arch specific CPU unrealize does not cleans up its CPU AddressSpace? Thanks Salil. > > thanks > -- PMM
Re: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace
On Mon, 6 May 2024 at 10:06, Salil Mehta wrote: > > Hi Peter, > > Thanks for the review. > > > From: Peter Maydell > > When do we need to destroy a single address space in this way that means > > we need to keep a count of how many ASes the CPU currently has? The > > commit message talks about the case when we unrealize the whole CPU > > object, but in that situation you can just throw away all the ASes at once > > (eg > > by calling some > > cpu_destroy_address_spaces() function from cpu_common_unrealizefn()). > > > Yes, maybe, we can destroy all at once from common leg as well. I'd prefer > this > to be done from the arch specific function for ARM to maintain the clarity & > symmetry of initialization and un-initialization legs. For now, all of these > address > space destruction is happening in context to the arm_cpu_unrealizefn(). > > It’s a kind of trade-off between little more code and clarity but I'm open to > further suggestions. > > > > > > Also, if we're leaking stuff here by failing to destroy it, is that a > > problem for > > existing CPU types like x86 that we can already hotplug? > > No we are not. We are taking care of these in the ARM arch specific legs > within functions arm_cpu_(un)realizefn(). How can you be taking care of *x86* CPU types in the Arm unrealize? thanks -- PMM
RE: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace
Hi Peter,
Thanks for the review.
> From: Peter Maydell
> Sent: Saturday, May 4, 2024 2:41 PM
>
> On Tue, 12 Mar 2024 at 02:02, Salil Mehta
> wrote:
> >
> > Virtual CPU Hot-unplug leads to unrealization of a CPU object. This
> > also involves destruction of the CPU AddressSpace. Add common function
> > to help destroy the CPU AddressSpace.
> >
> > Signed-off-by: Salil Mehta
> > Tested-by: Vishnu Pajjuri
> > Reviewed-by: Gavin Shan
> > Tested-by: Xianglai Li
> > Tested-by: Miguel Luis
> > Reviewed-by: Shaoqin Huang
>
> > diff --git a/system/physmem.c b/system/physmem.c index
> > 6e9ed97597..61b32ac4f2 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int
> > asidx,
> >
> > if (!cpu->cpu_ases) {
> > cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> > +cpu->cpu_ases_count = cpu->num_ases;
> > }
> >
> > newas = >cpu_ases[asidx];
> > @@ -774,6 +775,34 @@ void cpu_address_space_init(CPUState *cpu, int
> asidx,
> > }
> > }
> >
> > +void cpu_address_space_destroy(CPUState *cpu, int asidx) {
> > +CPUAddressSpace *cpuas;
> > +
> > +assert(cpu->cpu_ases);
> > +assert(asidx >= 0 && asidx < cpu->num_ases);
> > +/* KVM cannot currently support multiple address spaces. */
> > +assert(asidx == 0 || !kvm_enabled());
> > +
> > +cpuas = >cpu_ases[asidx];
> > +if (tcg_enabled()) {
> > +memory_listener_unregister(>tcg_as_listener);
> > +}
> > +
> > +address_space_destroy(cpuas->as);
> > +g_free_rcu(cpuas->as, rcu);
> > +
> > +if (asidx == 0) {
> > +/* reset the convenience alias for address space 0 */
> > +cpu->as = NULL;
> > +}
> > +
> > +if (--cpu->cpu_ases_count == 0) {
> > +g_free(cpu->cpu_ases);
> > +cpu->cpu_ases = NULL;
> > +}
> > +}
>
> When do we need to destroy a single address space in this way that means
> we need to keep a count of how many ASes the CPU currently has? The
> commit message talks about the case when we unrealize the whole CPU
> object, but in that situation you can just throw away all the ASes at once
> (eg
> by calling some
> cpu_destroy_address_spaces() function from cpu_common_unrealizefn()).
Yes, maybe, we can destroy all at once from common leg as well. I'd prefer this
to be done from the arch specific function for ARM to maintain the clarity &
symmetry of initialization and un-initialization legs. For now, all of these
address
space destruction is happening in context to the arm_cpu_unrealizefn().
It’s a kind of trade-off between little more code and clarity but I'm open to
further suggestions.
>
> Also, if we're leaking stuff here by failing to destroy it, is that a
> problem for
> existing CPU types like x86 that we can already hotplug?
No we are not. We are taking care of these in the ARM arch specific legs
within functions arm_cpu_(un)realizefn().
https://lore.kernel.org/all/20230926103654.34424-1-salil.me...@huawei.com/
Above change now would be part of Arch specific patch-set RFC V3 being prepared.
Thanks
Salil.
Re: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace
On Tue, 12 Mar 2024 at 02:02, Salil Mehta wrote:
>
> Virtual CPU Hot-unplug leads to unrealization of a CPU object. This also
> involves destruction of the CPU AddressSpace. Add common function to help
> destroy the CPU AddressSpace.
>
> Signed-off-by: Salil Mehta
> Tested-by: Vishnu Pajjuri
> Reviewed-by: Gavin Shan
> Tested-by: Xianglai Li
> Tested-by: Miguel Luis
> Reviewed-by: Shaoqin Huang
> diff --git a/system/physmem.c b/system/physmem.c
> index 6e9ed97597..61b32ac4f2 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -761,6 +761,7 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
>
> if (!cpu->cpu_ases) {
> cpu->cpu_ases = g_new0(CPUAddressSpace, cpu->num_ases);
> +cpu->cpu_ases_count = cpu->num_ases;
> }
>
> newas = >cpu_ases[asidx];
> @@ -774,6 +775,34 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
> }
> }
>
> +void cpu_address_space_destroy(CPUState *cpu, int asidx)
> +{
> +CPUAddressSpace *cpuas;
> +
> +assert(cpu->cpu_ases);
> +assert(asidx >= 0 && asidx < cpu->num_ases);
> +/* KVM cannot currently support multiple address spaces. */
> +assert(asidx == 0 || !kvm_enabled());
> +
> +cpuas = >cpu_ases[asidx];
> +if (tcg_enabled()) {
> +memory_listener_unregister(>tcg_as_listener);
> +}
> +
> +address_space_destroy(cpuas->as);
> +g_free_rcu(cpuas->as, rcu);
> +
> +if (asidx == 0) {
> +/* reset the convenience alias for address space 0 */
> +cpu->as = NULL;
> +}
> +
> +if (--cpu->cpu_ases_count == 0) {
> +g_free(cpu->cpu_ases);
> +cpu->cpu_ases = NULL;
> +}
> +}
When do we need to destroy a single address space in this way
that means we need to keep a count of how many ASes the CPU
currently has? The commit message talks about the case when we
unrealize the whole CPU object, but in that situation you can
just throw away all the ASes at once (eg by calling some
cpu_destroy_address_spaces() function from cpu_common_unrealizefn()).
Also, if we're leaking stuff here by failing to destroy it,
is that a problem for existing CPU types like x86 that we
can already hotplug?
thanks
-- PMM
Re: [PATCH V8 6/8] physmem: Add helper function to destroy CPU AddressSpace
Hi Zhukeqian,
On Fri, Mar 15, 2024 at 1:17 AM zhukeqian wrote:
> Hi Salil,
>
> [...]
>
> +void cpu_address_space_destroy(CPUState *cpu, int asidx) {
> +CPUAddressSpace *cpuas;
> +
> +assert(cpu->cpu_ases);
> +assert(asidx >= 0 && asidx < cpu->num_ases);
> +/* KVM cannot currently support multiple address spaces. */
> +assert(asidx == 0 || !kvm_enabled());
> +
> +cpuas = >cpu_ases[asidx];
> +if (tcg_enabled()) {
> +memory_listener_unregister(>tcg_as_listener);
> +}
> +
> +address_space_destroy(cpuas->as);
> +g_free_rcu(cpuas->as, rcu);
>
> In address_space_destroy(), it calls call_rcu1() on cpuas->as which will
> set do_address_space_destroy() as the rcu func.
> And g_free_rcu() also calls call_rcu1() on cpuas->as which will overwrite
> the rcu func as g_free().
>
> Then I think the g_free() may be called twice in rcu thread, please verify
> that.
>
> The source code of call_rcu1:
>
> void call_rcu1(struct rcu_head *node, void (*func)(struct rcu_head *node))
> {
> node->func = func;
> enqueue(node);
> qatomic_inc(_call_count);
> qemu_event_set(_call_ready_event);
> }
>
Thanks for testing and identifying this. Let me have a look and will get
back to you.
Thanks
Salil
>
> Thanks,
> Keqian
>
> +
> +if (asidx == 0) {
> +/* reset the convenience alias for address space 0 */
> +cpu->as = NULL;
> +}
> +
> +if (--cpu->cpu_ases_count == 0) {
> +g_free(cpu->cpu_ases);
> +cpu->cpu_ases = NULL;
> +}
> +}
> +
> AddressSpace *cpu_get_address_space(CPUState *cpu, int asidx) {
> /* Return the AddressSpace corresponding to the specified index */
> --
> 2.34.1
>
>
