Re: [PATCH v3 1/6] net/tap: add MAC address management ops

[Pascal Mazon <pascal.mazon@6wind.com>]

On Thu, 9 Mar 2017 14:05:51 +0000
Ferruh Yigit <ferruh.yigit@intel.com> 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 <pascal.mazon@6wind.com>
> > ---
> >  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