[patch] ipv4: initialize arp_tbl rw lock

[patch] ipv4: initialize arp_tbl rw lock

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

The qeth driver makes use of the arp_tbl rw lock. CONFIG_DEBUG_SPINLOCK
detects that this lock is not initialized as it is supposed to be.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 net/ipv4/arp.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 041dadd..ea54216 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -202,6 +202,7 @@ struct neigh_table arp_tbl = {
 	.gc_thresh1 =	128,
 	.gc_thresh2 =	512,
 	.gc_thresh3 =	1024,
+	.lock = RW_LOCK_UNLOCKED,
 };
 
 int arp_mc_map(u32 addr, u8 *haddr, struct net_device *dev, int dir)

Re: [patch] ipv4: initialize arp_tbl rw lock

[Stephen Hemminger]

On Fri, 7 Apr 2006 10:15:33 +0200
Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> The qeth driver makes use of the arp_tbl rw lock. CONFIG_DEBUG_SPINLOCK
> detects that this lock is not initialized as it is supposed to be.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---

This is a initialization order problem then, because:
	arp_init
	   neigh_table_init
		rwlock_init

does the initialization already. So fix the initialization sequence
of the qeth driver or you will have other problems.

My impression was the -rt folks wanted all lock initializations t be
done at runtime not compile time.

Re: [patch] ipv4: initialize arp_tbl rw lock

[Heiko Carstens]

> > The qeth driver makes use of the arp_tbl rw lock. CONFIG_DEBUG_SPINLOCK
> > detects that this lock is not initialized as it is supposed to be.
>
> This is a initialization order problem then, because:
> 	arp_init
> 	   neigh_table_init
> 		rwlock_init
> 
> does the initialization already. So fix the initialization sequence
> of the qeth driver or you will have other problems.
> 
> My impression was the -rt folks wanted all lock initializations t be
> done at runtime not compile time.

Ok, so the problem seems to be that inet_init gets called after qeth_init.
Looking at the top level Makefile this seems to be true for all network
drivers in drivers/net/ and drivers/s390/net/ since we have

vmlinux-main := $(core-y) $(libs-y) $(drivers-y) $(net-y)

The patch below works for me... I guess there must be a better way to make
sure that any networking driver's initcall is made before inet_init?

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 Makefile |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b401942..c5cea07 100644
--- a/Makefile
+++ b/Makefile
@@ -567,7 +567,7 @@ #
 # System.map is generated to document addresses of all kernel symbols
 
 vmlinux-init := $(head-y) $(init-y)
-vmlinux-main := $(core-y) $(libs-y) $(drivers-y) $(net-y)
+vmlinux-main := $(core-y) $(libs-y) $(net-y) $(drivers-y)
 vmlinux-all  := $(vmlinux-init) $(vmlinux-main)
 vmlinux-lds  := arch/$(ARCH)/kernel/vmlinux.lds
 

Re: [patch] ipv4: initialize arp_tbl rw lock

[Andrew Morton]

Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
>
> Ok, so the problem seems to be that inet_init gets called after qeth_init.
>  Looking at the top level Makefile this seems to be true for all network
>  drivers in drivers/net/ and drivers/s390/net/ since we have
> 
>  vmlinux-main := $(core-y) $(libs-y) $(drivers-y) $(net-y)
> 
>  The patch below works for me... I guess there must be a better way to make
>  sure that any networking driver's initcall is made before inet_init?
> 
>  Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>  ---
> 
>   Makefile |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
>  diff --git a/Makefile b/Makefile
>  index b401942..c5cea07 100644
>  --- a/Makefile
>  +++ b/Makefile
>  @@ -567,7 +567,7 @@ #
>   # System.map is generated to document addresses of all kernel symbols
>   
>   vmlinux-init := $(head-y) $(init-y)
>  -vmlinux-main := $(core-y) $(libs-y) $(drivers-y) $(net-y)
>  +vmlinux-main := $(core-y) $(libs-y) $(net-y) $(drivers-y)

<wonders what this will break>

I have a bad feeling that one day we're going to come unstuck with this
practice.  Is there anything which dictates that the linker has to lay
sections out in .o-file-order?

Perhaps net initcalls should be using something higher priority than
device_initcall().

Re: [patch] ipv4: initialize arp_tbl rw lock

[Christian Borntraeger]

As spinlock debugging still does not work with the qeth driver I want to pick 
up the discussion.

On Saturday 08 April 2006 12:12, Andrew Morton wrote:
> Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
[...]
> >  -vmlinux-main := $(core-y) $(libs-y) $(drivers-y) $(net-y)
> >  +vmlinux-main := $(core-y) $(libs-y) $(net-y) $(drivers-y)
>
> <wonders what this will break>

What about putting this patch into mm and find out?
>
> I have a bad feeling that one day we're going to come unstuck with this
> practice.  Is there anything which dictates that the linker has to lay
> sections out in .o-file-order?
>
> Perhaps net initcalls should be using something higher priority than
> device_initcall().

I agree that the initcall order offers a lot of room for improvement (like 
dependencies). Is anybody aware of any work into this direction?

-- 
Mit freundlichen Grüßen / Best Regards

Christian Borntraeger
Linux Software Engineer zSeries Linux & Virtualization

Re: [patch] ipv4: initialize arp_tbl rw lock

[David S. Miller]

From: Christian Borntraeger <borntrae@de.ibm.com>
Date: Wed, 19 Apr 2006 12:45:48 +0200

> As spinlock debugging still does not work with the qeth driver I
> want to pick up the discussion.

Does something like the patch below work?

But this all begs the question, what happens if you want to
dig into the internals of a protocol which is built modular and
hasn't been loaded yet?

diff --git a/include/linux/init.h b/include/linux/init.h
index 93dcbe1..8169f25 100644
--- a/include/linux/init.h
+++ b/include/linux/init.h
@@ -95,8 +95,9 @@ #define postcore_initcall(fn)		__define_
 #define arch_initcall(fn)		__define_initcall("3",fn)
 #define subsys_initcall(fn)		__define_initcall("4",fn)
 #define fs_initcall(fn)			__define_initcall("5",fn)
-#define device_initcall(fn)		__define_initcall("6",fn)
-#define late_initcall(fn)		__define_initcall("7",fn)
+#define net_initcall(fn)		__define_initcall("6",fn)
+#define device_initcall(fn)		__define_initcall("7",fn)
+#define late_initcall(fn)		__define_initcall("8",fn)
 
 #define __initcall(fn) device_initcall(fn)
 
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index dc206f1..9803a57 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1257,7 +1257,7 @@ out_unregister_udp_proto:
 	goto out;
 }
 
-module_init(inet_init);
+net_initcall(inet_init);
 
 /* ------------------------------------------------------------------------ */
 

Re: [patch] ipv4: initialize arp_tbl rw lock

[Heiko Carstens]

> > As spinlock debugging still does not work with the qeth driver I
> > want to pick up the discussion.
> 
> Does something like the patch below work?
> 
> But this all begs the question, what happens if you want to
> dig into the internals of a protocol which is built modular and
> hasn't been loaded yet?
> 
> diff --git a/include/linux/init.h b/include/linux/init.h
> index 93dcbe1..8169f25 100644
> --- a/include/linux/init.h
> +++ b/include/linux/init.h
> @@ -95,8 +95,9 @@ #define postcore_initcall(fn)		__define_
>  #define arch_initcall(fn)		__define_initcall("3",fn)
>  #define subsys_initcall(fn)		__define_initcall("4",fn)
>  #define fs_initcall(fn)			__define_initcall("5",fn)
> -#define device_initcall(fn)		__define_initcall("6",fn)
> -#define late_initcall(fn)		__define_initcall("7",fn)
> +#define net_initcall(fn)		__define_initcall("6",fn)
> +#define device_initcall(fn)		__define_initcall("7",fn)
> +#define late_initcall(fn)		__define_initcall("8",fn)
>  
>  #define __initcall(fn) device_initcall(fn)
>  
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index dc206f1..9803a57 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1257,7 +1257,7 @@ out_unregister_udp_proto:
>  	goto out;
>  }
>  
> -module_init(inet_init);
> +net_initcall(inet_init);

That's exactly the same thing that I tried to. It didn't work for me since I
saw "sometimes" the described rcu_update latencies.
Today I was able to boot the machine 30 times and just saw it once... Not very
helpful for debugging this :(
Btw.: I guess the linker scripts need an update too, so that the new
.initcall8.init section doesn't get discarded.

Re: [patch] ipv4: initialize arp_tbl rw lock

[David S. Miller]

From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Sat, 8 Apr 2006 12:02:13 +0200

> Ok, so the problem seems to be that inet_init gets called after qeth_init.
> Looking at the top level Makefile this seems to be true for all network
> drivers in drivers/net/ and drivers/s390/net/ since we have
> 
> vmlinux-main := $(core-y) $(libs-y) $(drivers-y) $(net-y)
> 
> The patch below works for me... I guess there must be a better way to make
> sure that any networking driver's initcall is made before inet_init?
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

We could make inet_init() a subsystem init but I vaguely recall
that we were doing that at one point and it broke things for
some reason.

Perhaps fs_initcall() would work better.  Or if that causes
problems we could create a net_initcall() that sits between
fs_initcall() and device_initcall().

Or any other ideas?

Re: [patch] ipv4: initialize arp_tbl rw lock

[Heiko Carstens]

> We could make inet_init() a subsystem init but I vaguely recall
> that we were doing that at one point and it broke things for
> some reason.
> 
> Perhaps fs_initcall() would work better.  Or if that causes
> problems we could create a net_initcall() that sits between
> fs_initcall() and device_initcall().
> 
> Or any other ideas?

Just tried fs_initcall() and net_initcall(). Both seem to have some
side effects:
Symptom is that console output sometimes hangs for several seconds at:
"NET: Registered protocol family 2" while all cpus are in cpu_idle().

Heiko

Re: [patch] ipv4: initialize arp_tbl rw lock

[Sam Ravnborg]

On Sat, Apr 08, 2006 at 03:14:04AM -0700, David S. Miller wrote:
 
> Perhaps fs_initcall() would work better.  Or if that causes
> problems we could create a net_initcall() that sits between
> fs_initcall() and device_initcall().

fs_initcall() seems to be used mainly for "init after subsystem" stuff.
Within fs/ only pipe.c uses fs_initcall().

If we are going to overload the usage of fs_initcall() even more then
we should maybe try to rename it?


	Sam

Re: [patch] ipv4: initialize arp_tbl rw lock

[Heiko Carstens]

> > Ok, so the problem seems to be that inet_init gets called after qeth_init.
> > Looking at the top level Makefile this seems to be true for all network
> > drivers in drivers/net/ and drivers/s390/net/ since we have
> > 
> > vmlinux-main := $(core-y) $(libs-y) $(drivers-y) $(net-y)
> > 
> > The patch below works for me... I guess there must be a better way to make
> > sure that any networking driver's initcall is made before inet_init?
> > 
> > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> We could make inet_init() a subsystem init but I vaguely recall
> that we were doing that at one point and it broke things for
> some reason.
> 
> Perhaps fs_initcall() would work better.  Or if that causes
> problems we could create a net_initcall() that sits between
> fs_initcall() and device_initcall().
> 
> Or any other ideas?

Tried to figure out what is causing the delays I experienced when I replaced
module_init() in af_inet.c with fs_initcall(). After all it turned out that
synchronize_net() which is basicically nothing else than synchronize_rcu()
sometimes takes several seconds to complete?! No idea why that is...

callchain: inet_init() -> inet_register_protosw() -> synchronize_net()

Re: [patch] ipv4: initialize arp_tbl rw lock

[David S. Miller]

From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Sat, 15 Apr 2006 09:27:45 +0200

> Tried to figure out what is causing the delays I experienced when I replaced
> module_init() in af_inet.c with fs_initcall(). After all it turned out that
> synchronize_net() which is basicically nothing else than synchronize_rcu()
> sometimes takes several seconds to complete?! No idea why that is...
> 
> callchain: inet_init() -> inet_register_protosw() -> synchronize_net()

The problem can't be rcu_init(), that gets done very early
in init/main.c

Maybe it's some timer or something else specific to s390?

It could also be that there's perhaps nothing to context
switch to, thus the RCU takes forever to "happen".

Re: [patch] ipv4: initialize arp_tbl rw lock

[Heiko Carstens]

> > callchain: inet_init() -> inet_register_protosw() -> synchronize_net()
> 
> The problem can't be rcu_init(), that gets done very early
> in init/main.c
> 
> Maybe it's some timer or something else specific to s390?
> 
> It could also be that there's perhaps nothing to context
> switch to, thus the RCU takes forever to "happen".

Changing inet_init to fs_initcall() moves it way up the chain...
There are quite a few __initcall()s (way is this mapped to
device_initcall()?) and module_init()s in places where I would
never have expected them (e.g. kernel/).
After all the dependencies are anything but obvious to me. The
only obvious solution which fixes my problem would be to convert
qeth's module_init() to late_initcall().

Re: [patch] ipv4: initialize arp_tbl rw lock

[Heiko Carstens]

> > Tried to figure out what is causing the delays I experienced when I replaced
> > module_init() in af_inet.c with fs_initcall(). After all it turned out that
> > synchronize_net() which is basicically nothing else than synchronize_rcu()
> > sometimes takes several seconds to complete?! No idea why that is...
> > 
> > callchain: inet_init() -> inet_register_protosw() -> synchronize_net()
> 
> The problem can't be rcu_init(), that gets done very early
> in init/main.c
> 
> Maybe it's some timer or something else specific to s390?
> 
> It could also be that there's perhaps nothing to context
> switch to, thus the RCU takes forever to "happen".

Yes, it's more or less s390 specific.

What happens is the following: synchronize_rcu() enqueues an RCU callback on
cpu 0. Later on cpu 0 handles a bunch of RCU batches, but without handling
this specific request (it's in rdp->curlist). Since this cpu has nothing else
to do it enters cpu_idle() (it's a nohz idle, therefore it might be quite a
long time in idle state).
While cpu 0 is in idle state cpu 2 calls cpu_quiet() which in turn will call
rcu_start_batch(). If cpu 0 would run now, it would notice rdp->curlist moved
to rdp->donelist and that there is something to do. Unfortunately it doesn't
get notified from rcu_start_batch(). That's why I ended up waiting several
seconds until finally some interrupt arrived at cpu 0 which made things go
on finally.

Avoiding this could be done if we look at rdp->curlist before going into
a nohz idle wait, or we could send an interprocessor interrupt to idle
cpus. Sending an interrupt while looking only on nohz_cpu_mask seems to
be a bit racy, since other cpus might have entered cpu idle after
nohz_cpu_mask has been read...

At least the initcall change for inet_init() can go in, since it just
revealed a problem that we have anyway.

[patch] ipv4: inet_init() -> fs_initcall

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Convert inet_init to an fs_initcall to make sure its called before any device
driver's initcall.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 net/ipv4/af_inet.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index dc206f1..0a27745 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1257,7 +1257,7 @@ out_unregister_udp_proto:
 	goto out;
 }
 
-module_init(inet_init);
+fs_initcall(inet_init);
 
 /* ------------------------------------------------------------------------ */
 

Re: [patch] ipv4: initialize arp_tbl rw lock

[David S. Miller]

From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Fri, 7 Apr 2006 10:15:33 +0200

> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> The qeth driver makes use of the arp_tbl rw lock. CONFIG_DEBUG_SPINLOCK
> detects that this lock is not initialized as it is supposed to be.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

As Stephen Hemminger pointed out this change is bogus, the
lock is initialized at run time by the ARP layer.