From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 80EBC1D7 for ; Tue, 13 Nov 2018 19:27:58 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Nov 2018 10:27:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,229,1539673200"; d="scan'208";a="273706175" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga005.jf.intel.com with ESMTP; 13 Nov 2018 10:27:56 -0800 Received: from fmsmsx123.amr.corp.intel.com (10.18.125.38) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 13 Nov 2018 10:27:56 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx123.amr.corp.intel.com (10.18.125.38) with Microsoft SMTP Server (TLS) id 14.3.408.0; Tue, 13 Nov 2018 10:27:56 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.161]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.199]) with mapi id 14.03.0415.000; Wed, 14 Nov 2018 02:27:54 +0800 From: "Zhang, Qi Z" To: Thomas Monjalon , "Yigit, Ferruh" CC: "dev@dpdk.org" , "Lin, Xueqin" Thread-Topic: [PATCH v2] net/pcap: enable data path on secondary Thread-Index: AQHUeqfEPDAcaNAvQ0iRKzTsGyWjJKVNaC+AgAAFFYCAAIZP0A== Date: Tue, 13 Nov 2018 18:27:53 +0000 Message-ID: <039ED4275CED7440929022BC67E70611532E343C@SHSMSX103.ccr.corp.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> <4093831.1gQSVtVNRZ@xps> In-Reply-To: <4093831.1gQSVtVNRZ@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOWZiNjdkNjktYjVjMy00MjMwLThmMmUtNWI3ZTA2MmViNzQ0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiaUVlQUlcL0V0ckJ0T28yXC9JR1wvbUJYUFlKR1wvXC90WjNKcEFQZjc4ZWRtRnZLTVBQdUhPK1c4eHpVZXBlM295RlhFIn0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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 18:27:59 -0000 First, apologies to make this in rush since I was somehow under pressure to= make pdump works in 18.11. I agree there is lot of things need to improve, but the strategy here is to= make it work quietly and not break anything else :)=20 add some comments inline. > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Tuesday, November 13, 2018 9:15 AM > To: Yigit, Ferruh ; Zhang, Qi Z > Cc: dev@dpdk.org; Lin, Xueqin > Subject: Re: [PATCH v2] net/pcap: enable data path on secondary >=20 > Just a quick comment: > There are probably some ideas to take from what was done for tap. >=20 >=20 > 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 n= ot > 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. You are right, we should prevent handler be opened in primary be corrupted = during probe at secondary. Now, I see this problem in pcap , as an example: internals->tx_queue[i].dum= per/pcap is shared but will be overwritten at secondary, we should fix them= by use process_private,=20 > > > > > > > > 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 =3D &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 lev= el? > > 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. Yes this is the workaround for quick fix. Ideally secondary process should not take care of devargs, it just attach. And it's better to only parse devargs on one process ( primary process), th= e parsed result could be stored to intermediate result in shared memory,(ex= amples, internal->nb_rx_queue_required) so secondary process don't need to = parse it again. > > > > <...> > > > > > @@ -1122,23 +1126,37 @@ pmd_pcap_probe(struct rte_vdev_device > *dev) > > > start_cycles =3D rte_get_timer_cycles(); > > > hz =3D rte_get_timer_hz(); > > > > > > - if (rte_eal_process_type() =3D=3D RTE_PROC_SECONDARY) { > > > + if (rte_eal_process_type() =3D=3D RTE_PROC_PRIMARY) { > > > + kvlist =3D rte_kvargs_parse(rte_vdev_device_args(dev), > > > + valid_arguments); > > > + if (kvlist =3D=3D NULL) > > > + return -EINVAL; > > > + if (rte_kvargs_count(kvlist, ETH_PCAP_IFACE_ARG) =3D=3D 1) > > > + nb_rx_queue =3D 1; > > > + else > > > + nb_rx_queue =3D > > > + rte_kvargs_count(kvlist, > > > + ETH_PCAP_RX_PCAP_ARG) ? 1 : 0; > > > + nb_tx_queue =3D 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= . Previously the nb_tx_queue and nb_rx_queue is decided by pcaps.num_of_queue= and dumpers.num_of_queues. I just can't figure out a way that we can have more than 1 queue during pro= be, look at below code. If ETH_PCAP_IFACE_ARG pcaps.num_of_queue =3D 1; dumpers.num_of_queue =3D 1; else is_rx_pcap =3D rte_kvargs_count(kvlist, ETH_PCAP_RX_PCAP_ARG) ? 1 : 0; pcaps.num_of_queue =3D 0; if (is_rx_pcap) { ret =3D rte_kvargs_process(kvlist, ETH_PCAP_RX_PCAP_ARG, &open_rx_pcap, &pcaps); // pcaps.num_of_queue =3D 1; } else { ret =3D rte_kvargs_process(kvlist, NULL, &rx_iface_args_process, &pcaps); // pcaps.num_of_queue =3D 0; } is_tx_pcap =3D rte_kvargs_count(kvlist, ETH_PCAP_TX_PCAP_ARG) ? 1 : 0; dumpers.num_of_queue =3D 0; if (is_tx_pcap) ret =3D rte_kvargs_process(kvlist, ETH_PCAP_TX_PCAP_ARG, &open_tx_pcap, &dumpers); // dumpers.num_of_queue =3D 1 else ret =3D rte_kvargs_process(kvlist, ETH_PCAP_TX_IFACE_ARG, &open_tx_iface, &dumpers); // dumpers.num_of_queue =3D 1 That's the same logic I applied, did I missed something, would you explain = more for this? Thanks Qi > > > > > + ret =3D 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` > > > > <...> > > >=20 >=20 >=20 >=20