Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
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 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
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
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 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
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
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 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
> 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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
On Tue, Apr 22, 2014 at 12: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 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
>> > >> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
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 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
> > >> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
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 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
> >> 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);
}
signature.asc
Description: Digital signature
Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez 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 >> 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);. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
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 > wrote: > > spin_lock_bh(&p->br->lock); > > err = br_setport(p, tb); > > + changed = br_stp_recalculate_bridge_id(p->br); > > Looks like you only want to check if the mac addr gets changed here, Nope, the reason why we want a full thorough check is that br_setport() may change currently any of these: * IFLA_BRPORT_MODE * IFLA_BRPORT_GUARD * IFLA_BRPORT_FAST_LEAVE * IFLA_BRPORT_PROTECT * IFLA_BRPORT_LEARNING, * IFLA_BRPORT_UNICAST_FLOOD * IFLA_BRPORT_COST * IFLA_BRPORT_PRIORITY * IFLA_BRPORT_STATE That's good reason to trigger a good inspection. Having the MAC address change would be simply collateral and its just something we need to do some additional work for outside of the locking context. > but br_stp_recalculate_bridge_id() does more than just checking it, > are you sure the side-effects are all what you want here? Yeap. > > 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. Luis pgppyS6NVteYj.pgp Description: PGP signature
Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes
On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez wrote: > spin_lock_bh(&p->br->lock); > err = br_setport(p, tb); > + changed = br_stp_recalculate_bridge_id(p->br); Looks like you only want to check if the mac addr gets changed here, but br_stp_recalculate_bridge_id() does more than just checking it, are you sure the side-effects are all what you want here? > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
