From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f50.google.com (mail-wm0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id A9C8AD148 for ; Tue, 11 Apr 2017 15:55:02 +0200 (CEST) Received: by mail-wm0-f50.google.com with SMTP id y18so13906898wmh.0 for ; Tue, 11 Apr 2017 06:55:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :organization:mime-version:content-transfer-encoding; bh=yhs+abaWKi0p//g6YxUaeLZakEanlD/CzTTyNvJfsSA=; b=pv3q0Kb97JQZn2icar6ywpUC+8qarKKgf+7j+YedDdy4cAwzyvTPx++0JNORKoxvzr 86v0fbhrtMeDgPWfKQSJe7hapWVJgI8l29g7S9P1VUfM2kKCrrCJ2Plyv1Vvdtng2oHO QkBXU54pdzjBsl5I6lGcc7oD06U+vxHcqOHZTr7NgbFzxu5FELC4qMG8lenXwsPa6k11 /fqzY69dhwibWITYcCpGx2w3kFZN0sDW5G7aRi5E11d0d+KKDpEpBycVCpt7hVHLAWmB +yXAOeRxk59MJLUktqN2frIPfWRzgxS6+oqsuCx5WDJtbwmeXDtfcc5qPITmbyVQVTkB t8BA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:organization:mime-version:content-transfer-encoding; bh=yhs+abaWKi0p//g6YxUaeLZakEanlD/CzTTyNvJfsSA=; b=BVydL0aCFBQTysE/atopPz2NrDYOJS241PovDGlMAiOdNw4zXBgwJgT7MzOv7VEBvQ ml1520j85m7eYcKQB7qzRKH+RMpZkJ44FwBISTvcPg3ODYhGbJQRBgNEbmHc4OQDBxeR 1PMDPqf5XYCwfOTaymZa7S3IUIjJyhWDAdONLjvcIE2IHWUKqmnmWD0VinBMplTTzwR3 x3FSCgAQhYMgkhTd8ok8jo4Ok1kzfmeVPKjaAgW2J976jfZ9s1YGIhxBNe6BdNRZSPAA ELOel1dhX8joTCfk0sxqJZf1tMmgct3aXL7DS5/TSJyBw8mh55WEFXoa1HekI5JrQ6to 5emg== X-Gm-Message-State: AN3rC/6FrXrY/hcC7gFboSuvcE//Z4idIaA/HPB5Nd+CQd6ydPwkF+pq pI5T2vGf44eM18fedHk= X-Received: by 10.28.146.207 with SMTP id u198mr14787933wmd.103.1491918902256; Tue, 11 Apr 2017 06:55:02 -0700 (PDT) Received: from paques.dev.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id 3sm15428341wrv.33.2017.04.11.06.55.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Apr 2017 06:55:02 -0700 (PDT) Date: Tue, 11 Apr 2017 15:54:53 +0200 From: Pascal Mazon To: "Wiles, Keith" Cc: "dev@dpdk.org" Message-ID: <20170411155453.4cdc3e85@paques.dev.6wind.com> In-Reply-To: References: <20170410181850.44845-1-keith.wiles@intel.com> <20170411091851.62ab28ad@paques.dev.6wind.com> Organization: 6WIND X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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:55:02 -0000 On Tue, 11 Apr 2017 13:23:24 +0000 "Wiles, Keith" wrote: > > > On Apr 11, 2017, at 2: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 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 > >> --- > >> 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=,speed=N"); > >> +RTE_PMD_REGISTER_PARAM_STRING(net_tap, > >> + "iface=," > >> + "speed=N," > >> + "remote=," > >> + "mac=fixed"); > > > > Indeed, I forgot to update that when I introduced the remote! > > > > That's it for me, > > Thank you. > > > > Pascal > > Regards, > Keith >