From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pankaj Chauhan Subject: Re: [RFC][PATCH V2 1/3] examples/vhost: Add vswitch (generic switch) framework Date: Tue, 27 Sep 2016 17:14:44 +0530 Message-ID: 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="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , To: Yuanhan Liu Return-path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0058.outbound.protection.outlook.com [104.47.34.58]) by dpdk.org (Postfix) with ESMTP id 57E40968 for ; Tue, 27 Sep 2016 13:44:54 +0200 (CEST) In-Reply-To: <20160926041209.GK23158@yliu-dev.sh.intel.com> 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 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. >> @@ -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? Thanks, Pankaj >> +struct vswitch_port *vs_add_port(struct vswitch_dev *vs_dev, int port_id, >> + enum vswitch_port_type type, void *priv) >> +{ >> + int rc = 0; >> + struct vswitch_port *vs_port = NULL; >> + struct vswitch_ops *vs_ops = vs_dev->ops; >> + >> + vs_port = vs_get_free_port(vs_dev); >> + if (!vs_port) { >> + RTE_LOG(DEBUG, VHOST_CONFIG, "Failed get free port in \ >> + vswitch %s\n", vs_dev->name); >> + rc = -EBUSY; >> + goto out; >> + } >> + >> + vs_port->port_id = port_id; >> + vs_port->type = type; >> + vs_port->priv = priv; >> + >> + /* Initialize default port operations. It should be noted that >> + * The switch ops->add_port can replace them with switch specefic >> + * operations if required. This gives us more flexibility in switch >> + * implementations. >> + */ >> + >> + switch (type) { >> + case VSWITCH_PTYPE_PHYS: >> + vs_port->do_tx = vs_do_tx_phys_port; >> + vs_port->do_rx = vs_do_rx_phys_port; >> + vs_port->get_txq = vs_get_txq_phys_port; >> + vs_port->get_rxq = vs_get_rxq_phys_port; >> + break; >> + case VSWITCH_PTYPE_VIRTIO: >> + vs_port->do_tx = vs_do_tx_virtio_port; >> + vs_port->do_rx = vs_do_rx_virtio_port; >> + vs_port->get_txq = vs_get_txq_virtio_port; >> + vs_port->get_rxq = vs_get_rxq_virtio_port; >> + break; >> + default: >> + RTE_LOG(DEBUG, VHOST_CONFIG, "Invalid port [id %d, type %d]", >> + port_id, type); >> + rc = -EINVAL; >> + goto out; >> + } >> + >> + if (vs_ops->add_port) >> + rc = vs_ops->add_port(vs_port); >> + >> + if (rc) >> + goto out; >> + >> + vs_port->state = VSWITCH_PSTATE_ADDED; >> + >> + rte_eth_macaddr_get(vs_port->port_id, &vs_port->mac_addr); > > <--- 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; >> +} >