[ovs-dev] [ovs-dev, v5, 1/2] netdev-dpdk: Helper function for vHost device setup
Ilya Maximets
i.maximets at samsung.com
Fri Dec 15 13:13:51 UTC 2017
One general concern:
Why we're adding new (experimental) feature to deprecated port type?
If we'll not do that, this patch will not be needed.
IMHO, it's better to keep vhostuser ports intact to avoid possible
regressive changes. Also, this patch increases code size.
Some comments about patch itself inline.
Best regards, Ilya Maximets.
On 08.12.2017 18:26, Ciara Loftus wrote:
> dpdkvhostuser and dpdkvhostuserclient ports share a lot of the same
> setup & configuration code. Create a common function they can share in
> order to remove some duplication of code.
>
> Signed-off-by: Ciara Loftus <ciara.loftus at intel.com>
> ---
> lib/netdev-dpdk.c | 122 +++++++++++++++++++++++++-----------------------------
> 1 file changed, 56 insertions(+), 66 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 9715c39..879019e 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -939,35 +939,31 @@ vhost_common_construct(struct netdev *netdev)
> }
>
> static int
> -netdev_dpdk_vhost_construct(struct netdev *netdev)
> +dpdk_setup_vhost_device(struct netdev_dpdk *dev, bool client_mode)
> {
> - struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> - const char *name = netdev->name;
> - int err;
> + const char *name = dev->up.name;
> + uint64_t flags = dev->vhost_driver_flags;
> + int err = 0;
>
> - /* 'name' is appended to 'vhost_sock_dir' and used to create a socket in
> - * the file system. '/' or '\' would traverse directories, so they're not
> - * acceptable in 'name'. */
> - if (strchr(name, '/') || strchr(name, '\\')) {
> - VLOG_ERR("\"%s\" is not a valid name for a vhost-user port. "
> - "A valid name must not include '/' or '\\'",
> - name);
> - return EINVAL;
> + if (client_mode) {
> + flags |= RTE_VHOST_USER_CLIENT;
> }
>
> - ovs_mutex_lock(&dpdk_mutex);
> - /* Take the name of the vhost-user port and append it to the location where
> - * the socket is to be created, then register the socket.
> - */
> - snprintf(dev->vhost_id, sizeof dev->vhost_id, "%s/%s",
> - dpdk_get_vhost_sock_dir(), name);
> + if (dpdk_vhost_iommu_enabled()) {
> + flags |= RTE_VHOST_USER_IOMMU_SUPPORT;
IOMMU support silently enabled for vhostuser ports. This must be fixed or documented.
> + }
>
> - dev->vhost_driver_flags &= ~RTE_VHOST_USER_CLIENT;
> - err = rte_vhost_driver_register(dev->vhost_id, dev->vhost_driver_flags);
> + err = rte_vhost_driver_register(dev->vhost_id, flags);
> if (err) {
> VLOG_ERR("vhost-user socket device setup failure for socket %s\n",
> dev->vhost_id);
> - goto out;
> + return err;
> + } else if (client_mode) {
> + /* Configuration successful */
> + dev->vhost_driver_flags |= RTE_VHOST_USER_CLIENT;
RTE_VHOST_USER_IOMMU_SUPPORT lost here.
> + VLOG_INFO("vHost User device '%s' created in 'client' mode, "
> + "using client socket '%s'",
> + name, dev->vhost_id);
> } else {
> fatal_signal_add_file_to_unlink(dev->vhost_id);
> VLOG_INFO("Socket %s created for vhost-user port %s\n",
> @@ -975,11 +971,11 @@ netdev_dpdk_vhost_construct(struct netdev *netdev)
> }
>
> err = rte_vhost_driver_callback_register(dev->vhost_id,
> - &virtio_net_device_ops);
> + &virtio_net_device_ops);
> if (err) {
> VLOG_ERR("rte_vhost_driver_callback_register failed for vhost user "
> "port: %s\n", name);
> - goto out;
> + return err;
> }
>
> err = rte_vhost_driver_disable_features(dev->vhost_id,
> @@ -989,13 +985,46 @@ netdev_dpdk_vhost_construct(struct netdev *netdev)
> if (err) {
> VLOG_ERR("rte_vhost_driver_disable_features failed for vhost user "
> "port: %s\n", name);
> - goto out;
> + return err;
> }
>
> err = rte_vhost_driver_start(dev->vhost_id);
> if (err) {
> VLOG_ERR("rte_vhost_driver_start failed for vhost user "
> - "port: %s\n", name);
> + "port: %s\n", name);
Some whitespace issue.
> + return err;
> + }
> +
> + return err;
> +}
> +
> +static int
> +netdev_dpdk_vhost_construct(struct netdev *netdev)
> +{
> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> + const char *name = netdev->name;
> + int err;
> +
> + /* 'name' is appended to 'vhost_sock_dir' and used to create a socket in
> + * the file system. '/' or '\' would traverse directories, so they're not
> + * acceptable in 'name'. */
> + if (strchr(name, '/') || strchr(name, '\\')) {
> + VLOG_ERR("\"%s\" is not a valid name for a vhost-user port. "
> + "A valid name must not include '/' or '\\'",
> + name);
> + return EINVAL;
> + }
> +
> + ovs_mutex_lock(&dpdk_mutex);
> + /* Take the name of the vhost-user port and append it to the location where
> + * the socket is to be created, then register the socket.
> + */
> + snprintf(dev->vhost_id, sizeof dev->vhost_id, "%s/%s",
> + dpdk_get_vhost_sock_dir(), name);
> +
> + dev->vhost_driver_flags &= ~RTE_VHOST_USER_CLIENT;
> + err = dpdk_setup_vhost_device(dev, false);
> + if (err) {
> goto out;
> }
>
> @@ -3253,7 +3282,6 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev)
> {
> struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
> int err;
> - uint64_t vhost_flags = 0;
>
> ovs_mutex_lock(&dev->mutex);
>
> @@ -3264,48 +3292,10 @@ netdev_dpdk_vhost_client_reconfigure(struct netdev *netdev)
> */
> if (!(dev->vhost_driver_flags & RTE_VHOST_USER_CLIENT)
> && strlen(dev->vhost_id)) {
> - /* Register client-mode device. */
> - vhost_flags |= RTE_VHOST_USER_CLIENT;
> -
> - /* Enable IOMMU support, if explicitly requested. */
> - if (dpdk_vhost_iommu_enabled()) {
> - vhost_flags |= RTE_VHOST_USER_IOMMU_SUPPORT;
> - }
> - err = rte_vhost_driver_register(dev->vhost_id, vhost_flags);
> - if (err) {
> - VLOG_ERR("vhost-user device setup failure for device %s\n",
> - dev->vhost_id);
> - goto unlock;
> - } else {
> - /* Configuration successful */
> - dev->vhost_driver_flags |= vhost_flags;
> - VLOG_INFO("vHost User device '%s' created in 'client' mode, "
> - "using client socket '%s'",
> - dev->up.name, dev->vhost_id);
> - }
> -
> - err = rte_vhost_driver_callback_register(dev->vhost_id,
> - &virtio_net_device_ops);
> - if (err) {
> - VLOG_ERR("rte_vhost_driver_callback_register failed for "
> - "vhost user client port: %s\n", dev->up.name);
> - goto unlock;
> - }
> -
> - err = rte_vhost_driver_disable_features(dev->vhost_id,
> - 1ULL << VIRTIO_NET_F_HOST_TSO4
> - | 1ULL << VIRTIO_NET_F_HOST_TSO6
> - | 1ULL << VIRTIO_NET_F_CSUM);
> - if (err) {
> - VLOG_ERR("rte_vhost_driver_disable_features failed for vhost user "
> - "client port: %s\n", dev->up.name);
> - goto unlock;
> - }
>
> - err = rte_vhost_driver_start(dev->vhost_id);
> + /* Register client-mode device */
> + err = dpdk_setup_vhost_device(dev, true);
> if (err) {
> - VLOG_ERR("rte_vhost_driver_start failed for vhost user "
> - "client port: %s\n", dev->up.name);
> goto unlock;
> }
> }
>
More information about the dev
mailing list