* [PATCH] net/tap: driver closing tx interface on queue setup @ 2017-01-29 2:12 Keith Wiles 2017-01-30 11:00 ` Ferruh Yigit 2017-01-30 20:54 ` [PATCH v2] net/tap: fix invalid queue file descriptor Ferruh Yigit 0 siblings, 2 replies; 12+ messages in thread From: Keith Wiles @ 2017-01-29 2:12 UTC (permalink / raw) To: dev 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. Signed-off-by: Keith Wiles <keith.wiles@intel.com> --- drivers/net/tap/rte_eth_tap.c | 48 ++++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c index c0afc2d..267b421 100644 --- a/drivers/net/tap/rte_eth_tap.c +++ b/drivers/net/tap/rte_eth_tap.c @@ -406,32 +406,52 @@ tap_link_update(struct rte_eth_dev *dev __rte_unused, } static int -tap_setup_queue(struct rte_eth_dev *dev, +rx_setup_queue(struct rte_eth_dev *dev, struct pmd_internals *internals, uint16_t qid) { struct rx_queue *rx = &internals->rxq[qid]; - struct tx_queue *tx = &internals->txq[qid]; int fd; fd = rx->fd; if (fd < 0) { - fd = tx->fd; + RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n", + dev->data->name, qid); + fd = tun_alloc(dev->data->name); if (fd < 0) { - RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n", - dev->data->name, qid); - fd = tun_alloc(dev->data->name); - if (fd < 0) { - RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", - dev->data->name); - return -1; - } + RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", + dev->data->name); + return -1; } } dev->data->rx_queues[qid] = rx; - dev->data->tx_queues[qid] = tx; rx->fd = fd; + + return fd; +} + +static int +tx_setup_queue(struct rte_eth_dev *dev, + struct pmd_internals *internals, + uint16_t qid) +{ + struct tx_queue *tx = &internals->txq[qid]; + int fd; + + fd = tx->fd; + if (fd < 0) { + RTE_LOG(INFO, PMD, "Add queue to TAP %s for qid %d\n", + dev->data->name, qid); + fd = tun_alloc(dev->data->name); + if (fd < 0) { + RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", + dev->data->name); + return -1; + } + } + dev->data->tx_queues[qid] = tx; + tx->fd = fd; return fd; @@ -469,7 +489,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 +513,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; -- 2.8.0.GIT ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] net/tap: driver closing tx interface on queue setup 2017-01-29 2:12 [PATCH] net/tap: driver closing tx interface on queue setup Keith Wiles @ 2017-01-30 11:00 ` Ferruh Yigit 2017-01-30 14:34 ` Wiles, Keith 2017-01-30 14:38 ` Pascal Mazon 2017-01-30 20:54 ` [PATCH v2] net/tap: fix invalid queue file descriptor Ferruh Yigit 1 sibling, 2 replies; 12+ messages in thread From: Ferruh Yigit @ 2017-01-30 11:00 UTC (permalink / raw) To: Keith Wiles, dev 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. 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. > > Signed-off-by: Keith Wiles <keith.wiles@intel.com> <...> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net/tap: driver closing tx interface on queue setup 2017-01-30 11:00 ` Ferruh Yigit @ 2017-01-30 14:34 ` Wiles, Keith 2017-01-30 17:42 ` Ferruh Yigit 2017-01-30 14:38 ` Pascal Mazon 1 sibling, 1 reply; 12+ messages in thread From: Wiles, Keith @ 2017-01-30 14:34 UTC (permalink / raw) To: Yigit, Ferruh; +Cc: dev > On Jan 30, 2017, at 5:00 AM, Yigit, Ferruh <ferruh.yigit@intel.com> 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. > > 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; } 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 <keith.wiles@intel.com> > > <...> Regards, Keith ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net/tap: driver closing tx interface on queue setup 2017-01-30 14:34 ` Wiles, Keith @ 2017-01-30 17:42 ` Ferruh Yigit 2017-01-30 18:20 ` Wiles, Keith 0 siblings, 1 reply; 12+ messages in thread From: Ferruh Yigit @ 2017-01-30 17:42 UTC (permalink / raw) To: Wiles, Keith; +Cc: dev On 1/30/2017 2:34 PM, Wiles, Keith wrote: > >> On Jan 30, 2017, at 5:00 AM, Yigit, Ferruh <ferruh.yigit@intel.com> 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. 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 <keith.wiles@intel.com> >> >> <...> > > Regards, > Keith > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net/tap: driver closing tx interface on queue setup 2017-01-30 17:42 ` Ferruh Yigit @ 2017-01-30 18:20 ` Wiles, Keith 2017-01-30 19:31 ` Ferruh Yigit 0 siblings, 1 reply; 12+ messages in thread From: Wiles, Keith @ 2017-01-30 18:20 UTC (permalink / raw) To: Yigit, Ferruh; +Cc: dev > On Jan 30, 2017, at 11:42 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote: > > On 1/30/2017 2:34 PM, Wiles, Keith wrote: >> >>> On Jan 30, 2017, at 5:00 AM, Yigit, Ferruh <ferruh.yigit@intel.com> 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 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 <keith.wiles@intel.com> >>> >>> <...> >> >> Regards, >> Keith Regards, Keith ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net/tap: driver closing tx interface on queue setup 2017-01-30 18:20 ` Wiles, Keith @ 2017-01-30 19:31 ` Ferruh Yigit 0 siblings, 0 replies; 12+ messages in thread From: Ferruh Yigit @ 2017-01-30 19:31 UTC (permalink / raw) To: Wiles, Keith; +Cc: dev On 1/30/2017 6:20 PM, Wiles, Keith wrote: > >> On Jan 30, 2017, at 11:42 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote: >> >> On 1/30/2017 2:34 PM, Wiles, Keith wrote: >>> >>>> On Jan 30, 2017, at 5:00 AM, Yigit, Ferruh <ferruh.yigit@intel.com> 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] --- 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; > > 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 <keith.wiles@intel.com> >>>> >>>> <...> >>> >>> Regards, >>> Keith > > Regards, > Keith > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net/tap: driver closing tx interface on queue setup 2017-01-30 11:00 ` Ferruh Yigit 2017-01-30 14:34 ` Wiles, Keith @ 2017-01-30 14:38 ` Pascal Mazon 2017-01-30 15:04 ` Wiles, Keith 2017-01-30 17:19 ` Ferruh Yigit 1 sibling, 2 replies; 12+ messages in thread From: Pascal Mazon @ 2017-01-30 14:38 UTC (permalink / raw) To: dev On 01/30/2017 12:00 PM, Ferruh Yigit 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. > > 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. > >> >> Signed-off-by: Keith Wiles <keith.wiles@intel.com> > > <...> > Hi, The tap PMD recently broke for me because of this patch [1]. During init (eth_dev_tap_create()), the tap PMD allocates a shared RX/TX queue through tun_alloc(). The recent patch now releases existing queues in rx_queue_setup(), before adding new ones. When rx_queue_setup() is called, it uses close() calls on all shared queues, effectively deleting the netdevice. That's the main issue here. I tested Keith's patch [2], and it fixes that issue, using separate queues. There is however a couple of other queues-related issues in the tap PMD, but I'm not sure how to address them properly: 1. internals->fds[] gets filled only with RX queues (appart from index 0 that is common to both RX and TX). It means that RX queues only will be deleted when calling rte_pmd_tap_remove() or tap_tx_queue_release(). 2. tap_dev_stop() is not symmetrical with tap_dev_start(): queues won't get re-created after a stop. It may be best to keep the very first fd (created with tun_alloc() in eth_dev_tap_create() during probe) apart. And then add separate TX/RX queues in internals->txq[] and internals->rxq[] respectively. What do you think? [1] d00d7cc88335 ("ethdev: release queue before setting up") [2] http://dpdk.org/ml/archives/dev/2017-January/056470.html -- Pascal Mazon www.6wind.com ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net/tap: driver closing tx interface on queue setup 2017-01-30 14:38 ` Pascal Mazon @ 2017-01-30 15:04 ` Wiles, Keith 2017-01-30 17:19 ` Ferruh Yigit 1 sibling, 0 replies; 12+ messages in thread From: Wiles, Keith @ 2017-01-30 15:04 UTC (permalink / raw) To: Pascal Mazon; +Cc: dev > On Jan 30, 2017, at 8:38 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote: > > On 01/30/2017 12:00 PM, Ferruh Yigit 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. >> >> 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. >> >>> >>> Signed-off-by: Keith Wiles <keith.wiles@intel.com> >> >> <...> >> > > Hi, > > The tap PMD recently broke for me because of this patch [1]. > > During init (eth_dev_tap_create()), the tap PMD allocates a shared RX/TX queue through tun_alloc(). > The recent patch now releases existing queues in rx_queue_setup(), before adding new ones. > > When rx_queue_setup() is called, it uses close() calls on all shared queues, effectively deleting the netdevice. > That's the main issue here. > > I tested Keith's patch [2], and it fixes that issue, using separate queues. > > There is however a couple of other queues-related issues in the tap PMD, but I'm not sure how to address them properly: > > 1. internals->fds[] gets filled only with RX queues (appart from index 0 that is common to both RX and TX). > It means that RX queues only will be deleted when calling rte_pmd_tap_remove() or tap_tx_queue_release(). > > 2. tap_dev_stop() is not symmetrical with tap_dev_start(): queues won't get re-created after a stop. > > It may be best to keep the very first fd (created with tun_alloc() in eth_dev_tap_create() during probe) apart. > And then add separate TX/RX queues in internals->txq[] and internals->rxq[] respectively. > What do you think? > > [1] d00d7cc88335 ("ethdev: release queue before setting up") > [2] http://dpdk.org/ml/archives/dev/2017-January/056470.html Lets keep the current patch just to get over the current problem if everyone agrees. I will address the comments Pascal brings up as a later updated to the TAP PMD or I can try to get the other issues cleaned up. > > > -- > Pascal Mazon > www.6wind.com Regards, Keith ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] net/tap: driver closing tx interface on queue setup 2017-01-30 14:38 ` Pascal Mazon 2017-01-30 15:04 ` Wiles, Keith @ 2017-01-30 17:19 ` Ferruh Yigit 1 sibling, 0 replies; 12+ messages in thread From: Ferruh Yigit @ 2017-01-30 17:19 UTC (permalink / raw) To: Pascal Mazon, dev On 1/30/2017 2:38 PM, Pascal Mazon wrote: > On 01/30/2017 12:00 PM, Ferruh Yigit 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. >> >> 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. >> >>> >>> Signed-off-by: Keith Wiles <keith.wiles@intel.com> >> >> <...> >> > > Hi, > > The tap PMD recently broke for me because of this patch [1]. > > During init (eth_dev_tap_create()), the tap PMD allocates a shared RX/TX > queue through tun_alloc(). > The recent patch now releases existing queues in rx_queue_setup(), > before adding new ones. > > When rx_queue_setup() is called, it uses close() calls on all shared > queues, effectively deleting the netdevice. > That's the main issue here. > > I tested Keith's patch [2], and it fixes that issue, using separate queues. Thanks for the clarification, and I am adding following patch to patch [2]: Tested-by: Pascal Mazon <pascal.mazon@6wind.com> <...> > > [1] d00d7cc88335 ("ethdev: release queue before setting up") > [2] http://dpdk.org/ml/archives/dev/2017-January/056470.html > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] net/tap: fix invalid queue file descriptor 2017-01-29 2:12 [PATCH] net/tap: driver closing tx interface on queue setup Keith Wiles 2017-01-30 11:00 ` Ferruh Yigit @ 2017-01-30 20:54 ` Ferruh Yigit 2017-01-30 20:57 ` Wiles, Keith 1 sibling, 1 reply; 12+ messages in thread From: Ferruh Yigit @ 2017-01-30 20:54 UTC (permalink / raw) To: Keith Wiles; +Cc: dev, Ferruh Yigit From: Keith Wiles <keith.wiles@intel.com> Rx and Tx queues share the common tap file descriptor, but save this value separately. Setting up Rx/Tx queue sets up both queues, release_queue close the tap file but update file descriptor only for that queue. This makes other queue's file descriptor invalid. As a workaround, prevent release_queue callback to be called by default. This is done by separating Rx/Tx setup functions, so that each only setup its own queue, this prevents rte_eth_rx/tx_queue_setup() calling release_queue before setup_queue. Signed-off-by: Keith Wiles <keith.wiles@intel.com> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> --- drivers/net/tap/rte_eth_tap.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c index c0afc2d..91f63f5 100644 --- 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; -- 2.9.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] net/tap: fix invalid queue file descriptor 2017-01-30 20:54 ` [PATCH v2] net/tap: fix invalid queue file descriptor Ferruh Yigit @ 2017-01-30 20:57 ` Wiles, Keith 2017-01-30 21:23 ` Ferruh Yigit 0 siblings, 1 reply; 12+ messages in thread From: Wiles, Keith @ 2017-01-30 20:57 UTC (permalink / raw) To: Yigit, Ferruh; +Cc: dev > On Jan 30, 2017, at 2:54 PM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote: > > From: Keith Wiles <keith.wiles@intel.com> > > Rx and Tx queues share the common tap file descriptor, but save this > value separately. > > Setting up Rx/Tx queue sets up both queues, release_queue close the > tap file but update file descriptor only for that queue. > > This makes other queue's file descriptor invalid. > > As a workaround, prevent release_queue callback to be called by default. > > This is done by separating Rx/Tx setup functions, so that each only > setup its own queue, this prevents rte_eth_rx/tx_queue_setup() calling > release_queue before setup_queue. > > Signed-off-by: Keith Wiles <keith.wiles@intel.com> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> Acked-by: Keith Wiles <keith.wiles@intel.com> > --- > drivers/net/tap/rte_eth_tap.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c > index c0afc2d..91f63f5 100644 > --- 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; > > -- > 2.9.3 > Regards, Keith ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] net/tap: fix invalid queue file descriptor 2017-01-30 20:57 ` Wiles, Keith @ 2017-01-30 21:23 ` Ferruh Yigit 0 siblings, 0 replies; 12+ messages in thread From: Ferruh Yigit @ 2017-01-30 21:23 UTC (permalink / raw) To: Wiles, Keith; +Cc: dev On 1/30/2017 8:57 PM, Wiles, Keith wrote: > >> On Jan 30, 2017, at 2:54 PM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote: >> >> From: Keith Wiles <keith.wiles@intel.com> >> >> Rx and Tx queues share the common tap file descriptor, but save this >> value separately. >> >> Setting up Rx/Tx queue sets up both queues, release_queue close the >> tap file but update file descriptor only for that queue. >> >> This makes other queue's file descriptor invalid. >> >> As a workaround, prevent release_queue callback to be called by default. >> >> This is done by separating Rx/Tx setup functions, so that each only >> setup its own queue, this prevents rte_eth_rx/tx_queue_setup() calling >> release_queue before setup_queue. >> >> Signed-off-by: Keith Wiles <keith.wiles@intel.com> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com> Applied to dpdk-next-net/master, thanks. (Just a reminder, defect still exists when app call rte_eth_rx/tx_queue_setup() multiple times) ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-01-30 21:23 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-29 2:12 [PATCH] net/tap: driver closing tx interface on queue setup Keith Wiles 2017-01-30 11:00 ` Ferruh Yigit 2017-01-30 14:34 ` Wiles, Keith 2017-01-30 17:42 ` Ferruh Yigit 2017-01-30 18:20 ` Wiles, Keith 2017-01-30 19:31 ` Ferruh Yigit 2017-01-30 14:38 ` Pascal Mazon 2017-01-30 15:04 ` Wiles, Keith 2017-01-30 17:19 ` Ferruh Yigit 2017-01-30 20:54 ` [PATCH v2] net/tap: fix invalid queue file descriptor Ferruh Yigit 2017-01-30 20:57 ` Wiles, Keith 2017-01-30 21:23 ` Ferruh Yigit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.