Re: [PATCH RFC V3 17/29] arm/virt: Release objects for *disabled* possible vCPUs after init

[Gavin Shan <gshan@redhat.com>]

Hi Salil,

On 8/23/24 11:17 PM, Salil Mehta wrote:
>>   On 8/22/24 8:58 PM, Salil Mehta wrote:
>>   >>   On 8/21/24 8:23 PM, Salil Mehta wrote:
>>   >>   >>
>>   >>   >>   On 8/21/24 2:40 AM, Salil Mehta wrote:
>>   >>   >>   >
>>   >>   >>   > I don’t understand this clearly. Are  you suggesting to reuse only
>>   >>   >>   > single vCPU object to initialize all KVM vCPUs not yet plugged? If
>>   >>   >>   > yes, then I'm not sure what do we gain here by adding this complexity?
>>   >>   >>   > It does not consume time or resources because we are not realizing any
>>   >>   >>   > of these vCPU object in any case.
>>   >>   >>   >
>>   >>   >>
>>   >>   >>   First of all, it seems we have different names and terms for those cold-
>>   >>   >>   booted vCPUs and hotpluggable vCPUs. For example, vCPU-0 and vCPU-1
>>   >>   >>   are cold-booted vCPUs while
>>   >>   >>   vCPU-2 and vCPU-3 are hotpluggable vCPUs when we have '-smp
>>   >>   >>   maxcpus=4,cpus=2'. Lets stick to convention and terms for easier discussion.
>>   >>   >>
>>   >>   >>   The idea is avoid instantiating hotpluggable vCPUs in virtmach_init() and
>>   >>   >>   released in the same function for those hotpluggable vCPUs. As I can
>>   >>   >>   understand, those hotpluggable vCPU instances are serving for two
>>   >>   >>   purposes: (1) Relax the constraint that all vCPU's (kvm) file descriptor have
>>   >>   >>   to be created and populated;
>>   >>   >
>>   >>   >
>>   >>   > We are devising *workarounds* in Qemu for the ARM CPU architectural
>>   >>   > constraints in KVM and in Guest Kernel,  *not relaxing* them. We are
>>   >>   > not allowed to meddle with the constraints. That is the whole point.
>>   >>   >
>>   >>   > Not having to respect those constraints led to rejection of the
>>   >>   > earlier attempts to upstream Virtual CPU Hotplug for ARM.
>>   >>   >
>>   >>
>>   >>   I meant to 'overcome' the constraints by 'relax'. My apologies if there're any
>>   >>   caused confusions.
>>   >
>>   >
>>   > Ok. No issues. It was important for me to clarify though.
>>   >
>>   >
>>   >   Previously, you had attempt to create all vCPU objects
>>   >>   and reuse them when vCPU hot added.
>>   >
>>   > Yes, at QOM level. But that approach did not realize the
>>   > unplugged/yet-to-be-plugged vCPUs. We were just using QOM vCPU objects
>>   > as the place holders
>>   >
>>   
>>   Right, my point was actually vCPU objects are too heavy as the place
>>   holders. It was reason why I had the concern: why those hotpluggable vCPU
>>   objects can't be avoided during the bootup time.
> 
> 
> Sure, to list down again. For the reasons I've already explained :
> 
> 1. KVM MUST have details about all the vCPUs and its features finalized before the VGIC
>      initialization. This is ARM CPU Architecture constraint. This is immutable requirement!
> 2. QOM vCPUs has got representational changes of the KVM vCPU like vcpu-id, features
>      list etc. These MUST be finalized even before QOM begins its own GICv3 initialization
>      which will end up in initialization of KVM VGIC. QOM vCPUs MUST be handed over to
>      the GICV3 QOM in fully initialized state. The same reason applies here as well.
>      Till this point we are architecture compliant. The only place where we are not ARM
>      Architecture compliant is where in QOM, we dissociate the vCPU state with GICV3
>      CPU State during unplug action or for the yet-to-be-plugged vCPUs. Later are
>      released as part of virt_cpu_post_init() in our current design.
> 

Thanks for your time to evaluate and reply. Sorry that I'm not convinced. You're explaining
what we already have in current design and implementation. It doesn't mean the current design
and implementation is 100% perfect and flawless.

In current design and implementation, all (QOM) vCPU objects have to be instantiated, even
though those hotpluggable (QOM) vCPU objects aren't realized at bootup time. After that, those
(QOM) hotpluggable vCPU objects are finalized and destroyed so that they can be hot added
afterwards. This sounds like we create (QOM) vCPU objects, remove those (QOM) vCPU objects
at booting time so that they can be hot-added at running time. The duplicate work is obvious
there. This scheme and design looks just-for-working, not in an optimized way to me.

I'm giving up the efforts to convince you. Lets see if other reviewers will have same concern
or not. Another possibility would be to have current implementation (with all fixes) merged
upstream firstly, and then seek optimizations after that. That time, the suggestion can be
re-evaluated.

> 
>>   >   In current implementation, the
>>   >>   hotpluggable vCPUs are instantiated and released pretty soon. I was
>>   >>   bringing the third possibility, to avoid instantiating those hotpluggable vCPU
>>   >>   objects, for discussion.
>>   >
>>   >
>>   > Are you suggesting not calling KVM_ARM_VCPU_INIT IOCTL as all for the
>>   > vCPUs which are part of possible list but not yet plugged?
>>   >
>>   > If yes, we cannot do that as KVM vCPUs should be fully initialized
>>   > even before VGIC is initialized inside the KVM. This is a constraint.
>>   > I've explained this in detail in the cover letter of this patch-set
>>   > and in the slides I have shared earlier.
>>   >
>>   
>>   No, it's not what I was suggesting. What I suggested is to avoid creating
>>   those hotpluggable vCPU objects (place holders) during the bootup time.
>>   However, all vCPU file descriptors (KVM
>>   objects) still need to be created and initialized before GICv3 is initialized. It's
>>   one of the constrains. So we need to create and populate all vCPU file
>>   descriptors through ioctl(vm_fd, CREATE_VCPU) and ioctl(vcpu_fd,
>>   INIT_VCPU) before GICv3 object is created and realized. As I explained in
>>   the previous reply, the hotpluggable vCPU objects (place holders) haven't
>>   to be created in order to initialize and populate the vCPU file descriptors for
>>   those hotpluggable vCPUs. I think the parameters used to create and
>>   initialize vCPU-0's file descriptor can be reused by all other vCPUs, because
>>   we don't support heterogeneous vCPUs.
>>   What I suggested is something like below: the point is to avoid instantiating
>>   those hotpluggable vCPUs, but their vCPU file descriptor (KVM object) are
>>   still created and initialized.
>>   
>>        static void machvirt_init(MachineState *machine)
>>        {
>>   
>>            /*
>>             * Instantiate and realize vCPU-0, record the parameter passed to
>>             * ioctl(vcpu-fd, VCPU_INIT, &init), or a better place to remember the
>>   parameter.
>>             * The point is the parameter can be shared by all vCPUs.
>>             */
>>   
>>            /*
>>             * Create vCPU descriptors for all other vCPUs (including hotpluggable
>>   vCPUs).
>>             * The remembered parameter is reused and passed to ioctl(vcpu-fd,
>>   VCPU_INIT, &init).
>>             */
>>   
>>            /* Instanaite and realize other cold-booted vCPUs */
>>   
>>            /* Instantiate and realize GICv3 */
>>   
>>        }
> 
> 
> No. For the reasons I've mentioned above, we MUST provide fully initialize the QOM
> vCPU objects before initialization of QOM GICV3 kicks in. This ensures that nothing
> breaks during initialization process of the QOM GICV3.  Therefore, the optimization
> steps mentioned above are unnecessary and could cause problems in future.
> Additionally, the evolution of the GICV3 QOM can be independent of the ARM Virt
> Machine as it can be used with other Machines as well so we MUST treat it as a black
> box which needs QOM vCPU objects as inputs during its initialization.
> 

Your explanation is not completely correct to me. It's what we had in current design. It
doesn't means the design should have to be like this. The (KVM) vCPU file descriptor must
be in place before QOM GICv3 object is instantiated and realized, but the (QOM) vCPU objects
don't have to exist before that. However, we may need a lot of code changes to tame GICv3Common
and GICv3KVM so that they can be vCPU hot-add/remove friendly and then to avoid the pre-created
(QOM) vCPU objects.

It can be something to be re-evaluated in the future, as I said above. Frankly, It's pointless
to take our time on the discussions without reaching to any conclusions. From my side, I've tried
my best to provide my comments and explain my thoughts. Appreciated for your patience, time on
evaluation, and replies :)

Thanks,
Gavin