Hi Ben
Thanks for the feedback, I will refactor and resubmit, just a question
regarding the section that sets the MAC address for the logical vhost port, is
the issue just the formatting you mention after the comment about it being
wrong or was there an issue with setting the MAC to something like
00:00:00:00:00:xx or 00:00:00:00:xx:00 where xx is the port number we assign to
that port.
Or is it ok to leave those ports without a MAC address at all? I noticed if I
don't set them they default to 0 for all the vhost ports I add. The interfaces
the guest uses that connect to these ports will have their own MAC addresses.
I'm thinking about updating to something like this:
/*
* Set the mac address to 00:00:00:00:00:xx or 00:00:00:00:xx:00 where xx
* is the port number.
*/
memset(ð_addr, 0x0, sizeof(eth_addr));
if (netdev->port_id < 0xff) {
eth_addr.addr_bytes[5] = netdev->port_id;
} else {
eth_addr.addr_bytes[4] = netdev->port_id;
}
ether_addr_copy(netdev->hwaddr, eth_addr.addr_bytes);
VLOG_INFO_RL(&rl, "Port %d: "ETH_ADDR_FMT"",
netdev->port_id, ETH_ADDR_ARGS(eth_addr.addr_bytes));
or deleting altogether, what do you recommend?
Regards
Maryam
-----Original Message-----
From: Ben Pfaff [mailto:[email protected]]
Sent: Tuesday, June 24, 2014 5:04 PM
To: Tahhan, Maryam
Cc: [email protected]; Rogers, Gerald
Subject: Re: [ovs-dev] [PATCH] netdev-dpdk / arbitrary port naming for vhost
ports
On Fri, Jun 20, 2014 at 04:23:48PM +0100, maryam.tahhan wrote:
> From: "maryam.tahhan" <[email protected]>
>
> This patch enables arbitrary port naming for vhost patches, so they no
> longer need to be called dpdkvhost<n>.
>
> Signed-off-by: maryam.tahhan <[email protected]>
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.
--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare
This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
