Re: [Qemu-devel] [PATCH v2 1/5] qxl: switch qxl.c to trace-events

[Gerd Hoffmann]

On 03/11/12 20:26, Alon Levy wrote:
> dprint is still used for qxl_init_common one time prints.

I think we shouldn't simply convert the dprintf's into trace-points.

We should look at each dprintf and check whenever it makes sense at all,
whenever it makes sense at that place before converting it over to a
tracepoint.

> @@ -409,7 +410,7 @@ static void interface_attach_worker(QXLInstance *sin, 
> QXLWorker *qxl_worker)
>  {
>      PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
>  
> -    dprint(qxl, 1, "%s:\n", __FUNCTION__);
> +    trace_qxl_interface_attach_worker();
>      qxl->ssd.worker = qxl_worker;
>  }

For example: Do we really need that one?

> @@ -505,9 +506,10 @@ static int interface_get_command(QXLInstance *sin, 
> struct QXLCommandExt *ext)
>      QXLCommand *cmd;
>      int notify, ret;
>  
> +    trace_qxl_interface_get_command_enter(qxl_mode_to_string(qxl->mode));
> +

Why this?

> -    dprint(qxl, 1, "%s: scheduling update_area_bh, #dirty %d\n",
> -           __func__, qxl->num_dirty_rects);
> +    
> trace_qxl_interface_update_area_complete_schedule_bh(qxl->num_dirty_rects);

I think it makes more sense to have the tracepoint in the bottom half
handler instead.

>  static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
>  {
> -    dprint(d, 1, "%s: start%s\n", __FUNCTION__,
> -           loadvm ? " (loadvm)" : "");
> +    trace_qxl_hard_reset_enter(loadvm);
>  
>      qxl_spice_reset_cursor(d);
>      qxl_spice_reset_image_cache(d);
> @@ -934,7 +935,7 @@ static void qxl_hard_reset(PCIQXLDevice *d, int loadvm)
>      qemu_spice_create_host_memslot(&d->ssd);
>      qxl_soft_reset(d);
>  
> -    dprint(d, 1, "%s: done\n", __FUNCTION__);
> +    trace_qxl_hard_reset_exit();
>  }

Do we need the exit tracepoint?

>  static void qxl_reset_memslots(PCIQXLDevice *d)
>  {
> -    dprint(d, 1, "%s:\n", __FUNCTION__);
> +    trace_qxl_reset_memslots();
>      qxl_spice_reset_memslots(d);
>      memset(&d->guest_slots, 0, sizeof(d->guest_slots));
>  }

Do we need that one?  qxl_hard_reset is the only caller of that function ...

> @@ -1216,8 +1213,8 @@ static void ioport_write(void *opaque, 
> target_phys_addr_t addr,
>          if (d->mode != QXL_MODE_VGA) {
>              break;
>          }
> -        dprint(d, 1, "%s: unexpected port 0x%x (%s) in vga mode\n",
> -            __func__, io_port, io_port_to_string(io_port));
> +        trace_qxl_io_unexpected_vga_mode(
> +            io_port, io_port_to_string(io_port));

We might want raise an error irq here, and have a tracepoint in
qxl_guest_bug() of course ...

>      case QXL_IO_SET_MODE:
> -        dprint(d, 1, "QXL_SET_MODE %d\n", (int)val);
> +        trace_qxl_io_set_mode(val);
>          qxl_set_mode(d, val, 0);

Needed?  There is a tracepoint in qxl_set_mode() ...

>      case QXL_IO_RESET:
> -        dprint(d, 1, "QXL_IO_RESET\n");
> +        trace_qxl_io_reset();
>          qxl_hard_reset(d, 0);

... likewise ...

>          break;
>      case QXL_IO_MEMSLOT_ADD:
> @@ -1337,7 +1334,7 @@ async_common:
>                            async);
>              goto cancel_async;
>          }
> -        dprint(d, 1, "QXL_IO_CREATE_PRIMARY async=%d\n", async);
> +        trace_qxl_io_create_primary(async);
>          d->guest_primary.surface = d->ram->create_surface;
>          qxl_create_guest_primary(d, 0, async);

... here too ...

We might want to have a "trace_qxl_io_write(addr, val)" at the start of
the function, so we see all guest writes.  Traces for the actual ops (if
needed at all) are probably much better placed into the functions
executing the op as they can trace more details (i.e. qxl_set_mode has
the tracepoint *after* looking up the mode so we can stick the mode info
into the trace too).

cheers,
  Gerd