From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [RFC][PATCH V2 1/3] examples/vhost: Add vswitch (generic switch) framework Date: Tue, 27 Sep 2016 20:03:44 +0800 Message-ID: <20160927120344.GM25823@yliu-dev.sh.intel.com> References: <1473072871-16108-1-git-send-email-pankaj.chauhan@nxp.com> <1473072871-16108-2-git-send-email-pankaj.chauhan@nxp.com> <20160926041209.GK23158@yliu-dev.sh.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, hemant.agrawal@nxp.com, jianfeng.tan@intel.com, maxime.coquelin@redhat.com To: Pankaj Chauhan Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 00BE912A8 for ; Tue, 27 Sep 2016 14:03:16 +0200 (CEST) Content-Disposition: inline In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > >>--- > >> 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; > >>+} > > >