Date:   Tue, 2 May 2017 15:12:49 +0000
From:   "Chiappero, Marco" <marco.chiappero@...el.com>
To:     "maheshb@...gle.com" <maheshb@...gle.com>,
        linux-netdev <netdev@...r.kernel.org>
CC:     "David S . Miller" <davem@...emloft.net>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "Duyck, Alexander H" <alexander.h.duyck@...el.com>,
        "Grandhi, Sainath" <sainath.grandhi@...el.com>
Subject: RE: [PATCH net-next 0/9] support unique MAC addresses for slave
 devices


> -----Original Message-----
> From: Mahesh Bandewar (महेश बंडेवार)
> 
> On Thu, Apr 27, 2017 at 7:51 AM, Marco Chiappero
> <marco.chiappero@...el.com> wrote:
> > Currently every slave device gets assigned the same MAC address, by
> > having it copied from the master interface. Since some code paths
> > depend on this identity, changing the MAC address on slave interfaces
> > is not supported. However identical MAC addresses can pose problems to
> > management and orchestration software that correctly expect network
> > interfaces on the same segment to have unique addresses.
> >
> Please understand that there are two distinct drivers IPvlan and MACvlan. They
> both exist together for good reasons and are trying to cater for different needs. I
> would love to combine them together if we don't mess / miss the goodies each
> of them have to offer... otherwise *NO*! Having said that if management /
> orchestration software has problems then clearly you should not use IPvlan for
> that use case.

I don't want to start a conversation on whether that software is the problem or not, which is probably arguable anyway, but their combination with ipvlan is a perfectly valid use case, the same way macvlan is.
Generally speaking, it's very reasonable and indeed common to look up interfaces by their MAC address. Being able to change the MAC address is also a reasonable expectation, especially for a soft interface which does not even use unicast L2 addresses for forwarding decisions.

> > Patches 1-8 include style fixes and refactoring (patch 9 depends upon)
> > that improve the overal quality and make the intruduction of the
> > feature straightforward.
> >
> Lots of this fall into I-say-potato-you-say-... category. My way of thinking /
> organizing code is different than yours and you don't have to like mine and I
> don't have to like yours.

A few patches are definitely subject to personal taste, I have no problem dropping them although I still see improvements in readability. Having that said:
- patch 1 is based on checkpatch.pl, so it's substantially objective.
- patches 3 and 4 are well motivated and driven by very specific reasons: both improve readability, remove unnecessary jumps and 4 in particular removes a good amount of duplicated code (52 insertions, 135 deletions).

> > Patch 9 enables slave devices to own unique MAC addresses and change
> > such addresses live, fixing lack of support and a related bug, as MAC
> > address changes on master were not propagated to slave devices.
> > In order to preserve the main peculiarity of this driver, that is
> > exposing only a single MAC address for outbound traffic, frames
> > egressing from master are now effectively masquerated when working in
> > L2 mode.
> >
> This enhancement is, however, coming via packet-header rewrite for every
> Tx/Rx packet which defeats the purpose. 

This only applies to N-S traffic in L2 mode, but nothing prevents you from reversing the logic by modifying ipvlan_hard_header and moving the rewrite to E-W traffic instead. I've made this decision in order to fully emulate an L2 network for slaves and make the master interface appear as a sort of L2 NAT. Alternatively you can limit yourself to the mangling done upfront in ipvlan_hard_header, although when sniffing from a slave things will appear confusing.

Now, if by "purpose" you mean performance implications in L2 mode, I doubt a small memcpy done while handling the skb has a major impact. In fact from the few performance tests I run I could not see any noticeable difference. I'd be happy to investigate this further with additional benchmarks in order to make a more informed decision.

L3 and L3s modes are not affected at all (actually they are on the reception side but this can be modified), so this change can be introduced at no cost here.

> The only good thing that came in light
> is the mac-addr change propagation from master issue; but if the fix is coming as
> a side-effect of header rewrite then it's not an acceptable fix either. 

It's not at all a side-effect of the header rewrite, it's rather the opposite. Either you force MAC addresses to stay in sync or let them vary individually, which is what I'm proposing for the reasons mentioned earlier. How you achieve this is a different story.

Of course, the former case can be fixed by simply doing what macvlan does in passthru mode, that is, sync up the slave(s) upon address change on the master via observer. The latter case can be addressed in one of the above ways. I would like to have such conversation especially in light of the following comments from you:

392         /* TODO Probably use a different field than dev_addr so that the
393          * mac-address on the virtual device is portable and can be carried
394          * while the packets use the mac-addr on the physical device.
395          */

536         /* TODO Probably put random address here to be presented to the
537          * world but keep using the physical-dev address for the outgoing
538          * packets.
539          */

> This can be
> simply fixed by changing a line in ipvlan_hard_header().

This is conceptually the same approach as above, you would still need to mangle ARP and ND frames for N-S traffic to make it work though, which is the biggest part of patch 9. 

Please, review the patch and elaborate specific concerns.

Thank you,
Marco
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.

Powered by blists - more mailing lists