From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id CDE4E1B2AB for ; Wed, 3 Oct 2018 20:00:17 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Oct 2018 11:00:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,336,1534834800"; d="scan'208";a="79607955" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.49]) ([10.237.221.49]) by orsmga006.jf.intel.com with ESMTP; 03 Oct 2018 11:00:15 -0700 To: Raslan Darawsheh , "keith.wiles@intel.com" Cc: Thomas Monjalon , "dev@dpdk.org" , Shahaf Shuler , Ori Kam References: <1538047196-13789-2-git-send-email-rasland@mellanox.com> <1538476438-20891-1-git-send-email-rasland@mellanox.com> <1538476438-20891-2-git-send-email-rasland@mellanox.com> From: Ferruh Yigit Openpgp: preference=signencrypt Message-ID: Date: Wed, 3 Oct 2018 19:00:14 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1538476438-20891-2-git-send-email-rasland@mellanox.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 2/2] 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: Wed, 03 Oct 2018 18:00:18 -0000 On 10/2/2018 11:34 AM, Raslan Darawsheh wrote: > In the case the device is created by the primary process, > the secondary must request some file descriptors to attach the queues. > The file descriptors are shared via IPC Unix socket. > > Thanks to the IPC synchronization, the secondary process > is now able to do Rx/Tx on a TAP created by the primary process. > > Signed-off-by: Raslan Darawsheh > Signed-off-by: Thomas Monjalon <...> > +/* Request queue file descriptors from secondary to primary. */ > +static int > +tap_mp_attach_queues(const char *port_name, struct rte_eth_dev *dev) > +{ > + int ret; > + struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0}; > + struct rte_mp_msg request, *reply; > + struct rte_mp_reply replies; > + struct ipc_queues *request_param = (struct ipc_queues *)request.param; > + struct ipc_queues *reply_param; > + int queue, fd_iterator; > + > + /* Prepare the request */ > + strlcpy(request.name, TAP_MP_KEY, sizeof(request.name)); > + strlcpy(request_param->port_name, port_name, > + sizeof(request_param->port_name)); > + request.len_param = sizeof(*request_param); > + /* Send request and receive reply */ > + ret = rte_mp_request_sync(&request, &replies, &timeout); > + if (ret < 0) { > + TAP_LOG(ERR, "Failed to request queues from primary: %d", > + rte_errno); > + return -1; > + } > + reply = &replies.msgs[0]; > + reply_param = (struct ipc_queues *)reply->param; > + TAP_LOG(DEBUG, "Received IPC reply for %s", reply_param->port_name); > + > + /* Attach the queues from received file descriptors */ > + > + dev->data->nb_rx_queues = reply_param->rxq_count; > + dev->data->nb_tx_queues = reply_param->txq_count; > + fd_iterator = 0; > + for (queue = 0; queue < reply_param->rxq_count; queue++) > + process_private->rxq_fds[queue] = reply->fds[fd_iterator++]; > + for (queue = 0; queue < reply_param->txq_count; queue++) > + process_private->txq_fds[queue] = reply->fds[fd_iterator++]; Based on comment in prev patch, "process_private" is the address of the memory block for last eth_dev probed, we need this something linked to dev, like "dev->data->nb_rx_queues". But this will be work fine because it is called just after memory allocated, please check below one. Also the problem here is, this is secondary process and nobody sets dev->data->rxq[i].fd to point process_private? So you have correct fds on "process_private" but your redirection doesn't point here. (Please remember redirection set in eth_dev_tap_create() which called only for primary.) > + > + return 0; > +} > + > +/* Send the queue file descriptors from the primary process to secondary. */ > +static int > +tap_mp_sync_queues(const struct rte_mp_msg *request, const void *peer) > +{ > + struct rte_eth_dev *dev; > + struct rte_mp_msg reply; > + const struct ipc_queues *request_param = > + (const struct ipc_queues *)request->param; > + struct ipc_queues *reply_param = > + (struct ipc_queues *)reply.param; > + uint16_t port_id; > + int queue; > + int ret; > + > + /* Get requested port */ > + TAP_LOG(DEBUG, "Received IPC request for %s", request_param->port_name); > + ret = rte_eth_dev_get_port_by_name(request_param->port_name, &port_id); > + if (ret) { > + TAP_LOG(ERR, "Failed to get port id for %s", > + request_param->port_name); > + return -1; > + } > + dev = &rte_eth_devices[port_id]; > + > + /* Fill file descriptors for all queues */ > + reply.num_fds = 0; > + reply_param->rxq_count = 0; > + for (queue = 0; queue < dev->data->nb_rx_queues; queue++) { > + reply.fds[reply.num_fds++] = process_private->rxq_fds[queue]; This is "process_private" in primary. If you have multiple tap device this point the memory block for last one. When secondary asks for fds for a device "dev", you are always sending the last ones, which is wrong. This can work if you have only one tap device but not if you have more. Briefly there are two problems with this patch: 1- "process_private" should be per eth_dev instead of globally for tap 2- Need a non shared memory location to save fds (or pointers to fds), redirection is not helping here.