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

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Wed, Nov 15, 2006 at 09:58:40PM -0600, Santos, Jose Renato G wrote:

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

Thank you for your review.


> 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?

On IA64 it is defined as
struct xenoprof_arch_shared_buffer {
        struct resource*        res;
};

On IA64, auto translated mode feature is enabled.
It means that page mapping from xen is based on pseudo physical address,
not virtual address. Pseudo physical address used to map pages
must be recorded somewhere in order to unmap.
I guess, if xenoprof/x86 supported auto translated guest mode
something similar structure would be necessary.


> > @@ -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? 

Because it is called by non-"__exit" function.
__init oprofile_init()
  => oprofile_arch_exit() // error recovery
     => xenoprof_arch_exit()
        => exit_driver()
When I compiled xenLinux/IA64, linker complained.
I suspect that "__exit" of the original oprofile_arch_exit() of
linux-2.6-xen-sparse/arch/i386/oprofile/xenropof.c is wrong.
Actually the vanilla oprofile_arch_exit() of
linux/arch/i386/oprofile/init.c doesn't carry "__exit".
Gcc didn't complain when I compiled xenLinux/i386, though.


> > +void xenoprof_arch_start(void) 
> > +{
> > +   /* nothing */
> > +}
> > +
> > +void xenoprof_arch_stop(void)
> > +{
> > +   /* nothing */
> > +}
> > +
> 
> Why do we need arch specific functions for start and stop?

On IA64 they are defined as
void xenoprof_arch_start(void) 
{
        perfmon_start();
}

void xenoprof_arch_stop(void)
{
        perfmon_stop();
}

struct op_counter_config is x86 specific. I chose to define
op_counter_config related operation as nop on IA64 and
to add those two hooks.
On IA64, registers related to performance monitoring are manipulated
through perfmonctl system call.
The oprofile/ia64 doesn't manipulate them directly.
It just tells perfmon to start/stop. So those hooks are needed.

It would be possible to define more generic interface to
manipulated performance monitoring registers.
Actually the perfmon project is working on that and
the effort is under way.
(It is held by hp lab so that you may already know that though.)
I think it is too eary to do so for xenoprof.


> > @@ -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)

I'll drop it.


> 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.

I'll remove it.

-- 
yamahata

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