On Tue, 11 Apr 2017 13:23:24 +0000
"Wiles, Keith" <keith.wi...@intel.com> wrote:
>
> > On Apr 11, 2017, at 2:18 AM, Pascal Mazon <pascal.ma...@6wind.com> wrote:
> >
> > Hi Keith,
> >
> > I have a few comments on your patch, see inline.
> >
> > On Mon, 10 Apr 2017 13:18:50 -0500
> > Keith Wiles <keith.wi...@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.wi...@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).
>
> 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=<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
>
> Regards,
> Keith
>
