Re: [PATCH] net: introduce read_pnet() and write_pnet() functions

[Alexey Dobriyan <adobriyan@gmail.com>]

On Fri, Nov 07, 2008 at 06:49:32PM +0100, Eric Dumazet wrote:
> Alexey Dobriyan a écrit :
>> On Fri, Nov 07, 2008 at 04:44:37PM +0100, Eric Dumazet wrote:
>>> CONFIG_NET_NS is not a widespread option, we can reduce kernel size
>>> not declaring useless "struct net" pointers in several structures.
>>
>> This can be done separatedly for each offending "struct net *".
>
> Sure I can split the patch if you think its too complex. I also can leave
> #ifdef CONFIG_NET_NS all over if you like them.
>
>>
>>> This patch declares three helper to clean various "ifdef CONFIG_NET_NS"
>>> that we have in many places.
>>
>> There is an implicit assumption, that all such ifdefs are bad, while if fact
>> there are nothing wrong with them:
>
> Well... we can hide the ugly details.
>
> Especially with #ifdef in C files.

They are ugly when embedded in C if/else constructs, like this:

	const struct user_regset_view *task_user_regset_view(struct task_struct *task)
	{
	#ifdef CONFIG_IA32_EMULATION
	        if (test_tsk_thread_flag(task, TIF_IA32))
	#endif
	#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
	                return &user_x86_32_view;
	#endif
	#ifdef CONFIG_X86_64
	        return &user_x86_64_view;
	#endif
	}

>> 	#ifdef CONFIG_NET_NS
>> 		struct net *ct_net;
>> 	#endif
>>
>>> #ifdef CONFIG_NET_NS
>>>
>>> #define DECLARE_PNET(name) struct net *name;
>>
>> One more macro, instead of immediately understandable thing.
>
> DECLARE_PNET() ?
> Did you check DECLARE_RWSEM, DECLARE_WAITQUEUE, DECLARE_PER_CPU, 
> DECLARE_BITMAP ... ???

Yes, DECLARE_PNET. The rest exists because of complex initialization
sometimes dependent on debugging options which all users would have
zero chance to fill correctly. struct net is one C assignment regardles
of anything.

>>> static inline void write_pnet(struct net **pnet, struct net *net)
>>> {
>>>        *pnet = net;
>>> }
>>>
>>> static inline struct net *read_pnet(struct net * const *pnet)
>>> {
>>>        return *pnet;
>>> }
>>> #else
>>>
>>> #define DECLARE_PNET(name)
>>> #define write_pnet(pnet, net)   do { (void)(net);} while (0)
>>> #define read_pnet(pnet)         (&init_net)
>>>
>>> #endif
>>>
>>> In particular, using these helpers permits a shrink of inet_bind_bucket
>>> (16 bytes instead of 32 on 32bit arches, and 32 bytes instead of 64 on 64bits)
>>
>> Why not just fix exactly bind bucket issue.
>
> Another #ifdef ? 

Another localized ifdef in header.

>> As I posted earlier, ->dst_net can go after IPv6 dst_ops can be embedded
>> directly into struct netns_ipv6, but header dependencies aren't trivial.
>>
>> As for netns comparisons, use net_eq() to amortize the cost somewhat.
>>
>>
>
> I believe all "struct net*" parameter passing could disappear with appropriate macros.
>
> This stuff currently eat a precious register on i386/x86_64/... architectures.

BTW, it'd be nice to check with current most popupar netdev benchmark :-)