Recent trace patch not arch-neutral

Recent trace patch not arch-neutral

[Magenheimer, Dan (HP Labs Fort Collins)]

A recent patch to trace.c uses a call to rdtscll()
which is x86-specific.  Is there an arch-neutral call
that can be used instead?  Or do arch's need to
implement this?  (And if the latter, should we choose
a more generic name?)

Thanks,
Dan

RE: Recent trace patch not arch-neutral

[Ian Pratt]

> A recent patch to trace.c uses a call to rdtscll() which is 
> x86-specific.  Is there an arch-neutral call that can be used 
> instead?  Or do arch's need to implement this?  (And if the 
> latter, should we choose a more generic name?)

The tracebuffer code has always used the cycle counter, so if you've
previously been compiling it for ia64 it must have previously been using
some more arch neutral way of accessing it...

Ian

RE: Recent trace patch not arch-neutral

[Magenheimer, Dan (HP Labs Fort Collins)]

> > A recent patch to trace.c uses a call to rdtscll() which is 
> > x86-specific.  Is there an arch-neutral call that can be used 
> > instead?  Or do arch's need to implement this?  (And if the 
> > latter, should we choose a more generic name?)
> 
> The tracebuffer code has always used the cycle counter, so if you've
> previously been compiling it for ia64 it must have previously 
> been using
> some more arch neutral way of accessing it...

OK, from the hg annotated manifest it appeared (on first
glance) to be recently added.

Still not positive, but I think Xen/ia64 was compiling but
never linking in trace.c because the call from dom0_ops.c
to tb_control was controlled by #ifdef TRACE_BUFFER,
that ifdef is now gone, and Xen/ia64 never turned it on.

Looking quickly over trace.c, all appears to be arch-neutral
except for the call to rdtscll(), so the questions remain...

Is there an arch-neutral call that can be used 
instead?  Or do arch's need to implement this?  (And if the 
latter, should we choose a more generic name?)

Dan

Re: Recent trace patch not arch-neutral

[Rob Gardner]

[-- Attachment #1.1: Type: text/plain, Size: 1164 bytes --]

Ian Pratt wrote:

>>A recent patch to trace.c uses a call to rdtscll() which is 
>>x86-specific.  Is there an arch-neutral call that can be used 
>>instead?  Or do arch's need to implement this?  (And if the 
>>latter, should we choose a more generic name?)
>>    
>>
>
>The tracebuffer code has always used the cycle counter, so if you've
>previously been compiling it for ia64 it must have previously been using
>some more arch neutral way of accessing it...
>  
>


The deal with this is that the default was always trace=n so the trace 
macros never expanded to anything unless you wanted them to. One of the 
things that Keir did in "cleaning up" my code was to totally eliminate 
all conditional compilation. That's why this problem is suddenly showing 
up on ia64.

Now, to answer Dan's question- the rdtscll thing is just a time stamp 
counter, expressed in cycles. So on ia64 you could probably replace it 
with an asm statement to read ar.itc to make everything work. We just 
need a little wrapper to do the right thing for each architecture. Now 
Dan, if you were more conveniently located, perhaps we could work 
together and fix this. ;)

Rob


Rob


[-- Attachment #1.2: Type: text/html, Size: 1606 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: Recent trace patch not arch-neutral

[Rob Gardner]

[-- Attachment #1.1: Type: text/plain, Size: 706 bytes --]

Rob Gardner wrote:

>
> Now, to answer Dan's question- the rdtscll thing is just a time stamp 
> counter, expressed in cycles. So on ia64 you could probably replace it 
> with an asm statement to read ar.itc to make everything work. We just 
> need a little wrapper to do the right thing for each architecture. Now 
> Dan, if you were more conveniently located, perhaps we could work 
> together and fix this. ;)
>

I imagine we just need something that looks like this in trace.c:

#ifdef x86
       rdtscll(rec->cycles);
#endif
#ifdef IA64
       __asm__ __volatile ("mov %0=ar.itc;;" : "=r"(rec->cycles) :: 
"memory");
#endif

Dan, perhaps you know the nice clean way of doing this sort of thing?

Rob


[-- Attachment #1.2: Type: text/html, Size: 1186 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: Recent trace patch not arch-neutral

[Muli Ben-Yehuda]

On Mon, Oct 31, 2005 at 04:26:19PM -0700, Rob Gardner wrote:
> Rob Gardner wrote:
> 
> >
> >Now, to answer Dan's question- the rdtscll thing is just a time stamp 
> >counter, expressed in cycles. So on ia64 you could probably replace it 
> >with an asm statement to read ar.itc to make everything work. We just 
> >need a little wrapper to do the right thing for each architecture. Now 
> >Dan, if you were more conveniently located, perhaps we could work 
> >together and fix this. ;)
> >
> 
> I imagine we just need something that looks like this in trace.c:
> 
> #ifdef x86
>       rdtscll(rec->cycles);
> #endif
> #ifdef IA64
>       __asm__ __volatile ("mov %0=ar.itc;;" : "=r"(rec->cycles) :: 
> "memory");
> #endif

Can we please stick with the Linux method and put such arch dependant
ifdefs in headers and not in .c files? e.g. define a cycle_counter()
or somesuch inline function that expands to the right thing for each
arch and put it in asm-arch/... Otherwise, it gets messy very quickly
with the addition of new architectures.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: Recent trace patch not arch-neutral

[Rob Gardner]

[-- Attachment #1.1: Type: text/plain, Size: 1304 bytes --]

Muli Ben-Yehuda wrote:

>On Mon, Oct 31, 2005 at 04:26:19PM -0700, Rob Gardner wrote:
>  
>
>>Rob Gardner wrote:
>>
>>    
>>
>>>Now, to answer Dan's question- the rdtscll thing is just a time stamp 
>>>counter, expressed in cycles. So on ia64 you could probably replace it 
>>>with an asm statement to read ar.itc to make everything work. We just 
>>>need a little wrapper to do the right thing for each architecture. Now 
>>>Dan, if you were more conveniently located, perhaps we could work 
>>>together and fix this. ;)
>>>
>>>      
>>>
>>I imagine we just need something that looks like this in trace.c:
>>
>>#ifdef x86
>>      rdtscll(rec->cycles);
>>#endif
>>#ifdef IA64
>>      __asm__ __volatile ("mov %0=ar.itc;;" : "=r"(rec->cycles) :: 
>>"memory");
>>#endif
>>    
>>
>
>Can we please stick with the Linux method and put such arch dependant
>ifdefs in headers and not in .c files? e.g. define a cycle_counter()
>or somesuch inline function that expands to the right thing for each
>arch and put it in asm-arch/... Otherwise, it gets messy very quickly
>with the addition of new architectures.
>
>  
>

Of course it'll be done the right way... That's why I sent out my code 
"suggestion" along with this comment:

> Dan, perhaps you know the nice clean way of doing this sort of thing?


Rob


[-- Attachment #1.2: Type: text/html, Size: 1815 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: Recent trace patch not arch-neutral

[Muli Ben-Yehuda]

On Mon, Oct 31, 2005 at 11:38:15PM -0700, Rob Gardner wrote:

> Of course it'll be done the right way... That's why I sent out my code 
> "suggestion" along with this comment:
> 
> >Dan, perhaps you know the nice clean way of doing this sort of
> >thing?

Sorry, -EPARSE -- I thought you meant a clean way of reading the cycle
counter on IA64.

Cheers,
Muli
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

Re: Recent trace patch not arch-neutral

[Keir Fraser]

>  #ifdef x86
>         rdtscll(rec->cycles);
>  #endif
>  #ifdef IA64
>         __asm__ __volatile ("mov %0=ar.itc;;" : "=r"(rec->cycles) :: 
> "memory");
>  #endif
>
>  Dan, perhaps you know the nice clean way of doing this sort of thing?

Okay, let's define the Linux function get_cycles() in 
include/asm/time.h. So common code can then get at that generic 
function by including <xen/time.h>. I don't much care for the cycles_t 
definition but I guess we may as well pull that in as well. Then 
trace.c can cast to u64 when writing the stamp into the trace record.

  -- Keir

RE: Recent trace patch not arch-neutral

[Magenheimer, Dan (HP Labs Fort Collins)]

How about just adding "#include <asm/timex.h>" and
stealing linux/include/asm-{i386,x86_64}/timex.h?
Xen/ia64 is already including asm-ia64/timex.h
in other code.

> -----Original Message-----
> From: Keir Fraser [mailto:Keir.Fraser@cl.cam.ac.uk] 
> Sent: Tuesday, November 01, 2005 2:28 AM
> To: Gardner, Robert D
> Cc: xen-devel@lists.xensource.com; Magenheimer, Dan (HP Labs 
> Fort Collins); Ian Pratt
> Subject: Re: [Xen-devel] Recent trace patch not arch-neutral
> 
> 
> >  #ifdef x86
> >         rdtscll(rec->cycles);
> >  #endif
> >  #ifdef IA64
> >         __asm__ __volatile ("mov %0=ar.itc;;" : 
> "=r"(rec->cycles) :: 
> > "memory");
> >  #endif
> >
> >  Dan, perhaps you know the nice clean way of doing this 
> sort of thing?
> 
> Okay, let's define the Linux function get_cycles() in 
> include/asm/time.h. So common code can then get at that generic 
> function by including <xen/time.h>. I don't much care for the 
> cycles_t 
> definition but I guess we may as well pull that in as well. Then 
> trace.c can cast to u64 when writing the stamp into the trace record.
> 
>   -- Keir
> 
> 

Re: Recent trace patch not arch-neutral

[Keir Fraser]

On 1 Nov 2005, at 13:25, Magenheimer, Dan (HP Labs Fort Collins) wrote:

> How about just adding "#include <asm/timex.h>" and
> stealing linux/include/asm-{i386,x86_64}/timex.h?
> Xen/ia64 is already including asm-ia64/timex.h
> in other code.

Including asm/timex.h from asm/time.h sounds like the best fix for 
xen/ia64 then.

The declaration for xen/x86 is now in our staging tree. Now grinding 
through regression tests on its way to the public tree.

  -- Keir

RE: Recent trace patch not arch-neutral

[Magenheimer, Dan (HP Labs Fort Collins)]

> > How about just adding "#include <asm/timex.h>" and
> > stealing linux/include/asm-{i386,x86_64}/timex.h?
> > Xen/ia64 is already including asm-ia64/timex.h
> > in other code.
> 
> Including asm/timex.h from asm/time.h sounds like the best fix for 
> xen/ia64 then.

Yep, looks like that works fine.  Will include
this in the next ia64 pull.

Thanks,
Dan