From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 920E1160 for ; Sat, 21 Jul 2018 15:45:48 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Jul 2018 06:45:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,385,1526367600"; d="scan'208";a="68777901" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga003.jf.intel.com with ESMTP; 21 Jul 2018 06:44:42 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 21 Jul 2018 06:44:42 -0700 Received: from fmsmsx117.amr.corp.intel.com ([169.254.3.3]) by FMSMSX110.amr.corp.intel.com ([169.254.14.196]) with mapi id 14.03.0319.002; Sat, 21 Jul 2018 06:44:41 -0700 From: "Wiles, Keith" To: Thomas Monjalon CC: "dev@dpdk.org" , Raslan Darawsheh , "ophirmu@mellanox.com" Thread-Topic: [dpdk-dev] [PATCH v3] net/tap: add queues when attaching from secondary process Thread-Index: AQHUIBsZNWHFL4UZz0qhG0RtLsBPE6SYs6MAgABpLICAAQpFAA== Date: Sat, 21 Jul 2018 13:44:41 +0000 Message-ID: <0EAF8388-4F19-4258-85FD-C18CFCCCE7BE@intel.com> References: <1528374591-26126-1-git-send-email-rasland@mellanox.com> <20180720111552.14132-1-thomas@monjalon.net> <2122883.JrNZV5Vflb@xps> In-Reply-To: <2122883.JrNZV5Vflb@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.255.77.92] Content-Type: text/plain; charset="us-ascii" Content-ID: <3264E1C49E8D414D931360F3638EF51D@intel.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3] net/tap: add queues when attaching from secondary process 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: Sat, 21 Jul 2018 13:45:49 -0000 > On Jul 20, 2018, at 4:51 PM, Thomas Monjalon wrote: >=20 > 20/07/2018 17:35, Wiles, Keith: >>> On Jul 20, 2018, at 4:15 AM, Thomas Monjalon wrot= e: >>> + /* FIXME: handle replies.nb_received > 1 */ >>=20 >> I am not a big fan of having TODO or FIXME comments in the code. >=20 > What don't you like in such comments? We should not have FIXME or TODO in the code it does not look like it is co= mplete, if we need to fix something then fix it or put it on a todo list no= t in the code. The same thing for the TODO which to me means a future enhan= cement we just need to add it to a future todo list. If the code in these sections have a limitation them describe the limitatio= n and remove the FIXME and TODOs from the code. >=20 >> Can we remove them and just describe the problem and what would happen >> or not happen if the condition occurs? >=20 > You mean describing the problem in the code? >=20 >> If we need to add this support in the future then we need to put these >> in a enhancement tracker or someplace else. >=20 > The limitation is documented in the guide (limit of 8 queues). >=20 >>> + reply =3D &replies.msgs[0]; >=20 > [...] >>> + /* FIXME: split message if more queues than RTE_MP_MAX_FD_NUM */ >>=20 >> Here too. >=20 > This limitation is related to the previous one (send only one message, > receive only message). >=20 >>> + RTE_ASSERT(reply.num_fds <=3D RTE_MP_MAX_FD_NUM); >>> + >>> + /* Send reply */ >>> + strcpy(reply.name, request->name); >>> + strcpy(reply_param->port_name, request_param->port_name); >>=20 >> Normally we use the snprintf or strlcpy() functions for the above should= we do that here too? >=20 > Yes it looks to be a good idea. >=20 >=20 >>> @@ -1946,8 +2056,18 @@ rte_pmd_tap_probe(struct rte_vdev_device *dev) >>> TAP_LOG(ERR, "Failed to probe %s", name); >>> return -1; >>> } >>> - /* TODO: request info from primary to set up Rx and Tx */ >>> eth_dev->dev_ops =3D &ops; >>> + eth_dev->rx_pkt_burst =3D pmd_rx_burst; >>> + eth_dev->tx_pkt_burst =3D pmd_tx_burst; >>> + >>> + if (!rte_eal_primary_proc_alive(NULL)) { >>> + TAP_LOG(ERR, "Primary process is missing"); >>> + return -1; >>> + } >>> + ret =3D tap_mp_attach_queues(name, eth_dev); >>> + if (ret !=3D 0) >>> + return -1; >>=20 >> Does the call above need to be wrapped using if secondary process or is = this for both primary and secondary? >=20 > It is already in a "secondary only" block. >=20 >>> + /* Register IPC feed callback */ >>> + ret =3D rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues); >>> + if (ret < 0 && rte_errno !=3D EEXIST) { >>> + TAP_LOG(ERR, "%s: Failed to register IPC callback: %s", >>> + tuntap_name, strerror(rte_errno)); >>> + goto leave; >>> + } >>=20 >> Same for this one as above? >=20 > This code path is executed only in primary or creation of port in seconda= ry. > I think it is fine. >=20 > However I am thinking it should be registered only once for all TAP ports= . >=20 >=20 Regards, Keith