Xen devel - Feb 2009 - [PATCH-0/2] Hypervisor profiling using GCOV (64bit Hypervisor)

On Thu, Feb 19, 2009 at 8:41 PM, Gianluca Guida
<gianluca.guida@eu.citrix.com> wrote:
>
> On Feb 19, 2009, at 3:53 PM, George Dunlap wrote:
>
>> So I think that it probably would be useful.  Unfortunately, I
don''t
>> have time in the near future to look at either of these (new
"xcov"
>> functionality, or fixing xenoprofile).  Gianluca''s been doing
some
>> interesting work with testing.  I''ll ask him if he''s
interested in
>> looking at it.
>
> Definitely. Getting code coverage reports, whatever the tools, may help a
> lot when trying to debug a bizarre memory corruption or even just to check
> that a test you''re writing does what you actually want.
>
> Keep me in the loop and feel free to ask questions, I''ll start
reading
> patches and experimenting with it. IMHO, I think that 64bit hypervisor
> support will be by far more useful, since it''s the most used
version of the
> hypervisor, nowadays.
Sending Patches for the 64 bit Hypervisor, We have tested patches on
AMD-64 (Athlon(tm) 64 X2 Dual Core Processor 4600+) machine with
gcc-4.2 & gcc-3.4. Please refer the previous GCOV RFC in same mail for
more info on Hypervisor profiling.
We have added support to 32bit and 64bit Kernel.

In addition to patches for hypervisor profiling, we did a little work
on *lcov* to work with hypervisor.
README could be useful, for naive lcov user. locv-diff.patch show our
change in lcov scripts.

any comments, feedback and suggestion are more than welcome

P.S. a lcov screenshot with hypervisor profling has been attached.


thanks
-tej & team
>
> Thanks,
> Gianluca
>
>






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

Hi,

Tej wrote:
> Sending Patches for the 64 bit Hypervisor, We have tested patches on
> AMD-64 (Athlon(tm) 64 X2 Dual Core Processor 4600+) machine with
> gcc-4.2 & gcc-3.4. Please refer the previous GCOV RFC in same mail for
> more info on Hypervisor profiling.
> We have added support to 32bit and 64bit Kernel.
> 
> In addition to patches for hypervisor profiling, we did a little work
> on *lcov* to work with hypervisor.
> README could be useful, for naive lcov user. locv-diff.patch show our
> change in lcov scripts.
> 
> any comments, feedback and suggestion are more than welcome
While I still need to test the patch (building 3.3 right now) and to 
understand gcov internals, I think that a few comments can be done, 
mostly aestethicals.

- xen coding style: Using four-spaces tabs is generally the tradition. 
Also I do prefer to have brackets that start code blocks on a new line 
aligned to the previous line, but that''s not followed everywhere in the
code.

- Makefiles: while the num=$*.c is still a mystery to me, my question 
is: do you really need to make links with different names to files 
compiled multiple times? If so, it would be useful to remove them on 
''make clean''.
Also, it would be useful to make this feature enabled with a 
compile-time option.

More comments to follow as I test it.

Thanks,
Gianluca

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

On Sat, Feb 28, 2009 at 12:32 AM, Gianluca Guida
<gianluca.guida@eu.citrix.com> wrote:
> Hi,
>
> Tej wrote:
>>
>> Sending Patches for the 64 bit Hypervisor, We have tested patches on
>> AMD-64 (Athlon(tm) 64 X2 Dual Core Processor 4600+) machine with
>> gcc-4.2 & gcc-3.4. Please refer the previous GCOV RFC in same mail
for
>> more info on Hypervisor profiling.
>> We have added support to 32bit and 64bit Kernel.
>>
>> In addition to patches for hypervisor profiling, we did a little work
>> on *lcov* to work with hypervisor.
>> README could be useful, for naive lcov user. locv-diff.patch show our
>> change in lcov scripts.
>>
>> any comments, feedback and suggestion are more than welcome
>
> While I still need to test the patch (building 3.3 right now) and to
> understand gcov internals, I think that a few comments can be done, mostly
> aestethicals.
>
> - xen coding style: Using four-spaces tabs is generally the tradition. Also
> I do prefer to have brackets that start code blocks on a new line aligned
to
> the previous line, but that''s not followed everywhere in the code.
thanks for your comments and general feedback
Generally we uses Lindent script for tab/alignment problem...  i will
incorporate above said coding styles and resubmit
>
> - Makefiles: while the num=$*.c is still a mystery to me, my question is:
do
> you really need to make links with different names to files compiled
num=$*.c changes mainly added to run lcov smoothly... Let me take an example

in hap directory Makefile generate guest_walk_%level.o and gcov option
generates guest_walk_%level.gcno... but there is no
guest_walk_%level.c files , so lcov script give me warning/error
saying guest_walk_%level.gcno file does not have correspoding *.c.. so
we have added some Makefile trick to generate *.c link at compile
time.
> multiple times? If so, it would be useful to remove them on ''make
clean''.
nice point
thats absolutely needed,  so i will incorporated the changes
> Also, it would be useful to make this feature enabled with a compile-time
> option.
i will look into this also and resubmit the patches on
Monday...
>
> More comments to follow as I test it.
thanks for your review and comments
>
> Thanks,
> Gianluca
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Sat, Feb 28, 2009 at 12:32 AM, Gianluca Guida
<gianluca.guida@eu.citrix.com> wrote:
> Hi,
>
> Tej wrote:
>>
>> Sending Patches for the 64 bit Hypervisor, We have tested patches on
>> AMD-64 (Athlon(tm) 64 X2 Dual Core Processor 4600+) machine with
>> gcc-4.2 & gcc-3.4. Please refer the previous GCOV RFC in same mail
for
>> more info on Hypervisor profiling.
>> We have added support to 32bit and 64bit Kernel.
>>
>> In addition to patches for hypervisor profiling, we did a little work
>> on *lcov* to work with hypervisor.
>> README could be useful, for naive lcov user. locv-diff.patch show our
>> change in lcov scripts.
>>
>> any comments, feedback and suggestion are more than welcome
>
> While I still need to test the patch (building 3.3 right now) and to
> understand gcov internals, I think that a few comments can be done, mostly
> aestethicals.
>
> - xen coding style: Using four-spaces tabs is generally the tradition. Also
> I do prefer to have brackets that start code blocks on a new line aligned
to
> the previous line, but that''s not followed everywhere in the code.
>
> - Makefiles: while the num=$*.c is still a mystery to me, my question is:
do
> you really need to make links with different names to files compiled
> multiple times? If so, it would be useful to remove them on ''make
clean''.
> Also, it would be useful to make this feature enabled with a compile-time
> option.
Attached patches address the four-spaces tabs and make clean issues.
Hope the code is more clean and readable
>
> More comments to follow as I test it.
>
> Thanks,
> Gianluca
>


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

Hi,

Sorry for the late reply.

On Mar 26, 2009, at 7:59 AM, Tej wrote:
> On Mon, Mar 2, 2009 at 9:36 PM, Tej <bewith.tej@gmail.com> wrote:
>> On Sat, Feb 28, 2009 at 12:32 AM, Gianluca Guida
>> <gianluca.guida@eu.citrix.com> wrote:
>>> While I still need to test the patch (building 3.3 right now) and
to
>>> understand gcov internals, I think that a few comments can be  
>>> done, mostly
>>> aestethicals.
>>>
>>> - xen coding style: Using four-spaces tabs is generally the  
>>> tradition. Also
>>> I do prefer to have brackets that start code blocks on a new line  
>>> aligned to
>>> the previous line, but that''s not followed everywhere in
the code.
>>>
>>> - Makefiles: while the num=$*.c is still a mystery to me, my  
>>> question is: do
>>> you really need to make links with different names to files
compiled
>>> multiple times? If so, it would be useful to remove them on
''make
>>> clean''.
>>> Also, it would be useful to make this feature enabled with a  
>>> compile-time
>>> option.
>>
>> Attached patches address the four-spaces tabs and make clean issues.
>> Hope the code is more clean and readable
>
> Hi
>
> Sorry for interrupting you guys from your busy schedule, as i just
> want to know the current status for GCOV patches i submitted early
> this month.
> any feedback on "testing/coding/design/feature" will be
appreciated.
I''ve been testing it for a while, and it seems an useful and  
interesting feature.

A few comments, though:

- As I said, while the patch is minimal enough, the feature requires  
some intrusive changes in Xen, like changing the linker script (via  
the definition of CONSTRUCTORS) and in general it changes the compiler  
behavior, so I think it should be mandatory to make this feature  
explicitely enabled at compile time, e.g.  gcov=y.
- I am talking about Xen code only, but I think a few part of the  
codes should be more clear. I find very confusing the difference  
between xen_gcov_info and gcov_info, and all the instructions that  
uses them. A better naming scheme for the variables and more comments  
would be appreciated.

Thanks!
Gianluca

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

On Thu, Mar 26, 2009 at 4:27 PM, Gianluca Guida
<Gianluca.Guida@eu.citrix.com> wrote:
> Hi,
>
> Sorry for the late reply.
>
> On Mar 26, 2009, at 7:59 AM, Tej wrote:
>
>> On Mon, Mar 2, 2009 at 9:36 PM, Tej <bewith.tej@gmail.com> wrote:
>>>
>>> On Sat, Feb 28, 2009 at 12:32 AM, Gianluca Guida
>>> <gianluca.guida@eu.citrix.com> wrote:
>>>>
>>>> While I still need to test the patch (building 3.3 right now)
and to
>>>> understand gcov internals, I think that a few comments can be
done,
>>>> mostly
>>>> aestethicals.
>>>>
>>>> - xen coding style: Using four-spaces tabs is generally the
tradition.
>>>> Also
>>>> I do prefer to have brackets that start code blocks on a new
line
>>>> aligned to
>>>> the previous line, but that''s not followed everywhere
in the code.
>>>>
>>>> - Makefiles: while the num=$*.c is still a mystery to me, my
question
>>>> is: do
>>>> you really need to make links with different names to files
compiled
>>>> multiple times? If so, it would be useful to remove them on
''make
>>>> clean''.
>>>> Also, it would be useful to make this feature enabled with a
>>>> compile-time
>>>> option.
>>>
>>> Attached patches address the four-spaces tabs and make clean
issues.
>>> Hope the code is more clean and readable
>>
>> Hi
>>
>> Sorry for interrupting you guys from your busy schedule, as i just
>> want to know the current status for GCOV patches i submitted early
>> this month.
>> any feedback on "testing/coding/design/feature" will be
appreciated.
>
> I''ve been testing it for a while, and it seems an useful and
interesting
> feature.
glad to hear this
>
> A few comments, though:
>
> - As I said, while the patch is minimal enough, the feature requires some
> intrusive changes in Xen, like changing the linker script (via the
> definition of CONSTRUCTORS) and in general it changes the compiler
behavior,
> so I think it should be mandatory to make this feature explicitely enabled
> at compile time, e.g.  gcov=y.
> - I am talking about Xen code only, but I think a few part of the codes
> should be more clear. I find very confusing the difference between
> xen_gcov_info and gcov_info, and all the instructions that uses them. A
> better naming scheme for the variables and more comments would be
> appreciated.
basically there should not be no difference between xen_gcov_info and
gcov_info, but due to some earlier implementation difficulties we kept
 two different structure. In final implementation those difference
narrow down drastically but we kept the both structures. I will look
back into this.

and All comments will be addressed asap.

thanks
-tej
>
> Thanks!
> Gianluca
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, Mar 26, 2009 at 4:27 PM, Gianluca Guida
<Gianluca.Guida@eu.citrix.com> wrote:
> Hi,
>
> Sorry for the late reply.
>
> On Mar 26, 2009, at 7:59 AM, Tej wrote:
>
>> On Mon, Mar 2, 2009 at 9:36 PM, Tej <bewith.tej@gmail.com> wrote:
>>>
>>> On Sat, Feb 28, 2009 at 12:32 AM, Gianluca Guida
>>> <gianluca.guida@eu.citrix.com> wrote:
>>>>
>>>> While I still need to test the patch (building 3.3 right now)
and to
>>>> understand gcov internals, I think that a few comments can be
done,
>>>> mostly
>>>> aestethicals.
>>>>
>>>> - xen coding style: Using four-spaces tabs is generally the
tradition.
>>>> Also
>>>> I do prefer to have brackets that start code blocks on a new
line
>>>> aligned to
>>>> the previous line, but that''s not followed everywhere
in the code.
>>>>
>>>> - Makefiles: while the num=$*.c is still a mystery to me, my
question
>>>> is: do
>>>> you really need to make links with different names to files
compiled
>>>> multiple times? If so, it would be useful to remove them on
''make
>>>> clean''.
>>>> Also, it would be useful to make this feature enabled with a
>>>> compile-time
>>>> option.
>>>
>>> Attached patches address the four-spaces tabs and make clean
issues.
>>> Hope the code is more clean and readable
>>
>> Hi
>>
>> Sorry for interrupting you guys from your busy schedule, as i just
>> want to know the current status for GCOV patches i submitted early
>> this month.
>> any feedback on "testing/coding/design/feature" will be
appreciated.
>
> I''ve been testing it for a while, and it seems an useful and
interesting
> feature.
>
> A few comments, though:
>
> - As I said, while the patch is minimal enough, the feature requires some
> intrusive changes in Xen, like changing the linker script (via the
> definition of CONSTRUCTORS) and in general it changes the compiler
behavior,
> so I think it should be mandatory to make this feature explicitely enabled
> at compile time, e.g.  gcov=y.
patches attached added the compile time options gcov=y
a small readme is attached, which can be probably help in compiling
correctly at first time

gcov=n (default) disable every code part added by xen-gcov-core patch
(entry.S, core module, gcov constructor etc.)

Now in case, we have configured proc module (y || m) but did not use
gcov=y  option during compilation then proc module refuses to insert
as there is no hypercall added for gcov and we return -ENOENT (insmod
error type). Better suggestion are more than welcome

(i really don''t know if we can make Kernel configuration and xen
configuration in sync e.g gcov=y depends on Kconfig "xen-gcov-proc" or
vice versa)
> - I am talking about Xen code only, but I think a few part of the codes
> should be more clear. I find very confusing the difference between
> xen_gcov_info and gcov_info, and all the instructions that uses them. A
> better naming scheme for the variables and more comments would be
> appreciated.
this issue still we are working on it. But if we keep two structure as
it is then we will have less complex core and proc implementation.

comments??????


Sorry for some delay.
>
> Thanks!
> Gianluca
>



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

On Thu, Mar 26, 2009 at 4:27 PM, Gianluca Guida
<Gianluca.Guida@eu.citrix.com> wrote:
> Hi,
>
> Sorry for the late reply.
>
> On Mar 26, 2009, at 7:59 AM, Tej wrote:
>
>> On Mon, Mar 2, 2009 at 9:36 PM, Tej <bewith.tej@gmail.com> wrote:
>>>
>>> On Sat, Feb 28, 2009 at 12:32 AM, Gianluca Guida
>>> <gianluca.guida@eu.citrix.com> wrote:
>>>>
>>>> While I still need to test the patch (building 3.3 right now)
and to
>>>> understand gcov internals, I think that a few comments can be
done,
>>>> mostly
>>>> aestethicals.
>>>>
>>>> - xen coding style: Using four-spaces tabs is generally the
tradition.
>>>> Also
>>>> I do prefer to have brackets that start code blocks on a new
line
>>>> aligned to
>>>> the previous line, but that''s not followed everywhere
in the code.
>>>>
>>>> - Makefiles: while the num=$*.c is still a mystery to me, my
question
>>>> is: do
>>>> you really need to make links with different names to files
compiled
>>>> multiple times? If so, it would be useful to remove them on
''make
>>>> clean''.
>>>> Also, it would be useful to make this feature enabled with a
>>>> compile-time
>>>> option.
>>>
>>> Attached patches address the four-spaces tabs and make clean
issues.
>>> Hope the code is more clean and readable
>>
>> Hi
>>
>> Sorry for interrupting you guys from your busy schedule, as i just
>> want to know the current status for GCOV patches i submitted early
>> this month.
>> any feedback on "testing/coding/design/feature" will be
appreciated.
>
> I''ve been testing it for a while, and it seems an useful and
interesting
> feature.
>
> A few comments, though:
>
> - As I said, while the patch is minimal enough, the feature requires some
> intrusive changes in Xen, like changing the linker script (via the
> definition of CONSTRUCTORS) and in general it changes the compiler
behavior,
> so I think it should be mandatory to make this feature explicitely enabled
> at compile time, e.g.  gcov=y.
> - I am talking about Xen code only, but I think a few part of the codes
> should be more clear. I find very confusing the difference between
> xen_gcov_info and gcov_info, and all the instructions that uses them. A
> better naming scheme for the variables and more comments would be
> appreciated.
Removed xen_gcov_info dependency all together. Complete implementation
is based on gcc "gcov_info" struct only.
We have also included some meaningful names and comments. I hope that
these changes makes code more clear/clean and readable.

I have tested code in amd-64 with gcov=y and gcov=n, all worked fine,

Are you looking for any other other specific feature to be included in this?
We are missing only modules implementation of gcov which is not
required for xen hypervisor.

Any further comments/suggestion are more than welcome.

thanks
-tej & team

>
> Thanks!
> Gianluca
>


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