Re: [RFC][PATCH V2 1/3] examples/vhost: Add vswitch (generic switch) framework

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Tue, Sep 27, 2016 at 05:14:44PM +0530, Pankaj Chauhan wrote:
> On 9/26/2016 9:42 AM, Yuanhan Liu wrote:
> >Besides the VMDq proposal, I got few more comments for you.
> >
> >On Mon, Sep 05, 2016 at 04:24:29PM +0530, Pankaj Chauhan wrote:
> >>Introduce support for a generic framework for handling of switching between
> >>physical and vhost devices. The vswitch framework introduces the following
> >>concept:
> >>
> >>1. vswitch_dev: Vswitch device
> >
> >It looks a bit confusing to me, to claim it as a "device": it's neither a
> >physical nic device nor a virtio net device. Something like "vswitch_unit",
> >or even "vswitch" is better and enough.
> >
> 
> Yes we can change it to 'vswitch' it suites better, i'll do that in v3.
> 
> >>Signed-off-by: Pankaj Chauhan <pankaj.chauhan@nxp.com>
> >>---
> >> examples/vhost/Makefile         |   2 +-
> >> examples/vhost/main.c           | 128 +++++++++--
> >> examples/vhost/vswitch_common.c | 499 ++++++++++++++++++++++++++++++++++++++++
> >> examples/vhost/vswitch_common.h | 186 +++++++++++++++
> >> examples/vhost/vswitch_txrx.c   |  97 ++++++++
> >> examples/vhost/vswitch_txrx.h   |  71 ++++++
> >
> >Seems that you forgot to include the file to implment all those ops for
> >"switch" vswitch mode? I mean, I just see a vs_lookup_n_fwd implmentation
> >of VMDq.
> >
> 
> No i didn't forget to include the file but wanted to implement, get reviewed
> and included (hopefully :)) the implementation of following first:
> 
> 1. vswitch framework
> 2. vmdq implementation plugged into the vswitch framework.
> 
> After above two i am planning to send the 'software switch' implementation
> in a separate patch, i hope that is fine.

It's better to shipt them together, so that we could do review once.
Besides, it helps to understand your framework design.

> 
> >>@@ -1241,7 +1296,7 @@ static int
> >> new_device(int vid)
> >> {
> >> 	int lcore, core_add = 0;
> >>-	uint32_t device_num_min = num_devices;
> >>+	uint32_t device_num_min;
> >> 	struct vhost_dev *vdev;
> >>
> >> 	vdev = rte_zmalloc("vhost device", sizeof(*vdev), RTE_CACHE_LINE_SIZE);
> >>@@ -1252,6 +1307,16 @@ new_device(int vid)
> >> 		return -1;
> >> 	}
> >> 	vdev->vid = vid;
> >>+	device_num_min = vs_get_max_vdevs(vswitch_dev_g);
> >>+	RTE_LOG(INFO, VHOST_PORT, "max virtio devices %d\n", device_num_min);
> >>+
> >>+	vs_port = vs_add_port(vswitch_dev_g, vid, VSWITCH_PTYPE_VIRTIO, vdev);
> >
> >Note that "vid" does not equal "port". They are two different counters
> >and both start from 0. That means, you will get unexpected results from
> >following piece of code ---->
> >
> Sorry i didn't get the inconsistency completely, please help me understand
> it.
> 
> I agree both port_id and vid counters start from zero. But when we add these
> as vswitch_port we'll pass different port type (VSWITCH_PTYPE_VIRTIO or
> VSWITCH_PTYPE_PHYS). And while searching for any vswitch port we use
> vs_port->port_id && vs_port->type as the key, thus we'll not get confused
> between ports even when both have same port_id.
> 
> Can you please help me understand the inconsistency that you thought we may
> have?
...
> >>+struct vswitch_port *vs_add_port(struct vswitch_dev *vs_dev, int port_id,
...
> >>+	rte_eth_macaddr_get(vs_port->port_id, &vs_port->mac_addr);

Will a virtio port invoke above function in your logic?

	--yliu
> >
> ><--- here.
> >
> >	--yliu
> >
> >>+	RTE_LOG(INFO, VHOST_PORT, "Port %u MAC: %02"PRIx8" %02"PRIx8" %02"PRIx8
> >>+			" %02"PRIx8" %02"PRIx8" %02"PRIx8"\n",
> >>+			(unsigned)port_id,
> >>+			vs_port->mac_addr.addr_bytes[0],
> >>+			vs_port->mac_addr.addr_bytes[1],
> >>+			vs_port->mac_addr.addr_bytes[2],
> >>+			vs_port->mac_addr.addr_bytes[3],
> >>+			vs_port->mac_addr.addr_bytes[4],
> >>+			vs_port->mac_addr.addr_bytes[5]);
> >>+
> >>+	RTE_LOG(DEBUG, VHOST_CONFIG, "Added port [%d, type %d] to \
> >>+			vswitch %s\n", vs_port->port_id, type, vs_dev->name);
> >>+out:
> >>+	if (rc){
> >>+		RTE_LOG(INFO, VHOST_CONFIG, "Failed to Add port [%d, type %d] to \
> >>+			vswitch %s\n", port_id, type, vs_dev->name);
> >>+		if (vs_port)
> >>+			vs_free_port(vs_port);
> >>+		vs_port = NULL;
> >>+	}
> >>+
> >>+	return vs_port;
> >>+}
> >
>