Re: [Qemu-devel] [PATCH v3 6/7] virtio-net: cleanup: use QOM cast.

[Paolo Bonzini <pbonzini@redhat.com>]

Il 18/04/2013 14:50, Anthony Liguori ha scritto:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
>> On Thu, Apr 11, 2013 at 04:30:01PM +0200, fred.konrad@greensocs.com wrote:
>>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>>
>>> As the virtio-net-pci and virtio-net-s390 are switched to the new API,
>>> we can use QOM casts.
>>>
>>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>>> ---
>>>  hw/net/virtio-net.c            | 141 +++++++++++++++++++++--------------------
>>>  include/hw/virtio/virtio-net.h |   2 +-
>>>  2 files changed, 75 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 988fe03..09890c1 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -65,17 +65,9 @@ static int vq2q(int queue_index)
>>>   * - we could suppress RX interrupt if we were so inclined.
>>>   */
>>>  
>>> -/*
>>> - * Moving to QOM later in this serie.
>>> - */
>>> -static VirtIONet *to_virtio_net(VirtIODevice *vdev)
>>> -{
>>> -    return (VirtIONet *)vdev;
>>> -}
>>> -
>>>  static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>  {
>>> -    VirtIONet *n = to_virtio_net(vdev);
>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>      struct virtio_net_config netcfg;
>>>  
>>>      stw_p(&netcfg.status, n->status);
>>> @@ -86,12 +78,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>  
>>>  static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>>>  {
>>> -    VirtIONet *n = to_virtio_net(vdev);
>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>      struct virtio_net_config netcfg = {};
>>>  
>>>      memcpy(&netcfg, config, n->config_size);
>>>  
>>> -    if (!(n->vdev.guest_features >> VIRTIO_NET_F_CTRL_MAC_ADDR & 1) &&
>>> +    if (!(vdev->guest_features >> VIRTIO_NET_F_CTRL_MAC_ADDR & 1) &&
>>>          memcmp(netcfg.mac, n->mac, ETH_ALEN)) {
>>>          memcpy(n->mac, netcfg.mac, ETH_ALEN);
>>>          qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
>>> @@ -100,12 +92,14 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>>>  
>>>  static bool virtio_net_started(VirtIONet *n, uint8_t status)
>>>  {
>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>      return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>> -        (n->status & VIRTIO_NET_S_LINK_UP) && n->vdev.vm_running;
>>> +        (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
>>>  }
>>>  
>>>  static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>  {
>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>      NetClientState *nc = qemu_get_queue(n->nic);
>>>      int queues = n->multiqueue ? n->max_queues : 1;
>>>  
>>> @@ -126,25 +120,25 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>      }
>>>      if (!n->vhost_started) {
>>>          int r;
>>> -        if (!vhost_net_query(tap_get_vhost_net(nc->peer), &n->vdev)) {
>>> +        if (!vhost_net_query(tap_get_vhost_net(nc->peer), vdev)) {
>>>              return;
>>>          }
>>>          n->vhost_started = 1;
>>> -        r = vhost_net_start(&n->vdev, n->nic->ncs, queues);
>>> +        r = vhost_net_start(vdev, n->nic->ncs, queues);
>>>          if (r < 0) {
>>>              error_report("unable to start vhost net: %d: "
>>>                           "falling back on userspace virtio", -r);
>>>              n->vhost_started = 0;
>>>          }
>>>      } else {
>>> -        vhost_net_stop(&n->vdev, n->nic->ncs, queues);
>>> +        vhost_net_stop(vdev, n->nic->ncs, queues);
>>>          n->vhost_started = 0;
>>>      }
>>>  }
>>>  
>>>  static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>>>  {
>>> -    VirtIONet *n = to_virtio_net(vdev);
>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>      VirtIONetQueue *q;
>>>      int i;
>>>      uint8_t queue_status;
>>> @@ -184,6 +178,7 @@ static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t status)
>>>  static void virtio_net_set_link_status(NetClientState *nc)
>>>  {
>>>      VirtIONet *n = qemu_get_nic_opaque(nc);
>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>      uint16_t old_status = n->status;
>>>  
>>>      if (nc->link_down)
>>> @@ -192,14 +187,14 @@ static void virtio_net_set_link_status(NetClientState *nc)
>>>          n->status |= VIRTIO_NET_S_LINK_UP;
>>>  
>>>      if (n->status != old_status)
>>> -        virtio_notify_config(&n->vdev);
>>> +        virtio_notify_config(vdev);
>>>  
>>> -    virtio_net_set_status(&n->vdev, n->vdev.status);
>>> +    virtio_net_set_status(vdev, vdev->status);
>>>  }
>>>  
>>>  static void virtio_net_reset(VirtIODevice *vdev)
>>>  {
>>> -    VirtIONet *n = to_virtio_net(vdev);
>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>  
>>>      /* Reset back to compatibility mode */
>>>      n->promisc = 1;
>>> @@ -318,7 +313,7 @@ static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue, int ctrl);
>>>  
>>>  static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
>>>  {
>>> -    VirtIONet *n = to_virtio_net(vdev);
>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>      NetClientState *nc = qemu_get_queue(n->nic);
>>>  
>>>      features |= (1 << VIRTIO_NET_F_MAC);
>>> @@ -366,7 +361,7 @@ static uint32_t virtio_net_bad_features(VirtIODevice *vdev)
>>>  
>>>  static void virtio_net_set_features(VirtIODevice *vdev, uint32_t features)
>>>  {
>>> -    VirtIONet *n = to_virtio_net(vdev);
>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>      int i;
>>>  
>>>      virtio_net_set_multiqueue(n, !!(features & (1 << VIRTIO_NET_F_MQ)),
>>> @@ -534,6 +529,7 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>>>  static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
>>>                                  struct iovec *iov, unsigned int iov_cnt)
>>>  {
>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>      struct virtio_net_ctrl_mq mq;
>>>      size_t s;
>>>      uint16_t queues;
>>> @@ -559,14 +555,14 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
>>>      n->curr_queues = queues;
>>>      /* stop the backend before changing the number of queues to avoid handling a
>>>       * disabled queue */
>>> -    virtio_net_set_status(&n->vdev, n->vdev.status);
>>> +    virtio_net_set_status(vdev, vdev->status);
>>>      virtio_net_set_queues(n);
>>>  
>>>      return VIRTIO_NET_OK;
>>>  }
>>>  static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>>>  {
>>> -    VirtIONet *n = to_virtio_net(vdev);
>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>      struct virtio_net_ctrl_hdr ctrl;
>>>      virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>>>      VirtQueueElement elem;
>>> @@ -609,7 +605,7 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>>>  
>>>  static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq)
>>>  {
>>> -    VirtIONet *n = to_virtio_net(vdev);
>>> +    VirtIONet *n = VIRTIO_NET(vdev);
>>>      int queue_index = vq2q(virtio_get_queue_index(vq));
>>>  
>>>      qemu_flush_queued_packets(qemu_get_subqueue(n->nic, queue_index));
>>> @@ -618,9 +614,10 @@ static void virtio_net_handle_rx(VirtIODevice *vdev, VirtQueue *vq)
>>>  static int virtio_net_can_receive(NetClientState *nc)
>>>  {
>>>      VirtIONet *n = qemu_get_nic_opaque(nc);
>>> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
>>>      VirtIONetQueue *q = virtio_net_get_subqueue(nc);
>>>  
>>> -    if (!n->vdev.vm_running) {
>>> +    if (!vdev->vm_running) {
>>>          return 0;
>>>      }
>>>  
>>
>> BTW this is data path so was supposed to use the faster non-QOM casts.
> 
> No, we're not.  I don't know where you got that idea from.
> 
> Unless you have actual performance numbers to show that it matters, then
> you're just speculating.

It is slow in two ways.

First, type_get_by_name is a useless hashtable lookup.  This one is
pretty hard to deny, we could memoize it like this:

diff --git a/include/qom/object.h b/include/qom/object.h
index d0f99c5..4c19978 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -475,8 +475,11 @@ struct TypeInfo
  * If an invalid object is passed to this function, a run time assert will be
  * generated.
  */
+#define TYPE_GET_BY_NAME(name) \
+    ({ static Type _type; \
+       if (!__builtin_constant_p(name)) { \
+           extern void do_not_use_TYPE_GET_BY_NAME(void); \
+           do_not_use_TYPE_GET_B_NAME(); \
+       } \
+       _type ?: (_type = type_get_by_name(name)); })
+
 #define OBJECT_CHECK(type, obj, name) \
-    ((type *)object_dynamic_cast_assert(OBJECT(obj), (name)))
+    ((type *)object_dynamic_cast_with_type_assert(OBJECT(obj),  \
+       __builtin_constant_p((name)) ? TYPE_GET_BY_NAME(name)    \
+                                    : type_get_by_name(name)))
 
 /**
  * OBJECT_CLASS_CHECK:

(incomplete of course).  (What glib does is instead a function call).

Second, it doesn't apply in this case, but this:

    if (type->class->interfaces &&
            type_is_ancestor(target_type, type_interface)) {

is pretty awful.  We really should cache the type_is_ancestor call
and make it something like

    if (target_type->type_is_interface && type->class->interfaces)

Paolo