[ovs-dev] [PATCH] netdev-dpdk / arbitrary port naming for vhost ports
Ben Pfaff
blp at nicira.com
Tue Jun 24 16:04:21 UTC 2014
On Fri, Jun 20, 2014 at 04:23:48PM +0100, maryam.tahhan wrote:
> From: "maryam.tahhan" <maryam.tahhan at intel.com>
>
> This patch enables arbitrary port naming for vhost patches, so
> they no longer need to be called dpdkvhost<n>.
>
> Signed-off-by: maryam.tahhan <maryam.tahhan at intel.com>
This is not a substantive review, but I do have a few comments.
> - **Note: tested with Qemu versions 1.4.2 and 1.6.2**
> + **Note 1: tested with Qemu versions 1.4.2 and 1.6.2**
Instead of adding the following note, perhaps your example could just
use a name other than dpdkvhost<n>:
> + Note 2: ports don't have to be called dpdkvhost<n>.
We put a space in "if(" and before the "{" (see CodingStyle):
> + if(!list_is_empty(&dpdk_list)){
> + LIST_FOR_EACH (tmp_netdev, list_node, &dpdk_list) {
Please put a space after "if" and on both sides of "==" not just one:
> + if(tmp_netdev->type== VHOST)
> + port_no++;
> + }
> + }
> +
> /* TODO: need to discover device node at run time. */
> netdev->socket_id = SOCKET0;
> netdev->port_id = port_no;
> @@ -505,7 +506,7 @@ netdev_dpdk_vhost_construct(struct netdev *netdev_)
> }
>
> memset(ð_addr, 0x0, sizeof(eth_addr));
OK, this code was just wrong. Nested "if"s without indentation? Also
we always write {} around an if's statements. I realize that's existing
code but please fix it up as you update here.
> - if(netdev->port_id)
> +
> if(netdev->port_id < 0xff)
> eth_addr.addr_bytes[5] = netdev->port_id;
> else
> @@ -519,12 +520,13 @@ netdev_dpdk_vhost_construct(struct netdev *netdev_)
> netdev->buf_size =
> mbp_priv->mbuf_data_room_size - RTE_PKTMBUF_HEADROOM;
>
Please don't put an explicit \n at the end of the log line. Also, why
the doubled space here and in the other log message?
> + VLOG_INFO("%s is associated with VHOST port #%d\n", netdev->name,
> + netdev->port_id);
>
> list_push_back(&dpdk_list, &netdev->list_node);
>
> unlock_dev:
> ovs_mutex_unlock(&netdev->mutex);
> -unlock_dpdk_vhost:
> ovs_mutex_unlock(&dpdk_mutex);
>
> return err;
> @@ -1748,8 +1750,8 @@ new_device (struct virtio_net *dev)
> return -1;
> }
>
> - VLOG_INFO("(%ld) VHOST Device has been added to dpdkvhost%d \n",
> - dev->device_fh, netdev->port_id);
> + VLOG_INFO("(%ld) VHOST Device has been added to vhost port %s \n",
> + dev->device_fh, netdev->name);
Thanks,
Ben.
More information about the dev
mailing list