RE: [Xen-devel] [PATCH 1/5 TAKE 2] xenoprof: split xen x86 xenoprof code

["Santos, Jose Renato G" <joserenato.santos@xxxxxx>]

Isaku,

Mostly, the patches look good. 
I have a few questions though , mostly for clarification

Keir,

Could you please wait until I have a chance to test the patches on my
machine before applying.
I will be out all day tomorrow (Thursday) to attend an external event so
I will only be able to do this on Friday.
Also, I think it would be good  if you could review parts of patch 5.
There are some changes on how xenoprof shares page with the guest. The
patch decouples the code for allocating xenoprof buffer from the code
for sharing it with the domain. This is very welcome as it has been on
my wish list for a while. This modification should remove the current
limitation that guests cannot use different profiling modes (active and
passive) in different profiling sessions. But I am afraid I do not
understand Xen memory management in detail to review this part of the
patch. Please, take a look.

-----------------------
QUESTIONS for Isaku:

patch 4/5:
==========

> +struct xenoprof_shared_buffer {
> +     char                                    *buffer;
> +     struct xenoprof_arch_shared_buffer      arch;
> +};

The arch field has no extra info for x86. Why do you need this?


> @@ -103,7 +98,7 @@ static int __init init_driverfs(void)
>  }
>  
>  
> -static void __exit exit_driverfs(void)
> +static void /* __exit */ exit_driverfs(void)
>  {
>       sysdev_unregister(&device_oprofile);
>       sysdev_class_unregister(&oprofile_sysclass);

Why removing "__exit" is needed? 


> +void xenoprof_arch_start(void) 
> +{
> +     /* nothing */
> +}
> +
> +void xenoprof_arch_stop(void)
> +{
> +     /* nothing */
> +}
> +

Why do we need arch specific functions for start and stop?

> @@ -22,9 +22,25 @@
>  
>  #ifndef __XEN_XENOPROF_H__
>  #define __XEN_XENOPROF_H__
> +
>  #ifdef CONFIG_OPROFILE
> -
>  #include <asm/xenoprof.h>
>  
> +struct oprofile_operations;
> +int xenoprofile_init(struct oprofile_operations * ops);
> +void xenoprofile_exit(void);
> +
> +// for perfmon/xen

Please drop the comment or change it (xenoprofile is not using perfmon)


patch 5/5:
=========

> -static char *alloc_xenoprof_buf(struct domain *d, int npages)
> +static char *alloc_xenoprof_buf(struct domain *d, int npages,
uint64_t gmaddr)

The extra parameter gmaddr is not needed since if it is not used by the
function.


Regards

Renato

 

> -----Original Message-----
> From: Isaku Yamahata [mailto:yamahata@xxxxxxxxxxxxx] 
> Sent: Tuesday, November 14, 2006 6:14 PM
> To: Santos, Jose Renato G
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx; xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH 1/5 TAKE 2] xenoprof: split 
> xen x86 xenoprof code
> 
> On Tue, Nov 14, 2006 at 11:23:42AM -0600, Santos, Jose Renato G wrote:
> 
> > This patch is corrupt. It does not apply cleanly. It has several 
> > missing "newlines" such as for example the line above.
> > Could you please regenerate ?
> > I am not sure about the other patches, but it would help if you can 
> > make sure they are all OK and resend.
> 
> I regenerated them for 12446:fb107b9eee86 of xen-unstable.hg.
> They should apply cleanly and compile.
> To make sure not to corrupt the patches, I attached them as a 
> tar ball.
> Please find it.
> 
> --
> yamahata
> 

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