Xen devel - Jan 2010 - [PATCH] HVM vcpu hotplug: Fix acpi method NTFY bug

[This email is either empty or too large to be displayed at this time]

[This email is either empty or too large to be displayed at this time]

On 27/01/2010 05:33, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> HVM vcpu hotplug: Fix acpi method NTFY bug
> 
> Cancel define method NTFY by using decision_tree, it will block vcpu add
and
> result in other unpredicted error. The second reason is method NTFY is not
key
> path and no necessary using complicated decision_tree to reduce scan time.
The
> third reason is, the scan loop defined by method PRSC is not necessary
equal
> to the scan loop defined by method NTFY, and not necessary equal to 2^n.
> 
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
You''ll have to point out how this fixes the NTFY method I''m
afraid -- the
two methods look exactly equivalent to me, except the existing method should
on average execute with far fewer compares, and hence looks preferable.

And please don''t send updates to dsdt.c in patches which update DSDT. I
always re-generate it myself anyway, and it makes your patch 1000x bigger.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser wrote:
> On 27/01/2010 05:33, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> 
>> HVM vcpu hotplug: Fix acpi method NTFY bug
>> 
>> Cancel define method NTFY by using decision_tree, it will block vcpu
>> add and result in other unpredicted error. The second reason is
>> method NTFY is not key path and no necessary using complicated
>> decision_tree to reduce scan time. The third reason is, the scan
>> loop defined by method PRSC is not necessary equal to the scan loop
>> defined by method NTFY, and not necessary equal to 2^n. 
>> 
>> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> You''ll have to point out how this fixes the NTFY method
I''m afraid --
> the two methods look exactly equivalent to me, except the existing
> method should on average execute with far fewer compares, and hence
> looks preferable. 
> 
> And please don''t send updates to dsdt.c in patches which update
DSDT.
> I always re-generate it myself anyway, and it makes your patch 1000x
> bigger. 
> 
>  -- Keir
suppose
x = scan loop number defined at method PRSC
y = scan loop number defined at method NTFY
in original way, there are 2 implicit precondition to make it work right:
1). y = x
2). y = 2^n
however, these preconditions are not always be satisfied.
for example, if x > y, it will produce unexpected scan and block vcpu
add/remove.

BTW, decision_tree() is too complicated to understand. it''s used to
produce dsdt.dsl middle code.
people need spend much time to image what''s the temporary dsdt.dsl
middle code like.
this is not good for maintain and debug.
for this reason, I''d like to change it back to normal for() loop, easy
to understand and maintain.
(it''s correct that decision_tree() will reduce scan loop greatly, for
example, 128 vcpus scan number will reduce from 64 to 7 times avg, however,
it''s not important since this code is not key path, seldom used).

Thanks,
Jinsong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 29/01/2010 07:32, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> suppose
> x = scan loop number defined at method PRSC
> y = scan loop number defined at method NTFY
> in original way, there are 2 implicit precondition to make it work right:
> 1). y = x
> 2). y = 2^n
> however, these preconditions are not always be satisfied.
> for example, if x > y, it will produce unexpected scan and block vcpu
> add/remove.
Well, that didn''t help. Some code comments would be handy.

Looking at method PRSC the algorithm seems pretty mad given we auto-generate
the code. Apparently we *always* call NTFY for every value of
0<=Arg0<=127,
in order? And then you would propose to do a further linear If chain within
NTFY? Why not merge NTFY into PRSC: if you must have a linear scan anyway in
PRSC, just unroll the while loops, and inline the relevant bit of NTFY for
every unrolled loop invocation.

And please provide some code comments relating this all back to the ACPI
Spec, and explaining what the local variables represent, etc. It''s
totally
impossible to understand right now. Although at least loop unrolling will
get rid of several local variables...

 -- Keir
> BTW, decision_tree() is too complicated to understand. it''s used
to produce
> dsdt.dsl middle code.
> people need spend much time to image what''s the temporary dsdt.dsl
middle code
> like.
> this is not good for maintain and debug.
> for this reason, I''d like to change it back to normal for() loop,
easy to
> understand and maintain.
> (it''s correct that decision_tree() will reduce scan loop greatly,
for example,
> 128 vcpus scan number will reduce from 64 to 7 times avg, however,
it''s not
> important since this code is not key path, seldom used).


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser wrote:
> On 29/01/2010 07:32, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> 
>> suppose
>> x = scan loop number defined at method PRSC
>> y = scan loop number defined at method NTFY
>> in original way, there are 2 implicit precondition to make it work
>> right: 1). y = x 2). y = 2^n
>> however, these preconditions are not always be satisfied.
>> for example, if x > y, it will produce unexpected scan and block
vcpu
>> add/remove.
> 
> Well, that didn''t help. Some code comments would be handy.
> 
> Looking at method PRSC the algorithm seems pretty mad given we
> auto-generate the code. Apparently we *always* call NTFY for every
> value of 0<=Arg0<=127, in order? And then you would propose to do a
> further linear If chain within NTFY? Why not merge NTFY into PRSC: if
> you must have a linear scan anyway in PRSC, just unroll the while
> loops, and inline the relevant bit of NTFY for every unrolled loop
> invocation. 
> 
> And please provide some code comments relating this all back to the
> ACPI Spec, and explaining what the local variables represent, etc.
> It''s totally impossible to understand right now. Although at least
> loop unrolling will get rid of several local variables...
> 
>  -- Keir
> 
Keir,

According to your suggestion, I plan to modify the patch as:
1. cancel method NTFY, merge it into method PRSC;
2. at method PRSC, only scan ''maxvcpus'' times. we can transfer
''maxvcpus'' (which comes from cmdline) from qemu to dsdt side
through bios_info;
3. at mk_dsdt.c, add some comments to make it clear;
Do you think it OK?

Thanks,
Jinsong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 31/01/2010 14:04, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> According to your suggestion, I plan to modify the patch as:
> 1. cancel method NTFY, merge it into method PRSC;
> 2. at method PRSC, only scan ''maxvcpus'' times. we can
transfer ''maxvcpus''
> (which comes from cmdline) from qemu to dsdt side through bios_info;
> 3. at mk_dsdt.c, add some comments to make it clear;
> Do you think it OK?
I don''t even mind if you don''t do #2. That''s up to
you. Bear in mind that to
be able to merge NTFY into PRSC, you will have to completely unroll PRSC
(one way to view it is moving the loops out of the DSDT and into mk_dsdt.c,
to be executed at build time and produce straight-line code). That would
mean you emit the code to check against max_vcpus 128 times. Is it worth it?
I don''t know.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser wrote:
> On 31/01/2010 14:04, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> 
>> According to your suggestion, I plan to modify the patch as:
>> 1. cancel method NTFY, merge it into method PRSC;
>> 2. at method PRSC, only scan ''maxvcpus'' times. we can
transfer
>> ''maxvcpus'' (which comes from cmdline) from qemu to
dsdt side through
>> bios_info; 
>> 3. at mk_dsdt.c, add some comments to make it clear;
>> Do you think it OK?
> 
> I don''t even mind if you don''t do #2. That''s up
to you. Bear in mind
> that to be able to merge NTFY into PRSC, you will have to completely
> unroll PRSC (one way to view it is moving the loops out of the DSDT
> and into mk_dsdt.c, to be executed at build time and produce
> straight-line code). That would mean you emit the code to check
> against max_vcpus 128 times. Is it worth it? I don''t know.
Keir,

How about the followed update:
1. keep original method NTFY, keep decision_tree to reduce scan loop;
2. update method PRSC
    1). transfer para ''maxvcpus'' (comes from config file) from
qemu to mk_dsdt.c through bios_info;
    2). at PRSC, only scan ''maxvcpus'' vcpus;
because maxvcpus< 128, no risk for NTFY then.

Thanks,
Jinsong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 01/02/2010 03:31, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> How about the followed update:
> 1. keep original method NTFY, keep decision_tree to reduce scan loop;
> 2. update method PRSC
>     1). transfer para ''maxvcpus'' (comes from config file)
from qemu to
> mk_dsdt.c through bios_info;
>     2). at PRSC, only scan ''maxvcpus'' vcpus;
> because maxvcpus< 128, no risk for NTFY then.
Well, I''m confused now. #2 is really no more than an optimisation,
right?
And #1 contradicts your original patch, which only affected NTFY, and you
claimed was a bug fix.

Is there, or is there not, currently a bug in NTFY? Or some bug in the way
it is called by PRSC?

I mean, if there''s no bug, let''s leave it alone. At least
until 4.0.0 is
done. I still haven''t been able to understand your original complaints
about
the current NTFY method by the way -- I still firmly believe it is
behaviourally identical to your patched version, for any given pair of
arguments passed to it.

I could be missing something. If so you''re going to have spell it out
very
slowly and clearly. :-)

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser wrote:
> On 01/02/2010 03:31, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> 
>> How about the followed update:
>> 1. keep original method NTFY, keep decision_tree to reduce scan loop;
>> 2. update method PRSC
>>     1). transfer para ''maxvcpus'' (comes from config
file) from qemu
>>     to mk_dsdt.c through bios_info; 2). at PRSC, only scan
>> ''maxvcpus'' vcpus; 
>> because maxvcpus< 128, no risk for NTFY then.
> 
> Well, I''m confused now. #2 is really no more than an optimisation,
> right? And #1 contradicts your original patch, which only affected
> NTFY, and you claimed was a bug fix.
> 
> Is there, or is there not, currently a bug in NTFY? Or some bug in
> the way it is called by PRSC?
> 
> I mean, if there''s no bug, let''s leave it alone. At least
until 4.0.0
> is done. I still haven''t been able to understand your original
> complaints about the current NTFY method by the way -- I still firmly
> believe it is behaviourally identical to your patched version, for
> any given pair of arguments passed to it.
> 
> I could be missing something. If so you''re going to have spell it
out
> very slowly and clearly. :-)

Sorry, I didn''t tell the story clear.

Sevreal days ago, when we pull from xen upstream (c/s 20840), we found vcpu 
add work fail.
For example, at config file, we set
    maxvcpus=8
    vcpus=3
When we add a new vcpu through monitor by ''cpu_set 3 online'',
it cannot add
new cpu subdir as /sys/devices/system/cpu/cpu3, and the subdir cpu1 & cpu2 
also disappear.

We debug it, and found some issue with method NTFY and PRSC:
1. for method NTFY, dsdt code auto-generated by mk_dsdt.c is
        Method (NTFY, 2, NotSerialized)
        {
            If (And (Arg0, 0x40))
            {
                If (And (Arg0, 0x20))
                {
                    If (And (Arg0, 0x10))
                    {
                        If (And (Arg0, 0x08))
                        {
                            If (And (Arg0, 0x04))
                            {
                                If (And (Arg0, 0x02))
                                {
                                    If (And (Arg0, One))
                                    {
                                        If (LNotEqual (Arg1, ^PR7F.FLG))
                                        {
                                            Store (Arg1, ^PR7F.FLG)
                                            If (LEqual (Arg1, One))
                                            {
                                                Notify (PR7F, One)
                                                Subtract (MSU, One, MSU)
                                            }
                                            Else
                                            {
                                                Notify (PR7F, 0x03)
                                                Add (MSU, One, MSU)
                                            }
                                        }
                                    }
                                    Else
                                ......

                                ......
                                Else
                                {
                                    If (And (Arg0, One))
                                    {
                                        If (LNotEqual (Arg1, ^PR01.FLG))
                                        {
                                            Store (Arg1, ^PR01.FLG)
                                            If (LEqual (Arg1, One))
                                            {
                                                Notify (PR01, One)
                                                Subtract (MSU, One, MSU)
                                            }
                                            Else
                                            {
                                                Notify (PR01, 0x03)
                                                Add (MSU, One, MSU)
                                            }
                                        }
                                    }
                                    Else
                                    {
                                        If (LNotEqual (Arg1, ^PR00.FLG))
                                        {
                                            Store (Arg1, ^PR00.FLG)
                                            If (LEqual (Arg1, One))
                                            {
                                                Notify (PR00, One)
                                                Subtract (MSU, One, MSU)
                                            }
                                            Else
                                            {
                                                Notify (PR00, 0x03)
                                                Add (MSU, One, MSU)
                                            }
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }

            Return (One)
        }
 
2. for method PRSC, dsdt code auto-generated by mk_dsdt.c is
        OperationRegion (PRST, SystemIO, 0xAF00, 0x20)
        Field (PRST, ByteAcc, NoLock, Preserve)
        {
            PRS,    256
        }

        Method (PRSC, 0, NotSerialized)
        {
            Store (PRS, Local3)
            Store (Zero, Local0)
            While (LLess (Local0, 0x20))
            {
                Store (Zero, Local1)
                Store (DerefOf (Index (Local3, Local0)), Local2)
                While (LLess (Local1, 0x08))
                {
                    NTFY (Add (Multiply (Local0, 0x08), Local1), And (Local2, 
                        One))
                    ShiftRight (Local2, One, Local2)
                    Increment (Local1)
                }

                Increment (Local0)
            }

            Return (One)
        }
   
3. so PRSC will scan vcpu from 0 to 255, the problem comes.
    for example, when we add vcpu3, PRSC will scan vcpu3, it will execute
          Notify (PR03, One)
          Subtract (MSU, One, MSU)
    until now, it''s OK as expected, will add node
/sys/devices/system/cpu3
    however, when PRSC continue scan vcpu 128+3, it will execute
          Notify (PR03, 0x03)
          Add (MSU, One, MSU)
    this is not we expected, will remove node /sys/devices/system/cpu3

4. the key issue comes from the scan loop of NTFY < scan loop of PRSC.
That''s the problem.

5. a simple way to solve the issue is, to make sure scan loop of NTFY = scan
loop of PRSC, and to make sure NTFY always scan 2^n vcpus.
    However, NTFY scan loop may change in the future, not necessary equal to
2^n, and not necessary equal to scan loop of PRSC. That''s the reason
why I use for() loop inside of decision_tree() in method NTFY at the patch I
send Jan 27. It will work correctly under whatever conditions, and keep
mk_dsdt.c easier to understand. Decision_tree indeed reduce scan greatly, but
it''s not in key path.


Thanks,
Jinsong
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 01/02/2010 11:21, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> 5. a simple way to solve the issue is, to make sure scan loop of NTFY =
scan
> loop of PRSC, and to make sure NTFY always scan 2^n vcpus.
>     However, NTFY scan loop may change in the future, not necessary equal
to
> 2^n, and not necessary equal to scan loop of PRSC. That''s the
reason why I use
> for() loop inside of decision_tree() in method NTFY at the patch I send Jan
> 27. It will work correctly under whatever conditions, and keep mk_dsdt.c
> easier to understand. Decision_tree indeed reduce scan greatly, but
it''s not
> in key path.
Ah, now I understand!

Okay, how about the attached patch? This fixes the bug by inlining NTFY into
PRSC, and also uses HVM_MAX_VCPUS as appropriate.

Please take a look and also test it. I''ll apply it if/when you Ack it.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser wrote:
> On 01/02/2010 11:21, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> 
>> 5. a simple way to solve the issue is, to make sure scan loop of
>> NTFY = scan loop of PRSC, and to make sure NTFY always scan 2^n
>>     vcpus. However, NTFY scan loop may change in the future, not
>> necessary equal to 2^n, and not necessary equal to scan loop of
>> PRSC. That''s the reason why I use for() loop inside of
>> decision_tree() in method NTFY at the patch I send Jan 
>> 27. It will work correctly under whatever conditions, and keep
>> mk_dsdt.c easier to understand. Decision_tree indeed reduce scan
>> greatly, but it''s not in key path.
> 
> Ah, now I understand!
> 
> Okay, how about the attached patch? This fixes the bug by inlining
> NTFY into PRSC, and also uses HVM_MAX_VCPUS as appropriate.
> 
> Please take a look and also test it. I''ll apply it if/when you Ack
it.
> 
>  -- Keir
Keir,

It''s really good to merge NTFY with PRSC, it''s easy to
understand and effective!
I just change a little, as attached 01-dsdt.
I test these 2 patches, it''s OK.

Thanks,
Jinsong



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 02/02/2010 05:47, "Liu, Jinsong" <jinsong.liu@intel.com>
wrote:
> It''s really good to merge NTFY with PRSC, it''s easy to
understand and
> effective!
> I just change a little, as attached 01-dsdt.
> I test these 2 patches, it''s OK.
Ah, I missed the Makefile chunk of my patch, hence you needed your fix.
Anyway, glad it works. I will check it in.

 Thanks,
 Keir




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel