[dpdk-dev] net/tap: add support for fixed mac addresses
Checks
Commit Message
Support for a fixed MAC address for testing with the last octet
incrementing by one for each interface defined with the new 'mac=fixed'
string on the --vdev option. The default option is still to randomize
the MAC address for each tap interface.
Signed-off-by: Keith Wiles <keith.wiles@intel.com>
---
doc/guides/nics/tap.rst | 13 +++++++++++-
drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
2 files changed, 58 insertions(+), 4 deletions(-)
Comments
Hi Keith,
I have a few comments on your patch, see inline.
On Mon, 10 Apr 2017 13:18:50 -0500
Keith Wiles <keith.wiles@intel.com> wrote:
> Support for a fixed MAC address for testing with the last octet
> incrementing by one for each interface defined with the new 'mac=fixed'
> string on the --vdev option. The default option is still to randomize
> the MAC address for each tap interface.
>
> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> ---
> doc/guides/nics/tap.rst | 13 +++++++++++-
> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
> 2 files changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> index 5c5ba5357..e3819836a 100644
> --- a/doc/guides/nics/tap.rst
> +++ b/doc/guides/nics/tap.rst
> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
> along with being able to be used as a network connection to the DPDK
> application. The method enable one or more interfaces is to use the
> ``--vdev=net_tap0`` option on the DPDK application command line. Each
> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
> and so on.
>
> The interface name can be changed by adding the ``iface=foo0``, for example::
> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
>
> --vdev=net_tap0,iface=foo0,speed=25000
>
> +Normally the PMD will generate random MAC address, but when testing or with
"random MAC" -> "a random MAC"
> +a static configurations the developer may need a fixed MAC address style.
"configurations" -> "configuration"
> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
> +
> + --vdev=net_tap0,mac=fixed
> +
> +The MAC address will be fixed value with the last octet incrementing by one
"be" -> "have a"
> +each time for each interface string containing ``mac=fixed``. The MAC address
"each time" -> ""
> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
" convert" -> ". Convert"
> +and you get ``00:64:74:61:70:[00-FF]``.
> +
> It is possible to specify a remote netdevice to capture packets from by adding
> ``remote=foo1``, for example::
>
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 347a80741..7a676c588 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -1,7 +1,7 @@
> /*-
> * BSD LICENSE
> *
> - * Copyright(c) 2016 Intel Corporation. All rights reserved.
> + * Copyright(c) 2017 Intel Corporation. All rights reserved.
Shouldn't it be "2016-2017"?
> * All rights reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> @@ -71,6 +71,13 @@
> #define ETH_TAP_IFACE_ARG "iface"
> #define ETH_TAP_SPEED_ARG "speed"
> #define ETH_TAP_REMOTE_ARG "remote"
> +#define ETH_TAP_MAC_ARG "mac"
You used tabs instead of spaces.
> +
> +#ifdef IFF_MULTI_QUEUE
> +#define RTE_PMD_TAP_MAX_QUEUES 16
> +#else
> +#define RTE_PMD_TAP_MAX_QUEUES 1
> +#endif
Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
>
> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
> ETH_TAP_IFACE_ARG,
> ETH_TAP_SPEED_ARG,
> ETH_TAP_REMOTE_ARG,
> + ETH_TAP_MAC_ARG,
> NULL
> };
>
> static int tap_unit;
> +static int fixed_mac_type;
There is no need for a global variable, especially as the value should not be the same for each driver instance.
Typically when one tap uses "mac=fixed" and the next one doesn't.
More comments bellow for that.
>
> static volatile uint32_t tap_trigger; /* Rx trigger */
>
> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
> rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
> ETHER_ADDR_LEN);
> } else {
> - eth_random_addr((uint8_t *)&pmd->eth_addr);
> + if (fixed_mac_type) {
> + static int iface_idx;
> +
> + pmd->eth_addr.addr_bytes[0] = 0x00;
> + pmd->eth_addr.addr_bytes[1] = 'd';
> + pmd->eth_addr.addr_bytes[2] = 't';
> + pmd->eth_addr.addr_bytes[3] = 'a';
> + pmd->eth_addr.addr_bytes[4] = 'p';
> + pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
> + } else
> + eth_random_addr((uint8_t *)&pmd->eth_addr);
To avoid checkpatch warning, use else { }.
> }
>
> return 0;
> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
> return 0;
> }
>
> +static int
> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
> +{
> + /* Assume random mac address */
> + *(int *)extra_args = 0;
With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.
> + if (value && !strcasecmp("fixed", value))
I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?
> + *(int *)extra_args = 1;
> + return 0;
> +}
> +
> /* Open a TAP interface device.
> */
> static int
> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
> DEFAULT_TAP_NAME, tap_unit++);
> memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
>
> + fixed_mac_type = 0;
Turn fixed_mac_type to an automatic variable, local to this function.
And add the argument to eth_dev_tap_create().
> if (params && (params[0] != '\0')) {
> RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
>
> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
> if (ret == -1)
> goto leave;
> }
> +
> + if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
> + ret = rte_kvargs_process(kvlist,
> + ETH_TAP_MAC_ARG,
> + &set_mac_type,
> + &fixed_mac_type);
> + if (ret == -1)
> + goto leave;
> + }
> }
> }
> pmd_link.link_speed = speed;
> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
> };
> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
> + "iface=<string>,"
> + "speed=N,"
> + "remote=<string>,"
> + "mac=fixed");
Indeed, I forgot to update that when I introduced the remote!
That's it for me,
Thank you.
Pascal
On 4/11/2017 8:18 AM, Pascal Mazon wrote:
> Hi Keith,
>
> I have a few comments on your patch, see inline.
>
> On Mon, 10 Apr 2017 13:18:50 -0500
> Keith Wiles <keith.wiles@intel.com> wrote:
>
>> Support for a fixed MAC address for testing with the last octet
>> incrementing by one for each interface defined with the new 'mac=fixed'
>> string on the --vdev option. The default option is still to randomize
>> the MAC address for each tap interface.
>>
>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
<...>
>> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>> + "iface=<string>,"
>> + "speed=N,"
>> + "remote=<string>,"
>> + "mac=fixed");
>
> Indeed, I forgot to update that when I introduced the remote!
Hi Pascal,
If you can send a patch for this, it can be quickly merged, so it won't
have to be in this patch.
>
> That's it for me,
> Thank you.
>
> Pascal
>
> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>
> Hi Keith,
>
> I have a few comments on your patch, see inline.
>
> On Mon, 10 Apr 2017 13:18:50 -0500
> Keith Wiles <keith.wiles@intel.com> wrote:
>
>> Support for a fixed MAC address for testing with the last octet
>> incrementing by one for each interface defined with the new 'mac=fixed'
>> string on the --vdev option. The default option is still to randomize
>> the MAC address for each tap interface.
>>
>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>> ---
>> doc/guides/nics/tap.rst | 13 +++++++++++-
>> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
>> 2 files changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
>> index 5c5ba5357..e3819836a 100644
>> --- a/doc/guides/nics/tap.rst
>> +++ b/doc/guides/nics/tap.rst
>> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
>> along with being able to be used as a network connection to the DPDK
>> application. The method enable one or more interfaces is to use the
>> ``--vdev=net_tap0`` option on the DPDK application command line. Each
>> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
>> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
>> and so on.
>>
>> The interface name can be changed by adding the ``iface=foo0``, for example::
>> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
>>
>> --vdev=net_tap0,iface=foo0,speed=25000
>>
>> +Normally the PMD will generate random MAC address, but when testing or with
>
> "random MAC" -> "a random MAC"
>
>> +a static configurations the developer may need a fixed MAC address style.
>
> "configurations" -> "configuration"
>
>> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
>> +
>> + --vdev=net_tap0,mac=fixed
>> +
>> +The MAC address will be fixed value with the last octet incrementing by one
>
> "be" -> "have a"
>
>> +each time for each interface string containing ``mac=fixed``. The MAC address
>
> "each time" -> ""
>
>> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
>
> " convert" -> ". Convert"
>
>> +and you get ``00:64:74:61:70:[00-FF]``.
>> +
>> It is possible to specify a remote netdevice to capture packets from by adding
>> ``remote=foo1``, for example::
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 347a80741..7a676c588 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -1,7 +1,7 @@
>> /*-
>> * BSD LICENSE
>> *
>> - * Copyright(c) 2016 Intel Corporation. All rights reserved.
>> + * Copyright(c) 2017 Intel Corporation. All rights reserved.
>
> Shouldn't it be "2016-2017"?
>
>> * All rights reserved.
>> *
>> * Redistribution and use in source and binary forms, with or without
>> @@ -71,6 +71,13 @@
>> #define ETH_TAP_IFACE_ARG "iface"
>> #define ETH_TAP_SPEED_ARG "speed"
>> #define ETH_TAP_REMOTE_ARG "remote"
>> +#define ETH_TAP_MAC_ARG "mac"
>
> You used tabs instead of spaces.
>
>> +
>> +#ifdef IFF_MULTI_QUEUE
>> +#define RTE_PMD_TAP_MAX_QUEUES 16
>> +#else
>> +#define RTE_PMD_TAP_MAX_QUEUES 1
>> +#endif
>
> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
This should have been removed in your patch and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
>
>>
>> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
>> ETH_TAP_IFACE_ARG,
>> ETH_TAP_SPEED_ARG,
>> ETH_TAP_REMOTE_ARG,
>> + ETH_TAP_MAC_ARG,
>> NULL
>> };
>>
>> static int tap_unit;
>> +static int fixed_mac_type;
>
> There is no need for a global variable, especially as the value should not be the same for each driver instance.
> Typically when one tap uses "mac=fixed" and the next one doesn't.
> More comments bellow for that.
>
>>
>> static volatile uint32_t tap_trigger; /* Rx trigger */
>>
>> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
>> rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
>> ETHER_ADDR_LEN);
>> } else {
>> - eth_random_addr((uint8_t *)&pmd->eth_addr);
>> + if (fixed_mac_type) {
>> + static int iface_idx;
>> +
>> + pmd->eth_addr.addr_bytes[0] = 0x00;
>> + pmd->eth_addr.addr_bytes[1] = 'd';
>> + pmd->eth_addr.addr_bytes[2] = 't';
>> + pmd->eth_addr.addr_bytes[3] = 'a';
>> + pmd->eth_addr.addr_bytes[4] = 'p';
>> + pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
>> + } else
>> + eth_random_addr((uint8_t *)&pmd->eth_addr);
>
> To avoid checkpatch warning, use else { }.
Ferruh, states this is OK and does not need to have the { } added. I will not add the { } here.
>
>> }
>>
>> return 0;
>> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
>> return 0;
>> }
>>
>> +static int
>> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
>> +{
>> + /* Assume random mac address */
>> + *(int *)extra_args = 0;
>
> With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.
It does need to be set of each instance of the call to this routine even if I remove the global. Each interface can have a different state.
>
>> + if (value && !strcasecmp("fixed", value))
>
> I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?
I will use the already existing macro ETH_TAP_MAC_ARG instead.
>
>> + *(int *)extra_args = 1;
>> + return 0;
>> +}
>> +
>> /* Open a TAP interface device.
>> */
>> static int
>> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
>> DEFAULT_TAP_NAME, tap_unit++);
>> memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
>>
>> + fixed_mac_type = 0;
>
> Turn fixed_mac_type to an automatic variable, local to this function.
> And add the argument to eth_dev_tap_create().
Need to have the global to exist, because I need to be able to test the variable later. The variable could be placed in a allocated structure, but does it really matter to have a static variable here?
>
>> if (params && (params[0] != '\0')) {
>> RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
>>
>> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
>> if (ret == -1)
>> goto leave;
>> }
>> +
>> + if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
>> + ret = rte_kvargs_process(kvlist,
>> + ETH_TAP_MAC_ARG,
>> + &set_mac_type,
>> + &fixed_mac_type);
>> + if (ret == -1)
>> + goto leave;
>> + }
>> }
>> }
>> pmd_link.link_speed = speed;
>> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
>> };
>> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>> + "iface=<string>,"
>> + "speed=N,"
>> + "remote=<string>,"
>> + "mac=fixed");
>
> Indeed, I forgot to update that when I introduced the remote!
>
> That's it for me,
> Thank you.
>
> Pascal
Regards,
Keith
> On Apr 11, 2017, at 8:23 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
>
>
>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> Hi Keith,
>>
>> I have a few comments on your patch, see inline.
>>
>> On Mon, 10 Apr 2017 13:18:50 -0500
>> Keith Wiles <keith.wiles@intel.com> wrote:
>>
>>> Support for a fixed MAC address for testing with the last octet
>>> incrementing by one for each interface defined with the new 'mac=fixed'
>>> string on the --vdev option. The default option is still to randomize
>>> the MAC address for each tap interface.
>>>
>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>>> ---
>>> doc/guides/nics/tap.rst | 13 +++++++++++-
>>> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
>>> 2 files changed, 58 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
>>> index 5c5ba5357..e3819836a 100644
>>> --- a/doc/guides/nics/tap.rst
>>> +++ b/doc/guides/nics/tap.rst
>>> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
>>> along with being able to be used as a network connection to the DPDK
>>> application. The method enable one or more interfaces is to use the
>>> ``--vdev=net_tap0`` option on the DPDK application command line. Each
>>> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
>>> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
>>> and so on.
>>>
>>> The interface name can be changed by adding the ``iface=foo0``, for example::
>>> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
>>>
>>> --vdev=net_tap0,iface=foo0,speed=25000
>>>
>>> +Normally the PMD will generate random MAC address, but when testing or with
>>
>> "random MAC" -> "a random MAC"
>>
>>> +a static configurations the developer may need a fixed MAC address style.
>>
>> "configurations" -> "configuration"
>>
>>> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
>>> +
>>> + --vdev=net_tap0,mac=fixed
>>> +
>>> +The MAC address will be fixed value with the last octet incrementing by one
>>
>> "be" -> "have a"
>>
>>> +each time for each interface string containing ``mac=fixed``. The MAC address
>>
>> "each time" -> ""
>>
>>> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
>>
>> " convert" -> ". Convert"
>>
>>> +and you get ``00:64:74:61:70:[00-FF]``.
>>> +
>>> It is possible to specify a remote netdevice to capture packets from by adding
>>> ``remote=foo1``, for example::
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 347a80741..7a676c588 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -1,7 +1,7 @@
>>> /*-
>>> * BSD LICENSE
>>> *
>>> - * Copyright(c) 2016 Intel Corporation. All rights reserved.
>>> + * Copyright(c) 2017 Intel Corporation. All rights reserved.
>>
>> Shouldn't it be "2016-2017"?
>>
>>> * All rights reserved.
>>> *
>>> * Redistribution and use in source and binary forms, with or without
>>> @@ -71,6 +71,13 @@
>>> #define ETH_TAP_IFACE_ARG "iface"
>>> #define ETH_TAP_SPEED_ARG "speed"
>>> #define ETH_TAP_REMOTE_ARG "remote"
>>> +#define ETH_TAP_MAC_ARG "mac"
>>
>> You used tabs instead of spaces.
>>
>>> +
>>> +#ifdef IFF_MULTI_QUEUE
>>> +#define RTE_PMD_TAP_MAX_QUEUES 16
>>> +#else
>>> +#define RTE_PMD_TAP_MAX_QUEUES 1
>>> +#endif
>>
>> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
>
> This should have been removed in your patch and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
>
> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
>
>>
>>>
>>> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>>> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>>> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
>>> ETH_TAP_IFACE_ARG,
>>> ETH_TAP_SPEED_ARG,
>>> ETH_TAP_REMOTE_ARG,
>>> + ETH_TAP_MAC_ARG,
>>> NULL
>>> };
>>>
>>> static int tap_unit;
>>> +static int fixed_mac_type;
>>
>> There is no need for a global variable, especially as the value should not be the same for each driver instance.
>> Typically when one tap uses "mac=fixed" and the next one doesn't.
>> More comments bellow for that.
>>
>>>
>>> static volatile uint32_t tap_trigger; /* Rx trigger */
>>>
>>> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
>>> rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
>>> ETHER_ADDR_LEN);
>>> } else {
>>> - eth_random_addr((uint8_t *)&pmd->eth_addr);
>>> + if (fixed_mac_type) {
>>> + static int iface_idx;
>>> +
>>> + pmd->eth_addr.addr_bytes[0] = 0x00;
>>> + pmd->eth_addr.addr_bytes[1] = 'd';
>>> + pmd->eth_addr.addr_bytes[2] = 't';
>>> + pmd->eth_addr.addr_bytes[3] = 'a';
>>> + pmd->eth_addr.addr_bytes[4] = 'p';
>>> + pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
>>> + } else
>>> + eth_random_addr((uint8_t *)&pmd->eth_addr);
>>
>> To avoid checkpatch warning, use else { }.
>
> Ferruh, states this is OK and does not need to have the { } added. I will not add the { } here.
>
>>
>>> }
>>>
>>> return 0;
>>> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
>>> return 0;
>>> }
>>>
>>> +static int
>>> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
>>> +{
>>> + /* Assume random mac address */
>>> + *(int *)extra_args = 0;
>>
>> With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.
>
> It does need to be set of each instance of the call to this routine even if I remove the global. Each interface can have a different state.
>
>>
>>> + if (value && !strcasecmp("fixed", value))
>>
>> I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?
>
> I will use the already existing macro ETH_TAP_MAC_ARG instead.
Opps! I will need a macros for this one.
>>
>>> + *(int *)extra_args = 1;
>>> + return 0;
>>> +}
>>> +
>>> /* Open a TAP interface device.
>>> */
>>> static int
>>> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
>>> DEFAULT_TAP_NAME, tap_unit++);
>>> memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
>>>
>>> + fixed_mac_type = 0;
>>
>> Turn fixed_mac_type to an automatic variable, local to this function.
>> And add the argument to eth_dev_tap_create().
>
> Need to have the global to exist, because I need to be able to test the variable later. The variable could be placed in a allocated structure, but does it really matter to have a static variable here?
>
>>
>>> if (params && (params[0] != '\0')) {
>>> RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
>>>
>>> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
>>> if (ret == -1)
>>> goto leave;
>>> }
>>> +
>>> + if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
>>> + ret = rte_kvargs_process(kvlist,
>>> + ETH_TAP_MAC_ARG,
>>> + &set_mac_type,
>>> + &fixed_mac_type);
>>> + if (ret == -1)
>>> + goto leave;
>>> + }
>>> }
>>> }
>>> pmd_link.link_speed = speed;
>>> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
>>> };
>>> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>>> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>>> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>>> + "iface=<string>,"
>>> + "speed=N,"
>>> + "remote=<string>,"
>>> + "mac=fixed");
>>
>> Indeed, I forgot to update that when I introduced the remote!
>>
>> That's it for me,
>> Thank you.
>>
>> Pascal
>
> Regards,
> Keith
>
Regards,
Keith
On 4/11/2017 2:23 PM, Wiles, Keith wrote:
>
>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>
>> Hi Keith,
>>
>> I have a few comments on your patch, see inline.
>>
>> On Mon, 10 Apr 2017 13:18:50 -0500
>> Keith Wiles <keith.wiles@intel.com> wrote:
>>
>>> Support for a fixed MAC address for testing with the last octet
>>> incrementing by one for each interface defined with the new 'mac=fixed'
>>> string on the --vdev option. The default option is still to randomize
>>> the MAC address for each tap interface.
>>>
>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
<...>
>>> +
>>> +#ifdef IFF_MULTI_QUEUE
>>> +#define RTE_PMD_TAP_MAX_QUEUES 16
>>> +#else
>>> +#define RTE_PMD_TAP_MAX_QUEUES 1
>>> +#endif
>>
>> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
>
> This should have been removed in your patch
That is already done, and patch is in main repo:
Fixes: 947d949de7de ("net/tap: fix max queues redefinition")
> and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
Pascal already send the patch and it is in next-net now:
Fixes: f0e5085f4677 ("net/tap: update driver param string")
>
> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
There is no more updated expected, using latest next-net should be OK to
submit your patch.
Thanks,
ferruh
> On Apr 11, 2017, at 8:39 AM, Yigit, Ferruh <ferruh.yigit@intel.com> wrote:
>
> On 4/11/2017 2:23 PM, Wiles, Keith wrote:
>>
>>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>
>>> Hi Keith,
>>>
>>> I have a few comments on your patch, see inline.
>>>
>>> On Mon, 10 Apr 2017 13:18:50 -0500
>>> Keith Wiles <keith.wiles@intel.com> wrote:
>>>
>>>> Support for a fixed MAC address for testing with the last octet
>>>> incrementing by one for each interface defined with the new 'mac=fixed'
>>>> string on the --vdev option. The default option is still to randomize
>>>> the MAC address for each tap interface.
>>>>
>>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>
> <...>
>
>>>> +
>>>> +#ifdef IFF_MULTI_QUEUE
>>>> +#define RTE_PMD_TAP_MAX_QUEUES 16
>>>> +#else
>>>> +#define RTE_PMD_TAP_MAX_QUEUES 1
>>>> +#endif
>>>
>>> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
>>
>> This should have been removed in your patch
>
> That is already done, and patch is in main repo:
> Fixes: 947d949de7de ("net/tap: fix max queues redefinition")
>
>> and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
>
> Pascal already send the patch and it is in next-net now:
> Fixes: f0e5085f4677 ("net/tap: update driver param string")
>
>>
>> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
>
> There is no more updated expected, using latest next-net should be OK to
> submit your patch.
OK
>
> Thanks,
> ferruh
Regards,
Keith
On Tue, 11 Apr 2017 13:23:24 +0000
"Wiles, Keith" <keith.wiles@intel.com> wrote:
>
> > On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
> >
> > Hi Keith,
> >
> > I have a few comments on your patch, see inline.
> >
> > On Mon, 10 Apr 2017 13:18:50 -0500
> > Keith Wiles <keith.wiles@intel.com> wrote:
> >
> >> Support for a fixed MAC address for testing with the last octet
> >> incrementing by one for each interface defined with the new 'mac=fixed'
> >> string on the --vdev option. The default option is still to randomize
> >> the MAC address for each tap interface.
> >>
> >> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
> >> ---
> >> doc/guides/nics/tap.rst | 13 +++++++++++-
> >> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
> >> 2 files changed, 58 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
> >> index 5c5ba5357..e3819836a 100644
> >> --- a/doc/guides/nics/tap.rst
> >> +++ b/doc/guides/nics/tap.rst
> >> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
> >> along with being able to be used as a network connection to the DPDK
> >> application. The method enable one or more interfaces is to use the
> >> ``--vdev=net_tap0`` option on the DPDK application command line. Each
> >> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
> >> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
> >> and so on.
> >>
> >> The interface name can be changed by adding the ``iface=foo0``, for example::
> >> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
> >>
> >> --vdev=net_tap0,iface=foo0,speed=25000
> >>
> >> +Normally the PMD will generate random MAC address, but when testing or with
> >
> > "random MAC" -> "a random MAC"
> >
> >> +a static configurations the developer may need a fixed MAC address style.
> >
> > "configurations" -> "configuration"
> >
> >> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
> >> +
> >> + --vdev=net_tap0,mac=fixed
> >> +
> >> +The MAC address will be fixed value with the last octet incrementing by one
> >
> > "be" -> "have a"
> >
> >> +each time for each interface string containing ``mac=fixed``. The MAC address
> >
> > "each time" -> ""
> >
> >> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
> >
> > " convert" -> ". Convert"
> >
> >> +and you get ``00:64:74:61:70:[00-FF]``.
> >> +
> >> It is possible to specify a remote netdevice to capture packets from by adding
> >> ``remote=foo1``, for example::
> >>
> >> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >> index 347a80741..7a676c588 100644
> >> --- a/drivers/net/tap/rte_eth_tap.c
> >> +++ b/drivers/net/tap/rte_eth_tap.c
> >> @@ -1,7 +1,7 @@
> >> /*-
> >> * BSD LICENSE
> >> *
> >> - * Copyright(c) 2016 Intel Corporation. All rights reserved.
> >> + * Copyright(c) 2017 Intel Corporation. All rights reserved.
> >
> > Shouldn't it be "2016-2017"?
> >
> >> * All rights reserved.
> >> *
> >> * Redistribution and use in source and binary forms, with or without
> >> @@ -71,6 +71,13 @@
> >> #define ETH_TAP_IFACE_ARG "iface"
> >> #define ETH_TAP_SPEED_ARG "speed"
> >> #define ETH_TAP_REMOTE_ARG "remote"
> >> +#define ETH_TAP_MAC_ARG "mac"
> >
> > You used tabs instead of spaces.
> >
> >> +
> >> +#ifdef IFF_MULTI_QUEUE
> >> +#define RTE_PMD_TAP_MAX_QUEUES 16
> >> +#else
> >> +#define RTE_PMD_TAP_MAX_QUEUES 1
> >> +#endif
> >
> > Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
>
> This should have been removed in your patch and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
>
> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
Ferruh details how it's been integrated in next-net, nothing to add there :)
>
> >
> >>
> >> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
> >> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
> >> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
> >> ETH_TAP_IFACE_ARG,
> >> ETH_TAP_SPEED_ARG,
> >> ETH_TAP_REMOTE_ARG,
> >> + ETH_TAP_MAC_ARG,
> >> NULL
> >> };
> >>
> >> static int tap_unit;
> >> +static int fixed_mac_type;
> >
> > There is no need for a global variable, especially as the value should not be the same for each driver instance.
> > Typically when one tap uses "mac=fixed" and the next one doesn't.
> > More comments bellow for that.
> >
> >>
> >> static volatile uint32_t tap_trigger; /* Rx trigger */
> >>
> >> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
> >> rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
> >> ETHER_ADDR_LEN);
> >> } else {
> >> - eth_random_addr((uint8_t *)&pmd->eth_addr);
> >> + if (fixed_mac_type) {
> >> + static int iface_idx;
I'd suggest using something like this, as there's no need for so much lines to do the same:
char mac[ETHER_ADDR_LEN] = "\x00\x64\x74\x61\x70\x00"; /* 0:d:t:a:p:0 */
mac[ETHER_ADDR_LEN-1] = iface_idx++;
rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
> >> +
> >> + pmd->eth_addr.addr_bytes[0] = 0x00;
> >> + pmd->eth_addr.addr_bytes[1] = 'd';
> >> + pmd->eth_addr.addr_bytes[2] = 't';
> >> + pmd->eth_addr.addr_bytes[3] = 'a';
> >> + pmd->eth_addr.addr_bytes[4] = 'p';
> >> + pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
> >> + } else
> >> + eth_random_addr((uint8_t *)&pmd->eth_addr);
> >
> > To avoid checkpatch warning, use else { }.
>
> Ferruh, states this is OK and does not need to have the { } added. I will not add the { } here.
Very well!
>
> >
> >> }
> >>
> >> return 0;
> >> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
> >> return 0;
> >> }
> >>
> >> +static int
> >> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
> >> +{
> >> + /* Assume random mac address */
> >> + *(int *)extra_args = 0;
> >
> > With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.
>
> It does need to be set of each instance of the call to this routine even if I remove the global. Each interface can have a different state.
>
> >
> >> + if (value && !strcasecmp("fixed", value))
> >
> > I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?
>
> I will use the already existing macro ETH_TAP_MAC_ARG instead.
> >
> >> + *(int *)extra_args = 1;
> >> + return 0;
> >> +}
> >> +
> >> /* Open a TAP interface device.
> >> */
> >> static int
> >> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
> >> DEFAULT_TAP_NAME, tap_unit++);
> >> memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
> >>
> >> + fixed_mac_type = 0;
> >
> > Turn fixed_mac_type to an automatic variable, local to this function.
> > And add the argument to eth_dev_tap_create().
>
> Need to have the global to exist, because I need to be able to test the variable later. The variable could be placed in a allocated structure, but does it really matter to have a static variable here?
I disagree here, I've seen fixed_mac_type only used during probing, in rte_pmd_tap_probe() and rte_eth_dev_tap_create().
The case is similar to the "iface" param, we need it during probing, where it is used appropriately (either setting the pmd->eth_addr for your fixed mac, or setting pmd->remote_iface for iface).
>
> >
> >> if (params && (params[0] != '\0')) {
> >> RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
> >>
> >> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
> >> if (ret == -1)
> >> goto leave;
> >> }
> >> +
> >> + if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
> >> + ret = rte_kvargs_process(kvlist,
> >> + ETH_TAP_MAC_ARG,
> >> + &set_mac_type,
> >> + &fixed_mac_type);
> >> + if (ret == -1)
> >> + goto leave;
> >> + }
> >> }
> >> }
> >> pmd_link.link_speed = speed;
> >> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
> >> };
> >> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
> >> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
> >> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
> >> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
> >> + "iface=<string>,"
> >> + "speed=N,"
> >> + "remote=<string>,"
> >> + "mac=fixed");
> >
> > Indeed, I forgot to update that when I introduced the remote!
> >
> > That's it for me,
> > Thank you.
> >
> > Pascal
>
> Regards,
> Keith
>
> On Apr 11, 2017, at 8:54 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>
> On Tue, 11 Apr 2017 13:23:24 +0000
> "Wiles, Keith" <keith.wiles@intel.com> wrote:
>
>>
>>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon@6wind.com> wrote:
>>>
>>> Hi Keith,
>>>
>>> I have a few comments on your patch, see inline.
>>>
>>> On Mon, 10 Apr 2017 13:18:50 -0500
>>> Keith Wiles <keith.wiles@intel.com> wrote:
>>>
>>>> Support for a fixed MAC address for testing with the last octet
>>>> incrementing by one for each interface defined with the new 'mac=fixed'
>>>> string on the --vdev option. The default option is still to randomize
>>>> the MAC address for each tap interface.
>>>>
>>>> Signed-off-by: Keith Wiles <keith.wiles@intel.com>
>>>> ---
>>>> doc/guides/nics/tap.rst | 13 +++++++++++-
>>>> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++++++---
>>>> 2 files changed, 58 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/doc/guides/nics/tap.rst b/doc/guides/nics/tap.rst
>>>> index 5c5ba5357..e3819836a 100644
>>>> --- a/doc/guides/nics/tap.rst
>>>> +++ b/doc/guides/nics/tap.rst
>>>> @@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
>>>> along with being able to be used as a network connection to the DPDK
>>>> application. The method enable one or more interfaces is to use the
>>>> ``--vdev=net_tap0`` option on the DPDK application command line. Each
>>>> -``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
>>>> +``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
>>>> and so on.
>>>>
>>>> The interface name can be changed by adding the ``iface=foo0``, for example::
>>>> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
>>>>
>>>> --vdev=net_tap0,iface=foo0,speed=25000
>>>>
>>>> +Normally the PMD will generate random MAC address, but when testing or with
>>>
>>> "random MAC" -> "a random MAC"
>>>
>>>> +a static configurations the developer may need a fixed MAC address style.
>>>
>>> "configurations" -> "configuration"
>>>
>>>> +Using the option ``mac=fixed`` you can create a fixed known MAC address::
>>>> +
>>>> + --vdev=net_tap0,mac=fixed
>>>> +
>>>> +The MAC address will be fixed value with the last octet incrementing by one
>>>
>>> "be" -> "have a"
>>>
>>>> +each time for each interface string containing ``mac=fixed``. The MAC address
>>>
>>> "each time" -> ""
>>>
>>>> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
>>>
>>> " convert" -> ". Convert"
>>>
>>>> +and you get ``00:64:74:61:70:[00-FF]``.
>>>> +
>>>> It is possible to specify a remote netdevice to capture packets from by adding
>>>> ``remote=foo1``, for example::
>>>>
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>> index 347a80741..7a676c588 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -1,7 +1,7 @@
>>>> /*-
>>>> * BSD LICENSE
>>>> *
>>>> - * Copyright(c) 2016 Intel Corporation. All rights reserved.
>>>> + * Copyright(c) 2017 Intel Corporation. All rights reserved.
>>>
>>> Shouldn't it be "2016-2017"?
>>>
>>>> * All rights reserved.
>>>> *
>>>> * Redistribution and use in source and binary forms, with or without
>>>> @@ -71,6 +71,13 @@
>>>> #define ETH_TAP_IFACE_ARG "iface"
>>>> #define ETH_TAP_SPEED_ARG "speed"
>>>> #define ETH_TAP_REMOTE_ARG "remote"
>>>> +#define ETH_TAP_MAC_ARG "mac"
>>>
>>> You used tabs instead of spaces.
>>>
>>>> +
>>>> +#ifdef IFF_MULTI_QUEUE
>>>> +#define RTE_PMD_TAP_MAX_QUEUES 16
>>>> +#else
>>>> +#define RTE_PMD_TAP_MAX_QUEUES 1
>>>> +#endif
>>>
>>> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h now (needed for pmd_internals).
>>
>> This should have been removed in your patch and now that Ferruh wants you to submit a patch for the string at the bottom of the PMD, can you remove it?
>>
>> I can do both in my patch, but Ferruh and you need to agree before I can submit my patch.
>
> Ferruh details how it's been integrated in next-net, nothing to add there :)
>
>>
>>>
>>>>
>>>> #define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
>>>> #define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
>>>> @@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
>>>> ETH_TAP_IFACE_ARG,
>>>> ETH_TAP_SPEED_ARG,
>>>> ETH_TAP_REMOTE_ARG,
>>>> + ETH_TAP_MAC_ARG,
>>>> NULL
>>>> };
>>>>
>>>> static int tap_unit;
>>>> +static int fixed_mac_type;
>>>
>>> There is no need for a global variable, especially as the value should not be the same for each driver instance.
>>> Typically when one tap uses "mac=fixed" and the next one doesn't.
>>> More comments bellow for that.
>>>
>>>>
>>>> static volatile uint32_t tap_trigger; /* Rx trigger */
>>>>
>>>> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
>>>> rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
>>>> ETHER_ADDR_LEN);
>>>> } else {
>>>> - eth_random_addr((uint8_t *)&pmd->eth_addr);
>>>> + if (fixed_mac_type) {
>>>> + static int iface_idx;
>
> I'd suggest using something like this, as there's no need for so much lines to do the same:
>
> char mac[ETHER_ADDR_LEN] = "\x00\x64\x74\x61\x70\x00"; /* 0:d:t:a:p:0 */
>
> mac[ETHER_ADDR_LEN-1] = iface_idx++;
> rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
Here is the above code:
static int iface_idx;
char mac[ETHER_ADDR_LEN] =
"\x00\x64\x74\x61\x70\x00"; /* 0:d:t:a:p:0 */
mac[ETHER_ADDR_LEN - 1] = iface_idx++;
rte_memcpy(&pmd->eth_addr, mac, ETHER_ADDR_LEN);
Not much in the way of fewer lines and not really that readable, so your objection here is only that you would have coded it differently not that it is incorrect, right? Personally I like the code below as it is easier to ready IMHO, but I will covert it your suggestion.
>
>
>>>> +
>>>> + pmd->eth_addr.addr_bytes[0] = 0x00;
>>>> + pmd->eth_addr.addr_bytes[1] = 'd';
>>>> + pmd->eth_addr.addr_bytes[2] = 't';
>>>> + pmd->eth_addr.addr_bytes[3] = 'a';
>>>> + pmd->eth_addr.addr_bytes[4] = 'p';
>>>> + pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
>>>> + } else
>>>> + eth_random_addr((uint8_t *)&pmd->eth_addr);
>>>
>>> To avoid checkpatch warning, use else { }.
>>
>> Ferruh, states this is OK and does not need to have the { } added. I will not add the { } here.
>
> Very well!
>
>>
>>>
>>>> }
>>>>
>>>> return 0;
>>>> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
>>>> return 0;
>>>> }
>>>>
>>>> +static int
>>>> +set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
>>>> +{
>>>> + /* Assume random mac address */
>>>> + *(int *)extra_args = 0;
>>>
>>> With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no need for setting it to 0 here.
>>
>> It does need to be set of each instance of the call to this routine even if I remove the global. Each interface can have a different state.
>>
>>>
>>>> + if (value && !strcasecmp("fixed", value))
>>>
>>> I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top?
>>
>> I will use the already existing macro ETH_TAP_MAC_ARG instead.
>>>
>>>> + *(int *)extra_args = 1;
>>>> + return 0;
>>>> +}
>>>> +
>>>> /* Open a TAP interface device.
>>>> */
>>>> static int
>>>> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
>>>> DEFAULT_TAP_NAME, tap_unit++);
>>>> memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
>>>>
>>>> + fixed_mac_type = 0;
>>>
>>> Turn fixed_mac_type to an automatic variable, local to this function.
>>> And add the argument to eth_dev_tap_create().
>>
>> Need to have the global to exist, because I need to be able to test the variable later. The variable could be placed in a allocated structure, but does it really matter to have a static variable here?
>
> I disagree here, I've seen fixed_mac_type only used during probing, in rte_pmd_tap_probe() and rte_eth_dev_tap_create().
> The case is similar to the "iface" param, we need it during probing, where it is used appropriately (either setting the pmd->eth_addr for your fixed mac, or setting pmd->remote_iface for iface).
This code is not really close to “iface” code as the iface code has a place to put the string in the pmd structure and the pmd->eth_addr is setup later on in the code. I would have to add a holding place in the pmd structure to hold the random or fixed mac address or flag to be used in the routine eth_dev_tap_create().
Maybe I am just being slow this morning, but I do not see how you want the code to be done here as I do not see away around a flag someplace.
>
>>
>>>
>>>> if (params && (params[0] != '\0')) {
>>>> RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
>>>>
>>>> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
>>>> if (ret == -1)
>>>> goto leave;
>>>> }
>>>> +
>>>> + if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
>>>> + ret = rte_kvargs_process(kvlist,
>>>> + ETH_TAP_MAC_ARG,
>>>> + &set_mac_type,
>>>> + &fixed_mac_type);
>>>> + if (ret == -1)
>>>> + goto leave;
>>>> + }
>>>> }
>>>> }
>>>> pmd_link.link_speed = speed;
>>>> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
>>>> };
>>>> RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
>>>> RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
>>>> -RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
>>>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap,
>>>> + "iface=<string>,"
>>>> + "speed=N,"
>>>> + "remote=<string>,"
>>>> + "mac=fixed");
>>>
>>> Indeed, I forgot to update that when I introduced the remote!
>>>
>>> That's it for me,
>>> Thank you.
>>>
>>> Pascal
>>
>> Regards,
>> Keith
Regards,
Keith
@@ -46,7 +46,7 @@ These TAP interfaces can be used with Wireshark or tcpdump or Pktgen-DPDK
along with being able to be used as a network connection to the DPDK
application. The method enable one or more interfaces is to use the
``--vdev=net_tap0`` option on the DPDK application command line. Each
-``--vdev=net_tap1`` option give will create an interface named dtap0, dtap1,
+``--vdev=net_tap1`` option given will create an interface named dtap0, dtap1,
and so on.
The interface name can be changed by adding the ``iface=foo0``, for example::
@@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed, for example::
--vdev=net_tap0,iface=foo0,speed=25000
+Normally the PMD will generate random MAC address, but when testing or with
+a static configurations the developer may need a fixed MAC address style.
+Using the option ``mac=fixed`` you can create a fixed known MAC address::
+
+ --vdev=net_tap0,mac=fixed
+
+The MAC address will be fixed value with the last octet incrementing by one
+each time for each interface string containing ``mac=fixed``. The MAC address
+is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to hex
+and you get ``00:64:74:61:70:[00-FF]``.
+
It is possible to specify a remote netdevice to capture packets from by adding
``remote=foo1``, for example::
@@ -1,7 +1,7 @@
/*-
* BSD LICENSE
*
- * Copyright(c) 2016 Intel Corporation. All rights reserved.
+ * Copyright(c) 2017 Intel Corporation. All rights reserved.
* All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
@@ -71,6 +71,13 @@
#define ETH_TAP_IFACE_ARG "iface"
#define ETH_TAP_SPEED_ARG "speed"
#define ETH_TAP_REMOTE_ARG "remote"
+#define ETH_TAP_MAC_ARG "mac"
+
+#ifdef IFF_MULTI_QUEUE
+#define RTE_PMD_TAP_MAX_QUEUES 16
+#else
+#define RTE_PMD_TAP_MAX_QUEUES 1
+#endif
#define FLOWER_KERNEL_VERSION KERNEL_VERSION(4, 2, 0)
#define FLOWER_VLAN_KERNEL_VERSION KERNEL_VERSION(4, 9, 0)
@@ -81,10 +88,12 @@ static const char *valid_arguments[] = {
ETH_TAP_IFACE_ARG,
ETH_TAP_SPEED_ARG,
ETH_TAP_REMOTE_ARG,
+ ETH_TAP_MAC_ARG,
NULL
};
static int tap_unit;
+static int fixed_mac_type;
static volatile uint32_t tap_trigger; /* Rx trigger */
@@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_name, char *remote_iface)
rte_memcpy(&pmd->eth_addr, ifr.ifr_hwaddr.sa_data,
ETHER_ADDR_LEN);
} else {
- eth_random_addr((uint8_t *)&pmd->eth_addr);
+ if (fixed_mac_type) {
+ static int iface_idx;
+
+ pmd->eth_addr.addr_bytes[0] = 0x00;
+ pmd->eth_addr.addr_bytes[1] = 'd';
+ pmd->eth_addr.addr_bytes[2] = 't';
+ pmd->eth_addr.addr_bytes[3] = 'a';
+ pmd->eth_addr.addr_bytes[4] = 'p';
+ pmd->eth_addr.addr_bytes[5] = 0 + iface_idx++;
+ } else
+ eth_random_addr((uint8_t *)&pmd->eth_addr);
}
return 0;
@@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused,
return 0;
}
+static int
+set_mac_type(const char *key __rte_unused, const char *value, void *extra_args)
+{
+ /* Assume random mac address */
+ *(int *)extra_args = 0;
+ if (value && !strcasecmp("fixed", value))
+ *(int *)extra_args = 1;
+ return 0;
+}
+
/* Open a TAP interface device.
*/
static int
@@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *params)
DEFAULT_TAP_NAME, tap_unit++);
memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN);
+ fixed_mac_type = 0;
if (params && (params[0] != '\0')) {
RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params);
@@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *params)
if (ret == -1)
goto leave;
}
+
+ if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) == 1) {
+ ret = rte_kvargs_process(kvlist,
+ ETH_TAP_MAC_ARG,
+ &set_mac_type,
+ &fixed_mac_type);
+ if (ret == -1)
+ goto leave;
+ }
}
}
pmd_link.link_speed = speed;
@@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv = {
};
RTE_PMD_REGISTER_VDEV(net_tap, pmd_tap_drv);
RTE_PMD_REGISTER_ALIAS(net_tap, eth_tap);
-RTE_PMD_REGISTER_PARAM_STRING(net_tap, "iface=<string>,speed=N");
+RTE_PMD_REGISTER_PARAM_STRING(net_tap,
+ "iface=<string>,"
+ "speed=N,"
+ "remote=<string>,"
+ "mac=fixed");