From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f47.google.com (mail-wm0-f47.google.com [74.125.82.47]) by dpdk.org (Postfix) with ESMTP id D75BB2BD1 for ; Tue, 11 Apr 2017 09:19:01 +0200 (CEST) Received: by mail-wm0-f47.google.com with SMTP id o81so55629841wmb.1 for ; Tue, 11 Apr 2017 00:19:01 -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=TmEsE0KmXpZG9+JCH/wpz2hL+agSMVy4VtaOb1ap5k8=; b=uW6D6SswuaEepm7YQP7pZpDpAlbWpWXXdryRbjy6ddM/nZ7fEUezbyj05Oo/6Fv9yW uwNCknu7vGiCIWTDB56MDEALVJ0Uk5/TObE40r2s7C56HMIP2gTxE7vmZXIfkXI2PlYH fsZ0VsCsWHmO2B3JfdhJIoUAuDM/5zULdRDjSRJUCuJDmmd+ZVW8ZEw8kLUF3Hu8+k2w 5GekepCZODjbw68VuA5c7jLUFvP/ji+DG7JHeU2ydJWSWsOVX7DkPuqCwFZnX/RlPWWc gV1Y+JUFOQudiLN7NR9IJ5gq+YmXrIjXb0GNMAYPBD1MoUCIEMdW9LADDr15ZOtr1AQH uRSQ== 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=TmEsE0KmXpZG9+JCH/wpz2hL+agSMVy4VtaOb1ap5k8=; b=eK8D6xjHgUDOR0chqlVk88hNZNDkEW32IbwEBIQMkewCn6m2ylu+qw+HmhfQX0pLh2 NfJzjNcXQIhLO79GUGfVXXR+cenNbkzIBOtQxHtlJTD2zmDBNcvxDtcFwf9FvHoWBBcG 9bF3O6b28aFaP8XDoxJbJs1GES+wZr5wh+tpaM6QJzsxGvdOD0IGP1amvoVHaJVcjlwW KVZ8V8qvYTbVJkkkgPJ6rPrdvDy/LdfZzIdrc0tW9cqDQH4IxMOC3l8XVPVkc5NqmIkV HsHLnNsLVfXETwSrzFHr7pu9tKRB0lGAS7i2Xs6AkfM6VAfSu0YOP+mxFN9tiknKhCQN HsWw== X-Gm-Message-State: AN3rC/4WfCJ1khiF8QMUnTdKDqV+N6FL7CfctA0l44318K7LzlqJNqhH zhuu/7/Qo5j0iKD26NE= X-Received: by 10.28.28.69 with SMTP id c66mr14049372wmc.28.1491895141068; Tue, 11 Apr 2017 00:19:01 -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 l68sm9833560wrc.52.2017.04.11.00.19.00 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Apr 2017 00:19:00 -0700 (PDT) Date: Tue, 11 Apr 2017 09:18:51 +0200 From: Pascal Mazon To: Keith Wiles Cc: "dev@dpdk.org" Message-ID: <20170411091851.62ab28ad@paques.dev.6wind.com> In-Reply-To: <20170410181850.44845-1-keith.wiles@intel.com> References: <20170410181850.44845-1-keith.wiles@intel.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 07:19:02 -0000 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). > > #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 { }. > } > > 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. > + if (value && !strcasecmp("fixed", value)) I think a macro for "fixed" would be better. Maybe a ETH_TAP_MAC_FIXED "fixed" at the top? > + *(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(). > 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