[dpdk-dev,1/6] net/tap: use correct tap name
Checks
Commit Message
dev->data->name contains "net_tap", the device driver name.
dev->data->dev_private->name contains the actual iface name,
e.g. "dtap0".
In tun_alloc() especially, we want to use the latter. Otherwise the
netdevice would be wrongly named "net_tap". Furthermore, creating
several tap vdev would point to the same netdevice.
In any case, it must to be consistent with the tun_alloc() call in
eth_dev_tap_create().
Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
---
drivers/net/tap/rte_eth_tap.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
Comments
On 1/31/2017 9:42 AM, Pascal Mazon wrote:
> dev->data->name contains "net_tap", the device driver name.
I see what patch does, just as a note to commit log:
AFAIK, "dev->data->name" is device name, and for this case it is
"net_tap#", like "net_tap0", "net_tap1" ...
"dev->data_drv_name" is the driver name which is "net_tap"
> dev->data->dev_private->name contains the actual iface name,
> e.g. "dtap0".
Right, I agree this is correct comparing "dev->data->name"
But the problem is pmd->name is per eth_dev.
If I read code correct, for multiple queue support, each queue pair will
create a tap device, so each needs a different name.
So can't just use pmd->name. Need to create a name per queue pair, it
can be combination of pmd->name + "_" + queue_id? Or can keep a name per
queue pair, instead of eth_dev.
What do you think?
>
> In tun_alloc() especially, we want to use the latter. Otherwise the
> netdevice would be wrongly named "net_tap". Furthermore, creating
> several tap vdev would point to the same netdevice.
>
> In any case, it must to be consistent with the tun_alloc() call in
> eth_dev_tap_create().
>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
<...>
On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
Pascal Mazon wrote:
>> dev->data->name contains "net_tap", the device driver name.
>
> I see what patch does, just as a note to commit log:
>
> AFAIK, "dev->data->name" is device name, and for this case it is
> "net_tap#", like "net_tap0", "net_tap1" ...
>
> "dev->data_drv_name" is the driver name which is "net_tap"
Indeed, dev->data->name is the device name, looking like "net_tap#",
with number increasing for each vdev.
I'll put the following commit log line if that's ok:
dev->data->name contains the device name, e.g. "net_tap0".
>
>> dev->data->dev_private->name contains the actual iface name,
>> e.g. "dtap0".
>
> Right, I agree this is correct comparing "dev->data->name"
>
> But the problem is pmd->name is per eth_dev.
>
> If I read code correct, for multiple queue support, each queue pair will
> create a tap device, so each needs a different name.
>
> So can't just use pmd->name. Need to create a name per queue pair, it
> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
> queue pair, instead of eth_dev.
>
> What do you think?
Actually that's not exactly how it goes.
Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
TUNSETIFF (through ioctl) on the resulting fd.
That's the important part: queues for a given tap device must set TUNSETIFF
with a common ifreq.ifr_name (in our case, pmd->name).
This is best explained in the kernel doc, there:
[1]
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>
>>
>> In tun_alloc() especially, we want to use the latter. Otherwise the
>> netdevice would be wrongly named "net_tap". Furthermore, creating
>> several tap vdev would point to the same netdevice.
>>
>> In any case, it must to be consistent with the tun_alloc() call in
>> eth_dev_tap_create().
>>
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> <...>
>
--
Pascal Mazon
www.6wind.com
> On Jan 31, 2017, at 3:42 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>
> dev->data->name contains "net_tap", the device driver name.
> dev->data->dev_private->name contains the actual iface name,
> e.g. "dtap0".
>
> In tun_alloc() especially, we want to use the latter. Otherwise the
> netdevice would be wrongly named "net_tap". Furthermore, creating
> several tap vdev would point to the same netdevice.
>
> In any case, it must to be consistent with the tun_alloc() call in
> eth_dev_tap_create().
>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 91f63f5468b2..8faf08551b9e 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
> struct pmd_internals *internals,
> uint16_t qid)
> {
> + struct pmd_internals *pmd = dev->data->dev_private;
> struct rx_queue *rx = &internals->rxq[qid];
> struct tx_queue *tx = &internals->txq[qid];
> int fd;
> @@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
> 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);
> + pmd->name, qid);
> + fd = tun_alloc(pmd->name);
> if (fd < 0) {
> - RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
> - dev->data->name);
> + RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
> return -1;
> }
> }
> @@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>
> internals->fds[rx_queue_id] = fd;
> RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
> - dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
> + internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>
> return 0;
> }
> @@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
> return -1;
>
> RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
> - dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
> + internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>
> return 0;
> }
> --
> 2.8.0.rc0
I have not looked at the code completely yet, but it seems reasonable. The only problem is I did have a cleanup patch for the TAP PMD, but Ferruh suggested it was way too many changes at this time for RC2. Are we still under that restriction or when would you suggest this be applied?
>
Regards,
Keith
On 1/31/2017 2:52 PM, Wiles, Keith wrote:
>
>> On Jan 31, 2017, at 3:42 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> dev->data->name contains "net_tap", the device driver name.
>> dev->data->dev_private->name contains the actual iface name,
>> e.g. "dtap0".
>>
>> In tun_alloc() especially, we want to use the latter. Otherwise the
>> netdevice would be wrongly named "net_tap". Furthermore, creating
>> several tap vdev would point to the same netdevice.
>>
>> In any case, it must to be consistent with the tun_alloc() call in
>> eth_dev_tap_create().
>>
>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 91f63f5468b2..8faf08551b9e 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
>> struct pmd_internals *internals,
>> uint16_t qid)
>> {
>> + struct pmd_internals *pmd = dev->data->dev_private;
>> struct rx_queue *rx = &internals->rxq[qid];
>> struct tx_queue *tx = &internals->txq[qid];
>> int fd;
>> @@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
>> 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);
>> + pmd->name, qid);
>> + fd = tun_alloc(pmd->name);
>> if (fd < 0) {
>> - RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
>> - dev->data->name);
>> + RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
>> return -1;
>> }
>> }
>> @@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>>
>> internals->fds[rx_queue_id] = fd;
>> RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
>> - dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>> + internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>>
>> return 0;
>> }
>> @@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
>> return -1;
>>
>> RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
>> - dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>> + internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>>
>> return 0;
>> }
>> --
>> 2.8.0.rc0
>
> I have not looked at the code completely yet, but it seems reasonable. The only problem is I did have a cleanup patch for the TAP PMD, but Ferruh suggested it was way too many changes at this time for RC2. Are we still under that restriction or when would you suggest this be applied?
RC2 is out now, I was aiming that tap PMD works with testpmd with RC2.
Now there is some time for RC3 and some fixes can go in, no new features
but fixes.
>
>>
>
> Regards,
> Keith
>
> On Jan 31, 2017, at 9:14 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>
> On 1/31/2017 2:52 PM, Wiles, Keith wrote:
>>
>>> On Jan 31, 2017, at 3:42 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>
>>> dev->data->name contains "net_tap", the device driver name.
>>> dev->data->dev_private->name contains the actual iface name,
>>> e.g. "dtap0".
>>>
>>> In tun_alloc() especially, we want to use the latter. Otherwise the
>>> netdevice would be wrongly named "net_tap". Furthermore, creating
>>> several tap vdev would point to the same netdevice.
>>>
>>> In any case, it must to be consistent with the tun_alloc() call in
>>> eth_dev_tap_create().
>>>
>>> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 12 ++++++------
>>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 91f63f5468b2..8faf08551b9e 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
>>> struct pmd_internals *internals,
>>> uint16_t qid)
>>> {
>>> + struct pmd_internals *pmd = dev->data->dev_private;
>>> struct rx_queue *rx = &internals->rxq[qid];
>>> struct tx_queue *tx = &internals->txq[qid];
>>> int fd;
>>> @@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
>>> 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);
>>> + pmd->name, qid);
>>> + fd = tun_alloc(pmd->name);
>>> if (fd < 0) {
>>> - RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
>>> - dev->data->name);
>>> + RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
>>> return -1;
>>> }
>>> }
>>> @@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>>>
>>> internals->fds[rx_queue_id] = fd;
>>> RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
>>> - dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>>> + internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
>>>
>>> return 0;
>>> }
>>> @@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
>>> return -1;
>>>
>>> RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
>>> - dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>>> + internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
>>>
>>> return 0;
>>> }
>>> --
>>> 2.8.0.rc0
>>
>> I have not looked at the code completely yet, but it seems reasonable. The only problem is I did have a cleanup patch for the TAP PMD, but Ferruh suggested it was way too many changes at this time for RC2. Are we still under that restriction or when would you suggest this be applied?
>
> RC2 is out now, I was aiming that tap PMD works with testpmd with RC2.
> Now there is some time for RC3 and some fixes can go in, no new features
> but fixes.
OK, I see. Let me integrate the changes suggested and see how they work with my changes. I may send a patch with a combination of the changes and see what everyone thinks.
>
>>
>>>
>>
>> Regards,
>> Keith
Regards,
Keith
On 1/31/2017 2:23 PM, Pascal Mazon wrote:
> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
> Pascal Mazon wrote:
>>> dev->data->name contains "net_tap", the device driver name.
>>
>> I see what patch does, just as a note to commit log:
>>
>> AFAIK, "dev->data->name" is device name, and for this case it is
>> "net_tap#", like "net_tap0", "net_tap1" ...
>>
>> "dev->data_drv_name" is the driver name which is "net_tap"
>
> Indeed, dev->data->name is the device name, looking like "net_tap#",
> with number increasing for each vdev.
> I'll put the following commit log line if that's ok:
>
> dev->data->name contains the device name, e.g. "net_tap0".
>
>>
>>> dev->data->dev_private->name contains the actual iface name,
>>> e.g. "dtap0".
>>
>> Right, I agree this is correct comparing "dev->data->name"
>>
>> But the problem is pmd->name is per eth_dev.
>>
>> If I read code correct, for multiple queue support, each queue pair will
>> create a tap device, so each needs a different name.
>>
>> So can't just use pmd->name. Need to create a name per queue pair, it
>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>> queue pair, instead of eth_dev.
>>
>> What do you think?
>
> Actually that's not exactly how it goes.
> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
> TUNSETIFF (through ioctl) on the resulting fd.
> That's the important part: queues for a given tap device must set TUNSETIFF
> with a common ifreq.ifr_name (in our case, pmd->name).
>
> This is best explained in the kernel doc, there:
Thank you for the clarification.
If so, why PMD keeps a fd per queue?
Overall, patch looks good except mentioned detail in commit log.
I suggest waiting Keith's patch, and rebase this set on top of it.
Thanks,
ferruh
>
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>
<...>
On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>> Pascal Mazon wrote:
>>>> dev->data->name contains "net_tap", the device driver name.
>>>
>>> I see what patch does, just as a note to commit log:
>>>
>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>
>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>
>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>> with number increasing for each vdev.
>> I'll put the following commit log line if that's ok:
>>
>> dev->data->name contains the device name, e.g. "net_tap0".
>>
>>>
>>>> dev->data->dev_private->name contains the actual iface name,
>>>> e.g. "dtap0".
>>>
>>> Right, I agree this is correct comparing "dev->data->name"
>>>
>>> But the problem is pmd->name is per eth_dev.
>>>
>>> If I read code correct, for multiple queue support, each queue pair will
>>> create a tap device, so each needs a different name.
>>>
>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>> queue pair, instead of eth_dev.
>>>
>>> What do you think?
>>
>> Actually that's not exactly how it goes.
>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>> TUNSETIFF (through ioctl) on the resulting fd.
>> That's the important part: queues for a given tap device must set TUNSETIFF
>> with a common ifreq.ifr_name (in our case, pmd->name).
>>
>> This is best explained in the kernel doc, there:
>
> Thank you for the clarification.
> If so, why PMD keeps a fd per queue?
>
> Overall, patch looks good except mentioned detail in commit log.
>
> I suggest waiting Keith's patch, and rebase this set on top of it.
>
> Thanks,
> ferruh
>
>>
>> [1]
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>
> <...>
>
I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
In these functions, we need to make our read() and write() accesses on the
right fd considering the queue we're required to process.
I'll wait for Keith's patch, then.
Best regards,
On 1/31/2017 3:30 PM, Pascal Mazon wrote:
> On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
>> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>>> Pascal Mazon wrote:
>>>>> dev->data->name contains "net_tap", the device driver name.
>>>>
>>>> I see what patch does, just as a note to commit log:
>>>>
>>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>>
>>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>>
>>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>>> with number increasing for each vdev.
>>> I'll put the following commit log line if that's ok:
>>>
>>> dev->data->name contains the device name, e.g. "net_tap0".
>>>
>>>>
>>>>> dev->data->dev_private->name contains the actual iface name,
>>>>> e.g. "dtap0".
>>>>
>>>> Right, I agree this is correct comparing "dev->data->name"
>>>>
>>>> But the problem is pmd->name is per eth_dev.
>>>>
>>>> If I read code correct, for multiple queue support, each queue pair will
>>>> create a tap device, so each needs a different name.
>>>>
>>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>>> queue pair, instead of eth_dev.
>>>>
>>>> What do you think?
>>>
>>> Actually that's not exactly how it goes.
>>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>>> TUNSETIFF (through ioctl) on the resulting fd.
>>> That's the important part: queues for a given tap device must set TUNSETIFF
>>> with a common ifreq.ifr_name (in our case, pmd->name).
>>>
>>> This is best explained in the kernel doc, there:
>>
>> Thank you for the clarification.
>> If so, why PMD keeps a fd per queue?
>>
>> Overall, patch looks good except mentioned detail in commit log.
>>
>> I suggest waiting Keith's patch, and rebase this set on top of it.
>>
>> Thanks,
>> ferruh
>>
>>>
>>> [1]
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>>
>> <...>
>>
>
> I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
> In these functions, we need to make our read() and write() accesses on the
> right fd considering the queue we're required to process.
If fd is same for all queues, it is possible to keep one instance in pmd
private_data, and access it from queues. I think it is confusing to have
multiple copy of it, but I see your point.
>
> I'll wait for Keith's patch, then.
Thanks again.
>
> Best regards,
>
> On Jan 31, 2017, at 9:38 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>
> On 1/31/2017 3:30 PM, Pascal Mazon wrote:
>> On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
>>> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>>>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>>>> Pascal Mazon wrote:
>>>>>> dev->data->name contains "net_tap", the device driver name.
>>>>>
>>>>> I see what patch does, just as a note to commit log:
>>>>>
>>>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>>>
>>>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>>>
>>>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>>>> with number increasing for each vdev.
>>>> I'll put the following commit log line if that's ok:
>>>>
>>>> dev->data->name contains the device name, e.g. "net_tap0".
>>>>
>>>>>
>>>>>> dev->data->dev_private->name contains the actual iface name,
>>>>>> e.g. "dtap0".
>>>>>
>>>>> Right, I agree this is correct comparing "dev->data->name"
>>>>>
>>>>> But the problem is pmd->name is per eth_dev.
>>>>>
>>>>> If I read code correct, for multiple queue support, each queue pair will
>>>>> create a tap device, so each needs a different name.
>>>>>
>>>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>>>> queue pair, instead of eth_dev.
>>>>>
>>>>> What do you think?
>>>>
>>>> Actually that's not exactly how it goes.
>>>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>>>> TUNSETIFF (through ioctl) on the resulting fd.
>>>> That's the important part: queues for a given tap device must set TUNSETIFF
>>>> with a common ifreq.ifr_name (in our case, pmd->name).
>>>>
>>>> This is best explained in the kernel doc, there:
>>>
>>> Thank you for the clarification.
>>> If so, why PMD keeps a fd per queue?
>>>
>>> Overall, patch looks good except mentioned detail in commit log.
>>>
>>> I suggest waiting Keith's patch, and rebase this set on top of it.
>>>
>>> Thanks,
>>> ferruh
>>>
>>>>
>>>> [1]
>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>>>
>>> <...>
>>>
>>
>> I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
>> In these functions, we need to make our read() and write() accesses on the
>> right fd considering the queue we're required to process.
>
> If fd is same for all queues, it is possible to keep one instance in pmd
> private_data, and access it from queues. I think it is confusing to have
> multiple copy of it, but I see your point.
In my changes I removed the fds[] array as it was not required only the rx/tx_queue has an fd variable.
>
>>
>> I'll wait for Keith's patch, then.
>
> Thanks again.
>
>>
>> Best regards,
Regards,
Keith
On 01/31/2017 04:44 PM, Wiles, Keith wrote:
>
>> On Jan 31, 2017, at 9:38 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>
>> On 1/31/2017 3:30 PM, Pascal Mazon wrote:
>>> On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
>>>> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>>>>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>>>>> Pascal Mazon wrote:
>>>>>>> dev->data->name contains "net_tap", the device driver name.
>>>>>>
>>>>>> I see what patch does, just as a note to commit log:
>>>>>>
>>>>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>>>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>>>>
>>>>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>>>>
>>>>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>>>>> with number increasing for each vdev.
>>>>> I'll put the following commit log line if that's ok:
>>>>>
>>>>> dev->data->name contains the device name, e.g. "net_tap0".
>>>>>
>>>>>>
>>>>>>> dev->data->dev_private->name contains the actual iface name,
>>>>>>> e.g. "dtap0".
>>>>>>
>>>>>> Right, I agree this is correct comparing "dev->data->name"
>>>>>>
>>>>>> But the problem is pmd->name is per eth_dev.
>>>>>>
>>>>>> If I read code correct, for multiple queue support, each queue pair will
>>>>>> create a tap device, so each needs a different name.
>>>>>>
>>>>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>>>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>>>>> queue pair, instead of eth_dev.
>>>>>>
>>>>>> What do you think?
>>>>>
>>>>> Actually that's not exactly how it goes.
>>>>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>>>>> TUNSETIFF (through ioctl) on the resulting fd.
>>>>> That's the important part: queues for a given tap device must set TUNSETIFF
>>>>> with a common ifreq.ifr_name (in our case, pmd->name).
>>>>>
>>>>> This is best explained in the kernel doc, there:
>>>>
>>>> Thank you for the clarification.
>>>> If so, why PMD keeps a fd per queue?
>>>>
>>>> Overall, patch looks good except mentioned detail in commit log.
>>>>
>>>> I suggest waiting Keith's patch, and rebase this set on top of it.
>>>>
>>>> Thanks,
>>>> ferruh
>>>>
>>>>>
>>>>> [1]
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>>>>
>>>> <...>
>>>>
>>>
>>> I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
>>> In these functions, we need to make our read() and write() accesses on the
>>> right fd considering the queue we're required to process.
>>
>> If fd is same for all queues, it is possible to keep one instance in pmd
>> private_data, and access it from queues. I think it is confusing to have
>> multiple copy of it, but I see your point.
>
> In my changes I removed the fds[] array as it was not required only the rx/tx_queue has an fd variable.
Sounds good to me.
Ferruh, just to make sure we're on the same page, in multiqueue, there
is definitely a different fd for each queue.
>
>>
>>>
>>> I'll wait for Keith's patch, then.
>>
>> Thanks again.
>>
>>>
>>> Best regards,
>
> Regards,
> Keith
>
> On Jan 31, 2017, at 9:44 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>
> On 01/31/2017 04:44 PM, Wiles, Keith wrote:
>>
>>> On Jan 31, 2017, at 9:38 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>>
>>> On 1/31/2017 3:30 PM, Pascal Mazon wrote:
>>>> On 01/31/2017 04:28 PM, Ferruh Yigit wrote:
>>>>> On 1/31/2017 2:23 PM, Pascal Mazon wrote:
>>>>>> On 01/31/2017 02:06 PM, Ferruh Yigit wrote:> On 1/31/2017 9:42 AM,
>>>>>> Pascal Mazon wrote:
>>>>>>>> dev->data->name contains "net_tap", the device driver name.
>>>>>>>
>>>>>>> I see what patch does, just as a note to commit log:
>>>>>>>
>>>>>>> AFAIK, "dev->data->name" is device name, and for this case it is
>>>>>>> "net_tap#", like "net_tap0", "net_tap1" ...
>>>>>>>
>>>>>>> "dev->data_drv_name" is the driver name which is "net_tap"
>>>>>>
>>>>>> Indeed, dev->data->name is the device name, looking like "net_tap#",
>>>>>> with number increasing for each vdev.
>>>>>> I'll put the following commit log line if that's ok:
>>>>>>
>>>>>> dev->data->name contains the device name, e.g. "net_tap0".
>>>>>>
>>>>>>>
>>>>>>>> dev->data->dev_private->name contains the actual iface name,
>>>>>>>> e.g. "dtap0".
>>>>>>>
>>>>>>> Right, I agree this is correct comparing "dev->data->name"
>>>>>>>
>>>>>>> But the problem is pmd->name is per eth_dev.
>>>>>>>
>>>>>>> If I read code correct, for multiple queue support, each queue pair will
>>>>>>> create a tap device, so each needs a different name.
>>>>>>>
>>>>>>> So can't just use pmd->name. Need to create a name per queue pair, it
>>>>>>> can be combination of pmd->name + "_" + queue_id? Or can keep a name per
>>>>>>> queue pair, instead of eth_dev.
>>>>>>>
>>>>>>> What do you think?
>>>>>>
>>>>>> Actually that's not exactly how it goes.
>>>>>> Adding a queue to a netdevice requires to open("/dev/net/tun") and setting
>>>>>> TUNSETIFF (through ioctl) on the resulting fd.
>>>>>> That's the important part: queues for a given tap device must set TUNSETIFF
>>>>>> with a common ifreq.ifr_name (in our case, pmd->name).
Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>>
>>>>>> This is best explained in the kernel doc, there:
>>>>>
>>>>> Thank you for the clarification.
>>>>> If so, why PMD keeps a fd per queue?
>>>>>
>>>>> Overall, patch looks good except mentioned detail in commit log.
>>>>>
>>>>> I suggest waiting Keith's patch, and rebase this set on top of it.
>>>>>
>>>>> Thanks,
>>>>> ferruh
>>>>>
>>>>>>
>>>>>> [1]
>>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/tuntap.txt#n108
>>>>>>
>>>>> <...>
>>>>>
>>>>
>>>> I would say it is because dev->{r/t}x_pkt_burst() is done per queue.
>>>> In these functions, we need to make our read() and write() accesses on the
>>>> right fd considering the queue we're required to process.
>>>
>>> If fd is same for all queues, it is possible to keep one instance in pmd
>>> private_data, and access it from queues. I think it is confusing to have
>>> multiple copy of it, but I see your point.
>>
>> In my changes I removed the fds[] array as it was not required only the rx/tx_queue has an fd variable.
>
> Sounds good to me.
>
> Ferruh, just to make sure we're on the same page, in multiqueue, there is definitely a different fd for each queue.
>
>>
>>>
>>>>
>>>> I'll wait for Keith's patch, then.
>>>
>>> Thanks again.
>>>
>>>>
>>>> Best regards,
>>
>> Regards,
>> Keith
>>
>
>
> --
> Pascal Mazon
> www.6wind.com
Regards,
Keith
On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>
Well, my patch is probably wrong.
The best option would probably be to set dev->data->dev_link.link_status
appropriately inside tap_link_set() only.
I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
(respectively DOWN in tap_dev_stop()).
If it is, however, it would be best done using tap_link_set() in those
functions.
--
Pascal Mazon
www.6wind.com
> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>
> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>
>
> Well, my patch is probably wrong.
> The best option would probably be to set dev->data->dev_link.link_status
> appropriately inside tap_link_set() only.
>
> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
> (respectively DOWN in tap_dev_stop()).
> If it is, however, it would be best done using tap_link_set() in those
> functions.
I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
My only solution I guess is to add the link up/down code to the start/stop API.
>
> --
> Pascal Mazon
> www.6wind.com
Regards,
Keith
On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>
>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>
>>
>> Well, my patch is probably wrong.
>> The best option would probably be to set dev->data->dev_link.link_status
>> appropriately inside tap_link_set() only.
>>
>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>> (respectively DOWN in tap_dev_stop()).
>> If it is, however, it would be best done using tap_link_set() in those
>> functions.
>
> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>
> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>
> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>
> My only solution I guess is to add the link up/down code to the start/stop API.
I'm not sure I understand your conclusion.
If the apps usually call start/stop only, then definitely those
functions should set the link state appropriately.
To that effect, I think it best to just call tap_link_set() in
tap_dev_start() (and similar for stopping).
Apps with just start/stop functions would get the expected behavior, and
the tap PMD would also support setting the link up/down independently,
for testpmd and ip_pipeline for example.
Does that sound fine?
>
>>
>> --
>> Pascal Mazon
>> www.6wind.com
>
> Regards,
> Keith
>
> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>
> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>
>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>
>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>
>>>
>>> Well, my patch is probably wrong.
>>> The best option would probably be to set dev->data->dev_link.link_status
>>> appropriately inside tap_link_set() only.
>>>
>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>> (respectively DOWN in tap_dev_stop()).
>>> If it is, however, it would be best done using tap_link_set() in those
>>> functions.
>>
>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>
>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>
>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>
>> My only solution I guess is to add the link up/down code to the start/stop API.
>
> I'm not sure I understand your conclusion.
> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>
> Does that sound fine?
Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.
>
>
>>
>>>
>>> --
>>> Pascal Mazon
>>> www.6wind.com
>>
>> Regards,
>> Keith
>>
>
>
> --
> Pascal Mazon
> www.6wind.com
Regards,
Keith
On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>
>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>
>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>
>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>
>>>>
>>>> Well, my patch is probably wrong.
>>>> The best option would probably be to set dev->data->dev_link.link_status
>>>> appropriately inside tap_link_set() only.
>>>>
>>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>>> (respectively DOWN in tap_dev_stop()).
>>>> If it is, however, it would be best done using tap_link_set() in those
>>>> functions.
>>>
>>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>>
>>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>>
>>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>>
>>> My only solution I guess is to add the link up/down code to the start/stop API.
>>
>> I'm not sure I understand your conclusion.
>> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
>> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
>> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>>
>> Does that sound fine?
>
> Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.
Great, that looks good to me!
Regards,
Pascal
> On Feb 1, 2017, at 9:40 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>
> On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>>
>>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>
>>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>>
>>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>>
>>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>>
>>>>>
>>>>> Well, my patch is probably wrong.
>>>>> The best option would probably be to set dev->data->dev_link.link_status
>>>>> appropriately inside tap_link_set() only.
>>>>>
>>>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>>>> (respectively DOWN in tap_dev_stop()).
>>>>> If it is, however, it would be best done using tap_link_set() in those
>>>>> functions.
>>>>
>>>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>>>
>>>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>>>
>>>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>>>
>>>> My only solution I guess is to add the link up/down code to the start/stop API.
>>>
>>> I'm not sure I understand your conclusion.
>>> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
>>> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
>>> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>>>
>>> Does that sound fine?
>>
>> Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.
>
> Great, that looks good to me!
Ferruh, Please apply Pascal’s 6 patches and I will based my changes on top of those changes. Does that sound reasonable?
>
> Regards,
> Pascal
Regards,
Keith
On 2/1/2017 3:55 PM, Wiles, Keith wrote:
>
>> On Feb 1, 2017, at 9:40 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>>>
>>>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>
>>>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>>>
>>>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>>>
>>>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>>>
>>>>>>
>>>>>> Well, my patch is probably wrong.
>>>>>> The best option would probably be to set dev->data->dev_link.link_status
>>>>>> appropriately inside tap_link_set() only.
>>>>>>
>>>>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>>>>> (respectively DOWN in tap_dev_stop()).
>>>>>> If it is, however, it would be best done using tap_link_set() in those
>>>>>> functions.
>>>>>
>>>>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>>>>
>>>>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>>>>
>>>>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>>>>
>>>>> My only solution I guess is to add the link up/down code to the start/stop API.
>>>>
>>>> I'm not sure I understand your conclusion.
>>>> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
>>>> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
>>>> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>>>>
>>>> Does that sound fine?
>>>
>>> Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.
>>
>> Great, that looks good to me!
>
> Ferruh, Please apply Pascal’s 6 patches and I will based my changes on top of those changes. Does that sound reasonable?
That is good, I will.
Pascal,
Only patch 1/6 commit log needs reworking, rest looks good, (although I
still will do one more round of basic tests). For commit log update, do
you want to send a v2 or prefer me do the update?
Thanks,
ferruh
>
>>
>> Regards,
>> Pascal
>
> Regards,
> Keith
>
On 02/01/2017 06:50 PM, Ferruh Yigit wrote:
> On 2/1/2017 3:55 PM, Wiles, Keith wrote:
>>
>>> On Feb 1, 2017, at 9:40 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>
>>> On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>>>>
>>>>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>>
>>>>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>>>>
>>>>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>>>>
>>>>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>>>>> Looking at the changes to set the link up/down and the adding the two functions. I noticed in the stop/start routines I was set the link in DPDK and not adjusting the interface link. Should the stop/start routine also do the same thing?
>>>>>>>>
>>>>>>>
>>>>>>> Well, my patch is probably wrong.
>>>>>>> The best option would probably be to set dev->data->dev_link.link_status
>>>>>>> appropriately inside tap_link_set() only.
>>>>>>>
>>>>>>> I'm not sure it's compulsory to actually set the link UP in tap_dev_start()
>>>>>>> (respectively DOWN in tap_dev_stop()).
>>>>>>> If it is, however, it would be best done using tap_link_set() in those
>>>>>>> functions.
>>>>>>
>>>>>> I was setting the link up/down in both places in the old code. The gotta is link up/down came later (I guess) and applications only call start/stop. In the other drivers like ring the like they tend to set link in start/stop and in link up/down, which is what I patterned my driver on.
>>>>>>
>>>>>> I looked around and the only applications calling link up/down was testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs to not suggest that link up/down be used it appears start/stop must set the Link state and the developer can call link up/down APIs if needed for others reasons.
>>>>>>
>>>>>> I assume the link up/down only effects the link state and the start/stop is creating/destroying resources.
>>>>>>
>>>>>> My only solution I guess is to add the link up/down code to the start/stop API.
>>>>>
>>>>> I'm not sure I understand your conclusion.
>>>>> If the apps usually call start/stop only, then definitely those functions should set the link state appropriately.
>>>>> To that effect, I think it best to just call tap_link_set() in tap_dev_start() (and similar for stopping).
>>>>> Apps with just start/stop functions would get the expected behavior, and the tap PMD would also support setting the link up/down independently, for testpmd and ip_pipeline for example.
>>>>>
>>>>> Does that sound fine?
>>>>
>>>> Yes, this was what I was trying to say and calling tap_link_set() in tap_dev_start() is the solution.
>>>
>>> Great, that looks good to me!
>>
>> Ferruh, Please apply Pascal’s 6 patches and I will based my changes on top of those changes. Does that sound reasonable?
>
> That is good, I will.
>
> Pascal,
>
> Only patch 1/6 commit log needs reworking, rest looks good, (although I
> still will do one more round of basic tests). For commit log update, do
> you want to send a v2 or prefer me do the update?
I'd prefer if you did the update. Thank you!
Pascal
>
> Thanks,
> ferruh
>
>>
>>>
>>> Regards,
>>> Pascal
>>
>> Regards,
>> Keith
>>
>
On Thu, Feb 2, 2017 at 9:05 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> On 02/01/2017 06:50 PM, Ferruh Yigit wrote:
>
>> On 2/1/2017 3:55 PM, Wiles, Keith wrote:
>>
>>>
>>> On Feb 1, 2017, at 9:40 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>>
>>>> On 02/01/2017 04:25 PM, Wiles, Keith wrote:
>>>>
>>>>>
>>>>> On Feb 1, 2017, at 2:11 AM, Pascal Mazon <pascal.mazon@6wind.com>
>>>>>> wrote:
>>>>>>
>>>>>> On 02/01/2017 12:29 AM, Wiles, Keith wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jan 31, 2017, at 10:39 AM, Pascal Mazon <pascal.mazon@6wind.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>> On 01/31/2017 05:06 PM, Wiles, Keith wrote:>
>>>>>>>>
>>>>>>>>> Looking at the changes to set the link up/down and the adding the
>>>>>>>>> two functions. I noticed in the stop/start routines I was set the link in
>>>>>>>>> DPDK and not adjusting the interface link. Should the stop/start routine
>>>>>>>>> also do the same thing?
>>>>>>>>>
>>>>>>>>>
>>>>>>>> Well, my patch is probably wrong.
>>>>>>>> The best option would probably be to set
>>>>>>>> dev->data->dev_link.link_status
>>>>>>>> appropriately inside tap_link_set() only.
>>>>>>>>
>>>>>>>> I'm not sure it's compulsory to actually set the link UP in
>>>>>>>> tap_dev_start()
>>>>>>>> (respectively DOWN in tap_dev_stop()).
>>>>>>>> If it is, however, it would be best done using tap_link_set() in
>>>>>>>> those
>>>>>>>> functions.
>>>>>>>>
>>>>>>>
>>>>>>> I was setting the link up/down in both places in the old code. The
>>>>>>> gotta is link up/down came later (I guess) and applications only call
>>>>>>> start/stop. In the other drivers like ring the like they tend to set link
>>>>>>> in start/stop and in link up/down, which is what I patterned my driver on.
>>>>>>>
>>>>>>> I looked around and the only applications calling link up/down was
>>>>>>> testpmd and ip_pipeline, but all of the apps call start/stop. Even the docs
>>>>>>> to not suggest that link up/down be used it appears start/stop must set the
>>>>>>> Link state and the developer can call link up/down APIs if needed for
>>>>>>> others reasons.
>>>>>>>
>>>>>>> I assume the link up/down only effects the link state and the
>>>>>>> start/stop is creating/destroying resources.
>>>>>>>
>>>>>>> My only solution I guess is to add the link up/down code to the
>>>>>>> start/stop API.
>>>>>>>
>>>>>>
>>>>>> I'm not sure I understand your conclusion.
>>>>>> If the apps usually call start/stop only, then definitely those
>>>>>> functions should set the link state appropriately.
>>>>>> To that effect, I think it best to just call tap_link_set() in
>>>>>> tap_dev_start() (and similar for stopping).
>>>>>> Apps with just start/stop functions would get the expected behavior,
>>>>>> and the tap PMD would also support setting the link up/down independently,
>>>>>> for testpmd and ip_pipeline for example.
>>>>>>
>>>>>> Does that sound fine?
>>>>>>
>>>>>
>>>>> Yes, this was what I was trying to say and calling tap_link_set() in
>>>>> tap_dev_start() is the solution.
>>>>>
>>>>
>>>> Great, that looks good to me!
>>>>
>>>
>>> Ferruh, Please apply Pascal’s 6 patches and I will based my changes on
>>> top of those changes. Does that sound reasonable?
>>>
>>
>> That is good, I will.
>>
>> Pascal,
>>
>> Only patch 1/6 commit log needs reworking, rest looks good, (although I
>> still will do one more round of basic tests). For commit log update, do
>> you want to send a v2 or prefer me do the update?
>>
>
> I'd prefer if you did the update. Thank you!
>
> Pascal
Actually, I'm working on a patch to implement promiscuous_enable/disable
and allmulticast_enable/disable.
I'll have to change the tap_link_set() to be more generic and support the
appropriate flags.
So I'll send a v2 of my patches with patch 1/6 commit-log updated, and the
promisc allmulti support, later today.
Forget my previous email, please.
Regards,
Pascal
>
>
>
>> Thanks,
>> ferruh
>>
>>
>>>
>>>> Regards,
>>>> Pascal
>>>>
>>>
>>> Regards,
>>> Keith
>>>
>>>
On 2/2/2017 8:25 AM, Pascal Mazon wrote:
<...>
>
> Actually, I'm working on a patch to implement promiscuous_enable/disable
> and allmulticast_enable/disable.
> I'll have to change the tap_link_set() to be more generic and support
> the appropriate flags.
> So I'll send a v2 of my patches with patch 1/6 commit-log updated, and
> the promisc allmulti support, later today.
OK, thanks.
> On Jan 31, 2017, at 3:42 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>
> dev->data->name contains "net_tap", the device driver name.
> dev->data->dev_private->name contains the actual iface name,
> e.g. "dtap0".
>
> In tun_alloc() especially, we want to use the latter. Otherwise the
> netdevice would be wrongly named "net_tap". Furthermore, creating
> several tap vdev would point to the same netdevice.
>
> In any case, it must to be consistent with the tun_alloc() call in
> eth_dev_tap_create().
>
> Signed-off-by: Pascal Mazon <pascal.mazon@6wind.com>
> —
Acked by Keith Wiles <keith.wiles@intel.com> for the series 1-6
Regards,
Keith
@@ -410,6 +410,7 @@ tap_setup_queue(struct rte_eth_dev *dev,
struct pmd_internals *internals,
uint16_t qid)
{
+ struct pmd_internals *pmd = dev->data->dev_private;
struct rx_queue *rx = &internals->rxq[qid];
struct tx_queue *tx = &internals->txq[qid];
int fd;
@@ -419,11 +420,10 @@ tap_setup_queue(struct rte_eth_dev *dev,
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);
+ pmd->name, qid);
+ fd = tun_alloc(pmd->name);
if (fd < 0) {
- RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n",
- dev->data->name);
+ RTE_LOG(ERR, PMD, "tun_alloc(%s) failed\n", pmd->name);
return -1;
}
}
@@ -493,7 +493,7 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
internals->fds[rx_queue_id] = fd;
RTE_LOG(INFO, PMD, "RX TAP device name %s, qid %d on fd %d\n",
- dev->data->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
+ internals->name, rx_queue_id, internals->rxq[rx_queue_id].fd);
return 0;
}
@@ -516,7 +516,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
return -1;
RTE_LOG(INFO, PMD, "TX TAP device name %s, qid %d on fd %d\n",
- dev->data->name, tx_queue_id, internals->txq[tx_queue_id].fd);
+ internals->name, tx_queue_id, internals->txq[tx_queue_id].fd);
return 0;
}