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 <[email protected]> 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
> > >> <[email protected]> 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
On Tue, Apr 22, 2014 at 12:43 PM, Luis R. Rodriguez <[email protected]> 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 <[email protected]> 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
>> > >> <[email protected]> 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
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 <[email protected]> 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
>>>>> <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]> 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
>>>> <[email protected]> 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
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 <[email protected]> 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
>>>>> <[email protected]> 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
>