[dpdk-dev] [PATCH] net/tap: add support for fixed mac addresses

Wiles, Keith keith.wiles at intel.com
Tue Apr 11 16:17:55 CEST 2017


> On Apr 11, 2017, at 8:54 AM, Pascal Mazon <pascal.mazon at 6wind.com> wrote:
> 
> On Tue, 11 Apr 2017 13:23:24 +0000
> "Wiles, Keith" <keith.wiles at intel.com> wrote:
> 
>> 
>>> On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.mazon at 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 at 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 at 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



More information about the dev mailing list