Re: [PATCH net-next 2/2] net: phy: phy_link_topology: Lazy-initialize the link topology

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Heiner,

On Wed, 8 May 2024 07:44:22 +0200
Heiner Kallweit <hkallweit1@gmail.com> wrote:

> On 07.05.2024 12:28, Maxime Chevallier wrote:
> > Having the net_device's init path for the link_topology depend on
> > IS_REACHABLE(PHYLIB)-protected helpers triggers errors when modules are being
> > built with phylib as a module as-well, as they expect netdev->link_topo
> > to be initialized.
> > 
> > Move the link_topo initialization at the first PHY insertion, which will
> > both improve the memory usage, and make the behaviour more predicatble
> > and robust.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > Fixes: 6916e461e793 ("net: phy: Introduce ethernet link topology representation")
> > Closes: https://lore.kernel.org/netdev/2e11b89d-100f-49e7-9c9a-834cc0b82f97@gmail.com/
> > Closes: https://lore.kernel.org/netdev/20240409201553.GA4124869@dev-arch.thelio-3990X/
> > ---
> >  drivers/net/phy/phy_link_topology.c    | 31 ++++++---------------
> >  include/linux/netdevice.h              |  2 ++
> >  include/linux/phy_link_topology.h      | 23 ++++++++--------
> >  include/linux/phy_link_topology_core.h | 23 +++-------------
> >  net/core/dev.c                         | 38 ++++++++++++++++++++++----
> >  5 files changed, 58 insertions(+), 59 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> > index 0e36bd7c15dc..b1aba9313e73 100644
> > --- a/drivers/net/phy/phy_link_topology.c
> > +++ b/drivers/net/phy/phy_link_topology.c
> > @@ -12,29 +12,6 @@
> >  #include <linux/rtnetlink.h>
> >  #include <linux/xarray.h>
> >  
> > -struct phy_link_topology *phy_link_topo_create(struct net_device *dev)
> > -{
> > -	struct phy_link_topology *topo;
> > -
> > -	topo = kzalloc(sizeof(*topo), GFP_KERNEL);
> > -	if (!topo)
> > -		return ERR_PTR(-ENOMEM);
> > -
> > -	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
> > -	topo->next_phy_index = 1;
> > -
> > -	return topo;
> > -}
> > -
> > -void phy_link_topo_destroy(struct phy_link_topology *topo)
> > -{
> > -	if (!topo)
> > -		return;
> > -
> > -	xa_destroy(&topo->phys);
> > -	kfree(topo);
> > -}
> > -
> >  int phy_link_topo_add_phy(struct net_device *dev,
> >  			  struct phy_device *phy,
> >  			  enum phy_upstream upt, void *upstream)
> > @@ -43,6 +20,14 @@ int phy_link_topo_add_phy(struct net_device *dev,
> >  	struct phy_device_node *pdn;
> >  	int ret;
> >  
> > +	if (!topo) {
> > +		ret = netdev_alloc_phy_link_topology(dev);  
> 
> This function is implemented in net core, but used only here.
> So move the implementation here?

If it's OK not to have both helpers to alloc and destroy in different
files, then I'll move it :)

> 
> > +		if (ret)
> > +			return ret;
> > +
> > +		topo = dev->link_topo;
> > +	}
> > +
> >  	pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> >  	if (!pdn)
> >  		return -ENOMEM;
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index cf261fb89d73..25a0a77cfadc 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -4569,6 +4569,8 @@ void __hw_addr_unsync_dev(struct netdev_hw_addr_list *list,
> >  					const unsigned char *));
> >  void __hw_addr_init(struct netdev_hw_addr_list *list);
> >  
> > +int netdev_alloc_phy_link_topology(struct net_device *dev);
> > +
> >  /* Functions used for device addresses handling */
> >  void dev_addr_mod(struct net_device *dev, unsigned int offset,
> >  		  const void *addr, size_t len);
> > diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
> > index 166a01710aa2..3501f9a9e932 100644
> > --- a/include/linux/phy_link_topology.h
> > +++ b/include/linux/phy_link_topology.h
> > @@ -32,10 +32,12 @@ struct phy_device_node {
> >  	struct phy_device *phy;
> >  };
> >  
> > -struct phy_link_topology {
> > -	struct xarray phys;
> > -	u32 next_phy_index;
> > -};
> > +#if IS_ENABLED(CONFIG_PHYLIB)
> > +int phy_link_topo_add_phy(struct net_device *dev,
> > +			  struct phy_device *phy,
> > +			  enum phy_upstream upt, void *upstream);
> > +
> > +void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy);
> >  
> >  static inline struct phy_device
> >  *phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
> > @@ -53,13 +55,6 @@ static inline struct phy_device
> >  	return NULL;
> >  }
> >  
> > -#if IS_REACHABLE(CONFIG_PHYLIB)
> > -int phy_link_topo_add_phy(struct net_device *dev,
> > -			  struct phy_device *phy,
> > -			  enum phy_upstream upt, void *upstream);
> > -
> > -void phy_link_topo_del_phy(struct net_device *dev, struct phy_device *phy);
> > -
> >  #else
> >  static inline int phy_link_topo_add_phy(struct net_device *dev,
> >  					struct phy_device *phy,
> > @@ -72,6 +67,12 @@ static inline void phy_link_topo_del_phy(struct net_device *dev,
> >  					 struct phy_device *phy)
> >  {
> >  }
> > +
> > +static inline struct phy_device *
> > +phy_link_topo_get_phy(struct net_device *dev, u32 phyindex)
> > +{
> > +	return NULL;
> > +}
> >  #endif
> >  
> >  #endif /* __PHY_LINK_TOPOLOGY_H */
> > diff --git a/include/linux/phy_link_topology_core.h b/include/linux/phy_link_topology_core.h
> > index 0a6479055745..f9c0520806fb 100644
> > --- a/include/linux/phy_link_topology_core.h
> > +++ b/include/linux/phy_link_topology_core.h
> > @@ -2,24 +2,9 @@
> >  #ifndef __PHY_LINK_TOPOLOGY_CORE_H
> >  #define __PHY_LINK_TOPOLOGY_CORE_H
> >  
> > -struct phy_link_topology;
> > -
> > -#if IS_REACHABLE(CONFIG_PHYLIB)
> > -
> > -struct phy_link_topology *phy_link_topo_create(struct net_device *dev);
> > -void phy_link_topo_destroy(struct phy_link_topology *topo);
> > -
> > -#else
> > -
> > -static inline struct phy_link_topology *phy_link_topo_create(struct net_device *dev)
> > -{
> > -	return NULL;
> > -}
> > -
> > -static inline void phy_link_topo_destroy(struct phy_link_topology *topo)
> > -{
> > -}
> > -
> > -#endif
> > +struct phy_link_topology {
> > +	struct xarray phys;
> > +	u32 next_phy_index;
> > +};
> >    
> This is all which is left in this header. As this header is public anyway,
> better move this definition to phy_link_topology.h?

Well I'll have to include the whole phy_link_topology.h in
net/core/dev.c, and I was trying to avoid including that whole header,
and keep the included content to a bare minimum.

> 
> >  #endif /* __PHY_LINK_TOPOLOGY_CORE_H */
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index d2ce91a334c1..1b4ffc273a04 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -10256,6 +10256,35 @@ static void netdev_do_free_pcpu_stats(struct net_device *dev)
> >  	}
> >  }
> >  
> > +int netdev_alloc_phy_link_topology(struct net_device *dev)
> > +{
> > +	struct phy_link_topology *topo;
> > +
> > +	topo = kzalloc(sizeof(*topo), GFP_KERNEL);
> > +	if (!topo)
> > +		return -ENOMEM;
> > +
> > +	xa_init_flags(&topo->phys, XA_FLAGS_ALLOC1);
> > +	topo->next_phy_index = 1;
> > +
> > +	dev->link_topo = topo;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(netdev_alloc_phy_link_topology);
> > +
> > +static void netdev_free_phy_link_topology(struct net_device *dev)
> > +{
> > +	struct phy_link_topology *topo = dev->link_topo;
> > +
> > +	if (!topo)
> > +		return;
> > +
> > +	xa_destroy(&topo->phys);
> > +	kfree(topo);
> > +	dev->link_topo = NULL;  
> 
> Give the compiler a chance to remove this function if
> CONFIG_PHYLIB isn't enabled.
> 
> if (IS_ENABLED(CONFIG_PHYLIB) && topo) {
> 	xa_destroy(&topo->phys);
> 	kfree(topo);
> 	dev->link_topo = NULL;
> }

Well if we add more things to the link topology, then it's going to be
easy to forget updating that without clear helpers for alloc/destroy,
don't you think ?

I can try to squeeze another iteration before net-next closes.

Maxime

> > +}
> > +
> >  /**
> >   * register_netdevice() - register a network device
> >   * @dev: device to register
> > @@ -10998,11 +11027,6 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
> >  #ifdef CONFIG_NET_SCHED
> >  	hash_init(dev->qdisc_hash);
> >  #endif
> > -	dev->link_topo = phy_link_topo_create(dev);
> > -	if (IS_ERR(dev->link_topo)) {
> > -		dev->link_topo = NULL;
> > -		goto free_all;
> > -	}
> >  
> >  	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
> >  	setup(dev);
> > @@ -11092,7 +11116,9 @@ void free_netdev(struct net_device *dev)
> >  	free_percpu(dev->xdp_bulkq);
> >  	dev->xdp_bulkq = NULL;
> >  
> > -	phy_link_topo_destroy(dev->link_topo);
> > +#if IS_ENABLED(CONFIG_PHYLIB)
> > +	netdev_free_phy_link_topology(dev);
> > +#endif
> >    
> Then the conditional compiling can be removed here.
> 
> >  	/*  Compatibility with error handling in drivers */
> >  	if (dev->reg_state == NETREG_UNINITIALIZED ||  
> 
> 
> 
>