From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wiles, Keith" Subject: Re: [PATCH] net/tap: add support for fixed mac addresses Date: Tue, 11 Apr 2017 13:25:05 +0000 Message-ID: References: <20170410181850.44845-1-keith.wiles@intel.com> <20170411091851.62ab28ad@paques.dev.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" To: Pascal Mazon Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 2FB3E2BB0 for ; Tue, 11 Apr 2017 15:25:07 +0200 (CEST) In-Reply-To: Content-Language: en-US Content-ID: <320B286ADA0F984C9AC0711D1BA9B496@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > On Apr 11, 2017, at 8:23 AM, Wiles, Keith wrote: >=20 >=20 >> On Apr 11, 2017, at 2:18 AM, Pascal Mazon wrote= : >>=20 >> Hi Keith, >>=20 >> I have a few comments on your patch, see inline. >>=20 >> On Mon, 10 Apr 2017 13:18:50 -0500 >> Keith Wiles wrote: >>=20 >>> Support for a fixed MAC address for testing with the last octet >>> incrementing by one for each interface defined with the new 'mac=3Dfixe= d' >>> string on the --vdev option. The default option is still to randomize >>> the MAC address for each tap interface. >>>=20 >>> Signed-off-by: Keith Wiles >>> --- >>> doc/guides/nics/tap.rst | 13 +++++++++++- >>> drivers/net/tap/rte_eth_tap.c | 49 ++++++++++++++++++++++++++++++++++++= ++++--- >>> 2 files changed, 58 insertions(+), 4 deletions(-) >>>=20 >>> 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 tc= pdump 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=3Dnet_tap0`` option on the DPDK application command line. Each >>> -``--vdev=3Dnet_tap1`` option give will create an interface named dtap0= , dtap1, >>> +``--vdev=3Dnet_tap1`` option given will create an interface named dtap= 0, dtap1, >>> and so on. >>>=20 >>> The interface name can be changed by adding the ``iface=3Dfoo0``, for e= xample:: >>> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that spee= d, for example:: >>>=20 >>> --vdev=3Dnet_tap0,iface=3Dfoo0,speed=3D25000 >>>=20 >>> +Normally the PMD will generate random MAC address, but when testing or= with >>=20 >> "random MAC" -> "a random MAC" >>=20 >>> +a static configurations the developer may need a fixed MAC address sty= le. >>=20 >> "configurations" -> "configuration" >>=20 >>> +Using the option ``mac=3Dfixed`` you can create a fixed known MAC addr= ess:: >>> + >>> + --vdev=3Dnet_tap0,mac=3Dfixed >>> + >>> +The MAC address will be fixed value with the last octet incrementing b= y one >>=20 >> "be" -> "have a" >>=20 >>> +each time for each interface string containing ``mac=3Dfixed``. The MA= C address >>=20 >> "each time" -> "" >>=20 >>> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to h= ex >>=20 >> " convert" -> ". Convert" >>=20 >>> +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=3Dfoo1``, for example:: >>>=20 >>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_ta= p.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. >>=20 >> Shouldn't it be "2016-2017"? >>=20 >>> * 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" >>=20 >> You used tabs instead of spaces. >>=20 >>> + >>> +#ifdef IFF_MULTI_QUEUE >>> +#define RTE_PMD_TAP_MAX_QUEUES 16 >>> +#else >>> +#define RTE_PMD_TAP_MAX_QUEUES 1 >>> +#endif >>=20 >> Remove this IFF_MULTI_QUEUE definition as it is done in rte_eth_tap.h no= w (needed for pmd_internals). >=20 > 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? >=20 > I can do both in my patch, but Ferruh and you need to agree before I can = submit my patch. >=20 >>=20 >>>=20 >>> #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[] =3D { >>> ETH_TAP_IFACE_ARG, >>> ETH_TAP_SPEED_ARG, >>> ETH_TAP_REMOTE_ARG, >>> + ETH_TAP_MAC_ARG, >>> NULL >>> }; >>>=20 >>> static int tap_unit; >>> +static int fixed_mac_type; >>=20 >> There is no need for a global variable, especially as the value should n= ot be the same for each driver instance. >> Typically when one tap uses "mac=3Dfixed" and the next one doesn't. >> More comments bellow for that. >>=20 >>>=20 >>> static volatile uint32_t tap_trigger; /* Rx trigger */ >>>=20 >>> @@ -1230,7 +1239,17 @@ eth_dev_tap_create(const char *name, char *tap_n= ame, 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] =3D 0x00; >>> + pmd->eth_addr.addr_bytes[1] =3D 'd'; >>> + pmd->eth_addr.addr_bytes[2] =3D 't'; >>> + pmd->eth_addr.addr_bytes[3] =3D 'a'; >>> + pmd->eth_addr.addr_bytes[4] =3D 'p'; >>> + pmd->eth_addr.addr_bytes[5] =3D 0 + iface_idx++; >>> + } else >>> + eth_random_addr((uint8_t *)&pmd->eth_addr); >>=20 >> To avoid checkpatch warning, use else { }. >=20 > Ferruh, states this is OK and does not need to have the { } added. I will= not add the { } here. >=20 >>=20 >>> } >>>=20 >>> return 0; >>> @@ -1285,6 +1304,16 @@ set_remote_iface(const char *key __rte_unused, >>> return 0; >>> } >>>=20 >>> +static int >>> +set_mac_type(const char *key __rte_unused, const char *value, void *ex= tra_args) >>> +{ >>> + /* Assume random mac address */ >>> + *(int *)extra_args =3D 0; >>=20 >> With an automatic variable for fixed_mac_type in rte_pmd_tap_probe(), no= need for setting it to 0 here. >=20 > 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. >=20 >>=20 >>> + if (value && !strcasecmp("fixed", value)) >>=20 >> I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "= fixed" at the top? >=20 > I will use the already existing macro ETH_TAP_MAC_ARG instead. Opps! I will need a macros for this one. >>=20 >>> + *(int *)extra_args =3D 1; >>> + return 0; >>> +} >>> + >>> /* Open a TAP interface device. >>> */ >>> static int >>> @@ -1301,6 +1330,7 @@ rte_pmd_tap_probe(const char *name, const char *p= arams) >>> DEFAULT_TAP_NAME, tap_unit++); >>> memset(remote_iface, 0, RTE_ETH_NAME_MAX_LEN); >>>=20 >>> + fixed_mac_type =3D 0; >>=20 >> Turn fixed_mac_type to an automatic variable, local to this function. >> And add the argument to eth_dev_tap_create(). >=20 > Need to have the global to exist, because I need to be able to test the v= ariable later. The variable could be placed in a allocated structure, but d= oes it really matter to have a static variable here? >=20 >>=20 >>> if (params && (params[0] !=3D '\0')) { >>> RTE_LOG(DEBUG, PMD, "paramaters (%s)\n", params); >>>=20 >>> @@ -1332,6 +1362,15 @@ rte_pmd_tap_probe(const char *name, const char *= params) >>> if (ret =3D=3D -1) >>> goto leave; >>> } >>> + >>> + if (rte_kvargs_count(kvlist, ETH_TAP_MAC_ARG) =3D=3D 1) { >>> + ret =3D rte_kvargs_process(kvlist, >>> + ETH_TAP_MAC_ARG, >>> + &set_mac_type, >>> + &fixed_mac_type); >>> + if (ret =3D=3D -1) >>> + goto leave; >>> + } >>> } >>> } >>> pmd_link.link_speed =3D speed; >>> @@ -1394,4 +1433,8 @@ static struct rte_vdev_driver pmd_tap_drv =3D { >>> }; >>> 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=3D,speed=3DN"); >>> +RTE_PMD_REGISTER_PARAM_STRING(net_tap, >>> + "iface=3D," >>> + "speed=3DN," >>> + "remote=3D," >>> + "mac=3Dfixed"); >>=20 >> Indeed, I forgot to update that when I introduced the remote! >>=20 >> That's it for me, >> Thank you. >>=20 >> Pascal >=20 > Regards, > Keith >=20 Regards, Keith