Re: [Xen-devel] [PATCH 2/3] trace: improve usefulness of hypercall trace record

[George Dunlap <dunlapg@xxxxxxxxx>]

On Mon, Oct 1, 2012 at 6:47 PM, David Vrabel <david.vrabel@xxxxxxxxxx> wrote:
> From: David Vrabel <david.vrabel@xxxxxxxxxx>
>
> Trace hypercalls using a more useful trace record format.
>
> The EIP field is removed (it was always somewhere in the hypercall
> page) and include selected hypercall arguments (e.g., the number of
> calls in a multicall, and the number of PTE updates in an mmu_update
> etc.).  12 bits in the first extra word are used to indicate which
> arguments are present in the record and what size they are (32 or
> 64-bit).
>
> This is an incompatible record format so a new event ID is used so
> tools can distinguish between the two formats.
>
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> Acked-by: George Dunlap <george.dunlap@xxxxxxxxxx>

Looking at it again, I do have one comment (see below) -- so I guess
that would be technically withdrawing the Ack until we sort it out.

> diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c
> index 27fe150..f2c75bc 100644
> --- a/xen/arch/x86/trace.c
> +++ b/xen/arch/x86/trace.c
> @@ -9,33 +9,28 @@
>  void trace_hypercall(void)

In most places, "trace_*" means "Call unconditionally; inside it will
check tb_init_done", while "__trace_*" means, "I have already checked
that tb_init_done is set, don't bother checking it."

It seems not to be the case here -- entry.S checks tb_init_done before
calling "trace_hypercall".  Could you either:
* Come up with a naming scheme consistent with the rest of the calls
(e.g., __trace_hypercall_x86 and __trace_hypercall), or
* Put a comment at the top of trace_hypercall() noting that it's an
exception to the general rule?

> +    switch ( op )
> +    {
> +    case __HYPERVISOR_mmu_update:
> +        APPEND_ARG32(1); /* count */
> +        break;
> +    case __HYPERVISOR_multicall:
> +        APPEND_ARG32(1); /* count */
> +        break;
> +    case __HYPERVISOR_grant_table_op:
> +        APPEND_ARG32(0); /* cmd */
> +        APPEND_ARG32(2); /* count */
> +        break;
> +    case __HYPERVISOR_vcpu_op:
> +        APPEND_ARG32(0); /* cmd */
> +        APPEND_ARG32(1); /* vcpuid */
> +        break;
> +    case __HYPERVISOR_mmuext_op:
> +        APPEND_ARG32(1); /* count */
> +        break;
> +    case __HYPERVISOR_sched_op:
> +        APPEND_ARG32(0); /* cmd */
> +        break;
> +    }

I may have commented on this before -- I wonder if doing some kind of
array might be better than a big switch statement.  I think with only
a few hypercalls, a switch statement is probably acceptable; but as we
add more, the code is going to get bloated.

So I guess, I'll accept it this time, but as you add more, Real Soon
I'll probably ask for a more efficient implementation. :-)

Thanks,

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel