From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by dpdk.org (Postfix) with ESMTP id 8DD262F42 for ; Tue, 13 Nov 2018 18:14:36 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 2863822200; Tue, 13 Nov 2018 12:14:36 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Tue, 13 Nov 2018 12:14:36 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=+F+JQzrC/U0CYkfX3DNAvwktm4mgId7hs4O4u+/qjKQ=; b=nqqr0MVpj0+b m2MNjdDciDrA7lkmT/V+qFSJyJ5kW1RSKjljCqnUyFJZ1TOO41Q7xd9Par1M8o6j a9lblm1xKZLCfjfFR/FefKJzA/QoMjbnNUYpgpPMlR9HZjs0GcG0x6KsNc4Hue47 at5/V0cwWdd0o28OMU1TFzeJt8OJN90= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=+F+JQzrC/U0CYkfX3DNAvwktm4mgId7hs4O4u+/qj KQ=; b=sTYXLJmFonzpxO9zZF6n0okl/mwF1vPZlircwaTHCpCTb1krny4PH5YCk rMD+3PZcjZHiTFpV/cKqfXYv1vYezf0nW+v46qiwzyLfdpBH6IGzQ503lX3bjlNG SHWhcYa32jIvBUKq9KUebXBtSC54b09D/xF+wlDE8hNO++CKN9YZXbidr+rP6lXj 6yhyLO1+Zi2q2hI+j1PaHICjV7DVqrsQgujpkSKF24EezQAYgHwew0XgmMpmEbfP LSO1KymJxS6smX0l9Mk5EIf/cnIesHt5seahTd/nvDSumztGO+TgTHyRXsWAnOwr /rJptVxXUywpVz521bzcgNyiB33sA== X-ME-Sender: X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 03798102F2; Tue, 13 Nov 2018 12:14:34 -0500 (EST) From: Thomas Monjalon To: Ferruh Yigit , Qi Zhang Cc: dev@dpdk.org, xueqin.lin@intel.com Date: Tue, 13 Nov 2018 18:14:33 +0100 Message-ID: <4093831.1gQSVtVNRZ@xps> In-Reply-To: <861c43b2-a082-fdf8-61b9-2df1f4ed919c@intel.com> References: <20181105210823.38757-1-qi.z.zhang@intel.com> <20181112165109.33296-1-qi.z.zhang@intel.com> <861c43b2-a082-fdf8-61b9-2df1f4ed919c@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v2] net/pcap: enable data path on secondary X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Nov 2018 17:14:36 -0000 Just a quick comment: There are probably some ideas to take from what was done for tap. 13/11/2018 17:56, Ferruh Yigit: > On 11/12/2018 4:51 PM, Qi Zhang wrote: > > Private vdev on secondary is never supported by the new shared > > device mode but pdump still relies on a private pcap PMD on secondary. > > The patch enables pcap PMD's data path on secondary so that pdump can > > work as usual. > > It would be great if you described the problem a little more. > > Private vdev was the way previously, when pdump developed, now with shared > device mode on virtual devices, pcap data path in secondary is not working. > > What exactly not working is (please correct me if I am wrong): > When secondary adds a virtual device, related data transferred to primary and > primary creates the device and shares device back with secondary. > When pcap device created in primary, pcap handlers (pointers) are process local > and they are not valid for secondary process. This breaks secondary. > > So we can't directly share the pcap handlers, but need to create a new set of > handlers for secondary, that is what you are doing in this patch, although I > have some comments, please check below. > > Since there is single storage for pcap handlers that primary and secondary > shares and they can't share the handlers, you can't make both primary and > secondary data path work. Also freeing handlers is another concern. What is > needed is `rte_eth_dev->process_private` which has been added in this release. > > > > > Signed-off-by: Qi Zhang > > Tested-by: Yufeng Mo > > <...> > > > @@ -934,6 +935,10 @@ pmd_init_internals(struct rte_vdev_device *vdev, > > */ > > (*eth_dev)->dev_ops = &ops; > > > > + /* store a copy of devargs for secondary process */ > > + strlcpy(internals->devargs, rte_vdev_device_args(vdev), > > + ETH_PCAP_ARG_MAXLEN); > > Why we need to cover this in PMD level? > > Why secondary probe isn't getting devargs? Can't we fix this in eal level? > It can be OK to workaround in the PMD taking account of the time of the release, > but for long term I think this should be fixed in eal. > > <...> > > > @@ -1122,23 +1126,37 @@ pmd_pcap_probe(struct rte_vdev_device *dev) > > start_cycles = rte_get_timer_cycles(); > > hz = rte_get_timer_hz(); > > > > - if (rte_eal_process_type() == RTE_PROC_SECONDARY) { > > + if (rte_eal_process_type() == RTE_PROC_PRIMARY) { > > + kvlist = rte_kvargs_parse(rte_vdev_device_args(dev), > > + valid_arguments); > > + if (kvlist == NULL) > > + return -EINVAL; > > + if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) == 1) > > + nb_rx_queue = 1; > > + else > > + nb_rx_queue = > > + rte_kvargs_count(kvlist, > > + ETH_PCAP_RX_PCAP_ARG) ? 1 : 0; > > + nb_tx_queue = 1; > > This part is wrong. pcap pmd supports multi queue, you can't hardcode the number > of queues. Also for Tx why it ignores `rx_iface` argument? > This is just hacking the driver for a specific use case breaking others. > > > + ret = pmd_init_internals(dev, nb_rx_queue, > > + nb_tx_queue, ð_dev); > > I think it is not required to move pmd_init_internals() here. > This can be done simpler, I will send a draft patch as a reply to this mail for > possible solution. > But again that can't be final solution, we need to use `process_private` > > <...> >