From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id BF2AA29D2 for ; Fri, 20 Jul 2018 23:51:43 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 1087821B4F; Fri, 20 Jul 2018 17:51:43 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Fri, 20 Jul 2018 17:51:43 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc; s=mesmtp; bh=dGiWPXoBpKaTm770CCUy+dr4jM ZEe00mEl3JoggaO1U=; b=d/zeJ2G8zapRczjVRf6IeL8rmmRkD1tDejHMVxAKBr DUJxZCA1rJ7uVS/QBmpU4kLEaVSSu7h5I8h7RIZhcayfj08jROEsV5KxnEPh6sxo ASUFpubrE0olpnXZK+7dMOWF32PRSTDuO6y0NzUvL+BnQTr/94+G4cTREz2ZSUiB g= 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-sender:x-me-sender:x-sasl-enc; s=fm3; bh=dGiWPX oBpKaTm770CCUy+dr4jMZEe00mEl3JoggaO1U=; b=GQcdD6hluu4KPIweIU3TWF AOUDf2Tk0n2UqnV5cKYXZTvMaD9ptyp2s0uHtkJiA9RfPv4iffWfgmU7SWOtJrjK 6pc9Q8zllWawjJ7fjheM3CUkRxnLcuWWTPNVBM5Wu4RPHtoDhNnK/svGO3PUUChd UHywbu9rjWcoB20fXe8eIOLFV0mfGvi+k+ukXU5Ab/+TgnHSSglyya4M7UXuEywt 8CBZNNU3F9yTig64fEDrHwEgSlrLC67+07+42He6yNDD8mtqwcTTX0cpIY/VDy/L CJsgsCPqP2tCKipWaBEeZBAoCxE29QolJyi/m12xKG9MwVsiF0lNOjYbnzVuLQCQ == X-ME-Proxy: X-ME-Sender: Received: from xps.localnet (unknown [207.232.18.42]) by mail.messagingengine.com (Postfix) with ESMTPA id 55D54E4040; Fri, 20 Jul 2018 17:51:41 -0400 (EDT) From: Thomas Monjalon To: "Wiles, Keith" Cc: dev@dpdk.org, Raslan Darawsheh , "ophirmu@mellanox.com" Date: Fri, 20 Jul 2018 23:51:39 +0200 Message-ID: <2122883.JrNZV5Vflb@xps> In-Reply-To: References: <1528374591-26126-1-git-send-email-rasland@mellanox.com> <20180720111552.14132-1-thomas@monjalon.net> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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: Fri, 20 Jul 2018 21:51:44 -0000 20/07/2018 17:35, Wiles, Keith: > > On Jul 20, 2018, at 4:15 AM, Thomas Monjalon wrote: > > + /* FIXME: handle replies.nb_received > 1 */ > > I am not a big fan of having TODO or FIXME comments in the code. What don't you like in such comments? > Can we remove them and just describe the problem and what would happen > or not happen if the condition occurs? You mean describing the problem in the code? > If we need to add this support in the future then we need to put these > in a enhancement tracker or someplace else. The limitation is documented in the guide (limit of 8 queues). > > + reply = &replies.msgs[0]; [...] > > + /* FIXME: split message if more queues than RTE_MP_MAX_FD_NUM */ > > Here too. This limitation is related to the previous one (send only one message, receive only message). > > + RTE_ASSERT(reply.num_fds <= RTE_MP_MAX_FD_NUM); > > + > > + /* Send reply */ > > + strcpy(reply.name, request->name); > > + strcpy(reply_param->port_name, request_param->port_name); > > Normally we use the snprintf or strlcpy() functions for the above should we do that here too? Yes it looks to be a good idea. > > @@ -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 = &ops; > > + eth_dev->rx_pkt_burst = pmd_rx_burst; > > + eth_dev->tx_pkt_burst = pmd_tx_burst; > > + > > + if (!rte_eal_primary_proc_alive(NULL)) { > > + TAP_LOG(ERR, "Primary process is missing"); > > + return -1; > > + } > > + ret = tap_mp_attach_queues(name, eth_dev); > > + if (ret != 0) > > + return -1; > > Does the call above need to be wrapped using if secondary process or is this for both primary and secondary? It is already in a "secondary only" block. > > + /* Register IPC feed callback */ > > + ret = rte_mp_action_register(TAP_MP_KEY, tap_mp_sync_queues); > > + if (ret < 0 && rte_errno != EEXIST) { > > + TAP_LOG(ERR, "%s: Failed to register IPC callback: %s", > > + tuntap_name, strerror(rte_errno)); > > + goto leave; > > + } > > Same for this one as above? This code path is executed only in primary or creation of port in secondary. I think it is fine. However I am thinking it should be registered only once for all TAP ports.