From: Pascal Mazon <pascal.mazon@6wind.com>
To: Keith Wiles <keith.wiles@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] net/tap: add support for fixed mac addresses
Date: Tue, 11 Apr 2017 09:18:51 +0200 [thread overview]
Message-ID: <20170411091851.62ab28ad@paques.dev.6wind.com> (raw)
In-Reply-To: <20170410181850.44845-1-keith.wiles@intel.com>
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@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@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).
>
> #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=<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
next prev parent reply other threads:[~2017-04-11 7:19 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-10 18:18 Keith Wiles
2017-04-11 7:18 ` Pascal Mazon [this message]
2017-04-11 8:31 ` Ferruh Yigit
2017-04-11 13:23 ` Wiles, Keith
2017-04-11 13:25 ` Wiles, Keith
2017-04-11 13:39 ` Ferruh Yigit
2017-04-11 13:41 ` Wiles, Keith
2017-04-11 13:54 ` Pascal Mazon
2017-04-11 14:17 ` Wiles, Keith
2017-04-11 15:09 ` Pascal Mazon
2017-04-11 15:38 ` Wiles, Keith
2017-04-11 15:42 ` Wiles, Keith
2017-04-11 15:49 ` Ferruh Yigit
2017-04-11 16:16 ` Wiles, Keith
2017-04-12 7:30 ` [dpdk-dev] [PATCH v2] " Pascal Mazon
2017-04-14 9:45 ` Ferruh Yigit
2017-05-12 12:24 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170411091851.62ab28ad@paques.dev.6wind.com \
--to=pascal.mazon@6wind.com \
--cc=dev@dpdk.org \
--cc=keith.wiles@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).