From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 909072B9B for ; Tue, 11 Apr 2017 15:23:43 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Apr 2017 06:23:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,185,1488873600"; d="scan'208";a="1118174249" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga001.jf.intel.com with ESMTP; 11 Apr 2017 06:23:41 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 11 Apr 2017 06:23:40 -0700 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.235]) by FMSMSX112.amr.corp.intel.com ([169.254.5.65]) with mapi id 14.03.0319.002; Tue, 11 Apr 2017 06:23:25 -0700 From: "Wiles, Keith" To: Pascal Mazon CC: "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH] net/tap: add support for fixed mac addresses Thread-Index: AQHSsib8ksIIBxsBsUq6QitCJudBlKHAOMeAgABl2oA= Date: Tue, 11 Apr 2017 13:23:24 +0000 Message-ID: References: <20170410181850.44845-1-keith.wiles@intel.com> <20170411091851.62ab28ad@paques.dev.6wind.com> In-Reply-To: <20170411091851.62ab28ad@paques.dev.6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.98.247] Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] net/tap: add support for fixed mac addresses X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Apr 2017 13:23:44 -0000 > 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=3Dfixed= ' >> 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 tcp= dump 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 dtap0= , dtap1, >> and so on. >>=20 >> The interface name can be changed by adding the ``iface=3Dfoo0``, for ex= ample:: >> @@ -58,6 +58,17 @@ needed, but the interface does not enforce that speed= , 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 styl= e. >=20 > "configurations" -> "configuration" >=20 >> +Using the option ``mac=3Dfixed`` you can create a fixed known MAC addre= ss:: >> + >> + --vdev=3Dnet_tap0,mac=3Dfixed >> + >> +The MAC address will be fixed value with the last octet incrementing by= one >=20 > "be" -> "have a" >=20 >> +each time for each interface string containing ``mac=3Dfixed``. The MAC= address >=20 > "each time" -> "" >=20 >> +is formatted as 00:'d':'t':'a':'p':[00-FF] convert the characters to he= x >=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_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. >=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 now= (needed for pmd_internals). This should have been removed in your patch and now that Ferruh wants you t= o 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 su= bmit my patch. >=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 no= t 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_na= me, 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 { }. Ferruh, states this is OK and does not need to have the { } added. I will n= ot add the { } here. >=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 *ext= ra_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. 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 >> + if (value && !strcasecmp("fixed", value)) >=20 > I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "f= ixed" at the top? I will use the already existing macro ETH_TAP_MAC_ARG instead. >=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 *pa= rams) >> 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(). Need to have the global to exist, because I need to be able to test the var= iable later. The variable could be placed in a allocated structure, but doe= s it really matter to have a static variable here? >=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 *p= arams) >> 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 Regards, Keith