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 at 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;
> >>+}
> >
>
