From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pascal Mazon Subject: Re: [PATCH v3 1/6] net/tap: add MAC address management ops Date: Fri, 10 Mar 2017 10:01:52 +0100 Message-ID: <20170310100152.7b9297db@paques.dev.6wind.com> References: <1488904298-31395-2-git-send-email-pascal.mazon@6wind.com> <53d1a212-9354-ee10-72fb-2f8bfb1d29f6@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: keith.wiles@intel.com, dev@dpdk.org To: Ferruh Yigit Return-path: Received: from mail-wr0-f172.google.com (mail-wr0-f172.google.com [209.85.128.172]) by dpdk.org (Postfix) with ESMTP id 99CE02C49 for ; Fri, 10 Mar 2017 10:02:32 +0100 (CET) Received: by mail-wr0-f172.google.com with SMTP id u108so60556939wrb.3 for ; Fri, 10 Mar 2017 01:02:32 -0800 (PST) In-Reply-To: <53d1a212-9354-ee10-72fb-2f8bfb1d29f6@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, 9 Mar 2017 14:05:51 +0000 Ferruh Yigit wrote: > On 3/7/2017 4:31 PM, Pascal Mazon wrote: > > Set a random MAC address when probing the device, as to not leave an > > empty MAC in pmd->eth_addr. > > > > This MAC will be set on the tap netdevice as soon as it's been > > created using tun_alloc(). As the tap_mac_add() function depend on > > the fd in the first rxq, move code from tun_alloc() to > > tap_setup_queue(), after it's been set. > > > > Signed-off-by: Pascal Mazon > > --- > > doc/guides/nics/features/tap.ini | 1 + > > drivers/net/tap/rte_eth_tap.c | 97 > > ++++++++++++++++++++++++++++++++++------ 2 files changed, 85 > > insertions(+), 13 deletions(-) > > > > diff --git a/doc/guides/nics/features/tap.ini > > b/doc/guides/nics/features/tap.ini index f4aca6921ddc..d9b47a003654 > > 100644 --- a/doc/guides/nics/features/tap.ini > > +++ b/doc/guides/nics/features/tap.ini > > @@ -9,6 +9,7 @@ Jumbo frame = Y > > Promiscuous mode = Y > > Allmulticast mode = Y > > Basic stats = Y > > +Unicast MAC filter = Y > > Other kdrv = Y > > ARMv7 = Y > > ARMv8 = Y > > diff --git a/drivers/net/tap/rte_eth_tap.c > > b/drivers/net/tap/rte_eth_tap.c index ece3a5fcc897..1e46ee36efa2 > > 100644 --- a/drivers/net/tap/rte_eth_tap.c > > +++ b/drivers/net/tap/rte_eth_tap.c > > @@ -63,6 +63,8 @@ > > #define RTE_PMD_TAP_MAX_QUEUES 1 > > #endif > > > > +#define RTE_PMD_TAP_MAX_MAC_ADDRS 1 > > mac_addr_add and mac_addr_remove not really supported, because only > one MAC is supported. For mac_addr_add() all indexes other than 0 > will give an error. So only mac_addr_set is supported. > > For this case what is the benefit of implementing these functions and > claim support, instead of just leaving mac_addr_add and > mac_addr_remove NULL? > Well, I wanted to implement those along with the mac_addr_set, as they all dealt with mac addresses. But you're right, I might as well leave the ops NULL. I'll send a new version reflecting this. > > <...> > > > + if (qid == 0) { > > + /* > > + * tap_setup_queue() is called for both tx and rx. > > + * Let's use dev->data->r/tx_queues[qid] to > > determine if init > > + * has already been done. > > + */ > > + if (dev->data->rx_queues[qid] && > > dev->data->tx_queues[qid]) > > + return fd; > > + > > + tap_mac_set(dev, &pmd->eth_addr); > > What is the reason of changing behavior here? > > Tap devices assigned random MAC by kernel, and previous implementation > was reading that value and using it for DPDK. > > Now kernel assigns a random MAC, and DPDK overwrites it another random > MAC, previous implementation was simpler I think. > > It is OK to move this code tap_setup_queue(), I just missed the > benefit of overwriting with DPDK random MAC? > > <...> As far as I remember, I did it because somewhere the mac_addr_set was checked as part of reconfiguration, which happenened before queue setup was done. The default mac address (dev->data->mac_addrs[0]) got set to 0 and later call for the default mac address tried using this mac address. Or something along those lines. I'll definitely re-take a closer look at all this for my next version. Regards, Pascal