From patchwork Mon Jan 30 19:31:27 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ferruh Yigit X-Patchwork-Id: 20077 Return-Path: X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 146683237; Mon, 30 Jan 2017 20:31:33 +0100 (CET) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 0811D2E8A for ; Mon, 30 Jan 2017 20:31:29 +0100 (CET) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga104.fm.intel.com with ESMTP; 30 Jan 2017 11:31:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,312,1477983600"; d="scan'208";a="59910063" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.38]) ([10.237.220.38]) by fmsmga006.fm.intel.com with ESMTP; 30 Jan 2017 11:31:28 -0800 To: "Wiles, Keith" References: <20170129021205.36860-1-keith.wiles@intel.com> <2c430141-94c1-5482-7fc2-94f5908b41e0@intel.com> <6B37A978-0F89-4299-86E1-5697392EDC49@intel.com> <62618BAE-2D3C-41AC-979B-85C67BEF5144@intel.com> Cc: "dev@dpdk.org" From: Ferruh Yigit Message-ID: Date: Mon, 30 Jan 2017 19:31:27 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <62618BAE-2D3C-41AC-979B-85C67BEF5144@intel.com> Subject: Re: [dpdk-dev] [PATCH] net/tap: driver closing tx interface on queue setup 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 1/30/2017 6:20 PM, Wiles, Keith wrote: > >> On Jan 30, 2017, at 11:42 AM, Yigit, Ferruh wrote: >> >> On 1/30/2017 2:34 PM, Wiles, Keith wrote: >>> >>>> On Jan 30, 2017, at 5:00 AM, Yigit, Ferruh wrote: >>>> >>>> On 1/29/2017 2:12 AM, Keith Wiles wrote: >>>>> The tap driver setup both rx and tx file descriptors when the >>>>> rte_eth_rx_queue_setup() causing the tx to be closed when tx setup >>>>> was called. >>>> >>>> Can you please describe the problem more. >>>> Without this patch rx->fd == tx->fd, with this patch rx and tx has >>>> different file descriptors. >>> >>> Let me look at this more, I am getting the same FD for both. Must be something else going on. >> >> After patch, tun_alloc() called twice, one for Rx_q and other for Tx_q. >> And tun_alloc does open() to "/dev/net/tun", I expect they get different >> file descriptors. > > It is not called twice, it is only called once in the eth_dev_tap_create() routine and the fd is placed in the rxq/txq using the same fd. Then look in the rx/tx_setup_queue routines only update the fd and call tun_alloc if the fd is -1. Now looking at this code it seems a bit silly, but it was trying to deal with the setting up the new queue. It seems to be this logic not going to work with multiple queues in the same device and needs to be reworked. I see, right, it is not called twice for first queue of device. But will be called twice for multiple queues. Also if application does multiple rx/tx_queue_setup() calls, again, tun_alloc() will be called twice. This means two tap interface will be created, one for rx and one for tx, which is wrong I guess. The tun_alloc() logic is correct in current code, just setting both rx_queues and tx_queues in rx_queue_setup() breaks logic, so I propose to update just that part. What do you think about following update [1], it will fix current code, also will keep correct tun_alloc() call logic. But this will be still broken for application does multiple rx/tx_queue_setup() calls case. [1] > > I need to rework the code and do some cleanup. The current patch should work for a single queue per device. > > Thanks > >> >> And if they have same FD, won't this cause same problem, >> rx_queue_setup() will close the FD, if Tx_q has same FD it will have >> invalid descriptor. >> >>> >>>> >>>> What was the wrong with rx and tx having same fd? >>>> >>>> As far as I can see, rte_eth_rx_queue_setup() won't close tx->fd, that >>>> function will do nothing if rx or tx has valid fd. >>> >>> The rte_eth_rx_queue_setup() look at line 1146 if rxq has a value then release it, which happens on both Rx/Tx code >>> >>> rxq = dev->data->rx_queues; >>> if (rxq[rx_queue_id]) { >>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release, >>> -ENOTSUP); >>> (*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]); >>> rxq[rx_queue_id] = NULL; >>> } >> >> Got it thanks, I missed (relatively new) above code piece. >> >>> >>> if (rx_conf == NULL) >>> rx_conf = &dev_info.default_rxconf; >>> >>> ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc, >>> socket_id, rx_conf, mp); >>> >>>> >>>>> >>>>> Signed-off-by: Keith Wiles >>>> >>>> <...> >>> >>> Regards, >>> Keith > > Regards, > Keith > --- a/drivers/net/tap/rte_eth_tap.c +++ b/drivers/net/tap/rte_eth_tap.c @@ -428,8 +428,6 @@ tap_setup_queue(struct rte_eth_dev *dev, } } } - dev->data->rx_queues[qid] = rx; - dev->data->tx_queues[qid] = tx; rx->fd = fd; tx->fd = fd; @@ -438,6 +436,26 @@ tap_setup_queue(struct rte_eth_dev *dev, } static int +rx_setup_queue(struct rte_eth_dev *dev, + struct pmd_internals *internals, + uint16_t qid) +{ + dev->data->rx_queues[qid] = &internals->rxq[qid]; + + return tap_setup_queue(dev, internals, qid); +} + +static int +tx_setup_queue(struct rte_eth_dev *dev, + struct pmd_internals *internals, + uint16_t qid) +{ + dev->data->tx_queues[qid] = &internals->txq[qid]; + + return tap_setup_queue(dev, internals, qid); +} + +static int tap_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id, uint16_t nb_rx_desc __rte_unused, @@ -469,7 +487,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev, return -ENOMEM; } - fd = tap_setup_queue(dev, internals, rx_queue_id); + fd = rx_setup_queue(dev, internals, rx_queue_id); if (fd == -1) return -1; @@ -493,7 +511,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev, if (tx_queue_id >= internals->nb_queues) return -1; - ret = tap_setup_queue(dev, internals, tx_queue_id); + ret = tx_setup_queue(dev, internals, tx_queue_id); if (ret == -1) return -1;