Re: [PATCH 2/3] migration: implement query migration threadinfo by name

[Juan Quintela <quintela@redhat.com>]

Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
> On 2023/1/30 12:27, Juan Quintela wrote:
>> Jiang Jiacheng <jiangjiacheng@huawei.com> wrote:
>>> Introduce interface query-migrationthreads. The interface is use
>>> with the migration thread name reported by qemu and returns with
>>> migration thread name and its pid.
>>> Introduce threadinfo.c to manage threads with migration.
>>>
>>> Signed-off-by: Jiang Jiacheng <jiangjiacheng@huawei.com>
>> 
>> I like this query interface better than the way you expose the thread
>> name in the 1st place.
>
> The event in patch1 is used to pass the thread name since libvirt
> doesn't know the name of the migration thread but the interface below
> need its name.
> I think the event can be dropped if we store the thread name in
> libvirt(if the migration thread name is fixed in qemu) or using the
> 'query-migrationthreads' you mentioned below.

I preffer the query migrationthreads, thanks.
>
>> 
>> But once that we are here, why don't we just "tweak" abit the interface:
>> 
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index b0cf366ac0..e93c3e560a 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1970,6 +1970,36 @@
>>>  { 'command': 'query-vcpu-dirty-limit',
>>>    'returns': [ 'DirtyLimitInfo' ] }
>>>  
>>> +##
>>> +# @MigrationThreadInfo:
>>> +#
>>> +# Information about migrationthreads
>>> +#
>>> +# @name: the name of migration thread
>>> +#
>>> +# @thread-id: ID of the underlying host thread
>>> +#
>>> +# Since: 7.2
>>> +##
>>> +{ 'struct': 'MigrationThreadInfo',
>>> +  'data': {'name': 'str',
>>> +           'thread-id': 'int'} }
>> 
>> 1st, it is an int enough for all architectures?  I know that for linux
>> and friends it is, but not sure about windows and other weird systems.
>> 
>
> It is only enough for migration pin which I want to implement. But I
> think this struct can be easily expand if we need other information
> about migration thread.

I mean that pthread_t (what you are passing here) is an int on linux.
Not sure on other OS's.

>>> +##
>>> +# @query-migrationthreads:
>> 
>> What about renaming this to:
>> 
>> @query-migrationthread (I removed the last 's')
>> 
>> because it returns the info of a single thread.
>> 
>>> +#
>>> +# Returns threadInfo about the thread specified by name
>>> +#
>>> +# data: migration thread name
>>> +#
>>> +# returns: information about the specified thread
>>> +#
>>> +# Since: 7.2
>>> +##
>>> +{ 'command': 'query-migrationthreads',
>>> +  'data': { 'name': 'str' },
>>> +  'returns': 'MigrationThreadInfo' }
>>> +
>>>  ##
>>>  # @snapshot-save:
>>>  #
>> 
>> And leaves the @query-migrationthreads name for something in the spirit of
>> 
>>> +{ 'command': 'query-migrationthreads',
>>> +  'returns': ['str'] }
>> 
>> That returns all the migration threads names.
>> 
>> Or perhaps even
>> 
>>> +{ 'command': 'query-migrationthreads',
>>> +  'returns': ['MigrationThreadInfo'] }
>> 
>> And call it a day?
>
> I think it's the best. If in this way, should we keep the interface to
> query specified thread?

I wouldn't do it, but it is up to you.

My (little) understanding of what you want to do is that you want all
the threads, so I see no reason to have a query for a single one.

Later, Juan.