[dpdk-dev,v1,net/tap] add logic to assign speed from user
Checks
Commit Message
TAP speed is passed as user argument, but never set to interface.
New logic brings speed get and set to LOCAL and REMOTE interfaces.
Updated the default PMD speeed to 10M as per Linux Kernel default
value.
Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
---
drivers/net/tap/rte_eth_tap.c | 185 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 182 insertions(+), 3 deletions(-)
Comments
> On Dec 21, 2017, at 10:53 AM, Vipin Varghese <vipin.varghese@intel.com> wrote:
>
> TAP speed is passed as user argument, but never set to interface.
> New logic brings speed get and set to LOCAL and REMOTE interfaces.
>
> Updated the default PMD speeed to 10M as per Linux Kernel default
> value.
The problem in setting the link speed to 10M is that TAP will not limit its traffic to 10M. Applications like pktgen and others use the Link speed to calculate the bit rate, which will be broken now.
I would suggest making the default value 10G or 40G instead as CPU speeds will continue to increase. Forcing someone to always add the link speed seems a bit much when we know the systems can send/receive much higher then 10M, which is the reason 10G was picked. Please set the default back to 10G or some much higher number.
>
> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
> ---
> drivers/net/tap/rte_eth_tap.c | 185 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 182 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 6b27679..7238504 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -62,6 +62,8 @@
> #include <linux/if_ether.h>
> #include <linux/version.h>
> #include <fcntl.h>
> +#include <linux/ethtool.h>
> +#include <linux/sockios.h>
>
Regards,
Keith
On 12/21/2017 1:38 PM, Wiles, Keith wrote:
>
>
>> On Dec 21, 2017, at 10:53 AM, Vipin Varghese <vipin.varghese@intel.com> wrote:
>>
>> TAP speed is passed as user argument, but never set to interface.
>> New logic brings speed get and set to LOCAL and REMOTE interfaces.
>>
>> Updated the default PMD speeed to 10M as per Linux Kernel default
>> value.
>
> The problem in setting the link speed to 10M is that TAP will not limit its traffic to 10M. Applications like pktgen and others use the Link speed to calculate the bit rate, which will be broken now.
>
> I would suggest making the default value 10G or 40G instead as CPU speeds will continue to increase. Forcing someone to always add the link speed seems a bit much when we know the systems can send/receive much higher then 10M, which is the reason 10G was picked. Please set the default back to 10G or some much higher number.
Hi Keith, Vipin, Pascal,
Since we really can't set the interface for Linux tap interface, what do you
think removing speed arg completely from tap PMD?
Thanks,
ferruh
>
>>
>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
>> ---
>> drivers/net/tap/rte_eth_tap.c | 185 +++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 182 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 6b27679..7238504 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -62,6 +62,8 @@
>> #include <linux/if_ether.h>
>> #include <linux/version.h>
>> #include <fcntl.h>
>> +#include <linux/ethtool.h>
>> +#include <linux/sockios.h>
>>
>
> Regards,
> Keith
>
> On Jan 17, 2018, at 10:06 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>
> On 12/21/2017 1:38 PM, Wiles, Keith wrote:
>>
>>
>>> On Dec 21, 2017, at 10:53 AM, Vipin Varghese <vipin.varghese@intel.com> wrote:
>>>
>>> TAP speed is passed as user argument, but never set to interface.
>>> New logic brings speed get and set to LOCAL and REMOTE interfaces.
>>>
>>> Updated the default PMD speeed to 10M as per Linux Kernel default
>>> value.
>>
>> The problem in setting the link speed to 10M is that TAP will not limit its traffic to 10M. Applications like pktgen and others use the Link speed to calculate the bit rate, which will be broken now.
>>
>> I would suggest making the default value 10G or 40G instead as CPU speeds will continue to increase. Forcing someone to always add the link speed seems a bit much when we know the systems can send/receive much higher then 10M, which is the reason 10G was picked. Please set the default back to 10G or some much higher number.
>
> Hi Keith, Vipin, Pascal,
>
> Since we really can't set the interface for Linux tap interface, what do you
> think removing speed arg completely from tap PMD?
I have no problems with it being remove from the PMD. The only problem is this being a virtual interface it can be any speed, but 10M is unreasonable IMO. I would like it to be set to something reasonable as the default (40G or 10G) or would zero be more reasonable.
I know the reported speed does not effect the performance, but tools that look at the speed and attempt to use that speed need a value greater then the max bit rate of the interface IMO. If we had a value to indicate a bogus speed then maybe the tools can adjust in some way.
>
> Thanks,
> ferruh
>
>>
>>>
>>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
>>> ---
>>> drivers/net/tap/rte_eth_tap.c | 185 +++++++++++++++++++++++++++++++++++++++++-
>>> 1 file changed, 182 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 6b27679..7238504 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -62,6 +62,8 @@
>>> #include <linux/if_ether.h>
>>> #include <linux/version.h>
>>> #include <fcntl.h>
>>> +#include <linux/ethtool.h>
>>> +#include <linux/sockios.h>
>>>
>>
>> Regards,
>> Keith
Regards,
Keith
On 1/17/2018 4:14 PM, Wiles, Keith wrote:
>
>
>> On Jan 17, 2018, at 10:06 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>
>> On 12/21/2017 1:38 PM, Wiles, Keith wrote:
>>>
>>>
>>>> On Dec 21, 2017, at 10:53 AM, Vipin Varghese <vipin.varghese@intel.com> wrote:
>>>>
>>>> TAP speed is passed as user argument, but never set to interface.
>>>> New logic brings speed get and set to LOCAL and REMOTE interfaces.
>>>>
>>>> Updated the default PMD speeed to 10M as per Linux Kernel default
>>>> value.
>>>
>>> The problem in setting the link speed to 10M is that TAP will not limit its traffic to 10M. Applications like pktgen and others use the Link speed to calculate the bit rate, which will be broken now.
>>>
>>> I would suggest making the default value 10G or 40G instead as CPU speeds will continue to increase. Forcing someone to always add the link speed seems a bit much when we know the systems can send/receive much higher then 10M, which is the reason 10G was picked. Please set the default back to 10G or some much higher number.
>>
>> Hi Keith, Vipin, Pascal,
>>
>> Since we really can't set the interface for Linux tap interface, what do you
>> think removing speed arg completely from tap PMD?
>
> I have no problems with it being remove from the PMD. The only problem is this being a virtual interface it can be any speed, but 10M is unreasonable IMO. I would like it to be set to something reasonable as the default (40G or 10G) or would zero be more reasonable.
Agreed.
When "speed" arg removed I think there is no reason to change default speed, it
can stay as 10G as it is now.
>
> I know the reported speed does not effect the performance, but tools that look at the speed and attempt to use that speed need a value greater then the max bit rate of the interface IMO. If we had a value to indicate a bogus speed then maybe the tools can adjust in some way.
>
>>
>> Thanks,
>> ferruh
>>
>>>
>>>>
>>>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
>>>> ---
>>>> drivers/net/tap/rte_eth_tap.c | 185 +++++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 182 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>> index 6b27679..7238504 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -62,6 +62,8 @@
>>>> #include <linux/if_ether.h>
>>>> #include <linux/version.h>
>>>> #include <fcntl.h>
>>>> +#include <linux/ethtool.h>
>>>> +#include <linux/sockios.h>
>>>>
>>>
>>> Regards,
>>> Keith
>
> Regards,
> Keith
>
I have no objection to having the 10G default speed on tap.
On 17/01/2018 17:22, Ferruh Yigit wrote:
> On 1/17/2018 4:14 PM, Wiles, Keith wrote:
>>
>>> On Jan 17, 2018, at 10:06 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>>>
>>> On 12/21/2017 1:38 PM, Wiles, Keith wrote:
>>>>
>>>>> On Dec 21, 2017, at 10:53 AM, Vipin Varghese <vipin.varghese@intel.com> wrote:
>>>>>
>>>>> TAP speed is passed as user argument, but never set to interface.
>>>>> New logic brings speed get and set to LOCAL and REMOTE interfaces.
>>>>>
>>>>> Updated the default PMD speeed to 10M as per Linux Kernel default
>>>>> value.
>>>> The problem in setting the link speed to 10M is that TAP will not limit its traffic to 10M. Applications like pktgen and others use the Link speed to calculate the bit rate, which will be broken now.
>>>>
>>>> I would suggest making the default value 10G or 40G instead as CPU speeds will continue to increase. Forcing someone to always add the link speed seems a bit much when we know the systems can send/receive much higher then 10M, which is the reason 10G was picked. Please set the default back to 10G or some much higher number.
>>> Hi Keith, Vipin, Pascal,
>>>
>>> Since we really can't set the interface for Linux tap interface, what do you
>>> think removing speed arg completely from tap PMD?
>> I have no problems with it being remove from the PMD. The only problem is this being a virtual interface it can be any speed, but 10M is unreasonable IMO. I would like it to be set to something reasonable as the default (40G or 10G) or would zero be more reasonable.
> Agreed.
> When "speed" arg removed I think there is no reason to change default speed, it
> can stay as 10G as it is now.
>
>> I know the reported speed does not effect the performance, but tools that look at the speed and attempt to use that speed need a value greater then the max bit rate of the interface IMO. If we had a value to indicate a bogus speed then maybe the tools can adjust in some way.
>>
>>> Thanks,
>>> ferruh
>>>
>>>>> Signed-off-by: Vipin Varghese <vipin.varghese@intel.com>
>>>>> ---
>>>>> drivers/net/tap/rte_eth_tap.c | 185 +++++++++++++++++++++++++++++++++++++++++-
>>>>> 1 file changed, 182 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>>> index 6b27679..7238504 100644
>>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>>> @@ -62,6 +62,8 @@
>>>>> #include <linux/if_ether.h>
>>>>> #include <linux/version.h>
>>>>> #include <fcntl.h>
>>>>> +#include <linux/ethtool.h>
>>>>> +#include <linux/sockios.h>
>>>>>
>>>> Regards,
>>>> Keith
>> Regards,
>> Keith
>>
@@ -62,6 +62,8 @@
#include <linux/if_ether.h>
#include <linux/version.h>
#include <fcntl.h>
+#include <linux/ethtool.h>
+#include <linux/sockios.h>
#include <rte_eth_tap.h>
#include <tap_flow.h>
@@ -96,7 +98,7 @@
static volatile uint32_t tap_trigger; /* Rx trigger */
static struct rte_eth_link pmd_link = {
- .link_speed = ETH_SPEED_NUM_10G,
+ .link_speed = ETH_SPEED_NUM_10M,
.link_duplex = ETH_LINK_FULL_DUPLEX,
.link_status = ETH_LINK_DOWN,
.link_autoneg = ETH_LINK_SPEED_AUTONEG
@@ -1229,6 +1231,178 @@ enum ioctl_mode {
};
static int
+tap_speed_set(struct pmd_internals *pmd, const char *iface_name,
+enum ioctl_mode mode, uint32_t speed)
+{
+ int err;
+ struct ifreq ifr;
+ struct ethtool_cmd edataCmd = {0};
+#ifdef ETHTOOL_SLINKSETTINGS
+ struct ethtool_link_settings edataLink = {0};
+#endif
+
+ if ((pmd == NULL) || (iface_name == NULL) ||
+ (speed == ETH_SPEED_NUM_NONE))
+ return -1;
+
+ RTE_LOG(DEBUG, PMD,
+ "SET speed %u for %s iface_name %s\n",
+ speed,
+ (mode == REMOTE_ONLY)?"REMOTE":"LOCAL",
+ iface_name);
+
+ switch (speed) {
+ case (ETH_SPEED_NUM_10M):
+ case (ETH_SPEED_NUM_100M):
+ case (ETH_SPEED_NUM_1G):
+ case (ETH_SPEED_NUM_2_5G):
+ case (ETH_SPEED_NUM_5G):
+ case (ETH_SPEED_NUM_10G):
+ case (ETH_SPEED_NUM_20G):
+ case (ETH_SPEED_NUM_25G):
+ case (ETH_SPEED_NUM_40G):
+ case (ETH_SPEED_NUM_50G):
+ case (ETH_SPEED_NUM_56G):
+ case (ETH_SPEED_NUM_100G):
+ break;
+
+ default:
+ RTE_LOG(ERR, PMD,
+ "Unsupported SET speed %u for %s iface_name %s.\n",
+ speed,
+ (mode == REMOTE_ONLY)?"REMOTE":"LOCAL",
+ iface_name);
+
+ return -2;
+ }
+
+#ifdef ETHTOOL_SLINKSETTINGS
+ edataLink.speed = speed;
+ edataLink.cmd = ETHTOOL_SLINKSETTINGS;
+ ifr.ifr_data = (caddr_t)&edataLink;
+
+ err = tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0,
+ (mode == REMOTE_ONLY)?REMOTE_ONLY:LOCAL_ONLY);
+ if (err == 0)
+ return speed;
+#endif
+
+ edataCmd.speed = speed;
+ edataCmd.cmd = ETHTOOL_SSET;
+ ifr.ifr_data = (caddr_t)&edataCmd;
+
+ err = tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0,
+ (mode == REMOTE_ONLY)?REMOTE_ONLY:LOCAL_ONLY);
+
+ if (err == 0)
+ return speed;
+
+ RTE_LOG(ERR, PMD,
+ "Failed to SET speed %u for %s iface_name %s.\n",
+ speed, (mode == REMOTE_ONLY)?"REMOTE":"LOCAL", iface_name);
+
+ return -2;
+}
+
+static int
+tap_speed_get(struct pmd_internals *pmd, const char *iface_name,
+enum ioctl_mode mode)
+{
+ int err;
+ struct ifreq ifr;
+ struct ethtool_cmd edataCmd = {0};
+#ifdef ETHTOOL_GLINKSETTINGS
+ struct ethtool_link_settings edataLink = {0};
+#endif
+
+ if ((pmd == NULL) || (iface_name == NULL))
+ return -1;
+
+ RTE_LOG(DEBUG, PMD,
+ "Retrive speed for %s iface_name %s.\n",
+ (mode == REMOTE_ONLY)?"REMOTE":"LOCAL", iface_name);
+
+#ifdef ETHTOOL_GLINKSETTINGS
+ edataLink.speed = 0;
+ edataLink.cmd = ETHTOOL_GLINKSETTINGS;
+ ifr.ifr_data = (caddr_t)&edataLink;
+
+ err = tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0,
+ (mode == REMOTE_ONLY)?REMOTE_ONLY:LOCAL_ONLY);
+
+ if ((err == 0) && (edataLink.speed))
+ return edataLink.speed;
+#endif
+
+ edataCmd.speed = 0;
+ edataCmd.cmd = ETHTOOL_GSET;
+ ifr.ifr_data = (caddr_t)&edataCmd;
+
+ err = tap_ioctl(pmd, SIOCETHTOOL, &ifr, 0,
+ (mode == REMOTE_ONLY)?REMOTE_ONLY:LOCAL_ONLY);
+ if ((err == 0) && (edataCmd.speed))
+ return edataCmd.speed;
+
+ RTE_LOG(ERR, PMD,
+ "Failed to retrive speed for %s iface_name %s.\n",
+ (mode == REMOTE_ONLY)?"REMOTE":"LOCAL", iface_name);
+
+ return -2;
+}
+
+static int
+eth_dev_tap_config_speed(struct pmd_internals *pmd)
+{
+ int err = 0, remoteSpeed = 0, localSpeed = 0;
+
+ err = tap_speed_get(pmd,
+ pmd->name, LOCAL_ONLY);
+ if (err <= ETH_SPEED_NUM_NONE)
+ return err;
+ localSpeed = err;
+
+ RTE_LOG(DEBUG, PMD,
+ "Local interface %s speed got %d desired %u\n",
+ pmd->name, localSpeed, pmd_link.link_speed);
+
+ if (localSpeed != (int) pmd_link.link_speed) {
+ err = tap_speed_set(pmd, pmd->name, LOCAL_ONLY,
+ pmd_link.link_speed);
+ if (err <= ETH_SPEED_NUM_NONE)
+ return 0;
+ localSpeed = err;
+ }
+ pmd_link.link_speed = (unsigned int) localSpeed;
+
+ RTE_LOG(INFO, PMD,
+ "speed %u for ifindex for %s.\n",
+ pmd_link.link_speed, pmd->name);
+
+ /*If remote interface is passed as argument, update speed*/
+ if (strlen(pmd->remote_iface)) {
+ err = tap_speed_get(pmd, pmd->remote_iface, REMOTE_ONLY);
+ if (err <= ETH_SPEED_NUM_NONE)
+ return err;
+ remoteSpeed = (unsigned int) err;
+
+ if ((unsigned int) remoteSpeed != pmd_link.link_speed) {
+ err = tap_speed_set(pmd, pmd->remote_iface,
+ REMOTE_ONLY, pmd_link.link_speed);
+ if (err <= ETH_SPEED_NUM_NONE)
+ return 0;
+
+ remoteSpeed = (int) pmd_link.link_speed;
+ }
+
+ RTE_LOG(INFO, PMD,
+ "Remote ifindex %s; speed %u matches to PMD\n",
+ pmd->remote_iface, remoteSpeed);
+ }
+
+ return pmd_link.link_speed;
+}
+
+static int
eth_dev_tap_create(struct rte_vdev_device *vdev, char *tap_name,
char *remote_iface, int fixed_mac_type)
{
@@ -1403,6 +1577,11 @@ enum ioctl_mode {
}
}
+ if (eth_dev_tap_config_speed(pmd) > 0)
+ data->dev_link = pmd_link;
+ else
+ goto error_exit;
+
return 0;
disable_rte_flow:
@@ -1448,7 +1627,7 @@ enum ioctl_mode {
const char *value,
void *extra_args)
{
- *(int *)extra_args = (value) ? atoi(value) : ETH_SPEED_NUM_10G;
+ *(int *)extra_args = (value) ? atoi(value) : ETH_SPEED_NUM_10M;
return 0;
}
@@ -1493,7 +1672,7 @@ enum ioctl_mode {
name = rte_vdev_device_name(dev);
params = rte_vdev_device_args(dev);
- speed = ETH_SPEED_NUM_10G;
+ speed = ETH_SPEED_NUM_10M;
snprintf(tap_name, sizeof(tap_name), "%s%d",
DEFAULT_TAP_NAME, tap_unit++);
memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);