[RFC PATCH] net: Add support for device specific address syncing

[RFC PATCH] net: Add support for device specific address syncing

[Alexander Duyck]

This change provides a function to be used in order to break the
ndo_set_rx_mode call into a set of address add and remove calls.  The code
is based on the implementation of dev_uc_sync/dev_mc_sync.  Since they
essentially do the same thing but with only one dev I simply named my
functions __dev_uc_sync/__dev_mc_sync.

I also implemented an unsync version of the functions as well to allow for
cleanup on close.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
---

I still have to do some testing on this patch, but I am looking to see if
this is the correct approach or if the community would prefer I take a
different one.

 include/linux/netdevice.h |   77 +++++++++++++++++++++++++++++++++++++++++++++
 net/core/dev_addr_lists.c |   46 +++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 0 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index adc4658..d2dabf4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2998,6 +2998,13 @@ int __hw_addr_sync(struct netdev_hw_addr_list *to_list,
 		   struct netdev_hw_addr_list *from_list, int addr_len);
 void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
 		      struct netdev_hw_addr_list *from_list, int addr_len);
+int __hw_addr_sync_dev(struct netdev_hw_addr_list *list,
+		struct net_device *dev,
+		int (*sync)(struct net_device *, const unsigned char *),
+		int (*unsync)(struct net_device *, const unsigned char *));
+void __hw_addr_unsync_dev(struct netdev_hw_addr_list *list,
+		struct net_device *dev,
+		int (*unsync)(struct net_device *, const unsigned char *));
 void __hw_addr_init(struct netdev_hw_addr_list *list);
 
 /* Functions used for device addresses handling */
@@ -3018,6 +3025,41 @@ void dev_uc_unsync(struct net_device *to, struct net_device *from);
 void dev_uc_flush(struct net_device *dev);
 void dev_uc_init(struct net_device *dev);
 
+/**
+ *  __dev_uc_sync - Synchonize device's unicast list
+ *  @dev:  device to sync
+ *  @sync: function to call if address should be added
+ *  @unsync: function to call if address should be removed
+ *
+ *  Add newly added addresses to the interface, and release
+ *  addresses that have been deleted.
+ *
+ *  This funciton is intended to be called from the ndo_set_rx_mode
+ *  function of devices that require explicit address add/remove
+ *  notifications.
+ **/
+static inline int __dev_uc_sync(struct net_device *dev,
+		int (*sync)(struct net_device *, const unsigned char *),
+		int (*unsync)(struct net_device *, const unsigned char *))
+{
+	return __hw_addr_sync_dev(&dev->uc, dev, sync, unsync);
+}
+
+/**
+ *  __dev_uc_unsync - Remove synchonized addresses from device
+ *  @dev:  device to sync
+ *  @unsync: function to call if address should be removed
+ *
+ *  Remove all addresses that were added to the device by dev_uc_sync().
+ *  This function is intended to be called from the ndo_stop function on devices
+ *  that require explicit address add/remove notifications.
+ **/
+static inline void __dev_uc_unsync(struct net_device *dev,
+		   int (*unsync)(struct net_device *, const unsigned char *))
+{
+	__hw_addr_unsync_dev(&dev->uc, dev, unsync);
+}
+
 /* Functions used for multicast addresses handling */
 int dev_mc_add(struct net_device *dev, const unsigned char *addr);
 int dev_mc_add_global(struct net_device *dev, const unsigned char *addr);
@@ -3030,6 +3072,41 @@ void dev_mc_unsync(struct net_device *to, struct net_device *from);
 void dev_mc_flush(struct net_device *dev);
 void dev_mc_init(struct net_device *dev);
 
+/**
+ *  __dev_mc_sync - Synchonize device's multicast list
+ *  @dev:  device to sync
+ *  @sync: function to call if address should be added
+ *  @unsync: function to call if address should be removed
+ *
+ *  Add newly added addresses to the interface, and release
+ *  addresses that have been deleted.
+ *
+ *  This funciton is intended to be called from the ndo_set_rx_mode
+ *  function of devices that require explicit address add/remove
+ *  notifications.
+ **/
+static inline int __dev_mc_sync(struct net_device *dev,
+		int (*sync)(struct net_device *, const unsigned char *),
+		int (*unsync)(struct net_device *, const unsigned char *))
+{
+	return __hw_addr_sync_dev(&dev->mc, dev, sync, unsync);
+}
+
+/**
+ *  __dev_mc_unsync - Remove synchonized addresses from device
+ *  @dev:  device to sync
+ *  @unsync: function to call if address should be removed
+ *
+ *  Remove all addresses that were added to the device by dev_mc_sync().
+ *  This function is intended to be called from the ndo_stop function on devices
+ *  that require explicit address add/remove notifications.
+ **/
+static inline void __dev_mc_unsync(struct net_device *dev,
+		int (*unsync)(struct net_device *, const unsigned char *))
+{
+	__hw_addr_unsync_dev(&dev->mc, dev, unsync);
+}
+
 /* Functions used for secondary unicast and multicast support */
 void dev_set_rx_mode(struct net_device *dev);
 void __dev_set_rx_mode(struct net_device *dev);
diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 329d579..e6f0f55 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -225,6 +225,52 @@ void __hw_addr_unsync(struct netdev_hw_addr_list *to_list,
 }
 EXPORT_SYMBOL(__hw_addr_unsync);
 
+int __hw_addr_sync_dev(struct netdev_hw_addr_list *list,
+		struct net_device *dev,
+		int (*sync)(struct net_device *, const unsigned char *),
+		int (*unsync)(struct net_device *, const unsigned char *))
+{
+	struct netdev_hw_addr *ha, *tmp;
+	int err = 0;
+
+	list_for_each_entry_safe(ha, tmp, &list->list, list) {
+		if (!ha->sync_cnt) {
+			err = sync(dev, ha->addr);
+			if (err)
+				break;
+			ha->sync_cnt++;
+			ha->refcount++;
+		} else if (ha->refcount == 1) {
+			err = unsync(dev, ha->addr);
+			if (!err) {
+				ha->sync_cnt--;
+				__hw_addr_del_entry(list, ha, false, false);
+			}
+		}
+	}
+	return err;
+}
+EXPORT_SYMBOL(__hw_addr_sync_dev);
+
+void __hw_addr_unsync_dev(struct netdev_hw_addr_list *list,
+		struct net_device *dev,
+		int (*unsync)(struct net_device *, const unsigned char *))
+{
+	struct netdev_hw_addr *ha, *tmp;
+	int err;
+
+	list_for_each_entry_safe(ha, tmp, &list->list, list) {
+		if (!ha->sync_cnt)
+			continue;
+		err = unsync(dev, ha->addr);
+		if (!err) {
+			ha->sync_cnt--;
+			__hw_addr_del_entry(list, ha, false, false);
+		}
+	}
+}
+EXPORT_SYMBOL(__hw_addr_unsync_dev);
+
 static void __hw_addr_flush(struct netdev_hw_addr_list *list)
 {
 	struct netdev_hw_addr *ha, *tmp;

Re: [RFC PATCH] net: Add support for device specific address syncing

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Wed, 14 May 2014 16:37:27 -0700

> This change provides a function to be used in order to break the
> ndo_set_rx_mode call into a set of address add and remove calls.  The code
> is based on the implementation of dev_uc_sync/dev_mc_sync.  Since they
> essentially do the same thing but with only one dev I simply named my
> functions __dev_uc_sync/__dev_mc_sync.
> 
> I also implemented an unsync version of the functions as well to allow for
> cleanup on close.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> ---
> 
> I still have to do some testing on this patch, but I am looking to see if
> this is the correct approach or if the community would prefer I take a
> different one.

I just wonder about error handling.

The code just seems to stop trying to sync() if one intermediate
sync() fails.

Shouldn't we unwind or signal errors to the caller?

Is the idea that the driver has internal state which will track if
something goes wrong and take care of error recovery itself?  If so,
that kind of indirect error handling doesn't sit too well with me.

Re: [RFC PATCH] net: Add support for device specific address syncing

[Alexander Duyck]

On 05/15/2014 08:05 PM, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Wed, 14 May 2014 16:37:27 -0700
> 
>> This change provides a function to be used in order to break the
>> ndo_set_rx_mode call into a set of address add and remove calls.  The code
>> is based on the implementation of dev_uc_sync/dev_mc_sync.  Since they
>> essentially do the same thing but with only one dev I simply named my
>> functions __dev_uc_sync/__dev_mc_sync.
>>
>> I also implemented an unsync version of the functions as well to allow for
>> cleanup on close.
>>
>> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
>> ---
>>
>> I still have to do some testing on this patch, but I am looking to see if
>> this is the correct approach or if the community would prefer I take a
>> different one.
> 
> I just wonder about error handling.
> 
> The code just seems to stop trying to sync() if one intermediate
> sync() fails.
> 
> Shouldn't we unwind or signal errors to the caller?

I'm not sure what you are talking about.  That is what the code does.
In the bit of code below on error we break out of the loop and return
the error value.  At that point it is up to the caller to determine the
best way to clean it up since the list may still be synced from the
previous call.  For example one response to the __dev_uc_sync call
failing might be to simply turn on promiscuous mode if the call returns
a value indicating that there aren't enough filters.

The unsync doesn't do this but that is partly because having an
additional address isn't really that big of an issue, and because this
can still be cleaned up the next time this function is called.

I suppose I should break up the loop below though.  It might be better
to do all of the usnync operations first before the sync in the case of
a interface with a limited number of unicast of multicast filters.

-- section from __hw_addr_sync_dev() in the original patch --
> +       list_for_each_entry_safe(ha, tmp, &list->list, list) {
> +               if (!ha->sync_cnt) {
> +                       err = sync(dev, ha->addr);
> +                       if (err)
> +                               break;
> +                       ha->sync_cnt++;
> +                       ha->refcount++;
> +               } else if (ha->refcount == 1) {
> +                       err = unsync(dev, ha->addr);
> +                       if (!err) {
> +                               ha->sync_cnt--;
> +                               __hw_addr_del_entry(list, ha, false, false);
> +                       }
> +               }
> +       }
> +       return err;
--

> 
> Is the idea that the driver has internal state which will track if
> something goes wrong and take care of error recovery itself?  If so,
> that kind of indirect error handling doesn't sit too well with me.
> 

My primary use case for this is to simplify mailbox operations between
two entities such as a PF and VF.  With this the VF only needs to send a
request for new addresses instead of having to send the entire list via
the mailbox.  The issue most likely to cause an error is a mailbox error
which I admit does have some of its own error recovery in the case of a
message timeout.

Thanks,

Alex

Re: [RFC PATCH] net: Add support for device specific address syncing

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Fri, 16 May 2014 11:47:57 -0700

> I suppose I should break up the loop below though.  It might be better
> to do all of the usnync operations first before the sync in the case of
> a interface with a limited number of unicast of multicast filters.
 ...
> My primary use case for this is to simplify mailbox operations between
> two entities such as a PF and VF.  With this the VF only needs to send a
> request for new addresses instead of having to send the entire list via
> the mailbox.  The issue most likely to cause an error is a mailbox error
> which I admit does have some of its own error recovery in the case of a
> message timeout.

I definitely agree that we should be doing the unsync()'s first.

>From a quality of implementation standpoint, the promisc mode needs
should be determined in both sync() and unsync().

Re: [RFC PATCH] net: Add support for device specific address syncing

[Alexander Duyck]

On 05/16/2014 12:01 PM, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Fri, 16 May 2014 11:47:57 -0700
> 
>> I suppose I should break up the loop below though.  It might be better
>> to do all of the usnync operations first before the sync in the case of
>> a interface with a limited number of unicast of multicast filters.
>  ...
>> My primary use case for this is to simplify mailbox operations between
>> two entities such as a PF and VF.  With this the VF only needs to send a
>> request for new addresses instead of having to send the entire list via
>> the mailbox.  The issue most likely to cause an error is a mailbox error
>> which I admit does have some of its own error recovery in the case of a
>> message timeout.
> 
> I definitely agree that we should be doing the unsync()'s first.
> 
> From a quality of implementation standpoint, the promisc mode needs
> should be determined in both sync() and unsync().
> 

I can understand going into promisc on a sync failure, but why would you
do it on an unsync failure, or are you saying that we would be clearing
the flag in unsync?

In general I intended for this to be called in set_rx_mode so if
__dev_uc_sync returns an error indicating insufficient resources we have
to force IFF_PROMISC on because adding a new address failed.  We could
also do the same thing for __dev_mc_sync and IFF_ALLMULTI.

Thanks,

Alex

Re: [RFC PATCH] net: Add support for device specific address syncing

[David Miller]

From: Alexander Duyck <alexander.h.duyck@intel.com>
Date: Fri, 16 May 2014 12:55:42 -0700

> I can understand going into promisc on a sync failure, but why would you
> do it on an unsync failure, or are you saying that we would be clearing
> the flag in unsync?

We should clear the promisc flag on unsync if the limitations are no
longer exceeded.

> In general I intended for this to be called in set_rx_mode so if
> __dev_uc_sync returns an error indicating insufficient resources we have
> to force IFF_PROMISC on because adding a new address failed.  We could
> also do the same thing for __dev_mc_sync and IFF_ALLMULTI.

Right.

Re: [RFC PATCH] net: Add support for device specific address syncing

[Alexander Duyck]

On 05/16/2014 01:47 PM, David Miller wrote:
> From: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Fri, 16 May 2014 12:55:42 -0700
> 
>> I can understand going into promisc on a sync failure, but why would you
>> do it on an unsync failure, or are you saying that we would be clearing
>> the flag in unsync?
> 
> We should clear the promisc flag on unsync if the limitations are no
> longer exceeded.
> 
>> In general I intended for this to be called in set_rx_mode so if
>> __dev_uc_sync returns an error indicating insufficient resources we have
>> to force IFF_PROMISC on because adding a new address failed.  We could
>> also do the same thing for __dev_mc_sync and IFF_ALLMULTI.
> 
> Right.
> 

Okay, I think that all works.  I'll probably resubmit it next week after
it has gone though a bit more testing.

Thanks,

Alex