2007-10-26 23:07:50

by Adrian Bunk

[permalink] [raw]
Subject: [2.6 patch] 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
diff --git a/net/core/sock.c b/net/core/sock.c
index febbcbc..ee1cc4f 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -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


2007-10-26 23:21:13

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [2.6 patch] always export sysctl_{r,w}mem_max

Adrian Bunk <[email protected]> writes:

> 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]>



> <-- snip -->
>
> Signed-off-by: Adrian Bunk <[email protected]>
>
> ---
> 22ea6cd56e4fa844b0b1bbab2542f09eb6c9a5ab
> diff --git a/net/core/sock.c b/net/core/sock.c
> index febbcbc..ee1cc4f 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -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

2007-10-26 23:32:01

by Rick Jones

[permalink] [raw]
Subject: Re: [2.6 patch] always export sysctl_{r,w}mem_max

Eric W. Biederman wrote:
> Adrian Bunk <[email protected]> writes:
>
>
>>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.

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

2007-10-26 23:43:26

by David Miller

[permalink] [raw]
Subject: Re: [2.6 patch] always export sysctl_{r,w}mem_max

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

> Eric W. Biederman wrote:
> > Adrian Bunk <[email protected]> writes:
> >
> >
> >>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.
>
> 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.

2007-10-26 23:46:55

by Rick Jones

[permalink] [raw]
Subject: Re: [2.6 patch] always export sysctl_{r,w}mem_max

David Miller wrote:
> 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

2007-10-26 23:52:45

by David Miller

[permalink] [raw]
Subject: Re: [2.6 patch] always export sysctl_{r,w}mem_max

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

> David Miller wrote:
> > 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 :)

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.

2007-10-27 00:05:15

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [2.6 patch] always export sysctl_{r,w}mem_max

David Miller <[email protected]> writes:

> From: Rick Jones <[email protected]>
> Date: Fri, 26 Oct 2007 16:31:47 -0700
>
>> Eric W. Biederman wrote:
>> > Adrian Bunk <[email protected]> writes:
>> >
>> >
>> >>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.
>>
>> 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.

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




2007-11-07 07:50:55

by David Miller

[permalink] [raw]
Subject: Re: [2.6 patch] always export sysctl_{r,w}mem_max

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

> 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.