From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 1E4112C35 for ; Tue, 14 Mar 2017 11:55:27 +0100 (CET) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 41D7351443; Tue, 14 Mar 2017 10:55:27 +0000 (UTC) Received: from [10.36.116.175] (ovpn-116-175.ams2.redhat.com [10.36.116.175]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2EAtMl2013324 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 14 Mar 2017 06:55:24 -0400 To: Yuanhan Liu , dev@dpdk.org References: <1488534682-3494-1-git-send-email-yuanhan.liu@linux.intel.com> <1488534682-3494-5-git-send-email-yuanhan.liu@linux.intel.com> Cc: Harris James R , Liu Changpeng From: Maxime Coquelin Message-ID: <979cc67e-b017-085d-27a0-33658c26acca@redhat.com> Date: Tue, 14 Mar 2017 11:55:20 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <1488534682-3494-5-git-send-email-yuanhan.liu@linux.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Tue, 14 Mar 2017 10:55:27 +0000 (UTC) Subject: Re: [dpdk-dev] [PATCH 04/17] vhost: make notify ops per vhost driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Mar 2017 10:55:27 -0000 On 03/03/2017 10:51 AM, Yuanhan Liu wrote: > Assume there is an application both support vhost-user net and > vhost-user scsi, the callback should be different. Making notify > ops per vhost driver allow application define different set of > callbacks for different driver. > > Signed-off-by: Yuanhan Liu > --- > drivers/net/vhost/rte_eth_vhost.c | 20 +++++++++++--------- > examples/tep_termination/main.c | 3 ++- > examples/vhost/main.c | 5 +++-- > lib/librte_vhost/rte_virtio_net.h | 3 ++- > lib/librte_vhost/socket.c | 32 ++++++++++++++++++++++++++++++++ > lib/librte_vhost/vhost.c | 16 +--------------- > lib/librte_vhost/vhost.h | 5 ++++- > lib/librte_vhost/vhost_user.c | 15 +++++++++------ > 8 files changed, 64 insertions(+), 35 deletions(-) > > diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c > index 0ebaa4a..816a9a0 100644 > --- a/drivers/net/vhost/rte_eth_vhost.c > +++ b/drivers/net/vhost/rte_eth_vhost.c > @@ -667,6 +667,12 @@ struct vhost_xstats_name_off { > return 0; > } > > +static struct virtio_net_device_ops vhost_ops = { > + .new_device = new_device, > + .destroy_device = destroy_device, > + .vring_state_changed = vring_state_changed, > +}; > + > int > rte_eth_vhost_get_queue_event(uint8_t port_id, > struct rte_eth_vhost_queue_event *event) > @@ -736,15 +742,6 @@ struct vhost_xstats_name_off { > static void * > vhost_driver_session(void *param __rte_unused) > { > - static struct virtio_net_device_ops vhost_ops; > - > - /* set vhost arguments */ > - vhost_ops.new_device = new_device; > - vhost_ops.destroy_device = destroy_device; > - vhost_ops.vring_state_changed = vring_state_changed; > - if (rte_vhost_driver_callback_register(&vhost_ops) < 0) > - RTE_LOG(ERR, PMD, "Can't register callbacks\n"); > - > /* start event handling */ > rte_vhost_driver_session_start(); > > @@ -1077,6 +1074,11 @@ struct vhost_xstats_name_off { > if (rte_vhost_driver_register(iface_name, flags)) > goto error; > > + if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) { > + RTE_LOG(ERR, PMD, "Can't register callbacks\n"); > + goto error; > + } > + > /* We need only one message handling thread */ > if (rte_atomic16_add_return(&nb_started_ports, 1) == 1) { > if (vhost_driver_session_start()) > diff --git a/examples/tep_termination/main.c b/examples/tep_termination/main.c > index 8c45128..03c0fbe 100644 > --- a/examples/tep_termination/main.c > +++ b/examples/tep_termination/main.c > @@ -1258,7 +1258,8 @@ static inline void __attribute__((always_inline)) > rte_vhost_driver_disable_features(dev_basename, > 1ULL << VIRTIO_NET_F_MRG_RXBUF); > > - rte_vhost_driver_callback_register(&virtio_net_device_ops); > + rte_vhost_driver_callback_register(dev_basename, > + &virtio_net_device_ops); Return should be checked here, as this function can now return -1. > > rte_vhost_driver_session_start(); > > diff --git a/examples/vhost/main.c b/examples/vhost/main.c > index 972a6a8..867efc6 100644 > --- a/examples/vhost/main.c > +++ b/examples/vhost/main.c > @@ -1538,9 +1538,10 @@ static inline void __attribute__((always_inline)) > rte_vhost_driver_enable_features(file, > 1ULL << VIRTIO_NET_F_CTRL_RX); > } > - } > > - rte_vhost_driver_callback_register(&virtio_net_device_ops); > + rte_vhost_driver_callback_register(file, > + &virtio_net_device_ops); > + } Ditto. > > rte_vhost_driver_session_start(); > return 0; > diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h > index 51f5166..3bfd0b7 100644 > --- a/lib/librte_vhost/rte_virtio_net.h > +++ b/lib/librte_vhost/rte_virtio_net.h > @@ -91,7 +91,8 @@ struct virtio_net_device_ops { > int rte_vhost_driver_disable_features(const char *path, uint64_t features); > > /* Register callbacks. */ > -int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const); > +int rte_vhost_driver_callback_register(const char *path, > + struct virtio_net_device_ops const * const); > /* Start vhost driver session blocking loop. */ > int rte_vhost_driver_session_start(void); > > diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c > index 9e0ec05..550af64 100644 > --- a/lib/librte_vhost/socket.c > +++ b/lib/librte_vhost/socket.c > @@ -73,6 +73,8 @@ struct vhost_user_socket { > */ > uint64_t supported_features; > uint64_t features; > + > + struct virtio_net_device_ops const *notify_ops; > }; > > struct vhost_user_connection { > @@ -718,6 +720,36 @@ struct vhost_user_reconnect_list { > return -1; > } > > +/* > + * Register ops so that we can add/remove device to data core. > + */ > +int > +rte_vhost_driver_callback_register(const char *path, > + struct virtio_net_device_ops const * const ops) > +{ > + struct vhost_user_socket *vsocket; > + > + pthread_mutex_lock(&vhost_user.mutex); > + vsocket = find_vhost_user_socket(path); > + if (vsocket) > + vsocket->notify_ops = ops; > + pthread_mutex_unlock(&vhost_user.mutex); > + > + return vsocket ? 0 : -1; > +} > + > +struct virtio_net_device_ops const * > +vhost_driver_callback_get(const char *path) > +{ > + struct vhost_user_socket *vsocket; > + > + pthread_mutex_lock(&vhost_user.mutex); > + vsocket = find_vhost_user_socket(path); > + pthread_mutex_unlock(&vhost_user.mutex); > + > + return vsocket->notify_ops; There should be a check against vsocket to avoid NULL pointer dereferencing. > +} > + > int > rte_vhost_driver_session_start(void) > { > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c > index 2790f17..0088f87 100644 > --- a/lib/librte_vhost/vhost.c > +++ b/lib/librte_vhost/vhost.c > @@ -51,9 +51,6 @@ > > struct virtio_net *vhost_devices[MAX_VHOST_DEVICE]; > > -/* device ops to add/remove device to/from data core. */ > -struct virtio_net_device_ops const *notify_ops; > - > struct virtio_net * > get_device(int vid) > { > @@ -253,7 +250,7 @@ struct virtio_net * > > if (dev->flags & VIRTIO_DEV_RUNNING) { > dev->flags &= ~VIRTIO_DEV_RUNNING; > - notify_ops->destroy_device(vid); > + dev->notify_ops->destroy_device(vid); > } > > cleanup_device(dev, 1); > @@ -377,14 +374,3 @@ struct virtio_net * > dev->virtqueue[queue_id]->used->flags = VRING_USED_F_NO_NOTIFY; > return 0; > } > - > -/* > - * Register ops so that we can add/remove device to data core. > - */ > -int > -rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const ops) > -{ > - notify_ops = ops; > - > - return 0; > -} > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h > index 61e7448..bc03e09 100644 > --- a/lib/librte_vhost/vhost.h > +++ b/lib/librte_vhost/vhost.h > @@ -177,6 +177,8 @@ struct virtio_net { > uint64_t log_addr; > struct ether_addr mac; > > + struct virtio_net_device_ops const *notify_ops; > + > uint32_t nr_guest_pages; > uint32_t max_guest_pages; > struct guest_page *guest_pages; > @@ -280,7 +282,6 @@ static inline phys_addr_t __attribute__((always_inline)) > return 0; > } > > -struct virtio_net_device_ops const *notify_ops; > struct virtio_net *get_device(int vid); > > int vhost_new_device(void); > @@ -293,6 +294,8 @@ static inline phys_addr_t __attribute__((always_inline)) > void vhost_set_ifname(int, const char *if_name, unsigned int if_len); > void vhost_enable_dequeue_zero_copy(int vid); > > +struct virtio_net_device_ops const *vhost_driver_callback_get(const char *path); > + > /* > * Backend-specific cleanup. > * > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index f7227bf..c101fbc 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -131,7 +131,7 @@ > { > if (dev->flags & VIRTIO_DEV_RUNNING) { > dev->flags &= ~VIRTIO_DEV_RUNNING; > - notify_ops->destroy_device(dev->vid); > + dev->notify_ops->destroy_device(dev->vid); > } > > cleanup_device(dev, 0); > @@ -499,7 +499,7 @@ > /* Remove from the data plane. */ > if (dev->flags & VIRTIO_DEV_RUNNING) { > dev->flags &= ~VIRTIO_DEV_RUNNING; > - notify_ops->destroy_device(dev->vid); > + dev->notify_ops->destroy_device(dev->vid); > } > > if (dev->mem) { > @@ -680,7 +680,7 @@ > "dequeue zero copy is enabled\n"); > } > > - if (notify_ops->new_device(dev->vid) == 0) > + if (dev->notify_ops->new_device(dev->vid) == 0) > dev->flags |= VIRTIO_DEV_RUNNING; > } > } > @@ -713,7 +713,7 @@ > /* We have to stop the queue (virtio) if it is running. */ > if (dev->flags & VIRTIO_DEV_RUNNING) { > dev->flags &= ~VIRTIO_DEV_RUNNING; > - notify_ops->destroy_device(dev->vid); > + dev->notify_ops->destroy_device(dev->vid); > } > > /* Here we are safe to get the last used index */ > @@ -753,8 +753,8 @@ > "set queue enable: %d to qp idx: %d\n", > enable, state->index); > > - if (notify_ops->vring_state_changed) > - notify_ops->vring_state_changed(dev->vid, state->index, enable); > + if (dev->notify_ops->vring_state_changed) > + dev->notify_ops->vring_state_changed(dev->vid, state->index, enable); > > dev->virtqueue[state->index]->enabled = enable; > > @@ -952,6 +952,9 @@ > if (dev == NULL) > return -1; > > + if (!dev->notify_ops) > + dev->notify_ops = vhost_driver_callback_get(dev->ifname); Once vhost_driver_callback_get() fixed, notify_ops can be NULL, and it seems to be dereferenced without being checked later on. > + > ret = read_vhost_message(fd, &msg); > if (ret <= 0 || msg.request >= VHOST_USER_MAX) { > if (ret < 0) >