Hello Jakub,
On Thu, Mar 21, 2024 at 03:17:44PM -0700, Jakub Kicinski wrote:
> On Thu, 21 Mar 2024 15:02:39 -0700 Jeff Johnson wrote:
> > >> As suggested there we should just use kmalloc/kfree to match the existing logic.
> > >
> > > Please no. There is no magic here. alloc + free must match whether
> > > you're using magic object alloc wrapper (alloc_netdev()) or straight
> > > up kzalloc().
> >
> > Based upon the ath11k patch there must be something going on with
> > alloc_netdev()/free_netdev() that doesn't occur when these aren't used.
>
> Looks like init_dummy_netdev wipes the netdev structure clean, so I
> don't think we can use it directly as the setup function, Breno :(
Before my patch, init_dummy_netdev was being also used. The patch was
basically replacing the init_dummy_netdev by alloc_netdev() with will
call "setup(dev);" later.
- init_dummy_netdev(&irq_grp->napi_ndev);
+ irq_grp->napi_ndev = alloc_netdev(0, "dummy", NET_NAME_UNKNOWN,
+ init_dummy_netdev);
I am wondering if alloc_netdev() is messing with something instead of
init_dummy_netdev().
Also, Kalle's crash is during rmmod, and not during initialization.
getting NULL after free_netdev() is called.
> Maybe we should add a new helper to "alloc dummy netdev" which can
> call alloc_netdev() with right arguments and do necessary init?
What are the right arguments in this case?
Thanks!
On Fri, 22 Mar 2024 07:58:02 -0700 Breno Leitao wrote:
> > Looks like init_dummy_netdev wipes the netdev structure clean, so I
> > don't think we can use it directly as the setup function, Breno :(
>
> Before my patch, init_dummy_netdev was being also used. The patch was
> basically replacing the init_dummy_netdev by alloc_netdev() with will
> call "setup(dev);" later.
>
> - init_dummy_netdev(&irq_grp->napi_ndev);
> + irq_grp->napi_ndev = alloc_netdev(0, "dummy", NET_NAME_UNKNOWN,
> + init_dummy_netdev);
>
> I am wondering if alloc_netdev() is messing with something instead of
> init_dummy_netdev().
alloc_netdev() allocates some memory and initializes lists which
free_netdev() wants to free, basically. But init_dummy_netdev() does:
/* Clear everything. Note we don't initialize spinlocks
* are they aren't supposed to be taken by any of the
* NAPI code and this dummy netdev is supposed to be
* only ever used for NAPI polls
*/
memset(dev, 0, sizeof(struct net_device));
so all those pointers and init alloc_netdev() did before calling setup
will get wiped.
> Also, Kalle's crash is during rmmod, and not during initialization.
> getting NULL after free_netdev() is called.
>
> > Maybe we should add a new helper to "alloc dummy netdev" which can
> > call alloc_netdev() with right arguments and do necessary init?
>
> What are the right arguments in this case?
I'm not sure we have a noop setup() callback today. If you define a
wrapper to allocate a dummy netdev you can define a new empty function
next to it and pass that as init? Hope I got the question right.
Hello Jakub,
On Fri, Mar 22, 2024 at 08:23:36AM -0700, Jakub Kicinski wrote:
> > > Maybe we should add a new helper to "alloc dummy netdev" which can
> > > call alloc_netdev() with right arguments and do necessary init?
> >
> > What are the right arguments in this case?
>
> I'm not sure we have a noop setup() callback today. If you define a
> wrapper to allocate a dummy netdev you can define a new empty function
> next to it and pass that as init? Hope I got the question right.
Thanks for the explanation, it is clear now. I've been working on it,
and this is what I came up with. This is compile-tested by now, and, if
this is what you had in mind, I will do more extensive testing.
commit db794d99950f68731884a67d911094d94179c522
Author: Breno Leitao <[email protected]>
Date: Wed Mar 27 07:20:03 2024 -0700
net: Create net_device allocator for dummy
Create a helper to allocate and initialize dummy netdevices. This
function basically simplify the allocation of dummy devices, by
allocating and initializing it.
Suggested-by: Jakub Kicinski <[email protected]>
Signed-off-by: Breno Leitao <[email protected]>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 643d2b469c49..9d1a5383c23f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4546,6 +4546,9 @@ static inline void netif_addr_unlock_bh(struct net_device *dev)
void ether_setup(struct net_device *dev);
+/* Allocate dummy net_device */
+struct net_device *alloc_netdev_dummy(int sizeof_priv, const char *name);
+
/* Support for loadable net-drivers */
struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
unsigned char name_assign_type,
diff --git a/net/core/dev.c b/net/core/dev.c
index a08d698fe45c..628f35c3cfa2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10998,6 +10998,13 @@ void free_netdev(struct net_device *dev)
}
EXPORT_SYMBOL(free_netdev);
+struct net_device *alloc_netdev_dummy(int sizeof_priv, const char *name)
+{
+ return alloc_netdev(sizeof_priv, name, NET_NAME_UNKNOWN,
+ init_dummy_netdev_core);
+}
+EXPORT_SYMBOL_GPL(alloc_netdev_dummy);
+
/**
* synchronize_net - Synchronize with packet receive processing
*
commit 35500cd6a5db0bbdedbc1067758948769c7ce57e
Author: Breno Leitao <[email protected]>
Date: Wed Mar 27 07:07:40 2024 -0700
net: Split init_dummy_netdev
It is impossible to use init_dummy_netdev together with alloc_netdev()
as the 'setup' argument.
This is because alloc_netdev() initializes some fields in the net_device
structure, and later init_dummy_netdev() memzero them all. This casues
some problems as reported here:
https://lore.kernel.org/all/[email protected]/
Split the function in two. Create a new function called
init_dummy_netdev_core() that does not memset the net_device structure.
Then have init_dummy_netdev() memseting and calling
init_dummy_netdev_core().
init_dummy_netdev_core() will be the function that could be called as
an argument for alloc_netdev().
Signed-off-by: Breno Leitao <[email protected]>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c6f6ac779b34..643d2b469c49 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3119,6 +3119,7 @@ int netdev_refcnt_read(const struct net_device *dev);
void free_netdev(struct net_device *dev);
void netdev_freemem(struct net_device *dev);
void init_dummy_netdev(struct net_device *dev);
+void init_dummy_netdev_core(struct net_device *dev);
struct net_device *netdev_get_xmit_slave(struct net_device *dev,
struct sk_buff *skb,
diff --git a/net/core/dev.c b/net/core/dev.c
index 0766a245816b..a08d698fe45c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10340,25 +10340,11 @@ int register_netdevice(struct net_device *dev)
}
EXPORT_SYMBOL(register_netdevice);
-/**
- * init_dummy_netdev - init a dummy network device for NAPI
- * @dev: device to init
- *
- * This takes a network device structure and initialize the minimum
- * amount of fields so it can be used to schedule NAPI polls without
- * registering a full blown interface. This is to be used by drivers
- * that need to tie several hardware interfaces to a single NAPI
- * poll scheduler due to HW limitations.
+/* Same as init_dummy_netdev, but, basically do not call memset.
+ * This is useful if you are calling this function after alloc_netdev()
*/
-void init_dummy_netdev(struct net_device *dev)
+void init_dummy_netdev_core(struct net_device *dev)
{
- /* Clear everything. Note we don't initialize spinlocks
- * are they aren't supposed to be taken by any of the
- * NAPI code and this dummy netdev is supposed to be
- * only ever used for NAPI polls
- */
- memset(dev, 0, sizeof(struct net_device));
-
/* make sure we BUG if trying to hit standard
* register/unregister code path
*/
@@ -10379,8 +10365,29 @@ void init_dummy_netdev(struct net_device *dev)
* its refcount.
*/
}
-EXPORT_SYMBOL_GPL(init_dummy_netdev);
+EXPORT_SYMBOL_GPL(init_dummy_netdev_core);
+/**
+ * init_dummy_netdev - init a dummy network device for NAPI
+ * @dev: device to init
+ *
+ * This takes a network device structure and initialize the minimum
+ * amount of fields so it can be used to schedule NAPI polls without
+ * registering a full blown interface. This is to be used by drivers
+ * that need to tie several hardware interfaces to a single NAPI
+ * poll scheduler due to HW limitations.
+ */
+void init_dummy_netdev(struct net_device *dev)
+{
+ /* Clear everything. Note we don't initialize spinlocks
+ * are they aren't supposed to be taken by any of the
+ * NAPI code and this dummy netdev is supposed to be
+ * only ever used for NAPI polls
+ */
+ memset(dev, 0, sizeof(struct net_device));
+ init_dummy_netdev_core(dev);
+}
+EXPORT_SYMBOL_GPL(init_dummy_netdev);
/**
* register_netdev - register a network device