[PATCH net] add support of IFF_XMIT_DST_RELEASE bit in vlan

[PATCH net] add support of IFF_XMIT_DST_RELEASE bit in vlan

[Vadim Fedorenko]

Some time ago Eric Dumazet suggested a "hack the IFF_XMIT_DST_RELEASE
flag on the vlan netdev". But the last comment was "does not support
properly bonding/team.(If the real_dev->privflags IFF_XMIT_DST_RELEASE
bit changes, we want to update all the vlans at the same time )"

I've extended that patch to support changes of IFF_XMIT_DST_RELEASE in
bonding/team.
Both bonding and team call netdev_change_features() after recalculation
of features including priv_flags IFF_XMIT_DST_RELEASE bit. So the only
thing needed to support is to recheck this bit in
vlan_transfer_features().

Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Vadim Fedorenko <vfedorenko@yandex-team.ru>
---
 net/8021q/vlan.c         | 3 +++
 net/8021q/vlan_netlink.c | 1 +
 2 files changed, 4 insertions(+)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 9649579..510986c 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -328,6 +328,9 @@ static void vlan_transfer_features(struct net_device *dev,
 	vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
 #endif
 
+	vlandev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
+	vlandev->priv_flags |= (vlan->real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
+
 	netdev_update_features(vlandev);
 }
 
diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
index 5e831de..9472de8 100644
--- a/net/8021q/vlan_netlink.c
+++ b/net/8021q/vlan_netlink.c
@@ -143,6 +143,7 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
 	vlan->vlan_proto = proto;
 	vlan->vlan_id	 = nla_get_u16(data[IFLA_VLAN_ID]);
 	vlan->real_dev	 = real_dev;
+	dev->priv_flags |= (real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
 	vlan->flags	 = VLAN_FLAG_REORDER_HDR;
 
 	err = vlan_check_real_dev(real_dev, vlan->vlan_proto, vlan->vlan_id);
-- 
1.9.1

Re: [PATCH net] add support of IFF_XMIT_DST_RELEASE bit in vlan

[Eric Dumazet]

On Thu, 2017-11-02 at 15:49 +0300, Vadim Fedorenko wrote:
> Some time ago Eric Dumazet suggested a "hack the IFF_XMIT_DST_RELEASE
> flag on the vlan netdev". But the last comment was "does not support
> properly bonding/team.(If the real_dev->privflags IFF_XMIT_DST_RELEASE
> bit changes, we want to update all the vlans at the same time )"
> 
> I've extended that patch to support changes of IFF_XMIT_DST_RELEASE in
> bonding/team.
> Both bonding and team call netdev_change_features() after recalculation
> of features including priv_flags IFF_XMIT_DST_RELEASE bit. So the only
> thing needed to support is to recheck this bit in
> vlan_transfer_features().
> 
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Vadim Fedorenko <vfedorenko@yandex-team.ru>
> ---
>  net/8021q/vlan.c         | 3 +++
>  net/8021q/vlan_netlink.c | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 9649579..510986c 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -328,6 +328,9 @@ static void vlan_transfer_features(struct net_device *dev,
>  	vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
>  #endif
>  
> +	vlandev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> +	vlandev->priv_flags |= (vlan->real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
> +
>  	netdev_update_features(vlandev);
>  }
>  
> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
> index 5e831de..9472de8 100644
> --- a/net/8021q/vlan_netlink.c
> +++ b/net/8021q/vlan_netlink.c
> @@ -143,6 +143,7 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
>  	vlan->vlan_proto = proto;
>  	vlan->vlan_id	 = nla_get_u16(data[IFLA_VLAN_ID]);
>  	vlan->real_dev	 = real_dev;
> +	dev->priv_flags |= (real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
>  	vlan->flags	 = VLAN_FLAG_REORDER_HDR;
>  
>  	err = vlan_check_real_dev(real_dev, vlan->vlan_proto, vlan->vlan_id);

What happens if a bonding is composed of two slaves, with different
IFF_XMIT_DST_RELEASE settings ?

Re: [PATCH net] add support of IFF_XMIT_DST_RELEASE bit in vlan

[Vadim Fedorenko]

On Thu, 2017-11-02 at 07:33 -7000, Eric Dumazet wrote:
> On Thu, 2017-11-02 at 15:49 +0300, Vadim Fedorenko wrote:
>> Some time ago Eric Dumazet suggested a "hack the IFF_XMIT_DST_RELEASE
>> flag on the vlan netdev". But the last comment was "does not support
>> properly bonding/team.(If the real_dev->privflags IFF_XMIT_DST_RELEASE
>> bit changes, we want to update all the vlans at the same time )"
>> 
>> I've extended that patch to support changes of IFF_XMIT_DST_RELEASE in
>> bonding/team.
>> Both bonding and team call netdev_change_features() after recalculation
>> of features including priv_flags IFF_XMIT_DST_RELEASE bit. So the only
>> thing needed to support is to recheck this bit in
>> vlan_transfer_features().
>> 
>> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
>> Signed-off-by: Vadim Fedorenko <vfedorenko@yandex-team.ru>
>> ---
>>  net/8021q/vlan.c         | 3 +++
>>  net/8021q/vlan_netlink.c | 1 +
>>  2 files changed, 4 insertions(+)
>> 
>> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
>> index 9649579..510986c 100644
>> --- a/net/8021q/vlan.c
>> +++ b/net/8021q/vlan.c
>> @@ -328,6 +328,9 @@ static void vlan_transfer_features(struct net_device *dev,
>>  	vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
>>  #endif
>>  
>> +	vlandev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
>> +	vlandev->priv_flags |= (vlan->real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
>> +
>>  	netdev_update_features(vlandev);
>>  }
>>  
>> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
>> index 5e831de..9472de8 100644
>> --- a/net/8021q/vlan_netlink.c
>> +++ b/net/8021q/vlan_netlink.c
>> @@ -143,6 +143,7 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
>>  	vlan->vlan_proto = proto;
>>  	vlan->vlan_id	 = nla_get_u16(data[IFLA_VLAN_ID]);
>>  	vlan->real_dev	 = real_dev;
>> +	dev->priv_flags |= (real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
>>  	vlan->flags	 = VLAN_FLAG_REORDER_HDR;
>>  
>>  	err = vlan_check_real_dev(real_dev, vlan->vlan_proto, vlan->vlan_id);
>
> What happens if a bonding is composed of two slaves, with different
> IFF_XMIT_DST_RELEASE settings ?

According to bond_compute_features() IFF_XMIT_DST_RELEASE flag is cleared from priv_flags
in such case, so it is cleared in vlan_transfer_features() and vlan device works as before
the patch.
The same code exists in team netdev.

Re: [PATCH net] add support of IFF_XMIT_DST_RELEASE bit in vlan

[Eric Dumazet]

On Thu, 2017-11-02 at 17:47 +0300, Vadim Fedorenko wrote:
> On Thu, 2017-11-02 at 07:33 -7000, Eric Dumazet wrote:
> > On Thu, 2017-11-02 at 15:49 +0300, Vadim Fedorenko wrote:
> >> Some time ago Eric Dumazet suggested a "hack the IFF_XMIT_DST_RELEASE
> >> flag on the vlan netdev". But the last comment was "does not support
> >> properly bonding/team.(If the real_dev->privflags IFF_XMIT_DST_RELEASE
> >> bit changes, we want to update all the vlans at the same time )"
> >> 
> >> I've extended that patch to support changes of IFF_XMIT_DST_RELEASE in
> >> bonding/team.
> >> Both bonding and team call netdev_change_features() after recalculation
> >> of features including priv_flags IFF_XMIT_DST_RELEASE bit. So the only
> >> thing needed to support is to recheck this bit in
> >> vlan_transfer_features().
> >> 
> >> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> >> Signed-off-by: Vadim Fedorenko <vfedorenko@yandex-team.ru>
> >> ---
> >>  net/8021q/vlan.c         | 3 +++
> >>  net/8021q/vlan_netlink.c | 1 +
> >>  2 files changed, 4 insertions(+)
> >> 
> >> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> >> index 9649579..510986c 100644
> >> --- a/net/8021q/vlan.c
> >> +++ b/net/8021q/vlan.c
> >> @@ -328,6 +328,9 @@ static void vlan_transfer_features(struct net_device *dev,
> >>  	vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
> >>  #endif
> >>  
> >> +	vlandev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
> >> +	vlandev->priv_flags |= (vlan->real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
> >> +
> >>  	netdev_update_features(vlandev);
> >>  }
> >>  
> >> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
> >> index 5e831de..9472de8 100644
> >> --- a/net/8021q/vlan_netlink.c
> >> +++ b/net/8021q/vlan_netlink.c
> >> @@ -143,6 +143,7 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
> >>  	vlan->vlan_proto = proto;
> >>  	vlan->vlan_id	 = nla_get_u16(data[IFLA_VLAN_ID]);
> >>  	vlan->real_dev	 = real_dev;
> >> +	dev->priv_flags |= (real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
> >>  	vlan->flags	 = VLAN_FLAG_REORDER_HDR;
> >>  
> >>  	err = vlan_check_real_dev(real_dev, vlan->vlan_proto, vlan->vlan_id);
> >
> > What happens if a bonding is composed of two slaves, with different
> > IFF_XMIT_DST_RELEASE settings ?
> 
> According to bond_compute_features() IFF_XMIT_DST_RELEASE flag is cleared from priv_flags
> in such case, so it is cleared in vlan_transfer_features() and vlan device works as before
> the patch.
> The same code exists in team netdev.

I repeat the question.

What happens if a bonding is composed of two slaves A & B,
A has IFF_XMIT_DST_RELEASE set
B has IFF_XMIT_DST_RELEASE cleared.

1) A is added , then B is added.
2) B is added to the bond, then A is added.
3) Before, and after your patches.

Thanks.

Re: [PATCH net] add support of IFF_XMIT_DST_RELEASE bit in vlan

[Vadim Fedorenko]

On 02.11.2017 19:25, Eric Dumazet wrote:
> On Thu, 2017-11-02 at 17:47 +0300, Vadim Fedorenko wrote:
>> On Thu, 2017-11-02 at 07:33 -7000, Eric Dumazet wrote:
>>> On Thu, 2017-11-02 at 15:49 +0300, Vadim Fedorenko wrote:
>>>> Some time ago Eric Dumazet suggested a "hack the IFF_XMIT_DST_RELEASE
>>>> flag on the vlan netdev". But the last comment was "does not support
>>>> properly bonding/team.(If the real_dev->privflags IFF_XMIT_DST_RELEASE
>>>> bit changes, we want to update all the vlans at the same time )"
>>>>
>>>> I've extended that patch to support changes of IFF_XMIT_DST_RELEASE in
>>>> bonding/team.
>>>> Both bonding and team call netdev_change_features() after recalculation
>>>> of features including priv_flags IFF_XMIT_DST_RELEASE bit. So the only
>>>> thing needed to support is to recheck this bit in
>>>> vlan_transfer_features().
>>>>
>>>> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
>>>> Signed-off-by: Vadim Fedorenko <vfedorenko@yandex-team.ru>
>>>> ---
>>>>   net/8021q/vlan.c         | 3 +++
>>>>   net/8021q/vlan_netlink.c | 1 +
>>>>   2 files changed, 4 insertions(+)
>>>>
>>>> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
>>>> index 9649579..510986c 100644
>>>> --- a/net/8021q/vlan.c
>>>> +++ b/net/8021q/vlan.c
>>>> @@ -328,6 +328,9 @@ static void vlan_transfer_features(struct net_device *dev,
>>>>   	vlandev->fcoe_ddp_xid = dev->fcoe_ddp_xid;
>>>>   #endif
>>>>   
>>>> +	vlandev->priv_flags &= ~IFF_XMIT_DST_RELEASE;
>>>> +	vlandev->priv_flags |= (vlan->real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
>>>> +
>>>>   	netdev_update_features(vlandev);
>>>>   }
>>>>   
>>>> diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
>>>> index 5e831de..9472de8 100644
>>>> --- a/net/8021q/vlan_netlink.c
>>>> +++ b/net/8021q/vlan_netlink.c
>>>> @@ -143,6 +143,7 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
>>>>   	vlan->vlan_proto = proto;
>>>>   	vlan->vlan_id	 = nla_get_u16(data[IFLA_VLAN_ID]);
>>>>   	vlan->real_dev	 = real_dev;
>>>> +	dev->priv_flags |= (real_dev->priv_flags & IFF_XMIT_DST_RELEASE);
>>>>   	vlan->flags	 = VLAN_FLAG_REORDER_HDR;
>>>>   
>>>>   	err = vlan_check_real_dev(real_dev, vlan->vlan_proto, vlan->vlan_id);
>>>
>>> What happens if a bonding is composed of two slaves, with different
>>> IFF_XMIT_DST_RELEASE settings ?
>>
>> According to bond_compute_features() IFF_XMIT_DST_RELEASE flag is cleared from priv_flags
>> in such case, so it is cleared in vlan_transfer_features() and vlan device works as before
>> the patch.
>> The same code exists in team netdev.
> 
> I repeat the question.
> 
> What happens if a bonding is composed of two slaves A & B,
> A has IFF_XMIT_DST_RELEASE set
> B has IFF_XMIT_DST_RELEASE cleared.
> 
> 1) A is added , then B is added.
> 2) B is added to the bond, then A is added.
> 3) Before, and after your patches.
> 
> Thanks.
> 
> 
Do you mean what happens with vlan device with real_dev is bonding ?

With patches:
1) A is added
   bond_enslave()
     bond_compute_features()
             -> bond_dev IFF_XMIT_DST_RELEASE is not changed (set)
       netdev_change_features()
         vlan_device_event(event=NETDEV_FEAT_CHANGE)
           vlan_transfer_features()
             -> vlan_dev IFF_XMIT_DST_RELEASE is not changed (still set)
Then B is added
   bond_enslave()
     bond_compute_features()
             -> bond_dev IFF_XMIT_DST_RELEASE is changed (cleared)
       netdev_change_features()
         vlan_device_event(event=NETDEV_FEAT_CHANGE)
           vlan_transfer_features()
             -> vlan_dev IFF_XMIT_DST_RELEASE is changed (cleared)

2) B is added
   bond_enslave()
     bond_compute_features()
             -> bond_dev IFF_XMIT_DST_RELEASE is changed (cleared)
       netdev_change_features()
         vlan_device_event(event=NETDEV_FEAT_CHANGE)
           vlan_transfer_features()
             -> vlan_dev IFF_XMIT_DST_RELEASE is changed (cleared)
Then A is added
   bond_enslave()
     bond_compute_features()
             -> bond_dev IFF_XMIT_DST_RELEASE is not changed (cleared)
       netdev_change_features()
         vlan_device_event(event=NETDEV_FEAT_CHANGE)
           vlan_transfer_features()
             -> vlan_dev IFF_XMIT_DST_RELEASE is not changed (cleared).

Without patches:
1) A is added
   bond_enslave()
     bond_compute_features()
             -> bond_dev IFF_XMIT_DST_RELEASE is not changed (set)
       netdev_change_features()
         vlan_device_event(event=NETDEV_FEAT_CHANGE)
           vlan_transfer_features()
              -> vlan_dev IFF_XMIT_DST_RELEASE is not changed (cleared)

Then B is added
   bond_enslave()
     bond_compute_features()
              -> bond_dev IFF_XMIT_DST_RELEASE is changed (cleared)
       netdev_change_features()
         vlan_device_event(event=NETDEV_FEAT_CHANGE)
           vlan_transfer_features()
              -> vlan_dev IFF_XMIT_DST_RELEASE is not changed(cleared)
2) B is added
   bond_enslave()
     bond_compute_features()
              -> bond_dev IFF_XMIT_DST_RELEASE is changed (cleared)
       netdev_change_features()
         vlan_device_event(event=NETDEV_FEAT_CHANGE)
           vlan_transfer_features()
              -> vlan_dev IFF_XMIT_DST_RELEASE is not changed(cleared)
Then A is added
   bond_enslave()
     bond_compute_features()
              -> bond_dev IFF_XMIT_DST_RELEASE is not changed (cleared)
       netdev_change_features()
         vlan_device_event(event=NETDEV_FEAT_CHANGE)
           vlan_transfer_features()
              -> vlan_dev IFF_XMIT_DST_RELEASE is not changed (cleared).

Do you mean that the problem is in the first case in the gap between 
netdev_change_features() and vlan_device_event() when bond_dev 
IFF_XMIT_DST_RELEASE is cleared and vlan_dev IFF_XMIT_DST_RELEASE is 
still set?

Re: [PATCH net] add support of IFF_XMIT_DST_RELEASE bit in vlan

[Eric Dumazet]

On Fri, 2017-11-03 at 01:39 +0300, Vadim Fedorenko wrote:

> Do you mean what happens with vlan device with real_dev is bonding ?
> 
> With patches:
> 1) A is added
>    bond_enslave()
>      bond_compute_features()
>              -> bond_dev IFF_XMIT_DST_RELEASE is not changed (set)
>        netdev_change_features()
>          vlan_device_event(event=NETDEV_FEAT_CHANGE)
>            vlan_transfer_features()
>              -> vlan_dev IFF_XMIT_DST_RELEASE is not changed (still set)
> Then B is added
>    bond_enslave()
>      bond_compute_features()
>              -> bond_dev IFF_XMIT_DST_RELEASE is changed (cleared)
>        netdev_change_features()
>          vlan_device_event(event=NETDEV_FEAT_CHANGE)
>            vlan_transfer_features()
>              -> vlan_dev IFF_XMIT_DST_RELEASE is changed (cleared)
> 
> 2) B is added
>    bond_enslave()
>      bond_compute_features()
>              -> bond_dev IFF_XMIT_DST_RELEASE is changed (cleared)
>        netdev_change_features()
>          vlan_device_event(event=NETDEV_FEAT_CHANGE)
>            vlan_transfer_features()
>              -> vlan_dev IFF_XMIT_DST_RELEASE is changed (cleared)
> Then A is added
>    bond_enslave()
>      bond_compute_features()
>              -> bond_dev IFF_XMIT_DST_RELEASE is not changed (cleared)
>        netdev_change_features()
>          vlan_device_event(event=NETDEV_FEAT_CHANGE)
>            vlan_transfer_features()
>              -> vlan_dev IFF_XMIT_DST_RELEASE is not changed (cleared).
> 
> Without patches:
> 1) A is added
>    bond_enslave()
>      bond_compute_features()
>              -> bond_dev IFF_XMIT_DST_RELEASE is not changed (set)
>        netdev_change_features()
>          vlan_device_event(event=NETDEV_FEAT_CHANGE)
>            vlan_transfer_features()
>               -> vlan_dev IFF_XMIT_DST_RELEASE is not changed (cleared)
> 
> Then B is added
>    bond_enslave()
>      bond_compute_features()
>               -> bond_dev IFF_XMIT_DST_RELEASE is changed (cleared)
>        netdev_change_features()
>          vlan_device_event(event=NETDEV_FEAT_CHANGE)
>            vlan_transfer_features()
>               -> vlan_dev IFF_XMIT_DST_RELEASE is not changed(cleared)
> 2) B is added
>    bond_enslave()
>      bond_compute_features()
>               -> bond_dev IFF_XMIT_DST_RELEASE is changed (cleared)
>        netdev_change_features()
>          vlan_device_event(event=NETDEV_FEAT_CHANGE)
>            vlan_transfer_features()
>               -> vlan_dev IFF_XMIT_DST_RELEASE is not changed(cleared)
> Then A is added
>    bond_enslave()
>      bond_compute_features()
>               -> bond_dev IFF_XMIT_DST_RELEASE is not changed (cleared)
>        netdev_change_features()
>          vlan_device_event(event=NETDEV_FEAT_CHANGE)
>            vlan_transfer_features()
>               -> vlan_dev IFF_XMIT_DST_RELEASE is not changed (cleared).
> 

Thanks for this investigation !

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] add support of IFF_XMIT_DST_RELEASE bit in vlan

[David Miller]

From: Vadim Fedorenko <vfedorenko@yandex-team.ru>
Date: Thu, 2 Nov 2017 15:49:08 +0300

> Some time ago Eric Dumazet suggested a "hack the IFF_XMIT_DST_RELEASE
> flag on the vlan netdev". But the last comment was "does not support
> properly bonding/team.(If the real_dev->privflags IFF_XMIT_DST_RELEASE
> bit changes, we want to update all the vlans at the same time )"
> 
> I've extended that patch to support changes of IFF_XMIT_DST_RELEASE in
> bonding/team.
> Both bonding and team call netdev_change_features() after recalculation
> of features including priv_flags IFF_XMIT_DST_RELEASE bit. So the only
> thing needed to support is to recheck this bit in
> vlan_transfer_features().
> 
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Vadim Fedorenko <vfedorenko@yandex-team.ru>

Applied, thank you.