diff mbox

[2/3] bridge: trigger a bridge calculation upon port changes

Message ID 20140318212243.GD28471@garbanzo.do-not-panic.com (mailing list archive)
State New, archived
Headers show

Commit Message

Luis R. Rodriguez March 18, 2014, 9:22 p.m. UTC
On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
> On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
> >> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> wrote:
> >> >                         spin_unlock_bh(&p->br->lock);
> >> > +                       if (changed)
> >> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> >> > +                                                        p->br->dev);
> >> > +                       netdev_update_features(p->br->dev);
> >>
> >> I think this is supposed to be in netdev event handler of br->dev
> >> instead of here.
> >
> > Do you mean netdev_update_features() ? I mimic'd what was being done on
> > br_del_if() given that root blocking is doing something similar. If
> > we need to change something for the above then I suppose it means we need
> > to change br_del_if() too. Let me know if you see any reason for something
> > else.
> >
> 
> Yeah, for me it looks like it's better to call netdev_update_features()
> in the event handler of br->dev, rather than where calling
> call_netdevice_notifiers(..., br->dev);.

I still don't see why, in fact trying to verify this I am wondering now
if instead we should actually fix br_features_recompute() to take into
consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
is called above even if the MAC address did not change, just as is done
on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
appropriate we just call

call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)

for both the above then and also br_del_if()? How about the below
change?

Comments

Luis Chamberlain April 22, 2014, 7:43 p.m. UTC | #1
On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
> > On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
> > >> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
> > >> <mcgrof@do-not-panic.com> wrote:
> > >> >                         spin_unlock_bh(&p->br->lock);
> > >> > +                       if (changed)
> > >> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> > >> > +                                                        p->br->dev);
> > >> > +                       netdev_update_features(p->br->dev);
> > >>
> > >> I think this is supposed to be in netdev event handler of br->dev
> > >> instead of here.
> > >
> > > Do you mean netdev_update_features() ? I mimic'd what was being done on
> > > br_del_if() given that root blocking is doing something similar. If
> > > we need to change something for the above then I suppose it means we need
> > > to change br_del_if() too. Let me know if you see any reason for something
> > > else.
> > >
> > 
> > Yeah, for me it looks like it's better to call netdev_update_features()
> > in the event handler of br->dev, rather than where calling
> > call_netdevice_notifiers(..., br->dev);.
> 
> I still don't see why, in fact trying to verify this I am wondering now
> if instead we should actually fix br_features_recompute() to take into
> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
> is called above even if the MAC address did not change, just as is done
> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
> appropriate we just call
> 
> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
> 
> for both the above then and also br_del_if()? How about the below
> change?
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 54d207d..dcd9378 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
>  	features &= ~NETIF_F_ONE_FOR_ALL;
>  
>  	list_for_each_entry(p, &br->port_list, list) {
> +		if (p->flags & BR_ROOT_BLOCK)
> +			continue;
>  		features = netdev_increment_features(features,
>  						     p->dev->features, mask);
>  	}

Cong, can you provide feedback on this? I tried to grow confidence on the
hunk above but its not clear but the other points still hold and I'd love
your feedback on those.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis R. Rodriguez April 30, 2014, 6:38 p.m. UTC | #2
On Tue, Apr 22, 2014 at 12:43 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
>> > On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> > > On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>> > >> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>> > >> <mcgrof@do-not-panic.com> wrote:
>> > >> >                         spin_unlock_bh(&p->br->lock);
>> > >> > +                       if (changed)
>> > >> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>> > >> > +                                                        p->br->dev);
>> > >> > +                       netdev_update_features(p->br->dev);
>> > >>
>> > >> I think this is supposed to be in netdev event handler of br->dev
>> > >> instead of here.
>> > >
>> > > Do you mean netdev_update_features() ? I mimic'd what was being done on
>> > > br_del_if() given that root blocking is doing something similar. If
>> > > we need to change something for the above then I suppose it means we need
>> > > to change br_del_if() too. Let me know if you see any reason for something
>> > > else.
>> > >
>> >
>> > Yeah, for me it looks like it's better to call netdev_update_features()
>> > in the event handler of br->dev, rather than where calling
>> > call_netdevice_notifiers(..., br->dev);.
>>
>> I still don't see why, in fact trying to verify this I am wondering now
>> if instead we should actually fix br_features_recompute() to take into
>> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
>> is called above even if the MAC address did not change, just as is done
>> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
>> appropriate we just call
>>
>> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
>>
>> for both the above then and also br_del_if()? How about the below
>> change?
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 54d207d..dcd9378 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
>>       features &= ~NETIF_F_ONE_FOR_ALL;
>>
>>       list_for_each_entry(p, &br->port_list, list) {
>> +             if (p->flags & BR_ROOT_BLOCK)
>> +                     continue;
>>               features = netdev_increment_features(features,
>>                                                    p->dev->features, mask);
>>       }
>
> Cong, can you provide feedback on this? I tried to grow confidence on the
> hunk above but its not clear but the other points still hold and I'd love
> your feedback on those.

Re-poke.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich April 30, 2014, 8:04 p.m. UTC | #3
On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
>>> On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>>> On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>>>>> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>>>>> <mcgrof@do-not-panic.com> wrote:
>>>>>>                         spin_unlock_bh(&p->br->lock);
>>>>>> +                       if (changed)
>>>>>> +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>>>>>> +                                                        p->br->dev);
>>>>>> +                       netdev_update_features(p->br->dev);
>>>>>
>>>>> I think this is supposed to be in netdev event handler of br->dev
>>>>> instead of here.
>>>>
>>>> Do you mean netdev_update_features() ? I mimic'd what was being done on
>>>> br_del_if() given that root blocking is doing something similar. If
>>>> we need to change something for the above then I suppose it means we need
>>>> to change br_del_if() too. Let me know if you see any reason for something
>>>> else.
>>>>
>>>
>>> Yeah, for me it looks like it's better to call netdev_update_features()
>>> in the event handler of br->dev, rather than where calling
>>> call_netdevice_notifiers(..., br->dev);.
>>
>> I still don't see why, in fact trying to verify this I am wondering now
>> if instead we should actually fix br_features_recompute() to take into
>> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
>> is called above even if the MAC address did not change, just as is done
>> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
>> appropriate we just call
>>
>> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
>>
>> for both the above then and also br_del_if()? How about the below
>> change?
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 54d207d..dcd9378 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
>>  	features &= ~NETIF_F_ONE_FOR_ALL;
>>  
>>  	list_for_each_entry(p, &br->port_list, list) {
>> +		if (p->flags & BR_ROOT_BLOCK)
>> +			continue;
>>  		features = netdev_increment_features(features,
>>  						     p->dev->features, mask);
>>  	}
> 
> Cong, can you provide feedback on this? I tried to grow confidence on the
> hunk above but its not clear but the other points still hold and I'd love
> your feedback on those.
> 

Hi Luis

The hunk above isn't right.  Just because you set ROOT_BLOCK on the port
doesn't mean that you should ignore it's device features.  If the device
you just added happens to disable or enable some device offload feature,
you should take that into account.

Thanks
-vlad

>   Luis
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis Chamberlain April 30, 2014, 10:59 p.m. UTC | #4
On Wed, Apr 30, 2014 at 04:04:34PM -0400, Vlad Yasevich wrote:
> On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
> > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> > index 54d207d..dcd9378 100644
> > --- a/net/bridge/br_if.c
> > +++ b/net/bridge/br_if.c
> > @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
> >  	features &= ~NETIF_F_ONE_FOR_ALL;
> >  
> >  	list_for_each_entry(p, &br->port_list, list) {
> > +		if (p->flags & BR_ROOT_BLOCK)
> > +			continue;
> >  		features = netdev_increment_features(features,
> >  						     p->dev->features, mask);
> >  	}
> >
> Hi Luis
> 
> The hunk above isn't right.  Just because you set ROOT_BLOCK on the port
> doesn't mean that you should ignore it's device features.  If the device
> you just added happens to disable or enable some device offload feature,
> you should take that into account.

OK thanks, how about this part:

On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
>> On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>> On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>>>> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>>>> <mcgrof@do-not-panic.com> wrote:
>>>>>                         spin_unlock_bh(&p->br->lock);
>>>>> +                       if (changed)
>>>>> +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>>>>> +                                                        p->br->dev);
>>>>> +                       netdev_update_features(p->br->dev);
>>>>
>>>> I think this is supposed to be in netdev event handler of br->dev
>>>> instead of here.
>>>
>>> Do you mean netdev_update_features() ? I mimic'd what was being done on
>>> br_del_if() given that root blocking is doing something similar. If
>>> we need to change something for the above then I suppose it means we need
>>> to change br_del_if() too. Let me know if you see any reason for something
>>> else.
>>>
>>
>> Yeah, for me it looks like it's better to call netdev_update_features()
>> in the event handler of br->dev, rather than where calling
>> call_netdevice_notifiers(..., br->dev);.
>
> I still don't see why, in fact trying to verify this I am wondering now
> if instead we should actually fix br_features_recompute() to take into
> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
> is called above even if the MAC address did not change, just as is done
> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
> appropriate we just call
>
> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
>
> for both the above then and also br_del_if()?

  Luis
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vlad Yasevich May 1, 2014, 12:12 a.m. UTC | #5
On 04/30/2014 06:59 PM, Luis R. Rodriguez wrote:
> On Wed, Apr 30, 2014 at 04:04:34PM -0400, Vlad Yasevich wrote:
>> On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>> index 54d207d..dcd9378 100644
>>> --- a/net/bridge/br_if.c
>>> +++ b/net/bridge/br_if.c
>>> @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
>>>  	features &= ~NETIF_F_ONE_FOR_ALL;
>>>  
>>>  	list_for_each_entry(p, &br->port_list, list) {
>>> +		if (p->flags & BR_ROOT_BLOCK)
>>> +			continue;
>>>  		features = netdev_increment_features(features,
>>>  						     p->dev->features, mask);
>>>  	}
>>>
>> Hi Luis
>>
>> The hunk above isn't right.  Just because you set ROOT_BLOCK on the port
>> doesn't mean that you should ignore it's device features.  If the device
>> you just added happens to disable or enable some device offload feature,
>> you should take that into account.
> 
> OK thanks, how about this part:
> 
> On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
>>> On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>>> On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>>>>> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>>>>> <mcgrof@do-not-panic.com> wrote:
>>>>>>                         spin_unlock_bh(&p->br->lock);
>>>>>> +                       if (changed)
>>>>>> +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>>>>>> +                                                        p->br->dev);
>>>>>> +                       netdev_update_features(p->br->dev);
>>>>>

This is actually just a part of it.  You also need to handle the sysfs
changing the flag.

Look at the first 2 patches in this series:
http://www.spinics.net/lists/netdev/msg280863.html

You might need that functionality.

-vlad


>>>>> I think this is supposed to be in netdev event handler of br->dev
>>>>> instead of here.
>>>>
>>>> Do you mean netdev_update_features() ? I mimic'd what was being done on
>>>> br_del_if() given that root blocking is doing something similar. If
>>>> we need to change something for the above then I suppose it means we need
>>>> to change br_del_if() too. Let me know if you see any reason for something
>>>> else.
>>>>
>>>
>>> Yeah, for me it looks like it's better to call netdev_update_features()
>>> in the event handler of br->dev, rather than where calling
>>> call_netdevice_notifiers(..., br->dev);.
>>
>> I still don't see why, in fact trying to verify this I am wondering now
>> if instead we should actually fix br_features_recompute() to take into
>> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
>> is called above even if the MAC address did not change, just as is done
>> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
>> appropriate we just call
>>
>> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
>>
>> for both the above then and also br_del_if()?
> 
>   Luis
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 54d207d..dcd9378 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -315,6 +315,8 @@  netdev_features_t br_features_recompute(struct net_bridge *br,
 	features &= ~NETIF_F_ONE_FOR_ALL;
 
 	list_for_each_entry(p, &br->port_list, list) {
+		if (p->flags & BR_ROOT_BLOCK)
+			continue;
 		features = netdev_increment_features(features,
 						     p->dev->features, mask);
 	}