[Qemu-devel] [PATCH] migration: Make analyze-migration script target-page-size aware

[Qemu-devel] [PATCH] migration: Make analyze-migration script target-page-size aware

[Alexander Graf]

The configuration section has a new subsection to transmit the target page
size along with the migration stream. The analyze migration script needs
to learn about that to read configuration streams that were triggering
this subsection to get transmitted.

With this patch applied, I can successfully analyze migration streams
on AArch64 again.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 scripts/analyze-migration.py | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index 1455387..02784f2 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -254,12 +254,25 @@ class HTABSection(object):
 
 
 class ConfigurationSection(object):
+    QEMU_VM_SUBSECTION    = 0x05
+
     def __init__(self, file):
         self.file = file
 
     def read(self):
         name_len = self.file.read32()
         name = self.file.readstr(len = name_len)
+        oldpos = self.file.tell()
+        if self.file.read8() == self.QEMU_VM_SUBSECTION:
+            name = self.file.readstr()
+            version_id = self.file.read32()
+            if name == "configuration/target-page-bits":
+                target_page_size = self.file.read32()
+            else:
+                raise Exception("Unknown config subsection: %s" % name)
+        else:
+            # No subsection following, forget that we ever read anything
+            self.file.seek(oldpos)
 
 class VMSDFieldGeneric(object):
     def __init__(self, desc, file):
-- 
1.8.5.6

Re: [Qemu-devel] [PATCH] migration: Make analyze-migration script target-page-size aware

[Dr. David Alan Gilbert]

* Alexander Graf (agraf@suse.de) wrote:
> The configuration section has a new subsection to transmit the target page
> size along with the migration stream. The analyze migration script needs
> to learn about that to read configuration streams that were triggering
> this subsection to get transmitted.
> 
> With this patch applied, I can successfully analyze migration streams
> on AArch64 again.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  scripts/analyze-migration.py | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index 1455387..02784f2 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -254,12 +254,25 @@ class HTABSection(object):

(Note I'm not a particularly python person, so take lightly)

>  class ConfigurationSection(object):
> +    QEMU_VM_SUBSECTION    = 0x05
> +

It's odd, you already have this constant defined twice in this script.

>      def __init__(self, file):
>          self.file = file
>  
>      def read(self):
>          name_len = self.file.read32()
>          name = self.file.readstr(len = name_len)
> +        oldpos = self.file.tell()
> +        if self.file.read8() == self.QEMU_VM_SUBSECTION:
> +            name = self.file.readstr()
> +            version_id = self.file.read32()
> +            if name == "configuration/target-page-bits":
> +                target_page_size = self.file.read32()

All of your other references to target_page_size in the script
are self.TARGET_PAGE_SIZE.

You might want to make the conditional subsection check into a function
somewhere, but that's OK for now.

Dave

> +            else:
> +                raise Exception("Unknown config subsection: %s" % name)
> +        else:
> +            # No subsection following, forget that we ever read anything
> +            self.file.seek(oldpos)
>  
>  class VMSDFieldGeneric(object):
>      def __init__(self, desc, file):
> -- 
> 1.8.5.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: Make analyze-migration script target-page-size aware

[Alexander Graf]

On 07.07.17 17:00, Dr. David Alan Gilbert wrote:
> * Alexander Graf (agraf@suse.de) wrote:
>> The configuration section has a new subsection to transmit the target page
>> size along with the migration stream. The analyze migration script needs
>> to learn about that to read configuration streams that were triggering
>> this subsection to get transmitted.
>>
>> With this patch applied, I can successfully analyze migration streams
>> on AArch64 again.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   scripts/analyze-migration.py | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
>> index 1455387..02784f2 100755
>> --- a/scripts/analyze-migration.py
>> +++ b/scripts/analyze-migration.py
>> @@ -254,12 +254,25 @@ class HTABSection(object):
> 
> (Note I'm not a particularly python person, so take lightly)
> 
>>   class ConfigurationSection(object):
>> +    QEMU_VM_SUBSECTION    = 0x05
>> +
> 
> It's odd, you already have this constant defined twice in this script.

Yes, it lives once per class. I am not sure how to easily make it a global.

> 
>>       def __init__(self, file):
>>           self.file = file
>>   
>>       def read(self):
>>           name_len = self.file.read32()
>>           name = self.file.readstr(len = name_len)
>> +        oldpos = self.file.tell()
>> +        if self.file.read8() == self.QEMU_VM_SUBSECTION:
>> +            name = self.file.readstr()
>> +            version_id = self.file.read32()
>> +            if name == "configuration/target-page-bits":
>> +                target_page_size = self.file.read32()
> 
> All of your other references to target_page_size in the script
> are self.TARGET_PAGE_SIZE.

Well, all other ones are actual page sizes :). This variable is never 
used anywhere - we just want to stash it somewhere.

> You might want to make the conditional subsection check into a function
> somewhere, but that's OK for now.

I really hope we don't need it again. All subsections should be 
described via the in-stream json description. We just missed out the 
configuration one because it's not part of the object model.


Alex

Re: [Qemu-devel] [PATCH] migration: Make analyze-migration script target-page-size aware

[Dr. David Alan Gilbert]

* Alexander Graf (agraf@suse.de) wrote:
> 
> 
> On 07.07.17 17:00, Dr. David Alan Gilbert wrote:
> > * Alexander Graf (agraf@suse.de) wrote:
> > > The configuration section has a new subsection to transmit the target page
> > > size along with the migration stream. The analyze migration script needs
> > > to learn about that to read configuration streams that were triggering
> > > this subsection to get transmitted.
> > > 
> > > With this patch applied, I can successfully analyze migration streams
> > > on AArch64 again.
> > > 
> > > Signed-off-by: Alexander Graf <agraf@suse.de>
> > > ---
> > >   scripts/analyze-migration.py | 13 +++++++++++++
> > >   1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> > > index 1455387..02784f2 100755
> > > --- a/scripts/analyze-migration.py
> > > +++ b/scripts/analyze-migration.py
> > > @@ -254,12 +254,25 @@ class HTABSection(object):
> > 
> > (Note I'm not a particularly python person, so take lightly)
> > 
> > >   class ConfigurationSection(object):
> > > +    QEMU_VM_SUBSECTION    = 0x05
> > > +
> > 
> > It's odd, you already have this constant defined twice in this script.
> 
> Yes, it lives once per class. I am not sure how to easily make it a global.
> 
> > 
> > >       def __init__(self, file):
> > >           self.file = file
> > >       def read(self):
> > >           name_len = self.file.read32()
> > >           name = self.file.readstr(len = name_len)
> > > +        oldpos = self.file.tell()
> > > +        if self.file.read8() == self.QEMU_VM_SUBSECTION:
> > > +            name = self.file.readstr()
> > > +            version_id = self.file.read32()
> > > +            if name == "configuration/target-page-bits":
> > > +                target_page_size = self.file.read32()
> > 
> > All of your other references to target_page_size in the script
> > are self.TARGET_PAGE_SIZE.
> 
> Well, all other ones are actual page sizes :). This variable is never used
> anywhere - we just want to stash it somewhere.

oh, so you're not actually using the value read here to influence the
parsing later?

> > You might want to make the conditional subsection check into a function
> > somewhere, but that's OK for now.
> 
> I really hope we don't need it again. All subsections should be described
> via the in-stream json description. We just missed out the configuration one
> because it's not part of the object model.

OK.

Dave

> 
> 
> Alex
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: Make analyze-migration script target-page-size aware

[Alexander Graf]

On 07.07.17 17:06, Dr. David Alan Gilbert wrote:
> * Alexander Graf (agraf@suse.de) wrote:
>>
>>
>> On 07.07.17 17:00, Dr. David Alan Gilbert wrote:
>>> * Alexander Graf (agraf@suse.de) wrote:
>>>> The configuration section has a new subsection to transmit the target page
>>>> size along with the migration stream. The analyze migration script needs
>>>> to learn about that to read configuration streams that were triggering
>>>> this subsection to get transmitted.
>>>>
>>>> With this patch applied, I can successfully analyze migration streams
>>>> on AArch64 again.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>    scripts/analyze-migration.py | 13 +++++++++++++
>>>>    1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
>>>> index 1455387..02784f2 100755
>>>> --- a/scripts/analyze-migration.py
>>>> +++ b/scripts/analyze-migration.py
>>>> @@ -254,12 +254,25 @@ class HTABSection(object):
>>>
>>> (Note I'm not a particularly python person, so take lightly)
>>>
>>>>    class ConfigurationSection(object):
>>>> +    QEMU_VM_SUBSECTION    = 0x05
>>>> +
>>>
>>> It's odd, you already have this constant defined twice in this script.
>>
>> Yes, it lives once per class. I am not sure how to easily make it a global.
>>
>>>
>>>>        def __init__(self, file):
>>>>            self.file = file
>>>>        def read(self):
>>>>            name_len = self.file.read32()
>>>>            name = self.file.readstr(len = name_len)
>>>> +        oldpos = self.file.tell()
>>>> +        if self.file.read8() == self.QEMU_VM_SUBSECTION:
>>>> +            name = self.file.readstr()
>>>> +            version_id = self.file.read32()
>>>> +            if name == "configuration/target-page-bits":
>>>> +                target_page_size = self.file.read32()
>>>
>>> All of your other references to target_page_size in the script
>>> are self.TARGET_PAGE_SIZE.
>>
>> Well, all other ones are actual page sizes :). This variable is never used
>> anywhere - we just want to stash it somewhere.
> 
> oh, so you're not actually using the value read here to influence the
> parsing later?

Exactly. As far as I can tell it doesn't have any impact on the 
migration format, so there's no need to use it.


Alex

Re: [Qemu-devel] [PATCH] migration: Make analyze-migration script target-page-size aware

[Dr. David Alan Gilbert]

* Alexander Graf (agraf@suse.de) wrote:
> 
> 
> On 07.07.17 17:00, Dr. David Alan Gilbert wrote:
> > * Alexander Graf (agraf@suse.de) wrote:
> > > The configuration section has a new subsection to transmit the target page
> > > size along with the migration stream. The analyze migration script needs
> > > to learn about that to read configuration streams that were triggering
> > > this subsection to get transmitted.
> > > 
> > > With this patch applied, I can successfully analyze migration streams
> > > on AArch64 again.
> > > 
> > > Signed-off-by: Alexander Graf <agraf@suse.de>
> > > ---
> > >   scripts/analyze-migration.py | 13 +++++++++++++
> > >   1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> > > index 1455387..02784f2 100755
> > > --- a/scripts/analyze-migration.py
> > > +++ b/scripts/analyze-migration.py
> > > @@ -254,12 +254,25 @@ class HTABSection(object):
> > 
> > (Note I'm not a particularly python person, so take lightly)
> > 
> > >   class ConfigurationSection(object):
> > > +    QEMU_VM_SUBSECTION    = 0x05
> > > +
> > 
> > It's odd, you already have this constant defined twice in this script.
> 
> Yes, it lives once per class. I am not sure how to easily make it a global.
> 
> > 
> > >       def __init__(self, file):
> > >           self.file = file
> > >       def read(self):
> > >           name_len = self.file.read32()
> > >           name = self.file.readstr(len = name_len)
> > > +        oldpos = self.file.tell()
> > > +        if self.file.read8() == self.QEMU_VM_SUBSECTION:
> > > +            name = self.file.readstr()
> > > +            version_id = self.file.read32()
> > > +            if name == "configuration/target-page-bits":
> > > +                target_page_size = self.file.read32()
> > 
> > All of your other references to target_page_size in the script
> > are self.TARGET_PAGE_SIZE.
> 
> Well, all other ones are actual page sizes :). This variable is never used
> anywhere - we just want to stash it somewhere.

Well you do read self.TARGET_PAGE_SIZE elsewhere - it determines the
amount of data to read from each RAM header in RamSections::read

Also note that the value being read in the config header is
target_page_bits - i.e. ln2(target_page_size).

> > You might want to make the conditional subsection check into a function
> > somewhere, but that's OK for now.
> 
> I really hope we don't need it again. All subsections should be described
> via the in-stream json description. We just missed out the configuration one
> because it's not part of the object model.

Dave

> 
> Alex
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] migration: Make analyze-migration script target-page-size aware

[Peter Xu]

On Fri, Jul 07, 2017 at 02:14:43PM +0200, Alexander Graf wrote:
> The configuration section has a new subsection to transmit the target page
> size along with the migration stream. The analyze migration script needs
> to learn about that to read configuration streams that were triggering
> this subsection to get transmitted.
> 
> With this patch applied, I can successfully analyze migration streams
> on AArch64 again.
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  scripts/analyze-migration.py | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
> index 1455387..02784f2 100755
> --- a/scripts/analyze-migration.py
> +++ b/scripts/analyze-migration.py
> @@ -254,12 +254,25 @@ class HTABSection(object):
>  
>  
>  class ConfigurationSection(object):
> +    QEMU_VM_SUBSECTION    = 0x05
> +
>      def __init__(self, file):
>          self.file = file
>  
>      def read(self):
>          name_len = self.file.read32()
>          name = self.file.readstr(len = name_len)
> +        oldpos = self.file.tell()
> +        if self.file.read8() == self.QEMU_VM_SUBSECTION:

Here name_len missing?

               name_len = self.file.read8()

(though all of them are going to be threw away...)

> +            name = self.file.readstr()
> +            version_id = self.file.read32()
> +            if name == "configuration/target-page-bits":
> +                target_page_size = self.file.read32()
> +            else:
> +                raise Exception("Unknown config subsection: %s" % name)
> +        else:
> +            # No subsection following, forget that we ever read anything
> +            self.file.seek(oldpos)

I guess we need self.file.file.seek(oldpos) here? Since otherwise on
x86 we'll get:

Traceback (most recent call last):
  File "./analyze-migration.py", line 615, in <module>
    dump.read(dump_memory = args.memory)
  File "./analyze-migration.py", line 542, in read
    section.read()
  File "./analyze-migration.py", line 275, in read
    self.file.seek(oldpos)
AttributeError: 'MigrationFile' object has no attribute 'seek'

Thanks,

-- 
Peter Xu