netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: Correctly sync addresses from multiple sources to single device
@ 2014-01-17 19:52 Vlad Yasevich
  2014-01-21 22:53 ` David Miller
  2014-01-22 14:18 ` Andrey Dmitrov
  0 siblings, 2 replies; 8+ messages in thread
From: Vlad Yasevich @ 2014-01-17 19:52 UTC (permalink / raw)
  To: netdev; +Cc: Vlad Yasevich, Andrey Dmitrov

When we have multiple devices attempting to sync the same address
to a single destination, each device should be permitted to sync
it once.  To accomplish this, pass the sync count of the source
address to __hw_addr_add_ex().  'sync_cnt' tracks how many time
a given address has been successfully synced.  If the address
is found in the destination list, but the 'sync_cnt' of the source
is 0, then this address has not been synced from this interface
and we need to allow the sync operation to succeed thus incrementing
reference counts.

Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
CC: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/core/dev_addr_lists.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index ec40a84..bd1b880 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -48,7 +48,8 @@ static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
 
 static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
 			    const unsigned char *addr, int addr_len,
-			    unsigned char addr_type, bool global, bool sync)
+			    unsigned char addr_type, bool global, bool sync,
+			    int sync_count)
 {
 	struct netdev_hw_addr *ha;
 
@@ -66,7 +67,7 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
 					ha->global_use = true;
 			}
 			if (sync) {
-				if (ha->synced)
+				if (ha->synced && sync_count)
 					return -EEXIST;
 				else
 					ha->synced = true;
@@ -84,7 +85,8 @@ static int __hw_addr_add(struct netdev_hw_addr_list *list,
 			 const unsigned char *addr, int addr_len,
 			 unsigned char addr_type)
 {
-	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false);
+	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false,
+				0);
 }
 
 static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
@@ -139,7 +141,7 @@ static int __hw_addr_sync_one(struct netdev_hw_addr_list *to_list,
 	int err;
 
 	err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
-			       false, true);
+			       false, true, ha->sync_count);
 	if (err && err != -EEXIST)
 		return err;
 
@@ -676,7 +678,7 @@ static int __dev_mc_add(struct net_device *dev, const unsigned char *addr,
 
 	netif_addr_lock_bh(dev);
 	err = __hw_addr_add_ex(&dev->mc, addr, dev->addr_len,
-			       NETDEV_HW_ADDR_T_MULTICAST, global, false);
+			       NETDEV_HW_ADDR_T_MULTICAST, global, false, 0);
 	if (!err)
 		__dev_set_rx_mode(dev);
 	netif_addr_unlock_bh(dev);
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: Correctly sync addresses from multiple sources to single device
  2014-01-17 19:52 [PATCH] net: Correctly sync addresses from multiple sources to single device Vlad Yasevich
@ 2014-01-21 22:53 ` David Miller
  2014-01-22 15:30   ` Vlad Yasevich
  2014-01-22 14:18 ` Andrey Dmitrov
  1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2014-01-21 22:53 UTC (permalink / raw)
  To: vyasevic; +Cc: netdev, andrey.dmitrov

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Fri, 17 Jan 2014 14:52:12 -0500

> When we have multiple devices attempting to sync the same address
> to a single destination, each device should be permitted to sync
> it once.  To accomplish this, pass the sync count of the source
> address to __hw_addr_add_ex().  'sync_cnt' tracks how many time
> a given address has been successfully synced.  If the address
> is found in the destination list, but the 'sync_cnt' of the source
> is 0, then this address has not been synced from this interface
> and we need to allow the sync operation to succeed thus incrementing
> reference counts.
> 
> Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
> CC: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

I applied this to net-next since 3.13 just got released, and it doesn't
compile.

net/core/dev_addr_lists.c: In function ‘__hw_addr_sync_one’:
net/core/dev_addr_lists.c:144:26: error: ‘struct netdev_hw_addr’ has no member named ‘sync_count’

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: Correctly sync addresses from multiple sources to single device
  2014-01-17 19:52 [PATCH] net: Correctly sync addresses from multiple sources to single device Vlad Yasevich
  2014-01-21 22:53 ` David Miller
@ 2014-01-22 14:18 ` Andrey Dmitrov
  2014-01-22 15:06   ` Vlad Yasevich
  2014-01-22 17:54   ` [PATCH v2] " Vlad Yasevich
  1 sibling, 2 replies; 8+ messages in thread
From: Andrey Dmitrov @ 2014-01-22 14:18 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: netdev, davem, Alexandra N. Kossovsky, Konstantin Ushakov

On 01/17/2014 11:52 PM, Vlad Yasevich wrote:
> When we have multiple devices attempting to sync the same address
> to a single destination, each device should be permitted to sync
> it once.  To accomplish this, pass the sync count of the source
> address to __hw_addr_add_ex().  'sync_cnt' tracks how many time
> a given address has been successfully synced.  If the address
> is found in the destination list, but the 'sync_cnt' of the source
> is 0, then this address has not been synced from this interface
> and we need to allow the sync operation to succeed thus incrementing
> reference counts.
>
> Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
> CC: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>   net/core/dev_addr_lists.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index ec40a84..bd1b880 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -48,7 +48,8 @@ static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
>   
>   static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>   			    const unsigned char *addr, int addr_len,
> -			    unsigned char addr_type, bool global, bool sync)
> +			    unsigned char addr_type, bool global, bool sync,
> +			    int sync_count)
>   {
>   	struct netdev_hw_addr *ha;
>   
> @@ -66,7 +67,7 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>   					ha->global_use = true;
>   			}
>   			if (sync) {
> -				if (ha->synced)
> +				if (ha->synced && sync_count)
>   					return -EEXIST;
>   				else
>   					ha->synced = true;
> @@ -84,7 +85,8 @@ static int __hw_addr_add(struct netdev_hw_addr_list *list,
>   			 const unsigned char *addr, int addr_len,
>   			 unsigned char addr_type)
>   {
> -	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false);
> +	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false,
> +				0);
>   }
>   
>   static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
> @@ -139,7 +141,7 @@ static int __hw_addr_sync_one(struct netdev_hw_addr_list *to_list,
>   	int err;
>   
>   	err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
> -			       false, true);
> +			       false, true, ha->sync_count);
>   	if (err && err != -EEXIST)
>   		return err;
>   
> @@ -676,7 +678,7 @@ static int __dev_mc_add(struct net_device *dev, const unsigned char *addr,
>   
>   	netif_addr_lock_bh(dev);
>   	err = __hw_addr_add_ex(&dev->mc, addr, dev->addr_len,
> -			       NETDEV_HW_ADDR_T_MULTICAST, global, false);
> +			       NETDEV_HW_ADDR_T_MULTICAST, global, false, 0);
>   	if (!err)
>   		__dev_set_rx_mode(dev);
>   	netif_addr_unlock_bh(dev);

Thanks. The patch was tested with linux-3.12.6.

However, while building the kernel I got complains on:

> 	err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
> -			       false, true);
> +			       false, true, ha->sync_count);

Namely ha (struct netdev_hw_addr) has no field sync_count. It has sync_cnt.
So I’ve fixed this when testing the patch. Is it a typo or something intentional?

Your patch solves the initial problem, but it looks like it introduces a new one.

When interface joins multicast group corresponding MAC address is added to the interface maddr list. It should be removed when socket explicitly leaves the group or is just closed. It’s the case without your patch. However, with the patched kernel address is still there after socket is closed.

You can reproduce it with mcast_client binary (mcast_client.c was attached to the first letter in the thread). To be specific:

$ ip maddr show eth3
9:  eth3
  link  33:33:00:00:00:01 users 3
  link  01:00:5e:00:00:01 users 3
  link  33:33:ff:01:39:7c users 3
  link  01:80:c2:00:00:21 users 2
  inet  224.0.0.1
  inet6 ff02::1:ff01:397c
  inet6 ff02::1
  inet6 ff01::1

$ gcc mcast_client.c -o cl
$ sudo ./cl
Abort it with Ctrl+c
$ ip maddr show eth3
9:  eth3
  link  33:33:00:00:00:01 users 3
  link  01:00:5e:00:00:01 users 3
  link  33:33:ff:01:39:7c users 3
  link  01:80:c2:00:00:21 users 2
  link  01:00:5e:11:58:a8
  inet  224.0.0.1
  inet6 ff02::1:ff01:397c
  inet6 ff02::1
  inet6 ff01::1

'link 01:00:5e:11:58:a8’ address is still there. Should’ve been removed.

Andrey

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: Correctly sync addresses from multiple sources to single device
  2014-01-22 14:18 ` Andrey Dmitrov
@ 2014-01-22 15:06   ` Vlad Yasevich
  2014-01-22 17:54   ` [PATCH v2] " Vlad Yasevich
  1 sibling, 0 replies; 8+ messages in thread
From: Vlad Yasevich @ 2014-01-22 15:06 UTC (permalink / raw)
  To: Andrey Dmitrov, Vlad Yasevich
  Cc: netdev, davem, Alexandra N. Kossovsky, Konstantin Ushakov

On 01/22/2014 09:18 AM, Andrey Dmitrov wrote:
> On 01/17/2014 11:52 PM, Vlad Yasevich wrote:
>> When we have multiple devices attempting to sync the same address
>> to a single destination, each device should be permitted to sync
>> it once.  To accomplish this, pass the sync count of the source
>> address to __hw_addr_add_ex().  'sync_cnt' tracks how many time
>> a given address has been successfully synced.  If the address
>> is found in the destination list, but the 'sync_cnt' of the source
>> is 0, then this address has not been synced from this interface
>> and we need to allow the sync operation to succeed thus incrementing
>> reference counts.
>>
>> Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
>> CC: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>>   net/core/dev_addr_lists.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
>> index ec40a84..bd1b880 100644
>> --- a/net/core/dev_addr_lists.c
>> +++ b/net/core/dev_addr_lists.c
>> @@ -48,7 +48,8 @@ static int __hw_addr_create_ex(struct
>> netdev_hw_addr_list *list,
>>     static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>>                   const unsigned char *addr, int addr_len,
>> -                unsigned char addr_type, bool global, bool sync)
>> +                unsigned char addr_type, bool global, bool sync,
>> +                int sync_count)
>>   {
>>       struct netdev_hw_addr *ha;
>>   @@ -66,7 +67,7 @@ static int __hw_addr_add_ex(struct
>> netdev_hw_addr_list *list,
>>                       ha->global_use = true;
>>               }
>>               if (sync) {
>> -                if (ha->synced)
>> +                if (ha->synced && sync_count)
>>                       return -EEXIST;
>>                   else
>>                       ha->synced = true;
>> @@ -84,7 +85,8 @@ static int __hw_addr_add(struct netdev_hw_addr_list
>> *list,
>>                const unsigned char *addr, int addr_len,
>>                unsigned char addr_type)
>>   {
>> -    return __hw_addr_add_ex(list, addr, addr_len, addr_type, false,
>> false);
>> +    return __hw_addr_add_ex(list, addr, addr_len, addr_type, false,
>> false,
>> +                0);
>>   }
>>     static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
>> @@ -139,7 +141,7 @@ static int __hw_addr_sync_one(struct
>> netdev_hw_addr_list *to_list,
>>       int err;
>>         err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
>> -                   false, true);
>> +                   false, true, ha->sync_count);
>>       if (err && err != -EEXIST)
>>           return err;
>>   @@ -676,7 +678,7 @@ static int __dev_mc_add(struct net_device *dev,
>> const unsigned char *addr,
>>         netif_addr_lock_bh(dev);
>>       err = __hw_addr_add_ex(&dev->mc, addr, dev->addr_len,
>> -                   NETDEV_HW_ADDR_T_MULTICAST, global, false);
>> +                   NETDEV_HW_ADDR_T_MULTICAST, global, false, 0);
>>       if (!err)
>>           __dev_set_rx_mode(dev);
>>       netif_addr_unlock_bh(dev);
> 
> Thanks. The patch was tested with linux-3.12.6.
> 
> However, while building the kernel I got complains on:
> 
>>     err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
>> -                   false, true);
>> +                   false, true, ha->sync_count);
> 
> Namely ha (struct netdev_hw_addr) has no field sync_count. It has sync_cnt.
> So I’ve fixed this when testing the patch. Is it a typo or something
> intentional?
> 
> Your patch solves the initial problem, but it looks like it introduces a
> new one.
> 
> When interface joins multicast group corresponding MAC address is added
> to the interface maddr list. It should be removed when socket explicitly
> leaves the group or is just closed. It’s the case without your patch.
> However, with the patched kernel address is still there after socket is
> closed.
> 
> You can reproduce it with mcast_client binary (mcast_client.c was
> attached to the first letter in the thread). To be specific:
> 
> $ ip maddr show eth3
> 9:  eth3
>  link  33:33:00:00:00:01 users 3
>  link  01:00:5e:00:00:01 users 3
>  link  33:33:ff:01:39:7c users 3
>  link  01:80:c2:00:00:21 users 2
>  inet  224.0.0.1
>  inet6 ff02::1:ff01:397c
>  inet6 ff02::1
>  inet6 ff01::1
> 
> $ gcc mcast_client.c -o cl
> $ sudo ./cl
> Abort it with Ctrl+c
> $ ip maddr show eth3
> 9:  eth3
>  link  33:33:00:00:00:01 users 3
>  link  01:00:5e:00:00:01 users 3
>  link  33:33:ff:01:39:7c users 3
>  link  01:80:c2:00:00:21 users 2
>  link  01:00:5e:11:58:a8
>  inet  224.0.0.1
>  inet6 ff02::1:ff01:397c
>  inet6 ff02::1
>  inet6 ff01::1
> 
> 'link 01:00:5e:11:58:a8’ address is still there. Should’ve been removed.

You are right.  Sync really doesn't deal well with with multiple sources
syncing the same address.  Looks like we need to keep track of how
many time the address has been synced from the different sources.

Looks like I have to restore
commit 4543fbefe6e06a9e40d9f2b28d688393a299f079. :)

-vlad
> 
> Andrey
> -- 
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: Correctly sync addresses from multiple sources to single device
  2014-01-21 22:53 ` David Miller
@ 2014-01-22 15:30   ` Vlad Yasevich
  0 siblings, 0 replies; 8+ messages in thread
From: Vlad Yasevich @ 2014-01-22 15:30 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, andrey.dmitrov

On 01/21/2014 05:53 PM, David Miller wrote:
> From: Vlad Yasevich <vyasevic@redhat.com>
> Date: Fri, 17 Jan 2014 14:52:12 -0500
> 
>> When we have multiple devices attempting to sync the same address
>> to a single destination, each device should be permitted to sync
>> it once.  To accomplish this, pass the sync count of the source
>> address to __hw_addr_add_ex().  'sync_cnt' tracks how many time
>> a given address has been successfully synced.  If the address
>> is found in the destination list, but the 'sync_cnt' of the source
>> is 0, then this address has not been synced from this interface
>> and we need to allow the sync operation to succeed thus incrementing
>> reference counts.
>>
>> Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
>> CC: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> 
> I applied this to net-next since 3.13 just got released, and it doesn't
> compile.
> 
> net/core/dev_addr_lists.c: In function ‘__hw_addr_sync_one’:
> net/core/dev_addr_lists.c:144:26: error: ‘struct netdev_hw_addr’ has no member named ‘sync_count’
> 

That does it...  Time to add a hook to format_patch to check for dirty
index.

-vlad

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] net: Correctly sync addresses from multiple sources to single device
  2014-01-22 14:18 ` Andrey Dmitrov
  2014-01-22 15:06   ` Vlad Yasevich
@ 2014-01-22 17:54   ` Vlad Yasevich
  2014-01-23 21:07     ` David Miller
  2014-01-24 11:33     ` Andrey Dmitrov
  1 sibling, 2 replies; 8+ messages in thread
From: Vlad Yasevich @ 2014-01-22 17:54 UTC (permalink / raw)
  To: netdev
  Cc: Vlad Yasevich, Andrey Dmitrov, Alexandra N. Kossovsky,
	Konstantin Ushakov

When we have multiple devices attempting to sync the same address
to a single destination, each device should be permitted to sync
it once.  To accomplish this, pass the 'sync_cnt' of the source
address when adding the addresss to the lower device.  'sync_cnt'
tracks how many time a given address has been succefully synced.
This way, we know that if the 'sync_cnt' passed in is 0, we should
sync this address.

Also, turn 'synced' member back into the counter as was originally
done in
   commit 4543fbefe6e06a9e40d9f2b28d688393a299f079.
   net: count hw_addr syncs so that unsync works properly.
It tracks how many time a given address has been added via a
'sync' operation.  For every successfull 'sync' the counter is
incremented, and for ever 'unsync', the counter is decremented.
This makes sure that the address will be properly removed from
the the lower device when all the upper devices have removed it.

Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
CC: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
CC: Alexandra N. Kossovsky <Alexandra.Kossovsky@oktetlabs.ru>
CC: Konstantin Ushakov <Konstantin.Ushakov@oktetlabs.ru>
Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
---
 net/core/dev_addr_lists.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index ec40a84..9974f48 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -38,7 +38,7 @@ static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
 	ha->type = addr_type;
 	ha->refcount = 1;
 	ha->global_use = global;
-	ha->synced = sync;
+	ha->synced = sync ? 1 : 0;
 	ha->sync_cnt = 0;
 	list_add_tail_rcu(&ha->list, &list->list);
 	list->count++;
@@ -48,7 +48,8 @@ static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
 
 static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
 			    const unsigned char *addr, int addr_len,
-			    unsigned char addr_type, bool global, bool sync)
+			    unsigned char addr_type, bool global, bool sync,
+			    int sync_count)
 {
 	struct netdev_hw_addr *ha;
 
@@ -66,10 +67,10 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
 					ha->global_use = true;
 			}
 			if (sync) {
-				if (ha->synced)
+				if (ha->synced && sync_count)
 					return -EEXIST;
 				else
-					ha->synced = true;
+					ha->synced++;
 			}
 			ha->refcount++;
 			return 0;
@@ -84,7 +85,8 @@ static int __hw_addr_add(struct netdev_hw_addr_list *list,
 			 const unsigned char *addr, int addr_len,
 			 unsigned char addr_type)
 {
-	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false);
+	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false,
+				0);
 }
 
 static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
@@ -101,7 +103,7 @@ static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
 		ha->global_use = false;
 
 	if (sync)
-		ha->synced = false;
+		ha->synced--;
 
 	if (--ha->refcount)
 		return 0;
@@ -139,7 +141,7 @@ static int __hw_addr_sync_one(struct netdev_hw_addr_list *to_list,
 	int err;
 
 	err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
-			       false, true);
+			       false, true, ha->sync_cnt);
 	if (err && err != -EEXIST)
 		return err;
 
@@ -676,7 +678,7 @@ static int __dev_mc_add(struct net_device *dev, const unsigned char *addr,
 
 	netif_addr_lock_bh(dev);
 	err = __hw_addr_add_ex(&dev->mc, addr, dev->addr_len,
-			       NETDEV_HW_ADDR_T_MULTICAST, global, false);
+			       NETDEV_HW_ADDR_T_MULTICAST, global, false, 0);
 	if (!err)
 		__dev_set_rx_mode(dev);
 	netif_addr_unlock_bh(dev);
-- 
1.8.4.2

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] net: Correctly sync addresses from multiple sources to single device
  2014-01-22 17:54   ` [PATCH v2] " Vlad Yasevich
@ 2014-01-23 21:07     ` David Miller
  2014-01-24 11:33     ` Andrey Dmitrov
  1 sibling, 0 replies; 8+ messages in thread
From: David Miller @ 2014-01-23 21:07 UTC (permalink / raw)
  To: vyasevic; +Cc: netdev, andrey.dmitrov, Alexandra.Kossovsky, Konstantin.Ushakov

From: Vlad Yasevich <vyasevic@redhat.com>
Date: Wed, 22 Jan 2014 12:54:15 -0500

> When we have multiple devices attempting to sync the same address
> to a single destination, each device should be permitted to sync
> it once.  To accomplish this, pass the 'sync_cnt' of the source
> address when adding the addresss to the lower device.  'sync_cnt'
> tracks how many time a given address has been succefully synced.
> This way, we know that if the 'sync_cnt' passed in is 0, we should
> sync this address.
> 
> Also, turn 'synced' member back into the counter as was originally
> done in
>    commit 4543fbefe6e06a9e40d9f2b28d688393a299f079.
>    net: count hw_addr syncs so that unsync works properly.
> It tracks how many time a given address has been added via a
> 'sync' operation.  For every successfull 'sync' the counter is
> incremented, and for ever 'unsync', the counter is decremented.
> This makes sure that the address will be properly removed from
> the the lower device when all the upper devices have removed it.
> 
> Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
> CC: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
> CC: Alexandra N. Kossovsky <Alexandra.Kossovsky@oktetlabs.ru>
> CC: Konstantin Ushakov <Konstantin.Ushakov@oktetlabs.ru>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>

This one compiles, great :-)

Applied, thanks Vlad.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] net: Correctly sync addresses from multiple sources to single device
  2014-01-22 17:54   ` [PATCH v2] " Vlad Yasevich
  2014-01-23 21:07     ` David Miller
@ 2014-01-24 11:33     ` Andrey Dmitrov
  1 sibling, 0 replies; 8+ messages in thread
From: Andrey Dmitrov @ 2014-01-24 11:33 UTC (permalink / raw)
  To: Vlad Yasevich; +Cc: netdev, Alexandra N. Kossovsky, Konstantin Ushakov

On 01/22/2014 09:54 PM, Vlad Yasevich wrote:
> When we have multiple devices attempting to sync the same address
> to a single destination, each device should be permitted to sync
> it once.  To accomplish this, pass the 'sync_cnt' of the source
> address when adding the addresss to the lower device.  'sync_cnt'
> tracks how many time a given address has been succefully synced.
> This way, we know that if the 'sync_cnt' passed in is 0, we should
> sync this address.
>
> Also, turn 'synced' member back into the counter as was originally
> done in
>     commit 4543fbefe6e06a9e40d9f2b28d688393a299f079.
>     net: count hw_addr syncs so that unsync works properly.
> It tracks how many time a given address has been added via a
> 'sync' operation.  For every successfull 'sync' the counter is
> incremented, and for ever 'unsync', the counter is decremented.
> This makes sure that the address will be properly removed from
> the the lower device when all the upper devices have removed it.
>
> Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
> CC: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
> CC: Alexandra N. Kossovsky <Alexandra.Kossovsky@oktetlabs.ru>
> CC: Konstantin Ushakov <Konstantin.Ushakov@oktetlabs.ru>
> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
> ---
>   net/core/dev_addr_lists.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
> index ec40a84..9974f48 100644
> --- a/net/core/dev_addr_lists.c
> +++ b/net/core/dev_addr_lists.c
> @@ -38,7 +38,7 @@ static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
>   	ha->type = addr_type;
>   	ha->refcount = 1;
>   	ha->global_use = global;
> -	ha->synced = sync;
> +	ha->synced = sync ? 1 : 0;
>   	ha->sync_cnt = 0;
>   	list_add_tail_rcu(&ha->list, &list->list);
>   	list->count++;
> @@ -48,7 +48,8 @@ static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
>   
>   static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>   			    const unsigned char *addr, int addr_len,
> -			    unsigned char addr_type, bool global, bool sync)
> +			    unsigned char addr_type, bool global, bool sync,
> +			    int sync_count)
>   {
>   	struct netdev_hw_addr *ha;
>   
> @@ -66,10 +67,10 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
>   					ha->global_use = true;
>   			}
>   			if (sync) {
> -				if (ha->synced)
> +				if (ha->synced && sync_count)
>   					return -EEXIST;
>   				else
> -					ha->synced = true;
> +					ha->synced++;
>   			}
>   			ha->refcount++;
>   			return 0;
> @@ -84,7 +85,8 @@ static int __hw_addr_add(struct netdev_hw_addr_list *list,
>   			 const unsigned char *addr, int addr_len,
>   			 unsigned char addr_type)
>   {
> -	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false);
> +	return __hw_addr_add_ex(list, addr, addr_len, addr_type, false, false,
> +				0);
>   }
>   
>   static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
> @@ -101,7 +103,7 @@ static int __hw_addr_del_entry(struct netdev_hw_addr_list *list,
>   		ha->global_use = false;
>   
>   	if (sync)
> -		ha->synced = false;
> +		ha->synced--;
>   
>   	if (--ha->refcount)
>   		return 0;
> @@ -139,7 +141,7 @@ static int __hw_addr_sync_one(struct netdev_hw_addr_list *to_list,
>   	int err;
>   
>   	err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
> -			       false, true);
> +			       false, true, ha->sync_cnt);
>   	if (err && err != -EEXIST)
>   		return err;
>   
> @@ -676,7 +678,7 @@ static int __dev_mc_add(struct net_device *dev, const unsigned char *addr,
>   
>   	netif_addr_lock_bh(dev);
>   	err = __hw_addr_add_ex(&dev->mc, addr, dev->addr_len,
> -			       NETDEV_HW_ADDR_T_MULTICAST, global, false);
> +			       NETDEV_HW_ADDR_T_MULTICAST, global, false, 0);
>   	if (!err)
>   		__dev_set_rx_mode(dev);
>   	netif_addr_unlock_bh(dev);
Everything works properly now. The patch has been tested with linux-3.12.6.

Thanks,
Andrey

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-01-24 11:33 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-01-17 19:52 [PATCH] net: Correctly sync addresses from multiple sources to single device Vlad Yasevich
2014-01-21 22:53 ` David Miller
2014-01-22 15:30   ` Vlad Yasevich
2014-01-22 14:18 ` Andrey Dmitrov
2014-01-22 15:06   ` Vlad Yasevich
2014-01-22 17:54   ` [PATCH v2] " Vlad Yasevich
2014-01-23 21:07     ` David Miller
2014-01-24 11:33     ` Andrey Dmitrov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).