[Qemu-devel] [PATCH 0/3] virtio-serial: Bug fix, add stats for bytes transferred

[Qemu-devel] [PATCH 0/3] virtio-serial: Bug fix, add stats for bytes transferred

[Amit Shah]

Hello,

These patches fix one bug (patch 2), and add some stats for bytes
sent, received and discarded, mainly for debugging purposes..  These
stats are shown in the 'info qtree' output.  More details in the
commit logs.

Please apply,


Amit Shah (3):
  virtio-serial-bus: add port arg to discard_vq_data()
  virtio-serial-bus: discard data in already popped-out elem
  virtio-serial-bus: Add per-port stats for received, sent, discarded
    bytes

 hw/virtio-serial-bus.c |   37 +++++++++++++++++++++++++++++++------
 hw/virtio-serial.h     |   11 +++++++++++
 2 files changed, 42 insertions(+), 6 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 1/3] virtio-serial-bus: add port arg to discard_vq_data()

[Amit Shah]

To discard throttled data as well as maintain statistics of bytes
received and discarded, discard_vq_data() will need the port associated
with the vq.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index a4825b9..6838d73 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -114,7 +114,8 @@ static size_t write_to_port(VirtIOSerialPort *port,
     return offset;
 }
 
-static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
+static void discard_vq_data(VirtIOSerialPort *port, VirtQueue *vq,
+                            VirtIODevice *vdev)
 {
     VirtQueueElement elem;
 
@@ -248,7 +249,7 @@ int virtio_serial_close(VirtIOSerialPort *port)
      * consume, reset the throttling flag and discard the data.
      */
     port->throttled = false;
-    discard_vq_data(port->ovq, &port->vser->vdev);
+    discard_vq_data(port, port->ovq, &port->vser->vdev);
 
     send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
@@ -473,7 +474,7 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
     info = port ? DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info) : NULL;
 
     if (!port || !port->host_connected || !info->have_data) {
-        discard_vq_data(vq, vdev);
+        discard_vq_data(port, vq, vdev);
         return;
     }
 
@@ -730,7 +731,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
 
     port = find_port_by_id(vser, port_id);
     /* Flush out any unconsumed buffers first */
-    discard_vq_data(port->ovq, &port->vser->vdev);
+    discard_vq_data(port, port->ovq, &port->vser->vdev);
 
     send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
 }
-- 
1.7.6

Re: [Qemu-devel] [PATCH 1/3] virtio-serial-bus: add port arg to discard_vq_data()

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> To discard throttled data as well as maintain statistics of bytes
> received and discarded, discard_vq_data() will need the port associated
> with the vq.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-serial-bus.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index a4825b9..6838d73 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -114,7 +114,8 @@ static size_t write_to_port(VirtIOSerialPort *port,
>      return offset;
>  }
>  
> -static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
> +static void discard_vq_data(VirtIOSerialPort *port, VirtQueue *vq,
> +                            VirtIODevice *vdev)
>  {
>      VirtQueueElement elem;
>  

Two of three callers pass port, port->ovq, &port->vser->vdev.  Third one
isn't as obvious, but could be the same (didn't check thoroughly).
Makes me wonder whether parameters vq and vdev are really needed.

> @@ -248,7 +249,7 @@ int virtio_serial_close(VirtIOSerialPort *port)
>       * consume, reset the throttling flag and discard the data.
>       */
>      port->throttled = false;
> -    discard_vq_data(port->ovq, &port->vser->vdev);
> +    discard_vq_data(port, port->ovq, &port->vser->vdev);
>  
>      send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
>  
> @@ -473,7 +474,7 @@ static void handle_output(VirtIODevice *vdev, VirtQueue *vq)
>      info = port ? DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info) : NULL;
>  
>      if (!port || !port->host_connected || !info->have_data) {
> -        discard_vq_data(vq, vdev);
> +        discard_vq_data(port, vq, vdev);
>          return;
>      }
>  
> @@ -730,7 +731,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>  
>      port = find_port_by_id(vser, port_id);
>      /* Flush out any unconsumed buffers first */
> -    discard_vq_data(port->ovq, &port->vser->vdev);
> +    discard_vq_data(port, port->ovq, &port->vser->vdev);
>  
>      send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
>  }

Re: [Qemu-devel] [PATCH 1/3] virtio-serial-bus: add port arg to discard_vq_data()

[Amit Shah]

On (Wed) 14 Sep 2011 [14:52:50], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > To discard throttled data as well as maintain statistics of bytes
> > received and discarded, discard_vq_data() will need the port associated
> > with the vq.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> >  hw/virtio-serial-bus.c |    9 +++++----
> >  1 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > index a4825b9..6838d73 100644
> > --- a/hw/virtio-serial-bus.c
> > +++ b/hw/virtio-serial-bus.c
> > @@ -114,7 +114,8 @@ static size_t write_to_port(VirtIOSerialPort *port,
> >      return offset;
> >  }
> >  
> > -static void discard_vq_data(VirtQueue *vq, VirtIODevice *vdev)
> > +static void discard_vq_data(VirtIOSerialPort *port, VirtQueue *vq,
> > +                            VirtIODevice *vdev)
> >  {
> >      VirtQueueElement elem;
> >  
> 
> Two of three callers pass port, port->ovq, &port->vser->vdev.  Third one
> isn't as obvious, but could be the same (didn't check thoroughly).
> Makes me wonder whether parameters vq and vdev are really needed.

Because port can be NULL (when a port has been hot-unplugged but guest
continues to send data, or when a malicious guest sends data to a
non-existent port).


		Amit

[Qemu-devel] [PATCH 2/3] virtio-serial-bus: discard data in already popped-out elem

[Amit Shah]

While discarding data previously any popped-out elem in the vq but not
yet pushed into the guest because the backend was throttled wasn't
pushed back into the guest.  Fix that by checking if we had any
in-progress elem, and pushing it out to the guest first before emptying
the vq.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 6838d73..2c84398 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -122,6 +122,10 @@ static void discard_vq_data(VirtIOSerialPort *port, VirtQueue *vq,
     if (!virtio_queue_ready(vq)) {
         return;
     }
+    if (port && port->elem.out_num) {
+        virtqueue_push(vq, &port->elem, 0);
+        port->elem.out_num = 0;
+    }
     while (virtqueue_pop(vq, &elem)) {
         virtqueue_push(vq, &elem, 0);
     }
-- 
1.7.6

[Qemu-devel] [PATCH 3/3] virtio-serial-bus: Add per-port stats for received, sent, discarded bytes

[Amit Shah]

This commit adds port-specific stats for the number of bytes received,
sent and discarded.  They can be seen in the 'info qtree' monitor output
for the specific port.

This data can be used to check for data loss bugs (or disprove such
claims). It can also be used for accounting, if there's such a need.

The stats remain valid throughout the lifetime of the port. Unplugging a
port will reset the stats.  The numbers are not reset across port
opens/closes.

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c |   24 ++++++++++++++++++++++--
 hw/virtio-serial.h     |   11 +++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 2c84398..deefda4 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -108,6 +108,7 @@ static size_t write_to_port(VirtIOSerialPort *port,
         offset += len;
 
         virtqueue_push(vq, &elem, len);
+        port->stats.bytes_sent += len;
     }
 
     virtio_notify(&port->vser->vdev, vq);
@@ -123,10 +124,24 @@ static void discard_vq_data(VirtIOSerialPort *port, VirtQueue *vq,
         return;
     }
     if (port && port->elem.out_num) {
+        port->stats.bytes_discarded += (iov_size(port->elem.out_sg,
+                                                 elem.out_num)
+                                        - iov_size(port->elem.out_sg,
+                                                   port->iov_idx)
+                                        - port->iov_offset);
         virtqueue_push(vq, &port->elem, 0);
         port->elem.out_num = 0;
     }
     while (virtqueue_pop(vq, &elem)) {
+        if (port) {
+            unsigned long size;
+
+            size = iov_size(elem.out_sg, elem.out_num);
+
+            /* We haven't counted these bytes in the received stats yet. */
+            port->stats.bytes_received += size;
+            port->stats.bytes_discarded += size;
+        }
         virtqueue_push(vq, &elem, 0);
     }
     virtio_notify(vdev, vq);
@@ -152,6 +167,8 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
             }
             port->iov_idx = 0;
             port->iov_offset = 0;
+            port->stats.bytes_received += iov_size(port->elem.out_sg,
+                                                   port->elem.out_num);
         }
 
         for (i = port->iov_idx; i < port->elem.out_num; i++) {
@@ -684,11 +701,14 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 {
     VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, qdev);
 
-    monitor_printf(mon, "%*sport %d, guest %s, host %s, throttle %s\n",
+    monitor_printf(mon, "%*sport %d, guest %s, host %s, throttle %s, bytes_sent %lu, bytes_received %lu, bytes_discarded: %lu\n",
                    indent, "", port->id,
                    port->guest_connected ? "on" : "off",
                    port->host_connected ? "on" : "off",
-                   port->throttled ? "on" : "off");
+                   port->throttled ? "on" : "off",
+                   port->stats.bytes_sent,
+                   port->stats.bytes_received,
+                   port->stats.bytes_discarded);
 }
 
 /* This function is only used if a port id is not provided by the user */
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index ab13803..34d36d7 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -67,6 +67,10 @@ typedef struct VirtIOSerialBus VirtIOSerialBus;
 typedef struct VirtIOSerialPort VirtIOSerialPort;
 typedef struct VirtIOSerialPortInfo VirtIOSerialPortInfo;
 
+typedef struct {
+    unsigned long bytes_sent, bytes_received, bytes_discarded;
+} PortStats;
+
 /*
  * This is the state that's shared between all the ports.  Some of the
  * state is configurable via command-line options. Some of it can be
@@ -87,6 +91,13 @@ struct VirtIOSerialPort {
     VirtQueue *ivq, *ovq;
 
     /*
+     * Keep count of the bytes sent, received and discarded for
+     * this port for accounting and debugging purposes.  These
+     * counts are not reset across port open / close events.
+     */
+    PortStats stats;
+
+    /*
      * This name is sent to the guest and exported via sysfs.
      * The guest could create symlinks based on this information.
      * The name is in the reverse fqdn format, like org.qemu.console.0
-- 
1.7.6

Re: [Qemu-devel] [PATCH 3/3] virtio-serial-bus: Add per-port stats for received, sent, discarded bytes

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> This commit adds port-specific stats for the number of bytes received,
> sent and discarded.  They can be seen in the 'info qtree' monitor output
> for the specific port.
>
> This data can be used to check for data loss bugs (or disprove such
> claims). It can also be used for accounting, if there's such a need.
>
> The stats remain valid throughout the lifetime of the port. Unplugging a
> port will reset the stats.  The numbers are not reset across port
> opens/closes.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

I'm not sure "info qtree" is the right place to show device stats.  I
guess it's the only existing place that comes to mind.  Anthony, what do
you think?

Re: [Qemu-devel] [PATCH 3/3] virtio-serial-bus: Add per-port stats for received, sent, discarded bytes

[Amit Shah]

On (Wed) 14 Sep 2011 [15:07:57], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > This commit adds port-specific stats for the number of bytes received,
> > sent and discarded.  They can be seen in the 'info qtree' monitor output
> > for the specific port.
> >
> > This data can be used to check for data loss bugs (or disprove such
> > claims). It can also be used for accounting, if there's such a need.
> >
> > The stats remain valid throughout the lifetime of the port. Unplugging a
> > port will reset the stats.  The numbers are not reset across port
> > opens/closes.
> >
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> I'm not sure "info qtree" is the right place to show device stats.  I
> guess it's the only existing place that comes to mind.  Anthony, what do
> you think?

Ping?

		Amit