Re: [Xen-devel] [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Vlad Yasevich <vyasevich@xxxxxxxxx>]

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@xxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel