| Message ID | 20230621132106.991342-1-piotrx.gardocki@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0ec92a8f56ff07237dbe8af7c7a72aba7f957baf |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: fix net device address assign type | expand |
On Wed, Jun 21, 2023 at 03:21:06PM +0200, Piotr Gardocki wrote: > Commit ad72c4a06acc introduced optimization to return from function > quickly if the MAC address is not changing at all. It was reported > that such change causes dev->addr_assign_type to not change > to NET_ADDR_SET from _PERM or _RANDOM. > Restore the old behavior and skip only call to ndo_set_mac_address. > > Fixes: ad72c4a06acc ("net: add check for current MAC address in dev_set_mac_address") > Reported-by: Gal Pressman <gal@nvidia.com> > Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
Wed, Jun 21, 2023 at 03:21:06PM CEST, piotrx.gardocki@intel.com wrote: >Commit ad72c4a06acc introduced optimization to return from function Out of curiosity, what impact does this optimization have? Is it worth it to have such optimization at all? Wouldn't simple revert of the fixes commit do the trick? If not, see below. >quickly if the MAC address is not changing at all. It was reported >that such change causes dev->addr_assign_type to not change >to NET_ADDR_SET from _PERM or _RANDOM. >Restore the old behavior and skip only call to ndo_set_mac_address. > >Fixes: ad72c4a06acc ("net: add check for current MAC address in dev_set_mac_address") >Reported-by: Gal Pressman <gal@nvidia.com> >Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com> >--- > net/core/dev.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > >diff --git a/net/core/dev.c b/net/core/dev.c >index e4ff0adf5523..69a3e544676c 100644 >--- a/net/core/dev.c >+++ b/net/core/dev.c >@@ -8781,14 +8781,14 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, > return -EINVAL; > if (!netif_device_present(dev)) > return -ENODEV; >- if (!memcmp(dev->dev_addr, sa->sa_data, dev->addr_len)) >- return 0; > err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack); > if (err) > return err; >- err = ops->ndo_set_mac_address(dev, sa); >- if (err) >- return err; >+ if (memcmp(dev->dev_addr, sa->sa_data, dev->addr_len)) { >+ err = ops->ndo_set_mac_address(dev, sa); >+ if (err) >+ return err; >+ } > dev->addr_assign_type = NET_ADDR_SET; > call_netdevice_notifiers(NETDEV_CHANGEADDR, dev); Although I don't think the notifiers here and dev_pre_changeaddr_notify() above have to be called in case the address didn't actually change, it restores the old behaviour, even with the netlink notification, which is probably good. Reviewed-by: Jiri Pirko <jiri@nvidia.com> > add_device_randomness(dev->dev_addr, dev->addr_len); >-- >2.34.1 > >
On 22.06.2023 10:22, Jiri Pirko wrote: > Wed, Jun 21, 2023 at 03:21:06PM CEST, piotrx.gardocki@intel.com wrote: >> Commit ad72c4a06acc introduced optimization to return from function > > Out of curiosity, what impact does this optimization have? Is it worth > it to have such optimization at all? Wouldn't simple revert of the fixes > commit do the trick? If not, see below. Thanks for review. My main goal originally was to skip call to ndo_set_mac_address. The benefit of this depends on how given driver handles such request. Some drivers notify their hardware about the "change", iavf for example sends a request to PF driver (and awaits for response). i40e and ice already had this check (I removed them in previous patch set) and we wanted to also introduce it in iavf. But it was suggested to move this check to core to have benefit for all drivers.
Thu, Jun 22, 2023 at 02:42:53PM CEST, piotrx.gardocki@intel.com wrote: >On 22.06.2023 10:22, Jiri Pirko wrote: >> Wed, Jun 21, 2023 at 03:21:06PM CEST, piotrx.gardocki@intel.com wrote: >>> Commit ad72c4a06acc introduced optimization to return from function >> >> Out of curiosity, what impact does this optimization have? Is it worth >> it to have such optimization at all? Wouldn't simple revert of the fixes >> commit do the trick? If not, see below. > >Thanks for review. My main goal originally was to skip call to ndo_set_mac_address. >The benefit of this depends on how given driver handles such request. Some drivers >notify their hardware about the "change", iavf for example sends a request to PF >driver (and awaits for response). i40e and ice already had this check (I removed >them in previous patch set) and we wanted to also introduce it in iavf. But it >was suggested to move this check to core to have benefit for all drivers. Okay. Makes sense.
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 21 Jun 2023 15:21:06 +0200 you wrote: > Commit ad72c4a06acc introduced optimization to return from function > quickly if the MAC address is not changing at all. It was reported > that such change causes dev->addr_assign_type to not change > to NET_ADDR_SET from _PERM or _RANDOM. > Restore the old behavior and skip only call to ndo_set_mac_address. > > Fixes: ad72c4a06acc ("net: add check for current MAC address in dev_set_mac_address") > Reported-by: Gal Pressman <gal@nvidia.com> > Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com> > > [...] Here is the summary with links: - [net-next] net: fix net device address assign type https://git.kernel.org/netdev/net-next/c/0ec92a8f56ff You are awesome, thank you!
diff --git a/net/core/dev.c b/net/core/dev.c index e4ff0adf5523..69a3e544676c 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -8781,14 +8781,14 @@ int dev_set_mac_address(struct net_device *dev, struct sockaddr *sa, return -EINVAL; if (!netif_device_present(dev)) return -ENODEV; - if (!memcmp(dev->dev_addr, sa->sa_data, dev->addr_len)) - return 0; err = dev_pre_changeaddr_notify(dev, sa->sa_data, extack); if (err) return err; - err = ops->ndo_set_mac_address(dev, sa); - if (err) - return err; + if (memcmp(dev->dev_addr, sa->sa_data, dev->addr_len)) { + err = ops->ndo_set_mac_address(dev, sa); + if (err) + return err; + } dev->addr_assign_type = NET_ADDR_SET; call_netdevice_notifiers(NETDEV_CHANGEADDR, dev); add_device_randomness(dev->dev_addr, dev->addr_len);
Commit ad72c4a06acc introduced optimization to return from function quickly if the MAC address is not changing at all. It was reported that such change causes dev->addr_assign_type to not change to NET_ADDR_SET from _PERM or _RANDOM. Restore the old behavior and skip only call to ndo_set_mac_address. Fixes: ad72c4a06acc ("net: add check for current MAC address in dev_set_mac_address") Reported-by: Gal Pressman <gal@nvidia.com> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com> --- net/core/dev.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)