Patch: [NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

Patch: [NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

[Oliver Hartkopp]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

Hi all,

attached you'll find a patch that fixes the depency that has been 
introduced in commit 65f7651788e18fadb2fbb7276af935d7871e1803 ([NET]: 
prot_inuse cleanups and optimizations).

As the inuse counters are only used by internet protocols right now, 
using CONFIG_INET would have been more obvious to recognize this illegal 
optimization here. Going a bit deeper into this problem we can see, that 
the pcounters are ONLY used for the internet protocols BUT initialized 
for ALL protocols in proto_[un|]register() in net/core/sock.c.

This forces all network protocols to initialize the pcounters and 
therefore request dynamic percpu memory even when it is not used at all.

I would suggest to

1. move the ..._inuse_[init|free]() stuff from sock.c to af_inet[|6].c 
and his friends

OR

2. add new parameters to proto_[un|]register() like 'alloc_inuse' and 
'free_inuse'

My favourite sollution would be the second one but before creating a 
patch for one of these suggestions, i wanted to ask for your opinion or 
if there is any 'even nicer' idea from your side.

Regards,
Oliver

[-- Attachment #2: remove_config_proc_fs_depency_for_pcounter_inuse.patch --]
[-- Type: text/x-patch, Size: 644 bytes --]


[NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

The pcounters are initialized for each protocol and could also be used outside
CONFIG_INET context which forces CONFIG_PROC_FS to be set.

Signed-off-by: Oliver Hartkopp <oliver@hartkopp.net>

diff --git a/include/net/sock.h b/include/net/sock.h
index fd98760..505ac6f 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -552,9 +552,7 @@ struct proto {
 	int			(*get_port)(struct sock *sk, unsigned short snum);
 
 	/* Keeping track of sockets in use */
-#ifdef CONFIG_PROC_FS
 	struct pcounter		inuse;
-#endif
 
 	/* Memory pressure */
 	void			(*enter_memory_pressure)(void);

Re: Patch: [NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

[Eric Dumazet]

Oliver Hartkopp a écrit :
> Hi all,
> 
> attached you'll find a patch that fixes the depency that has been 
> introduced in commit 65f7651788e18fadb2fbb7276af935d7871e1803 ([NET]: 
> prot_inuse cleanups and optimizations).
> 
> As the inuse counters are only used by internet protocols right now, 
> using CONFIG_INET would have been more obvious to recognize this illegal 
> optimization here. Going a bit deeper into this problem we can see, that 
> the pcounters are ONLY used for the internet protocols BUT initialized 
> for ALL protocols in proto_[un|]register() in net/core/sock.c.
> 
> This forces all network protocols to initialize the pcounters and 
> therefore request dynamic percpu memory even when it is not used at all.
> 
> I would suggest to
> 
> 1. move the ..._inuse_[init|free]() stuff from sock.c to af_inet[|6].c 
> and his friends
> 
> OR
> 
> 2. add new parameters to proto_[un|]register() like 'alloc_inuse' and 
> 'free_inuse'
> 
> My favourite sollution would be the second one but before creating a 
> patch for one of these suggestions, i wanted to ask for your opinion or 
> if there is any 'even nicer' idea from your side.

Hello Oliver

I am just coming back from hollidays.

Last thing I did before leaving was to post a patch to correct performance hit 
of percpu_counters in mainline. ([PATCH] alloc_percpu() fails to allocate 
percpu data http://lkml.org/lkml/2008/2/21/254 )

Before accepting Andrew Morton claims about percpu_counters being superior to 
pcounter, I benched them and found they were not.

As soon as percpu_counters are not grossly inefficient, the only move will be 
to just zap pcounter, as most people dont like it.

Only one patch will be necessary, please dont try to hide pcounter by small 
patches :)

Thank you

Re: Patch: [NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

[Oliver Hartkopp]

Eric Dumazet wrote:
> Oliver Hartkopp a écrit :
>> Hi all,
>>
>> attached you'll find a patch that fixes the depency that has been 
>> introduced in commit 65f7651788e18fadb2fbb7276af935d7871e1803 ([NET]: 
>> prot_inuse cleanups and optimizations).
>>
>> As the inuse counters are only used by internet protocols right now, 
>> using CONFIG_INET would have been more obvious to recognize this 
>> illegal optimization here. Going a bit deeper into this problem we 
>> can see, that the pcounters are ONLY used for the internet protocols 
>> BUT initialized for ALL protocols in proto_[un|]register() in 
>> net/core/sock.c.
>>
>> This forces all network protocols to initialize the pcounters and 
>> therefore request dynamic percpu memory even when it is not used at all.
>>
>> I would suggest to
>>
>> 1. move the ..._inuse_[init|free]() stuff from sock.c to 
>> af_inet[|6].c and his friends
>>
>> OR
>>
>> 2. add new parameters to proto_[un|]register() like 'alloc_inuse' and 
>> 'free_inuse'
>>
>> My favourite sollution would be the second one but before creating a 
>> patch for one of these suggestions, i wanted to ask for your opinion 
>> or if there is any 'even nicer' idea from your side.
>
> Hello Oliver
>
> I am just coming back from hollidays.

Lucky guy ;-)

>
> Last thing I did before leaving was to post a patch to correct 
> performance hit of percpu_counters in mainline. ([PATCH] 
> alloc_percpu() fails to allocate percpu data 
> http://lkml.org/lkml/2008/2/21/254 )
>
> Before accepting Andrew Morton claims about percpu_counters being 
> superior to pcounter, I benched them and found they were not.
>
> As soon as percpu_counters are not grossly inefficient, the only move 
> will be to just zap pcounter, as most people dont like it.
>
> Only one patch will be necessary, please dont try to hide pcounter by 
> small patches :)

Hm - i followed the discussion in it's major parts but my RFC hit's the 
question whether the integration of the what-ever-per-cpu-counter 
initialisation in proto_register() and proto_unregister() is the right 
way as only the internet protocols (v4/v6) are using inuse counters 
these days.

It's not about the counter implementation but the integration/usage in 
the networking subsystem.

Or does your mentioned patch mean, that the added functions in 
proto_[un|]register() will also be reverted?

Regards,
Oliver

Re: Patch: [NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

[Eric Dumazet]

Oliver Hartkopp a écrit :
> Eric Dumazet wrote:
>> Oliver Hartkopp a écrit :
>>> Hi all,
>>>
>>> attached you'll find a patch that fixes the depency that has been 
>>> introduced in commit 65f7651788e18fadb2fbb7276af935d7871e1803 ([NET]: 
>>> prot_inuse cleanups and optimizations).
>>>
>>> As the inuse counters are only used by internet protocols right now, 
>>> using CONFIG_INET would have been more obvious to recognize this 
>>> illegal optimization here. Going a bit deeper into this problem we 
>>> can see, that the pcounters are ONLY used for the internet protocols 
>>> BUT initialized for ALL protocols in proto_[un|]register() in 
>>> net/core/sock.c.
>>>
>>> This forces all network protocols to initialize the pcounters and 
>>> therefore request dynamic percpu memory even when it is not used at all.
>>>
>>> I would suggest to
>>>
>>> 1. move the ..._inuse_[init|free]() stuff from sock.c to 
>>> af_inet[|6].c and his friends
>>>
>>> OR
>>>
>>> 2. add new parameters to proto_[un|]register() like 'alloc_inuse' and 
>>> 'free_inuse'
>>>
>>> My favourite sollution would be the second one but before creating a 
>>> patch for one of these suggestions, i wanted to ask for your opinion 
>>> or if there is any 'even nicer' idea from your side.
>>
>> Hello Oliver
>>
>> I am just coming back from hollidays.
> 
> Lucky guy ;-)
> 
>>
>> Last thing I did before leaving was to post a patch to correct 
>> performance hit of percpu_counters in mainline. ([PATCH] 
>> alloc_percpu() fails to allocate percpu data 
>> http://lkml.org/lkml/2008/2/21/254 )
>>
>> Before accepting Andrew Morton claims about percpu_counters being 
>> superior to pcounter, I benched them and found they were not.
>>
>> As soon as percpu_counters are not grossly inefficient, the only move 
>> will be to just zap pcounter, as most people dont like it.
>>
>> Only one patch will be necessary, please dont try to hide pcounter by 
>> small patches :)
> 
> Hm - i followed the discussion in it's major parts but my RFC hit's the 
> question whether the integration of the what-ever-per-cpu-counter 
> initialisation in proto_register() and proto_unregister() is the right 
> way as only the internet protocols (v4/v6) are using inuse counters 
> these days.
> 
> It's not about the counter implementation but the integration/usage in 
> the networking subsystem.
> 
> Or does your mentioned patch mean, that the added functions in 
> proto_[un|]register() will also be reverted?
> 

A patch will make inet use percpu_counter instead of pcounter.

Then a zap patch will delete lib/pcounter.c & include/linux/pcounter.h

I dont understand why you say CONFIG_PROC_FS is *forced*.

I can build a kernel with CONFIG_PROC_FS=n, with working INET.

Re: Patch: [NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

[Oliver Hartkopp]

Eric Dumazet wrote:
> Oliver Hartkopp a écrit :
>> It's not about the counter implementation but the integration/usage 
>> in the networking subsystem.
>>
>> Or does your mentioned patch mean, that the added functions in 
>> proto_[un|]register() will also be reverted?
>>
>
> A patch will make inet use percpu_counter instead of pcounter.
>
> Then a zap patch will delete lib/pcounter.c & include/linux/pcounter.h
>
> I dont understand why you say CONFIG_PROC_FS is *forced*.
> I can build a kernel with CONFIG_PROC_FS=n, with working INET.
Right. With enabled CONFIG_EMBEDDED you might have CONFIG_INET with 
CONFIG_PROC_FS=n.

But this is not the thing, i wanted to point out.

My major concern was, that "whatever-per-cpu-counters" are 
allocated/initialized in "proto_register()" for *every* network protocol 
but *only* IPv[4|6] is using these counters (when CONFIG_PROC_FS is set).

I just wanted to point out the situation for network protocols that do 
not need any inuse counters. In the current implementation the pcounters 
are allocated for every networking protocol in proto_register() which 
does not look optimized to me.

Will this change with your patch that uses percpu_counter instead of 
pcounter??

Regards,
Oliver

Re: Patch: [NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

[Eric Dumazet]

Oliver Hartkopp a écrit :
> Eric Dumazet wrote:
>> Oliver Hartkopp a écrit :
>>> It's not about the counter implementation but the integration/usage 
>>> in the networking subsystem.
>>>
>>> Or does your mentioned patch mean, that the added functions in 
>>> proto_[un|]register() will also be reverted?
>>>
>>
>> A patch will make inet use percpu_counter instead of pcounter.
>>
>> Then a zap patch will delete lib/pcounter.c & include/linux/pcounter.h
>>
>> I dont understand why you say CONFIG_PROC_FS is *forced*.
>> I can build a kernel with CONFIG_PROC_FS=n, with working INET.
> Right. With enabled CONFIG_EMBEDDED you might have CONFIG_INET with 
> CONFIG_PROC_FS=n.
> 
> But this is not the thing, i wanted to point out.
> 
> My major concern was, that "whatever-per-cpu-counters" are 
> allocated/initialized in "proto_register()" for *every* network protocol 
> but *only* IPv[4|6] is using these counters (when CONFIG_PROC_FS is set).
> 
> I just wanted to point out the situation for network protocols that do 
> not need any inuse counters. In the current implementation the pcounters 
> are allocated for every networking protocol in proto_register() which 
> does not look optimized to me.
> 
> Will this change with your patch that uses percpu_counter instead of 
> pcounter??
> 

Yes, everything will be cleaned by me.

We dont need to *optimize* something that will die very soon.

Thank you

Re: Patch: [NET]: Remove CONFIG_PROC_FS depency for pcounter inuse

[Oliver Hartkopp]

Eric Dumazet wrote:
>
> Yes, everything will be cleaned by me.
>
> We dont need to *optimize* something that will die very soon.
>
> Thank you

Thanks Eric!

Sorry for the noise then ...

Best regards,
Oliver