On 1/30/2017 6:20 PM, Wiles, Keith wrote:
>
>> On Jan 30, 2017, at 11:42 AM, Yigit, Ferruh <ferruh.yi...@intel.com> wrote:
>>
>> On 1/30/2017 2:34 PM, Wiles, Keith wrote:
>>>
>>>> On Jan 30, 2017, at 5:00 AM, Yigit, Ferruh <ferruh.yi...@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.wi...@intel.com>
>>>>
>>>> <...>
>>>
>>> Regards,
>>> Keith
>
> Regards,
> Keith
>
