Re: [PATCH RESEND] bonding: remap muticast addresses without using dev_close() and dev_open()

[Moni Shoua <monis@Voltaire.COM>]

Or Gerlitz wrote:
> Moni Shoua wrote:
>> This patch fixes commit e36b9d16c6a6d0f59803b3ef04ff3c22c3844c10. The
>> approach there is to call dev_close()/dev_open() whenever the device
>> type is changed in order to remap the device IP multicast addresses to
>> HW multicast addresses. This approach suffers from 2 drawbacks [...]
>> The fix here is to directly remap the IP multicast addresses to HW
>> multicast addresses for a bonding device that changes its type, and
>> nothing else.
> 
> Moni,
> 
> The approach and patch look good. First, I think it may be more easier
> to review and maintain if you separate this to two patches, the first
> simply reverting e36b9d16c6a6d0f59803b3ef04ff3c22c3844c10 and the second
> the approach suggested by this patch. Second, I think you may be able to
> do well with only one event, see next
> 
I don't need to revert the entire patch. Only the dev_open() and dev_close() functions need to be removed and it is quite easy to review it in one patch.
>> @@ -1460,14 +1460,17 @@ int bond_enslave(struct net_device *bond_dev,
>> struct net_device *slave_dev)
>>       */
>>      if (bond->slave_cnt == 0) {
>>          if (bond_dev->type != slave_dev->type) {
>> -            dev_close(bond_dev);
>>              pr_debug("%s: change device type from %d to %d\n",
>>                  bond_dev->name, bond_dev->type, slave_dev->type);
>> +
>> +            netdev_bonding_change(bond_dev, NETDEV_BONDING_OLDTYPE);
>> +
>>              if (slave_dev->type != ARPHRD_ETHER)
>>                  bond_setup_by_slave(bond_dev, slave_dev);
>>              else
>>                  ether_setup(bond_dev);
>> -            dev_open(bond_dev);
>> +
>> +            netdev_bonding_change(bond_dev, NETDEV_BONDING_NEWTYPE);
>>          }
> can't you achieve the same impact if just calling
> netdev_bonding_change(bond_dev, NETDEV_BONDING_NEWTYPE) after doing the
> setup_by_slave, and have the stack call ip_mc_unmap(...) and then
> ip_mc_map(...) ???
> 
I thought about it but the function arp_mc_map() which is called before and after the change in dev->type, relies on the value of dev->type. I could write the patch with one event after the type has changed and passing the old device type somehow (field prev_type in struct net_device?) but the resulted code will look clumsy (at least to me).

> Or.
> 
> 
> -- 
> 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
>