Re: [Qemu-devel] [PATCH v2 2/2] docs: Document vCPU hotplug procedure

Re: [Qemu-devel] [PATCH v2 2/2] docs: Document vCPU hotplug procedure

[Kashyap Chamarthy]

On Thu, Sep 27, 2018 at 04:33:16PM +0200, Markus Armbruster wrote:
> Igor Mammedov <imammedo@redhat.com> writes:
> 
> > On Tue, 25 Sep 2018 18:02:48 +0200
> > Kashyap Chamarthy <kchamart@redhat.com> wrote:

[...]

> >> +(3) Check which socket is free to allow hotplugging a CPU::
> > may be: which cpus are possible to plug (an entry with qom-path
> > property describes an existing cpu)
>
> Suggest
> 
>     (3) Find out which CPU types could be plugged, and into which sockets:

Yeah, clearer.

[...]

> >> +(4) We can see that socket 1 is free,
> 
> How?  I know, but only because I just read the documentation of
> query-hotpluggable-cpus.  Which by the way sucks.  For instance, will
> the command always return exactly one HotpluggableCPU object per socket?

About the 'how', I was not entirely sure, hence my request in the cover
letter.

> Anyway, what about this:
> 
>     The command returns an object with a "qom-path" member for each
>     present CPU.  In this case, it shows an IvyBridge-IBRS-x86_64-cpu in
>     socket 0.
> 
>     It returns an object without a "qom-path" for every possibly CPU
>     hot-plug.  In this case, it shows you can plug an
>     IvyBridge-IBRS-x86_64-cpu into socket 1, and the additional
>     properties you need to pass to device_add for that.

Crystal clear.

Many thanks for the review!

> > ... and 'arguments' provide a list of property/value pairs to create
> > corresponding cpu.
> >
> >> +    "IvyBridge-IBRS-x86_64-cpu"::
> 
> Suggest
> 
>     (4) Hot-plug an additional CPU:

[...]

-- 
/kashyap

Re: [Qemu-devel] [PATCH v2 2/2] docs: Document vCPU hotplug procedure

[Igor Mammedov]

On Mon, 1 Oct 2018 10:18:45 +0200
Kashyap Chamarthy <kchamart@redhat.com> wrote:

> On Thu, Sep 27, 2018 at 04:33:16PM +0200, Markus Armbruster wrote:
> > Igor Mammedov <imammedo@redhat.com> writes:
> >   
> > > On Tue, 25 Sep 2018 18:02:48 +0200
> > > Kashyap Chamarthy <kchamart@redhat.com> wrote:  
> 
> [...]
> 
> > >> +(3) Check which socket is free to allow hotplugging a CPU::  
> > > may be: which cpus are possible to plug (an entry with qom-path
> > > property describes an existing cpu)  
> >
> > Suggest
> > 
> >     (3) Find out which CPU types could be plugged, and into which sockets:  
> 
> Yeah, clearer.
> 
> [...]
> 
> > >> +(4) We can see that socket 1 is free,  
> > 
> > How?  I know, but only because I just read the documentation of
> > query-hotpluggable-cpus.  Which by the way sucks.  For instance, will
> > the command always return exactly one HotpluggableCPU object per socket?  
> 
> About the 'how', I was not entirely sure, hence my request in the cover
> letter.
> 
> > Anyway, what about this:
> > 
> >     The command returns an object with a "qom-path" member for each
> >     present CPU.  In this case, it shows an IvyBridge-IBRS-x86_64-cpu in
> >     socket 0.
> > 
> >     It returns an object without a "qom-path" for every possibly CPU
> >     hot-plug.  In this case, it shows you can plug an
> >     IvyBridge-IBRS-x86_64-cpu into socket 1, and the additional
> >     properties you need to pass to device_add for that.
not really sure my English (CCed Eric) but to match 'an object' with
the rest of sentence:

 It returns an object without a "qom-path" for a possible to hot-plug CPU.
 +
 In this case, it shows you can plug an  IvyBridge-IBRS-x86_64-cpu
 into socket 1/core = 0/thread 0, where 'props' list describes
 additional properties you need to pass to device_add for hot-pluging
 that CPU.

> 
> Crystal clear.
> 
> Many thanks for the review!
> 
> > > ... and 'arguments' provide a list of property/value pairs to create
> > > corresponding cpu.
> > >  
> > >> +    "IvyBridge-IBRS-x86_64-cpu"::  
> > 
> > Suggest
> > 
> >     (4) Hot-plug an additional CPU:  
> 
> [...]
> 

Re: [Qemu-devel] [PATCH v2 2/2] docs: Document vCPU hotplug procedure

[Eric Blake]

On 10/1/18 3:59 AM, Igor Mammedov wrote:

>>> Anyway, what about this:
>>>
>>>      The command returns an object with a "qom-path" member for each
>>>      present CPU.  In this case, it shows an IvyBridge-IBRS-x86_64-cpu in
>>>      socket 0.
>>>
>>>      It returns an object without a "qom-path" for every possibly CPU
>>>      hot-plug.  In this case, it shows you can plug an
>>>      IvyBridge-IBRS-x86_64-cpu into socket 1, and the additional
>>>      properties you need to pass to device_add for that.
> not really sure my English (CCed Eric) but to match 'an object' with
> the rest of sentence:
> 
>   It returns an object without a "qom-path" for a possible to hot-plug CPU.
>   +
>   In this case, it shows you can plug an  IvyBridge-IBRS-x86_64-cpu
>   into socket 1/core = 0/thread 0, where 'props' list describes
>   additional properties you need to pass to device_add for hot-pluging
>   that CPU.

Maybe:

The command returns an object for CPUs that are present (containing a 
"qom-path" member) or which may be hot-plugged (no "qom-path" member). 
In this example, an IvyBridge-IBRS-x86_64-cpu is present in socket 0, 
while hot-plugging a CPU into socket 1 requires passing the listed 
properties to device_add.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 2/2] docs: Document vCPU hotplug procedure

[Igor Mammedov]

On Mon, 8 Oct 2018 14:10:50 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 10/1/18 3:59 AM, Igor Mammedov wrote:
> 
> >>> Anyway, what about this:
> >>>
> >>>      The command returns an object with a "qom-path" member for each
> >>>      present CPU.  In this case, it shows an IvyBridge-IBRS-x86_64-cpu in
> >>>      socket 0.
> >>>
> >>>      It returns an object without a "qom-path" for every possibly CPU
> >>>      hot-plug.  In this case, it shows you can plug an
> >>>      IvyBridge-IBRS-x86_64-cpu into socket 1, and the additional
> >>>      properties you need to pass to device_add for that.  
> > not really sure my English (CCed Eric) but to match 'an object' with
> > the rest of sentence:
> > 
> >   It returns an object without a "qom-path" for a possible to hot-plug CPU.
> >   +
> >   In this case, it shows you can plug an  IvyBridge-IBRS-x86_64-cpu
> >   into socket 1/core = 0/thread 0, where 'props' list describes
> >   additional properties you need to pass to device_add for hot-pluging
> >   that CPU.  
> 
> Maybe:
> 
> The command returns an object for CPUs that are present (containing a 
> "qom-path" member) or which may be hot-plugged (no "qom-path" member). 
> In this example, an IvyBridge-IBRS-x86_64-cpu is present in socket 0,
> while hot-plugging a CPU into socket 1 requires passing the listed 
> properties to device_add.
to be precise it's a logical cpu in socket/core/thread, but considering
example has only 2 socket and 2 cpus total, suggested variant probably
is good too.

Re: [Qemu-devel] [PATCH v2 1/2] Deprecate QMP `cpu-add`

[Thomas Huth]

On 2018-09-25 18:02, Kashyap Chamarthy wrote:
> The intended functionality of QMP `cpu-add` is replaced with
> `device_add` (and `query-hotpluggable-cpus`).  So let's deprecate
> `cpu-add`.
> 
> A complete example of vCPU hotplug with the recommended way (using
> `device_add`) is provided as part of a seperate docs patch.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com
> Signed-off-by: Kashyap Chamarthy <kchamart@redhat.com>
> ---
> ---
>  qapi/misc.json       | 8 +++++++-
>  qemu-deprecated.texi | 5 +++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/misc.json b/qapi/misc.json
> index d450cfef21..6479b8f8a6 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1104,7 +1104,11 @@
>  ##
>  # @cpu-add:
>  #
> -# Adds CPU with specified ID
> +# Adds CPU with specified ID.
> +#
> +# Notes: This command is deprecated.  The `device_add` command should be

s/Notes/Note/ ?

> +#        used instead.  See the `query-hotpluggable-cpus` command for
> +#        details.
>  #
>  # @id: ID of CPU to be created, valid values [0..max_cpus)
>  #
> @@ -3213,6 +3217,8 @@
>  ##
>  # @query-hotpluggable-cpus:
>  #
> +# TODO: Better documentation; currently there is none.
> +#
>  # Returns: a list of HotpluggableCPU objects.
>  #
>  # Since: 2.7
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 1b9c007f12..c86924ad9a 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -155,6 +155,11 @@ The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
>  The ``arch'' output member of the ``query-cpus-fast'' command is
>  replaced by the ``target'' output member.
>  
> +@subsection cpu-add (since 3.1)
> +
> +Use ``device_add'' for hotplugging vCPUs instead of ``cpu-add''.  See
> +documentation of ``query-hotpluggable-cpus'' for additional details.
> +
>  @section System emulator devices
>  
>  @subsection ivshmem (since 2.6.0)
> 

Do you plan to keep the "cpu-add" HMP command? hmp_cpu_add() currently
is only a wrapper for qmp_cpu_add(), so if you plan to get rid of the
QMP command, it might make sense to deprecate the HMP command in the
same breath, too.

 Thomas

Re: [Qemu-devel] [PATCH v2 1/2] Deprecate QMP `cpu-add`

[Kashyap Chamarthy]

On Mon, Oct 01, 2018 at 11:28:17AM +0200, Thomas Huth wrote:
> On 2018-09-25 18:02, Kashyap Chamarthy wrote:

[...]

> > +++ b/qapi/misc.json
> > @@ -1104,7 +1104,11 @@
> >  ##
> >  # @cpu-add:
> >  #
> > -# Adds CPU with specified ID
> > +# Adds CPU with specified ID.
> > +#
> > +# Notes: This command is deprecated.  The `device_add` command should be
> 
> s/Notes/Note/ ?

Yeah, first I wrote the singular.  But went with plural as I saw it as
it was the 'majority' pattern:

    $> git grep "Note:" qapi/misc.json | wc -l
    13
    $> git grep "Notes:" qapi/misc.json | wc -l
    18

Maybe people use the plural, "Notes", as they can add multiple entries.

[...]

> Do you plan to keep the "cpu-add" HMP command? hmp_cpu_add() currently
> is only a wrapper for qmp_cpu_add(), so if you plan to get rid of the
> QMP command, it might make sense to deprecate the HMP command in the
> same breath, too.

Yeah, I did think about deprecating the HMP variant; and even brought it
up with Dave Gilbert the other day.  He pointed out an example commit of
yours (559964a1) on how to mark an HMP command as deprecated. :-)

Thanks for the reminder.  Will add it as a TODO for the next revision.

-- 
/kashyap

Re: [Qemu-devel] [PATCH v2 1/2] Deprecate QMP `cpu-add`

[Markus Armbruster]

Kashyap Chamarthy <kchamart@redhat.com> writes:

> On Mon, Oct 01, 2018 at 11:28:17AM +0200, Thomas Huth wrote:
>> On 2018-09-25 18:02, Kashyap Chamarthy wrote:
>
> [...]
>
>> > +++ b/qapi/misc.json
>> > @@ -1104,7 +1104,11 @@
>> >  ##
>> >  # @cpu-add:
>> >  #
>> > -# Adds CPU with specified ID
>> > +# Adds CPU with specified ID.
>> > +#
>> > +# Notes: This command is deprecated.  The `device_add` command should be
>> 
>> s/Notes/Note/ ?
>
> Yeah, first I wrote the singular.  But went with plural as I saw it as
> it was the 'majority' pattern:
>
>     $> git grep "Note:" qapi/misc.json | wc -l
>     13
>     $> git grep "Notes:" qapi/misc.json | wc -l
>     18

Not exactly overwhelming majority :)

> Maybe people use the plural, "Notes", as they can add multiple entries.

For what it's worth, our doc generator recognizes both.

[...]