> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.ma...@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.wi...@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.wi...@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
