Xen devel - Apr 2006 - [PATCH] Xenoprof passive domain support

Hi Renato,
This patch is to add Xenoprof passive domain support in SMP environment.
Basically: 
- It allocates per vcpu buffers for passive domain and maps them into
primary domain''s space. 
- When primary domain gets sampled and triggers virq, its kernel module
will handle passive domain'' samples besides its owns. There is
potential
buffer overflow if passive is very busy while primary is idle. The
simple workaround is to allocate a rather big buffer for passive. For
now it works fine. A better way is to notify primary when passive buffer
exceeds the threshold.
- When oprofile gets passive domain samples, it splits them in to 3
parts by PC ranges pre-definition: xen/kernel/application and maps the
first two into functions. The oprofile version is a little old, for the
patch was made last August. 

The main usage is to use Xenoprof to profile HVM domain as passive
domain, which provides enough information for low level performance
tuning while no change in HVM is needed.

Thanks,
Xiaowei



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

Resent the patch due to cset 9866 change (Rename *GUEST_HANDLE to
*XEN_GUEST_HANDLE).
Thanks,
xiaowei

>-----Original Message-----
>From: Yang, Xiaowei
>Sent: 2006年4月28日 15:59
>To: ''Santos, Jose Renato G''
>Cc: ''xen-devel@lists.xensource.com''
>Subject: [PATCH] Xenoprof passive domain support
>
>Hi Renato,
>This patch is to add Xenoprof passive domain support in SMP environment.
>Basically:
>- It allocates per vcpu buffers for passive domain and maps them into
primary
>domain''s space.
>- When primary domain gets sampled and triggers virq, its kernel module will
>handle passive domain'' samples besides its owns. There is potential
buffer
>overflow if passive is very busy while primary is idle. The simple
workaround
>is to allocate a rather big buffer for passive. For now it works fine. A
better
>way is to notify primary when passive buffer exceeds the threshold.
>- When oprofile gets passive domain samples, it splits them in to 3 parts by
>PC ranges pre-definition: xen/kernel/application and maps the first two into
>functions. The oprofile version is a little old, for the patch was made last
>August.
>
>The main usage is to use Xenoprof to profile HVM domain as passive domain,
which
>provides enough information for low level performance tuning while no change
>in HVM is needed.
>
>Thanks,
>Xiaowei

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

Xiaowei,

Thanks for the patch.
I took a first look in the patch and have a few comments.

First, I think there is a concurrency problem that needs to
fixed. If dom0 has more than one VCPU, then 2 VCPUs can
access the same passive buffer at the same time. This
should not be allowed, since the buffer is not
protected by any lock. We should make sure that any
passive domain buffer is accessed by one and only one
dom0 VCPU. A simple approach would be to have only
VCPU 0 (in dom0) to handle all the passive domain
samples. However, this may cause an 
imbalance on the speed that oprofile CPU buffers are filled.
Probably the cpu buffer for VCPU 0 would fill much
earlier and cause more frequent flushes to the event buffer,
increasing the overhead.
I would prefer if we could distribute the passive
buffers to all VCPUs in dom0. I will think more about
this during the weekend and make additional comments,
earlier next week.

Second, I see that you reused most of the code that
was used in Xen 2.0, when passive domains was supported
for non SMP guests. 
John Levon (cc''ed on this message) has made some comments
on that code which I think we should address right now. 
More specifically, he suggested that we change the way
domain switches are represented in the CPU buffer. 
Please look at his comment below. 

I suggest that we address these 2 issues first and
then I can look more carefully at the rest of the code.
Please CC John Levon, on you next messages

Thanks

Renato

Old comments by John Levon on passive domain code:
============================================================I''m
uncomfortable with the chosen domain switching encoding. It''s pretty
confusing to follow all the escaping done already, and you''re making it
more complicated. Why can''t you use a normal ESCAPE_CODE? I.e. the
buffer would have

   EIP		Event
---------------------------------
0. ESCAPE_CODE 	DOMAIN_SWITCH
1. domain ID
2. EIP		Event

Sure, you lose an entry (1) in size but it means simpler code, and less
i-cache hurt.

Why is the state not persistent until the next escape code? i.e. it''s
only for one sample.

=====================================================================>>
-----Original Message-----
>> From: Yang, Xiaowei [mailto:xiaowei.yang@intel.com] 
>> Sent: Friday, April 28, 2006 12:59 AM
>> To: Santos, Jose Renato G
>> Cc: xen-devel@lists.xensource.com
>> Subject: [PATCH] Xenoprof passive domain support
>> 
>> Hi Renato,
>> This patch is to add Xenoprof passive domain support in SMP 
>> environment. Basically: 
>> - It allocates per vcpu buffers for passive domain and maps 
>> them into primary domain''s space. 
>> - When primary domain gets sampled and triggers virq, its 
>> kernel module will handle passive domain'' samples besides 
>> its owns. There is potential buffer overflow if passive is 
>> very busy while primary is idle. The simple workaround is to 
>> allocate a rather big buffer for passive. For now it works 
>> fine. A better way is to notify primary when passive buffer 
>> exceeds the threshold.
>> - When oprofile gets passive domain samples, it splits them 
>> in to 3 parts by PC ranges pre-definition: 
>> xen/kernel/application and maps the first two into 
>> functions. The oprofile version is a little old, for the 
>> patch was made last August. 
>> 
>> The main usage is to use Xenoprof to profile HVM domain as 
>> passive domain, which provides enough information for low 
>> level performance tuning while no change in HVM is needed.
>> 
>> Thanks,
>> Xiaowei
>> 
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>First, I think there is a concurrency problem that needs to
>fixed. If dom0 has more than one VCPU, then 2 VCPUs can
>access the same passive buffer at the same time. This
>should not be allowed, since the buffer is not
>protected by any lock. We should make sure that any
>passive domain buffer is accessed by one and only one
>dom0 VCPU. A simple approach would be to have only
>VCPU 0 (in dom0) to handle all the passive domain
>samples. However, this may cause an
>imbalance on the speed that oprofile CPU buffers are filled.
>Probably the cpu buffer for VCPU 0 would fill much
>earlier and cause more frequent flushes to the event buffer,
>increasing the overhead.
>I would prefer if we could distribute the passive
>buffers to all VCPUs in dom0. I will think more about
>this during the weekend and make additional comments,
>earlier next week.
>
Renato,
Thanks for pointing this out. This is really a concurrency issue!
We can use a mutex and trylock mechanism. If one LP of dom0 is handling
passive domain buffer, the other can just ignore doing it. Since
different LP may acquires the lock every time, it satisfies your
requirement:)
>Second, I see that you reused most of the code that
>was used in Xen 2.0, when passive domains was supported
>for non SMP guests.
>John Levon (cc''ed on this message) has made some comments
>on that code which I think we should address right now.
>More specifically, he suggested that we change the way
>domain switches are represented in the CPU buffer.
>Please look at his comment below.
>
Yes, currently the way to handle passive samples is not efficient. I''m
taking the advice into account and doing changes.

Thanks,
Xiaowei

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

Renato,
I remade the patch. It has several improvements.
1) Add a lock for preventing more than one LPs of dom0 from accessing to passive
domain concurrently.
2) Add escape code only at the beginning and end of a bunch of passive domain
samples, which dom0 handles one time. Originally we added escape code in front
of every passive domain sample. That''s inefficient.
3) Reuse "normal ESCAPE_CODE" of oprofile to add passive domain switch
event into the CPU buffer.

Thanks,
-Xiaowei
>-----Original Message-----
>From: Santos, Jose Renato G [mailto:joserenato.santos@hp.com]
>Sent: 2006年4月29日 9:43
>To: Yang, Xiaowei
>Cc: xen-devel@lists.xensource.com; John Levon; Yoshio Turner; G John
>Janakiraman
>Subject: RE: [PATCH] Xenoprof passive domain support
>
>Xiaowei,
>
>Thanks for the patch.
>I took a first look in the patch and have a few comments.
>
>First, I think there is a concurrency problem that needs to
>fixed. If dom0 has more than one VCPU, then 2 VCPUs can
>access the same passive buffer at the same time. This
>should not be allowed, since the buffer is not
>protected by any lock. We should make sure that any
>passive domain buffer is accessed by one and only one
>dom0 VCPU. A simple approach would be to have only
>VCPU 0 (in dom0) to handle all the passive domain
>samples. However, this may cause an
>imbalance on the speed that oprofile CPU buffers are filled.
>Probably the cpu buffer for VCPU 0 would fill much
>earlier and cause more frequent flushes to the event buffer,
>increasing the overhead.
>I would prefer if we could distribute the passive
>buffers to all VCPUs in dom0. I will think more about
>this during the weekend and make additional comments,
>earlier next week.
>
>Second, I see that you reused most of the code that
>was used in Xen 2.0, when passive domains was supported
>for non SMP guests.
>John Levon (cc''ed on this message) has made some comments
>on that code which I think we should address right now.
>More specifically, he suggested that we change the way
>domain switches are represented in the CPU buffer.
>Please look at his comment below.
>
>I suggest that we address these 2 issues first and
>then I can look more carefully at the rest of the code.
>Please CC John Levon, on you next messages
>
>Thanks
>
>Renato
>
>Old comments by John Levon on passive domain code:
>============================================================>I''m
uncomfortable with the chosen domain switching encoding. It''s pretty
>confusing to follow all the escaping done already, and you''re
making it more
>complicated. Why can''t you use a normal ESCAPE_CODE? I.e. the
buffer would have
>
>   EIP		Event
>---------------------------------
>0. ESCAPE_CODE 	DOMAIN_SWITCH
>1. domain ID
>2. EIP		Event
>
>Sure, you lose an entry (1) in size but it means simpler code, and less
i-cache
>hurt.
>
>Why is the state not persistent until the next escape code? i.e.
it''s only for
>one sample.
>
>=====================================================================

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

"Yang, Xiaowei" <xiaowei.yang@intel.com> wrote on 04/29/2006
05:46:27 AM:
> Renato,
> I remade the patch. It has several improvements.
> 1) Add a lock for preventing more than one LPs of dom0 from
> accessing to passive domain concurrently.
> 2) Add escape code only at the beginning and end of a bunch of
> passive domain samples, which dom0 handles one time. Originally we
> added escape code in front of every passive domain sample. That''s
inefficient.
> 3) Reuse "normal ESCAPE_CODE" of oprofile to add passive domain
> switch event into the CPU buffer.
>
> Thanks,
> -Xiaowei
I tried the patch.  The build of a xen0 kernel fails with:
arch/i386/oprofile/../../../drivers/oprofile/oprof.c:41: error:
?MAX_OPROF_DOMAINS? undeclared here (not in a function)
arch/i386/oprofile/../../../drivers/oprofile/oprof.c: In function
?oprofile_set_active?:
arch/i386/oprofile/../../../drivers/oprofile/oprof.c:45: error: ?struct
oprofile_operations? has no member named ?set_active?
arch/i386/oprofile/../../../drivers/oprofile/oprof.c:46: error: ?struct
oprofile_operations? has no member named ?set_active?
arch/i386/oprofile/../../../drivers/oprofile/oprof.c: In function
?oprofile_set_passive?:
arch/i386/oprofile/../../../drivers/oprofile/oprof.c:56: error: ?struct
oprofile_operations? has no member named ?set_passive?
arch/i386/oprofile/../../../drivers/oprofile/oprof.c:57: error: ?struct
oprofile_operations? has no member named ?set_passive?
make[1]: *** [arch/i386/oprofile/../../../drivers/oprofile/oprof.o] Error 1
make: *** [arch/i386/oprofile] Error 2

It appears that the patch to patches/linux-2.6.16/xenoprof-generic.patch is
not correct.  For example:

-diff -pruN ../pristine-linux-2.6.16/include/linux/oprofile.h
./include/linux/oprofile.h
---- ../pristine-linux-2.6.16/include/linux/oprofile.h  2006-03-20
05:53:29.000000000 +0000
-+++ ./include/linux/oprofile.h 2006-04-03 15:53:05.000000000 +0100
-@@ -16,6 +16,8 @@
- #include <linux/types.h>
- #include <linux/spinlock.h>
- #include <asm/atomic.h>
-+
-+#include <xen/interface/xenoprof.h>
-
- struct super_block;
- struct dentry;
-@@ -27,6 +29,8 @@ struct oprofile_operations {
-       /* create any necessary configuration files in the oprofile fs.
-        * Optional. */
-       int (*create_files)(struct super_block * sb, struct dentry * root);
-+      /* setup active domains with Xen */
-+      int (*set_active)(int *active_domains, unsigned int adomains);
-       /* Do any necessary interrupt setup. Optional. */
-       int (*setup)(void);
-       /* Do any necessary interrupt shutdown. Optional. */

removes the xenoprof support from include/linux/oprofile.h rather than
adding the filed for set_passive to struct oprofile_operations.

The initial patch released on 2006/04/28 had it correct:

 diff -pruN ../pristine-linux-2.6.16/include/linux/oprofile.h
./include/linux/oprofile.h
---- ../pristine-linux-2.6.16/include/linux/oprofile.h  2006-03-20
05:53:29.000000000 +0000
-+++ ./include/linux/oprofile.h 2006-04-03 15:53:05.000000000 +0100
+--- ../pristine-linux-2.6.16/include/linux/oprofile.h  2006-04-27
16:22:31.000000000 +0800
++++ ./include/linux/oprofile.h 2006-04-25 17:20:36.000000000 +0800
 @@ -16,6 +16,8 @@
  #include <linux/types.h>
  #include <linux/spinlock.h>
@@ -373,12 +508,14 @@

  struct super_block;
  struct dentry;
-@@ -27,6 +29,8 @@ struct oprofile_operations {
+@@ -27,6 +29,10 @@ struct oprofile_operations {
        /* create any necessary configuration files in the oprofile fs.
         * Optional. */
        int (*create_files)(struct super_block * sb, struct dentry * root);
 +      /* setup active domains with Xen */
 +      int (*set_active)(int *active_domains, unsigned int adomains);
++      /* setup passive domains with Xen */
++      int (*set_passive)(struct xenoprof_passive *passive_domains,
unsigned int pdomains);
        /* Do any necessary interrupt setup. Optional. */
        int (*setup)(void);
        /* Do any necessary interrupt shutdown. Optional. */

I''ll hack my copy of the patch for now.  You may want to revisit how
the
patch is created.

Steve D.


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

>I tried the patch.  The build of a xen0 kernel fails with:
>arch/i386/oprofile/../../../drivers/oprofile/oprof.c:41: error:
>?MAX_OPROF_DOMAINS? undeclared here (not in a function)
Steve,
Sorry for late response, for I''m taking vacation this week. Thanks for
pointing this out. For me it''s easy to make mistake to produce a patch
of a patch. I may write a script to do this:) 

Thanks,
Xiaowei



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

Xiaowei,

Do you have a more recent patch for this?   I''m having a hard time
sorting out
the rejects, particularly the ones related to xenoprof-generic.patch. 
(I''m
working against change set 10031.)  Renato sent me something called 
xenoprof3.patch (could have been his name for this patch), but that could 
have just been his name for it.

Particularly confusing are the changes related to CPU_MODE_XEN in 
add_kernel_ctx_swtich().   There''s no case statement there at all
anymore,
just an "if (in_kernel)" statement.  And all I find in oprofile.h are 
#defines''s for CPU_IS_KERNEL, CPU_TRACE_BEGIN...

Any help would be appreciated.

Which change set were your patches of 4/28 against?

-- 
Ray Bryant
AMD Performance Labs                   Austin, Tx
512-602-0038 (o)                 512-507-7807 (c)


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

>Do you have a more recent patch for this?   I''m having a hard time
sorting out
>the rejects, particularly the ones related to xenoprof-generic.patch.
(I''m
>working against change set 10031.)  Renato sent me something called
>xenoprof3.patch (could have been his name for this patch), but that
could
>have just been his name for it.
>
>Particularly confusing are the changes related to CPU_MODE_XEN in
>add_kernel_ctx_swtich().   There''s no case statement there at all
anymore,
>just an "if (in_kernel)" statement.  And all I find in oprofile.h
are
>#defines''s for CPU_IS_KERNEL, CPU_TRACE_BEGIN...
>
>Any help would be appreciated.
>
>Which change set were your patches of 4/28 against?
>
Ray,
My last patch is against 9872, which is sent out 5/6. The strange thing
you met may be due to the failure of applying the patch to a newer
changeset. Renato is reviewing the patch. After that, I can port it the
latest.

Thanks,
Xiaowei

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