[PATCH 2/2] net: dpaa: use __dev_mc_sync in dpaa_set_rx_mode()

Jonas Rebmann posted 2 patches 1 month, 3 weeks ago
[PATCH 2/2] net: dpaa: use __dev_mc_sync in dpaa_set_rx_mode()
Posted by Jonas Rebmann 1 month, 3 weeks ago
The original driver first unregisters then re-registers all multicast
addresses in the struct net_device_ops::ndo_set_rx_mode() callback.

As the networking stack calls ndo_set_rx_mode() if a single multicast
address change occurs, a significant amount of time may be used to first
unregister and then re-register unchanged multicast addresses. This
leads to performance issues when tracking large numbers of multicast
addresses.

Replace the unregister and register loop and the hand crafted
mc_addr_list list handling with __dev_mc_sync(), to only update entries
which have changed.

On profiling with an fsl_dpa NIC, this patch presented a speedup of
around 40 when successively setting up 2000 multicast groups using
setsockopt(), without drawbacks on smaller numbers of multicast groups.

Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
---
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c   | 20 +++++++++--
 drivers/net/ethernet/freescale/fman/fman_dtsec.c |  1 -
 drivers/net/ethernet/freescale/fman/fman_memac.c |  1 -
 drivers/net/ethernet/freescale/fman/fman_tgec.c  |  1 -
 drivers/net/ethernet/freescale/fman/mac.c        | 42 ------------------------
 drivers/net/ethernet/freescale/fman/mac.h        |  2 --
 6 files changed, 18 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index 6b9b6d72db98c22b9c104833b3c8c675931fd1aa..ac06b01fe93401b0416cd6a654bac2cb40ce12aa 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -463,6 +463,22 @@ static int dpaa_set_mac_address(struct net_device *net_dev, void *addr)
 	return 0;
 }
 
+static int dpaa_addr_sync(struct net_device *net_dev, const u8 *addr)
+{
+	const struct dpaa_priv *priv = netdev_priv(net_dev);
+
+	return priv->mac_dev->add_hash_mac_addr(priv->mac_dev->fman_mac,
+						(enet_addr_t *)addr);
+}
+
+static int dpaa_addr_unsync(struct net_device *net_dev, const u8 *addr)
+{
+	const struct dpaa_priv *priv = netdev_priv(net_dev);
+
+	return priv->mac_dev->remove_hash_mac_addr(priv->mac_dev->fman_mac,
+						   (enet_addr_t *)addr);
+}
+
 static void dpaa_set_rx_mode(struct net_device *net_dev)
 {
 	const struct dpaa_priv	*priv;
@@ -490,9 +506,9 @@ static void dpaa_set_rx_mode(struct net_device *net_dev)
 				  err);
 	}
 
-	err = priv->mac_dev->set_multi(net_dev, priv->mac_dev);
+	err = __dev_mc_sync(net_dev, dpaa_addr_sync, dpaa_addr_unsync);
 	if (err < 0)
-		netif_err(priv, drv, net_dev, "mac_dev->set_multi() = %d\n",
+		netif_err(priv, drv, net_dev, "dpaa_addr_sync() = %d\n",
 			  err);
 }
 
diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
index 3088da7adf0f846744079107f7f72fea74114f4a..85617bb94959f3789d75766bce8f3e11a7b321d5 100644
--- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c
+++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
@@ -1415,7 +1415,6 @@ int dtsec_initialization(struct mac_device *mac_dev,
 	mac_dev->set_exception		= dtsec_set_exception;
 	mac_dev->set_allmulti		= dtsec_set_allmulti;
 	mac_dev->set_tstamp		= dtsec_set_tstamp;
-	mac_dev->set_multi		= fman_set_multi;
 	mac_dev->enable			= dtsec_enable;
 	mac_dev->disable		= dtsec_disable;
 
diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
index 796e6f4e583d18be6069f78af15fbedf9557378e..3925441143fac9eecc40ea45d05f36be63b16a78 100644
--- a/drivers/net/ethernet/freescale/fman/fman_memac.c
+++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
@@ -1087,7 +1087,6 @@ int memac_initialization(struct mac_device *mac_dev,
 	mac_dev->set_exception		= memac_set_exception;
 	mac_dev->set_allmulti		= memac_set_allmulti;
 	mac_dev->set_tstamp		= memac_set_tstamp;
-	mac_dev->set_multi		= fman_set_multi;
 	mac_dev->enable			= memac_enable;
 	mac_dev->disable		= memac_disable;
 
diff --git a/drivers/net/ethernet/freescale/fman/fman_tgec.c b/drivers/net/ethernet/freescale/fman/fman_tgec.c
index c2261d26db5b9374a5e52bac41c25ed8831f4822..fecfca6eba03e571cfb569b8aad20dc3fa8dc1c7 100644
--- a/drivers/net/ethernet/freescale/fman/fman_tgec.c
+++ b/drivers/net/ethernet/freescale/fman/fman_tgec.c
@@ -771,7 +771,6 @@ int tgec_initialization(struct mac_device *mac_dev,
 	mac_dev->set_exception		= tgec_set_exception;
 	mac_dev->set_allmulti		= tgec_set_allmulti;
 	mac_dev->set_tstamp		= tgec_set_tstamp;
-	mac_dev->set_multi		= fman_set_multi;
 	mac_dev->enable			= tgec_enable;
 	mac_dev->disable		= tgec_disable;
 
diff --git a/drivers/net/ethernet/freescale/fman/mac.c b/drivers/net/ethernet/freescale/fman/mac.c
index 43f4ad29eadd495ce7f4861b3e635e22379ddc72..974d2e7e131c087ddbb41dcb906f6144a150db46 100644
--- a/drivers/net/ethernet/freescale/fman/mac.c
+++ b/drivers/net/ethernet/freescale/fman/mac.c
@@ -32,8 +32,6 @@ MODULE_DESCRIPTION("FSL FMan MAC API based driver");
 struct mac_priv_s {
 	u8				cell_index;
 	struct fman			*fman;
-	/* List of multicast addresses */
-	struct list_head		mc_addr_list;
 	struct platform_device		*eth_dev;
 	u16				speed;
 };
@@ -57,44 +55,6 @@ static void mac_exception(struct mac_device *mac_dev,
 		__func__, ex);
 }
 
-int fman_set_multi(struct net_device *net_dev, struct mac_device *mac_dev)
-{
-	struct mac_priv_s	*priv;
-	struct mac_address	*old_addr, *tmp;
-	struct netdev_hw_addr	*ha;
-	int			err;
-	enet_addr_t		*addr;
-
-	priv = mac_dev->priv;
-
-	/* Clear previous address list */
-	list_for_each_entry_safe(old_addr, tmp, &priv->mc_addr_list, list) {
-		addr = (enet_addr_t *)old_addr->addr;
-		err = mac_dev->remove_hash_mac_addr(mac_dev->fman_mac, addr);
-		if (err < 0)
-			return err;
-
-		list_del(&old_addr->list);
-		kfree(old_addr);
-	}
-
-	/* Add all the addresses from the new list */
-	netdev_for_each_mc_addr(ha, net_dev) {
-		addr = (enet_addr_t *)ha->addr;
-		err = mac_dev->add_hash_mac_addr(mac_dev->fman_mac, addr);
-		if (err < 0)
-			return err;
-
-		tmp = kmalloc(sizeof(*tmp), GFP_ATOMIC);
-		if (!tmp)
-			return -ENOMEM;
-
-		ether_addr_copy(tmp->addr, ha->addr);
-		list_add(&tmp->list, &priv->mc_addr_list);
-	}
-	return 0;
-}
-
 static DEFINE_MUTEX(eth_lock);
 
 static struct platform_device *dpaa_eth_add_device(int fman_id,
@@ -181,8 +141,6 @@ static int mac_probe(struct platform_device *_of_dev)
 	mac_dev->priv = priv;
 	mac_dev->dev = dev;
 
-	INIT_LIST_HEAD(&priv->mc_addr_list);
-
 	/* Get the FM node */
 	dev_node = of_get_parent(mac_node);
 	if (!dev_node) {
diff --git a/drivers/net/ethernet/freescale/fman/mac.h b/drivers/net/ethernet/freescale/fman/mac.h
index fe747915cc73792b66d8bfe4339894476fc841af..be9d48aad5ef16d6826e0dc3c93b8c456cdfa925 100644
--- a/drivers/net/ethernet/freescale/fman/mac.h
+++ b/drivers/net/ethernet/freescale/fman/mac.h
@@ -39,8 +39,6 @@ struct mac_device {
 	int (*change_addr)(struct fman_mac *mac_dev, const enet_addr_t *enet_addr);
 	int (*set_allmulti)(struct fman_mac *mac_dev, bool enable);
 	int (*set_tstamp)(struct fman_mac *mac_dev, bool enable);
-	int (*set_multi)(struct net_device *net_dev,
-			 struct mac_device *mac_dev);
 	int (*set_exception)(struct fman_mac *mac_dev,
 			     enum fman_mac_exceptions exception, bool enable);
 	int (*add_hash_mac_addr)(struct fman_mac *mac_dev,

-- 
2.39.5
Re: [PATCH 2/2] net: dpaa: use __dev_mc_sync in dpaa_set_rx_mode()
Posted by Sean Anderson 1 month, 3 weeks ago
On 10/7/24 10:17, Jonas Rebmann wrote:
> The original driver first unregisters then re-registers all multicast
> addresses in the struct net_device_ops::ndo_set_rx_mode() callback.
>
> As the networking stack calls ndo_set_rx_mode() if a single multicast
> address change occurs, a significant amount of time may be used to first
> unregister and then re-register unchanged multicast addresses. This
> leads to performance issues when tracking large numbers of multicast
> addresses.
>
> Replace the unregister and register loop and the hand crafted
> mc_addr_list list handling with __dev_mc_sync(), to only update entries
> which have changed.
>
> On profiling with an fsl_dpa NIC, this patch presented a speedup of
> around 40 when successively setting up 2000 multicast groups using
> setsockopt(), without drawbacks on smaller numbers of multicast groups.
>
> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
> ---
>  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c   | 20 +++++++++--
>  drivers/net/ethernet/freescale/fman/fman_dtsec.c |  1 -
>  drivers/net/ethernet/freescale/fman/fman_memac.c |  1 -
>  drivers/net/ethernet/freescale/fman/fman_tgec.c  |  1 -
>  drivers/net/ethernet/freescale/fman/mac.c        | 42 ------------------------
>  drivers/net/ethernet/freescale/fman/mac.h        |  2 --
>  6 files changed, 18 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index 6b9b6d72db98c22b9c104833b3c8c675931fd1aa..ac06b01fe93401b0416cd6a654bac2cb40ce12aa 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -463,6 +463,22 @@ static int dpaa_set_mac_address(struct net_device *net_dev, void *addr)
>         return 0;
>  }
>
> +static int dpaa_addr_sync(struct net_device *net_dev, const u8 *addr)
> +{
> +       const struct dpaa_priv *priv = netdev_priv(net_dev);
> +
> +       return priv->mac_dev->add_hash_mac_addr(priv->mac_dev->fman_mac,
> +                                               (enet_addr_t *)addr);
> +}
> +
> +static int dpaa_addr_unsync(struct net_device *net_dev, const u8 *addr)
> +{
> +       const struct dpaa_priv *priv = netdev_priv(net_dev);
> +
> +       return priv->mac_dev->remove_hash_mac_addr(priv->mac_dev->fman_mac,
> +                                                  (enet_addr_t *)addr);
> +}
> +
>  static void dpaa_set_rx_mode(struct net_device *net_dev)
>  {
>         const struct dpaa_priv  *priv;
> @@ -490,9 +506,9 @@ static void dpaa_set_rx_mode(struct net_device *net_dev)
>                                   err);
>         }
>
> -       err = priv->mac_dev->set_multi(net_dev, priv->mac_dev);
> +       err = __dev_mc_sync(net_dev, dpaa_addr_sync, dpaa_addr_unsync);
>         if (err < 0)
> -               netif_err(priv, drv, net_dev, "mac_dev->set_multi() = %d\n",
> +               netif_err(priv, drv, net_dev, "dpaa_addr_sync() = %d\n",
>                           err);
>  }
>
> diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> index 3088da7adf0f846744079107f7f72fea74114f4a..85617bb94959f3789d75766bce8f3e11a7b321d5 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> @@ -1415,7 +1415,6 @@ int dtsec_initialization(struct mac_device *mac_dev,
>         mac_dev->set_exception          = dtsec_set_exception;
>         mac_dev->set_allmulti           = dtsec_set_allmulti;
>         mac_dev->set_tstamp             = dtsec_set_tstamp;
> -       mac_dev->set_multi              = fman_set_multi;
>         mac_dev->enable                 = dtsec_enable;
>         mac_dev->disable                = dtsec_disable;
>
> diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
> index 796e6f4e583d18be6069f78af15fbedf9557378e..3925441143fac9eecc40ea45d05f36be63b16a78 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_memac.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
> @@ -1087,7 +1087,6 @@ int memac_initialization(struct mac_device *mac_dev,
>         mac_dev->set_exception          = memac_set_exception;
>         mac_dev->set_allmulti           = memac_set_allmulti;
>         mac_dev->set_tstamp             = memac_set_tstamp;
> -       mac_dev->set_multi              = fman_set_multi;
>         mac_dev->enable                 = memac_enable;
>         mac_dev->disable                = memac_disable;
>
> diff --git a/drivers/net/ethernet/freescale/fman/fman_tgec.c b/drivers/net/ethernet/freescale/fman/fman_tgec.c
> index c2261d26db5b9374a5e52bac41c25ed8831f4822..fecfca6eba03e571cfb569b8aad20dc3fa8dc1c7 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_tgec.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_tgec.c
> @@ -771,7 +771,6 @@ int tgec_initialization(struct mac_device *mac_dev,
>         mac_dev->set_exception          = tgec_set_exception;
>         mac_dev->set_allmulti           = tgec_set_allmulti;
>         mac_dev->set_tstamp             = tgec_set_tstamp;
> -       mac_dev->set_multi              = fman_set_multi;
>         mac_dev->enable                 = tgec_enable;
>         mac_dev->disable                = tgec_disable;
>
> diff --git a/drivers/net/ethernet/freescale/fman/mac.c b/drivers/net/ethernet/freescale/fman/mac.c
> index 43f4ad29eadd495ce7f4861b3e635e22379ddc72..974d2e7e131c087ddbb41dcb906f6144a150db46 100644
> --- a/drivers/net/ethernet/freescale/fman/mac.c
> +++ b/drivers/net/ethernet/freescale/fman/mac.c
> @@ -32,8 +32,6 @@ MODULE_DESCRIPTION("FSL FMan MAC API based driver");
>  struct mac_priv_s {
>         u8                              cell_index;
>         struct fman                     *fman;
> -       /* List of multicast addresses */
> -       struct list_head                mc_addr_list;
>         struct platform_device          *eth_dev;
>         u16                             speed;
>  };
> @@ -57,44 +55,6 @@ static void mac_exception(struct mac_device *mac_dev,
>                 __func__, ex);
>  }
>
> -int fman_set_multi(struct net_device *net_dev, struct mac_device *mac_dev)
> -{
> -       struct mac_priv_s       *priv;
> -       struct mac_address      *old_addr, *tmp;
> -       struct netdev_hw_addr   *ha;
> -       int                     err;
> -       enet_addr_t             *addr;
> -
> -       priv = mac_dev->priv;
> -
> -       /* Clear previous address list */
> -       list_for_each_entry_safe(old_addr, tmp, &priv->mc_addr_list, list) {
> -               addr = (enet_addr_t *)old_addr->addr;
> -               err = mac_dev->remove_hash_mac_addr(mac_dev->fman_mac, addr);
> -               if (err < 0)
> -                       return err;
> -
> -               list_del(&old_addr->list);
> -               kfree(old_addr);
> -       }
> -
> -       /* Add all the addresses from the new list */
> -       netdev_for_each_mc_addr(ha, net_dev) {
> -               addr = (enet_addr_t *)ha->addr;
> -               err = mac_dev->add_hash_mac_addr(mac_dev->fman_mac, addr);
> -               if (err < 0)
> -                       return err;
> -
> -               tmp = kmalloc(sizeof(*tmp), GFP_ATOMIC);
> -               if (!tmp)
> -                       return -ENOMEM;
> -
> -               ether_addr_copy(tmp->addr, ha->addr);
> -               list_add(&tmp->list, &priv->mc_addr_list);
> -       }
> -       return 0;
> -}
> -
>  static DEFINE_MUTEX(eth_lock);
>
>  static struct platform_device *dpaa_eth_add_device(int fman_id,
> @@ -181,8 +141,6 @@ static int mac_probe(struct platform_device *_of_dev)
>         mac_dev->priv = priv;
>         mac_dev->dev = dev;
>
> -       INIT_LIST_HEAD(&priv->mc_addr_list);
> -
>         /* Get the FM node */
>         dev_node = of_get_parent(mac_node);
>         if (!dev_node) {
> diff --git a/drivers/net/ethernet/freescale/fman/mac.h b/drivers/net/ethernet/freescale/fman/mac.h
> index fe747915cc73792b66d8bfe4339894476fc841af..be9d48aad5ef16d6826e0dc3c93b8c456cdfa925 100644
> --- a/drivers/net/ethernet/freescale/fman/mac.h
> +++ b/drivers/net/ethernet/freescale/fman/mac.h
> @@ -39,8 +39,6 @@ struct mac_device {
>         int (*change_addr)(struct fman_mac *mac_dev, const enet_addr_t *enet_addr);
>         int (*set_allmulti)(struct fman_mac *mac_dev, bool enable);
>         int (*set_tstamp)(struct fman_mac *mac_dev, bool enable);
> -       int (*set_multi)(struct net_device *net_dev,
> -                        struct mac_device *mac_dev);
>         int (*set_exception)(struct fman_mac *mac_dev,
>                              enum fman_mac_exceptions exception, bool enable);
>         int (*add_hash_mac_addr)(struct fman_mac *mac_dev,
>
> --
> 2.39.5
>

Reviewed-by: Sean Anderson <sean.anderson@seco.com>

[EmbeddedWorldNorthAmerica2024, SECO SpA]<https://s2.goeshow.com/nmna/ew/2024/registration_form.cfm>