Skip to contentSkip to conversation

always export sysctl_{r,w}mem_max

PATCH In review Adrian Bunk 4 participants 8 messages Oct 26, 2007 – Nov 7, 2007

Subsystems (3)
Networking Network drivers Sockets
lore
Share & tools
lore

Adrian Bunk

author
#0 lore raw

PATCH 1/1 always export sysctl_{r,w}mem_max

This patch fixes the following build error with CONFIG_SYSCTL=n:

<-- snip -->

...
ERROR: "sysctl_rmem_max" [fs/dlm/dlm.ko] undefined!
ERROR: "sysctl_wmem_max" [drivers/net/rrunner.ko] undefined!
ERROR: "sysctl_rmem_max" [drivers/net/rrunner.ko] undefined! make[2]: *** [__modpost] Error 1

<-- snip -->

Signed-off-by Adrian Bunk <[email protected]>

---
22ea6cd56e4fa844b0b1bbab2542f09eb6c9a5ab

net/core/sock.c +0 -2
index febbcbc..ee1cc4f 100644
@@ -2004,7 +2004,5 @@ EXPORT_SYMBOL(sock_wmalloc);
 EXPORT_SYMBOL(sock_i_uid);
 EXPORT_SYMBOL(sock_i_ino);
 EXPORT_SYMBOL(sysctl_optmem_max);
-#ifdef CONFIG_SYSCTL
 EXPORT_SYMBOL(sysctl_rmem_max);
 EXPORT_SYMBOL(sysctl_wmem_max);
-#endif
Acked-by on v1 1/1
#1 lore raw
Quoted 9 lines · Adrian Bunk

This patch fixes the following build error with CONFIG_SYSCTL=n:

<-- snip -->

...
ERROR: "sysctl_rmem_max" [fs/dlm/dlm.ko] undefined!
ERROR: "sysctl_wmem_max" [drivers/net/rrunner.ko] undefined!
ERROR: "sysctl_rmem_max" [drivers/net/rrunner.ko] undefined! make[2]: *** [__modpost] Error 1

I was going to ask if allowing drivers to increase rmem_max is something that we want to do. Apparently the road runner driver has been doing this since the 2.6.12-rc1 when the git repository starts so this probably isn't a latent bug.

So removing unnecessary #ifdef sounds good to me.

Acked-by "Eric W. Biederman" <[email protected]>
Quoted 18 lines

<-- snip -->

Signed-off-by Adrian Bunk <[email protected]>

---
22ea6cd56e4fa844b0b1bbab2542f09eb6c9a5ab

net/core/sock.c +0 -2
index febbcbc..ee1cc4f 100644
@@ -2004,7 +2004,5 @@ EXPORT_SYMBOL(sock_wmalloc);
 EXPORT_SYMBOL(sock_i_uid);
 EXPORT_SYMBOL(sock_i_ino);
 EXPORT_SYMBOL(sysctl_optmem_max);
-#ifdef CONFIG_SYSCTL
 EXPORT_SYMBOL(sysctl_rmem_max);
 EXPORT_SYMBOL(sysctl_wmem_max);
-#endif
#2 lore raw
Quoted 14 lines · Eric W. Biederman
Quoted 9 lines · Adrian Bunk

This patch fixes the following build error with CONFIG_SYSCTL=n:

<-- snip -->

...
ERROR: "sysctl_rmem_max" [fs/dlm/dlm.ko] undefined!
ERROR: "sysctl_wmem_max" [drivers/net/rrunner.ko] undefined!
ERROR: "sysctl_rmem_max" [drivers/net/rrunner.ko] undefined! make[2]: *** [__modpost] Error 1

Quoted 4 lines

I was going to ask if allowing drivers to increase rmem_max is something that we want to do. Apparently the road runner driver has been doing this since the 2.6.12-rc1 when the git repository starts so this probably isn't a latent bug.

Although it does rather sound like a driver writer yanking the rope from the hand's of the sysadmin and hanging him with it rather than letting the sysadmin do it himself. I've seen other drivers' README's suggesting larger mem's but not their sources doing it.

rick jones

maintainer
#3 lore raw

From: Rick Jones <[email protected]>
Date: Fri, 26 Oct 2007 16:31:47 -0700

Quoted 20 lines
Quoted 14 lines · Eric W. Biederman
Quoted 9 lines · Adrian Bunk

This patch fixes the following build error with CONFIG_SYSCTL=n:

<-- snip -->

...
ERROR: "sysctl_rmem_max" [fs/dlm/dlm.ko] undefined!
ERROR: "sysctl_wmem_max" [drivers/net/rrunner.ko] undefined!
ERROR: "sysctl_rmem_max" [drivers/net/rrunner.ko] undefined! make[2]: *** [__modpost] Error 1

Quoted 4 lines

I was going to ask if allowing drivers to increase rmem_max is something that we want to do. Apparently the road runner driver has been doing this since the 2.6.12-rc1 when the git repository starts so this probably isn't a latent bug.

Quoted 4 lines

Although it does rather sound like a driver writer yanking the rope from the hand's of the sysadmin and hanging him with it rather than letting the sysadmin do it himself. I've seen other drivers' README's suggesting larger mem's but not their sources doing it.

I really don't think what the roadrunner driver is doing is correct at all.

I also think what DLM is doing is wrong too.

If DLM really wants minimum, it can use SO_SNDBUFFORCE and SO_RCVBUFFORCE socket options and use whatever limits it likes.

But even this is questionable.

I'll put in Adrian's patch to fix the build as a first priority, but in the long term this cruft has gotta go.

#4 lore raw
Quoted 5 lines · David Miller

If DLM really wants minimum, it can use SO_SNDBUFFORCE and SO_RCVBUFFORCE socket options and use whatever limits it likes.

But even this is questionable.

Drift...

Is that something netperf should be using though? Right now it uses the regular SO_[SND|RCV]BUF calls and is at the mercy of sysctls. I wonder if it would be better to have it use their FORCE versions to make life easier on the benchmarker - such as myself - who has an unfortunate habit of forgetting to update sysctl.conf :)

rick jones

maintainer
#5 lore raw

From: Rick Jones <[email protected]>
Date: Fri, 26 Oct 2007 16:46:36 -0700

Quoted 7 lines
Quoted 5 lines · David Miller

If DLM really wants minimum, it can use SO_SNDBUFFORCE and SO_RCVBUFFORCE socket options and use whatever limits it likes.

But even this is questionable.

Quoted 7 lines

Drift...

Is that something netperf should be using though? Right now it uses the regular SO_[SND|RCV]BUF calls and is at the mercy of sysctls. I wonder if it would be better to have it use their FORCE versions to make life easier on the benchmarker - such as myself - who has an unfortunate habit of forgetting to update sysctl.conf :)

The force calls are for root only.

And I want to remind you that explicitly setting socket buffer sizes hurts performance with TCP. I know you know this but it bears restating for the benefit of others.

#6 lore raw
In reply to David Miller
Quoted 34 lines · David Miller

From: Rick Jones <[email protected]>
Date: Fri, 26 Oct 2007 16:31:47 -0700

Quoted 15 lines
Quoted 14 lines · Eric W. Biederman
Quoted 9 lines · Adrian Bunk

This patch fixes the following build error with CONFIG_SYSCTL=n:

<-- snip -->

...
ERROR: "sysctl_rmem_max" [fs/dlm/dlm.ko] undefined!
ERROR: "sysctl_wmem_max" [drivers/net/rrunner.ko] undefined!
ERROR: "sysctl_rmem_max" [drivers/net/rrunner.ko] undefined! make[2]: *** [__modpost] Error 1

Quoted 7 lines
Quoted 4 lines

I was going to ask if allowing drivers to increase rmem_max is something that we want to do. Apparently the road runner driver has been doing this since the 2.6.12-rc1 when the git repository starts so this probably isn't a latent bug.

Although it does rather sound like a driver writer yanking the rope from the hand's of the sysadmin and hanging him with it rather than letting the

sysadmin

Quoted 2 lines

do it himself. I've seen other drivers' README's suggesting larger mem's but not their sources doing it.

I really don't think what the roadrunner driver is doing is correct at all.

I also think what DLM is doing is wrong too.

Quoted 8 lines

If DLM really wants minimum, it can use SO_SNDBUFFORCE and SO_RCVBUFFORCE socket options and use whatever limits it likes.

But even this is questionable.

I'll put in Adrian's patch to fix the build as a first priority, but in the long term this cruft has gotta go.

As it stands this is a very old build bug. I believe those symbols have always been exported inside of #ifdef CONFIG_SYSCTL.

So if this is really something we want to stop doing we should be able to take a few extra moments remove the code from the two problem drivers, and remove the exports.

It didn't look like any of the other users could possibly be modular.

Eric

maintainer
#7 lore raw

From: [email protected] (Eric W. Biederman)
Date: Fri, 26 Oct 2007 18:04:22 -0600

Quoted 3 lines

So if this is really something we want to stop doing we should be able to take a few extra moments remove the code from the two problem drivers, and remove the exports.

I've killed the references in dlm and rrunner in the net-2.6 tree.