Date:   Fri, 12 Jan 2018 19:10:33 +0100
From:   Jiri Benc <jbenc@...hat.com>
To:     "Mahesh Bandewar (महेश बंडेवार)" <maheshb@...gle.com>
Cc:     liuqifa@...wei.com, David Miller <davem@...emloft.net>,
        dsahern@...il.com, mschiffer@...verse-factory.net,
        idosch@...lanox.com, fw@...len.de, kjlx@...pleofstupid.com,
        girish.moodalbail@...cle.com, sainath.grandhi@...el.com,
        linux-netdev <netdev@...r.kernel.org>, weiyongjun1@...wei.com,
        chenweilong@...wei.com, maowenan@...wei.com, wangyufen@...wei.com,
        yuehaibing@...wei.com, liujian56@...wei.com, liuzhe24@...wei.com,
        wangkefeng.wang@...wei.com, dingtianhong@...wei.com
Subject: Re: [PATCH V2] ipvlan: fix ipvlan MTU limits

On Fri, 12 Jan 2018 09:50:35 -0800, Mahesh Bandewar (महेश बंडेवार) wrote:
> (Looks like you missed the last update I mentioned)

I did not miss it. The proposed behavior is inconsistent and has no
clear pattern (I used the word "magic" for that). I guess examples will
help more. See below.

> Here is the approach in details -

> (a) At slave creation time - it's exactly how it's done currently
> where slave copies masters' mtu. at the same time max_mtu of the slave
> is set as the current mtu of the master.
> (b) If slave updates mtu - ipvlan_change_mtu() will be called and the
> slave's mtu will get set and it will set a flag indicating that slave
> has changed it's mtu (dissociation from master if the mtu is different
> from masters'). If slave mtu is set same as masters' then this flag
> will get reset-ed indicating it wants to follow master (current
> behavior).

Consider these two cases:

# ip l a link eth0 type ipvlan 
# ip l s ipvlan0 mtu 1400
# ip l s eth0 mtu 1400
# ip l s eth0 mtu 1500

Now MTU of ipvlan0 is 1400.

# ip l a link eth0 type ipvlan 
# ip l s eth0 mtu 1400
# ip l s ipvlan0 mtu 1400
# ip l s eth0 mtu 1500

Now MTU of ipvlan0 is 1500.

See why I call that behavior "magic"? Before the last step, it's
impossible to tell from the user space what will happen. And no, don't
propose to artificially cover the first example by forcing the auto
follow flag, it doesn't help. It just moves the magic behavior to
different scenarios.

> (c) When master updates mtu - ipvlan_adj_mtu() gets called where all
> slaves' max_mtu changes to the master's mtu value (clamping applies
> for the all slaves which are not following master). All the slaves
> which are following master (flag per slave) will get this new mtu.
> Another consequence of this is that slaves' flag might get reset-ed if
> the master's mtu is reduced to the value that was set earlier for the
> slave (and it will start following slave).

Okay, you're actually proposing that. So, another example:

# ip l a link eth0 type ipvlan 
# ip l s ipvlan0 mtu 1400
# ip l s eth0 mtu 1400
# ip l s eth0 mtu 1500

Now MTU of ipvlan0 is 1500.

# ip l a link eth0 type ipvlan 
# ip l s ipvlan0 mtu 1400
# ip l s eth0 mtu 1600
# ip l s eth0 mtu 1500

Now MTU of ipvlan0 is 1400. There's still no consistency here.

> The above should work? The user-space can query the mtu of the slave
> device just like any other device. I was thinking about 'mtu_adj' with
> some additional future extention but for now; we can live with a flag
> on the slave device(s).

It absolutely does not. Never introduce magic behavior, it just forces
you to add more layers of hacks later.

Really, the only way is to introduce user visible and user changeable
flag. Yes, it's more work. But that's something you'll have to swallow.
Introducing hacky behavior is not the way to go. We try hard to
preserve user space compatibility which is pretty much impossible if
you introduce magic hacky behavior. Do it right from the start.

 Jiri

Powered by blists - more mailing lists