Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Wed, Nov 12, 2008 at 04:24:23AM -0800, David Miller wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Wed, 12 Nov 2008 15:24:48 +0300
>
> > +static inline void ib_net_set(struct inet_bind_bucket *ib, struct net *net)
> > +{
> > +#ifdef CONFIG_NET_NS
> > +   ib->ib_net = net;
> > +#endif
> > +}
> > +
>
> It's basically read_pnet() hidden behind another name.
> And you'll add new "aliases" for read_pnet() over and over
> again.
>
> That makes no sense to me.

It also make no sense to expose write_pnet() for one(!) user and
simultaneously hide read_pnet() under ib_net() as committed patches do.

Something is wrong with read_pnet() as nobody suggested to mass use it
or send a patch doing it.


        #ifdef CONFIG_NET_NS
                ib->ib_net = net;
        #endif

It's _obvious_ from this code that it's a C assignment or nop. It's also
obvious depending on what config option.

        write_pnet(&ib->ib_net, net);

What is & operator doing here? Is it important? '&' is syntaxic noise.

And netns assignments are exactly this: assigment or a nop.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[David Miller]

From: "Alexey Dobriyan" <adobriyan@gmail.com>
Date: Fri, 14 Nov 2008 01:53:26 +0300

> Something is wrong with read_pnet() as nobody suggested to mass use it
> or send a patch doing it.
> 
>         #ifdef CONFIG_NET_NS
>                 ib->ib_net = net;
>         #endif
> 
> It's _obvious_ from this code that it's a C assignment or nop. It's also
> obvious depending on what config option.

obvious, but ugly

>         write_pnet(&ib->ib_net, net);
> 
> What is & operator doing here? Is it important? '&' is syntaxic noise.

'&' is necessary for the assignment.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

Alexey Dobriyan a écrit :
> On Wed, Nov 12, 2008 at 04:24:23AM -0800, David Miller wrote:
>> From: Alexey Dobriyan <adobriyan@gmail.com>
>> Date: Wed, 12 Nov 2008 15:24:48 +0300
>>
>>> +static inline void ib_net_set(struct inet_bind_bucket *ib, struct net *net)
>>> +{
>>> +#ifdef CONFIG_NET_NS
>>> +   ib->ib_net = net;
>>> +#endif
>>> +}
>>> +
>> It's basically read_pnet() hidden behind another name.
>> And you'll add new "aliases" for read_pnet() over and over
>> again.
>>
>> That makes no sense to me.
> 
> It also make no sense to expose write_pnet() for one(!) user and
> simultaneously hide read_pnet() under ib_net() as committed patches do.
> 
> Something is wrong with read_pnet() as nobody suggested to mass use it
> or send a patch doing it.

I did. My plan is to zap all superflous #ifdef CONFIG_NET_NS if possible.


> 
> 
>         #ifdef CONFIG_NET_NS
>                 ib->ib_net = net;
>         #endif
> 
> It's _obvious_ from this code that it's a C assignment or nop. It's also
> obvious depending on what config option.
> 
>         write_pnet(&ib->ib_net, net);
> 
> What is & operator doing here? Is it important? '&' is syntaxic noise.
> 
> And netns assignments are exactly this: assigment or a nop.

You obviously didnt read my patches.

How do you want to implement a C function that can write to ib->ib_net ?

Yes, I prefer a function over a macro, as most kernel developpers,
for obvious reasons.

Only sane way is :

static inline void write_pnet(struct net **pnet, struct net *net)
{
	*pnet = net;
}

and call write_pnet(&ip->ib_net, net);

Strange, this is what I did.

Take the time to read the patch, please.

Thank you

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

> > Something is wrong with read_pnet() as nobody suggested to mass use it
> > or send a patch doing it.
>
> I did. My plan is to zap all superflous #ifdef CONFIG_NET_NS if possible.

That's not mass usage.

Mass usage is, say, s/dev_net/read_pnet/.

Do you want to do this too?

> How do you want to implement a C function that can write to ib->ib_net ?

I've sent ib_net_set().

> Take the time to read the patch, please.

You too.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

Alexey Dobriyan a écrit :
>>> Something is wrong with read_pnet() as nobody suggested to mass use it
>>> or send a patch doing it.
>> I did. My plan is to zap all superflous #ifdef CONFIG_NET_NS if possible.
> 
> That's not mass usage.
> 
> Mass usage is, say, s/dev_net/read_pnet/.
> 
> Do you want to do this too?

Yes, it was present on my original first patch.
I said I was going to split the big patch.
How many mails will be necessary until you get the point ?

I was waiting *you* change "read_pnet()/write_pnet()" names as you
intended, *before* submitting new patches, in order not to
duplicate work.

For instance, I dont like :

static inline
struct net *dev_net(const struct net_device *dev)
{
#ifdef CONFIG_NET_NS
        return dev->nd_net;
#else
        return &init_net;
#endif
}


I prefer :

static inline
struct net *dev_net(const struct net_device *dev)
{
	return read_pnet(&dev->nd_net);
}

This is better because :

1) No #ifdef CONFIG_NET_NS

2) The magic about &init_net is not duplicated in ten different include files, but
   centralized in the right file : include/net/net_namespace.h

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 14 Nov 2008 05:36:15 +0100

> This is better because :
> 
> 1) No #ifdef CONFIG_NET_NS
> 
> 2) The magic about &init_net is not duplicated in ten different include files, but
>    centralized in the right file : include/net/net_namespace.h

I %100 agree.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Fri, 14 Nov 2008 05:36:15 +0100
> 
>> This is better because :
>>
>> 1) No #ifdef CONFIG_NET_NS
>>
>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>    centralized in the right file : include/net/net_namespace.h
> 
> I %100 agree.

Speaking of those functions, what do you think of this one ?

static inline
void dev_net_set(struct net_device *dev, struct net *net)
{
#ifdef CONFIG_NET_NS
        release_net(dev->nd_net);
        dev->nd_net = hold_net(net);
#endif
}

I believe that its safer to hold a reference on "new" *before*
releasing reference on "old" object.

Also, release_net() and hold_net() can be defined to do
the use_count refcounting regardless of CONFIG_NET_NS
(Its a different NETNS_REFCNT_DEBUG #ifdef)

Yet another example where read_pnet() and write_pnet()
are the right answer : Its cleaner and fixes *bugs*.

static inline
void dev_net_set(struct net_device *dev, struct net *net)
{
	hold_net(net);
	release_net(read_pnet(&dev->nd_net);
        write_pnet(&dev->nd_net, net);
}

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Fri, Nov 14, 2008 at 05:54:26AM +0100, Eric Dumazet wrote:
> David Miller a écrit :
>> From: Eric Dumazet <dada1@cosmosbay.com>
>> Date: Fri, 14 Nov 2008 05:36:15 +0100
>>
>>> This is better because :
>>>
>>> 1) No #ifdef CONFIG_NET_NS
>>>
>>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>>    centralized in the right file : include/net/net_namespace.h
>>
>> I %100 agree.
>
> Speaking of those functions, what do you think of this one ?
>
> static inline
> void dev_net_set(struct net_device *dev, struct net *net)
> {
> #ifdef CONFIG_NET_NS
>        release_net(dev->nd_net);
>        dev->nd_net = hold_net(net);
> #endif
> }
>
> I believe that its safer to hold a reference on "new" *before*
> releasing reference on "old" object.
>
> Also, release_net() and hold_net() can be defined to do
> the use_count refcounting regardless of CONFIG_NET_NS
> (Its a different NETNS_REFCNT_DEBUG #ifdef)

NETNS_REFCNT_DEBUG makes sense only with NET_NS=y because init_net
is never freed.

> Yet another example where read_pnet() and write_pnet()
> are the right answer : Its cleaner and fixes *bugs*.

pnet stuff by definition can't fix bugs :-)

Ask from where new net comes?

If from current, nsproxy refcount is at least 1, so netns refcount is at least 1,
so shutdown sequence can't start.

If from userspace socket, there is task in netns -- see #1

If from netdevice on which ioctl is done, some task did ioctl -- see #1.

And so on.

But this is peanuts, because your race matters only when netns is almost freed
(in kmem_cache_free sense), so you're stashing dangling pointer somewhere else
which is a bug by itself.

> static inline
> void dev_net_set(struct net_device *dev, struct net *net)
> {
> 	hold_net(net);
> 	release_net(read_pnet(&dev->nd_net);
>        write_pnet(&dev->nd_net, net);
> }

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

Alexey Dobriyan a écrit :
> On Fri, Nov 14, 2008 at 05:54:26AM +0100, Eric Dumazet wrote:
>> David Miller a écrit :
>>> From: Eric Dumazet <dada1@cosmosbay.com>
>>> Date: Fri, 14 Nov 2008 05:36:15 +0100
>>>
>>>> This is better because :
>>>>
>>>> 1) No #ifdef CONFIG_NET_NS
>>>>
>>>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>>>    centralized in the right file : include/net/net_namespace.h
>>> I %100 agree.
>> Speaking of those functions, what do you think of this one ?
>>
>> static inline
>> void dev_net_set(struct net_device *dev, struct net *net)
>> {
>> #ifdef CONFIG_NET_NS
>>        release_net(dev->nd_net);
>>        dev->nd_net = hold_net(net);
>> #endif
>> }
>>
>> I believe that its safer to hold a reference on "new" *before*
>> releasing reference on "old" object.
>>
>> Also, release_net() and hold_net() can be defined to do
>> the use_count refcounting regardless of CONFIG_NET_NS
>> (Its a different NETNS_REFCNT_DEBUG #ifdef)
> 
> NETNS_REFCNT_DEBUG makes sense only with NET_NS=y because init_net
> is never freed.

It makes sense when you want to debug things, without full NET_NS

Check CONFIG_PROVE_LOCKING or other debugging stuff

CONFIG_SMP can be unset, still we are able to check spinlocks are
lock()/unlock() are correctly paired

> 
>> Yet another example where read_pnet() and write_pnet()
>> are the right answer : Its cleaner and fixes *bugs*.
> 
> pnet stuff by definition can't fix bugs :-)

It will, I bet it.

Cant you see difference between

#ifdef CONFIG_NET_NS
	ptr->net = expression;
#endif

and

write_pnet(&ptr->net, expression);

Answer : In the 2nd case, (expression) is evaluated.

In case its evaluation has side effects (like... refcounting), it makes a huge difference.

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Fri, Nov 14, 2008 at 07:21:13AM +0100, Eric Dumazet wrote:
> Alexey Dobriyan a écrit :
>> On Fri, Nov 14, 2008 at 05:54:26AM +0100, Eric Dumazet wrote:
>>> David Miller a écrit :
>>>> From: Eric Dumazet <dada1@cosmosbay.com>
>>>> Date: Fri, 14 Nov 2008 05:36:15 +0100
>>>>
>>>>> This is better because :
>>>>>
>>>>> 1) No #ifdef CONFIG_NET_NS
>>>>>
>>>>> 2) The magic about &init_net is not duplicated in ten different include files, but
>>>>>    centralized in the right file : include/net/net_namespace.h
>>>> I %100 agree.
>>> Speaking of those functions, what do you think of this one ?
>>>
>>> static inline
>>> void dev_net_set(struct net_device *dev, struct net *net)
>>> {
>>> #ifdef CONFIG_NET_NS
>>>        release_net(dev->nd_net);
>>>        dev->nd_net = hold_net(net);
>>> #endif
>>> }
>>>
>>> I believe that its safer to hold a reference on "new" *before*
>>> releasing reference on "old" object.
>>>
>>> Also, release_net() and hold_net() can be defined to do
>>> the use_count refcounting regardless of CONFIG_NET_NS
>>> (Its a different NETNS_REFCNT_DEBUG #ifdef)
>>
>> NETNS_REFCNT_DEBUG makes sense only with NET_NS=y because init_net
>> is never freed.
>
> It makes sense when you want to debug things, without full NET_NS

It doesn't work without NET_NS at all.

Check triggers right before netns is freed, and init_net is never freed.
You can move the check (interesting, to where?) or watch usecount live
somehow.

> Check CONFIG_PROVE_LOCKING or other debugging stuff
>
> CONFIG_SMP can be unset, still we are able to check spinlocks are
> lock()/unlock() are correctly paired
>
>>
>>> Yet another example where read_pnet() and write_pnet()
>>> are the right answer : Its cleaner and fixes *bugs*.
>>
>> pnet stuff by definition can't fix bugs :-)
>
> It will, I bet it.
>
> Cant you see difference between
>
> #ifdef CONFIG_NET_NS
> 	ptr->net = expression;
> #endif
>
> and
>
> write_pnet(&ptr->net, expression);
>
> Answer : In the 2nd case, (expression) is evaluated.
>
> In case its evaluation has side effects (like... refcounting), it makes a huge difference.

We don't do such tricky thing with netns pointers. It either comes from somewhere (netdevice)
and you use it as is or it's &init_net.

It's almost a cookie, nobody evaluates cookies. Such code should rejected.

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

[David Miller]

Eric, I'm going to let you and Alexey work out how to implement
this :-)

So for now I'll mark your patch as 'deferred' on patchwork.

[PATCH] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/net/inet_hashtables.h   |   11 +++++++++++
 net/ipv4/inet_connection_sock.c |    4 ++--
 net/ipv4/inet_hashtables.c      |    6 +++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -77,13 +77,24 @@ struct inet_ehash_bucket {
  * ports are created in O(1) time?  I thought so. ;-)	-DaveM
  */
 struct inet_bind_bucket {
+#ifdef CONFIG_NET_NS
 	struct net		*ib_net;
+#endif
 	unsigned short		port;
 	signed short		fastreuse;
 	struct hlist_node	node;
 	struct hlist_head	owners;
 };
 
+static inline struct net *ib_net(struct inet_bind_bucket *ib)
+{
+#ifdef CONFIG_NET_NS
+	return ib->ib_net;
+#else
+	return &init_net;
+#endif
+}
+
 #define inet_bind_bucket_for_each(tb, node, head) \
 	hlist_for_each_entry(tb, node, head, node)
 
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -109,7 +109,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 					hashinfo->bhash_size)];
 			spin_lock(&head->lock);
 			inet_bind_bucket_for_each(tb, node, &head->chain)
-				if (tb->ib_net == net && tb->port == rover)
+				if (ib_net(tb) == net && tb->port == rover)
 					goto next;
 			break;
 		next:
@@ -137,7 +137,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 				hashinfo->bhash_size)];
 		spin_lock(&head->lock);
 		inet_bind_bucket_for_each(tb, node, &head->chain)
-			if (tb->ib_net == net && tb->port == snum)
+			if (ib_net(tb) == net && tb->port == snum)
 				goto tb_found;
 	}
 	tb = NULL;
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -35,7 +35,9 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
 	struct inet_bind_bucket *tb = kmem_cache_alloc(cachep, GFP_ATOMIC);
 
 	if (tb != NULL) {
+#ifdef CONFIG_NET_NS
 		tb->ib_net       = hold_net(net);
+#endif
 		tb->port      = snum;
 		tb->fastreuse = 0;
 		INIT_HLIST_HEAD(&tb->owners);
@@ -51,7 +53,9 @@ void inet_bind_bucket_destroy(struct kmem_cache *cachep, struct inet_bind_bucket
 {
 	if (hlist_empty(&tb->owners)) {
 		__hlist_del(&tb->node);
+#ifdef CONFIG_NET_NS
 		release_net(tb->ib_net);
+#endif
 		kmem_cache_free(cachep, tb);
 	}
 }
@@ -449,7 +453,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
 			 * unique enough.
 			 */
 			inet_bind_bucket_for_each(tb, node, &head->chain) {
-				if (tb->ib_net == net && tb->port == port) {
+				if (ib_net(tb) == net && tb->port == port) {
 					WARN_ON(hlist_empty(&tb->owners));
 					if (tb->fastreuse >= 0)
 						goto next_port;

[PATCH v2] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/net/inet_hashtables.h   |   11 +++++++++++
 net/ipv4/inet_connection_sock.c |    4 ++--
 net/ipv4/inet_hashtables.c      |    6 ++++--
 3 files changed, 17 insertions(+), 4 deletions(-)

--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -77,13 +77,24 @@ struct inet_ehash_bucket {
  * ports are created in O(1) time?  I thought so. ;-)	-DaveM
  */
 struct inet_bind_bucket {
+#ifdef CONFIG_NET_NS
 	struct net		*ib_net;
+#endif
 	unsigned short		port;
 	signed short		fastreuse;
 	struct hlist_node	node;
 	struct hlist_head	owners;
 };
 
+static inline struct net *ib_net(struct inet_bind_bucket *ib)
+{
+#ifdef CONFIG_NET_NS
+	return ib->ib_net;
+#else
+	return &init_net;
+#endif
+}
+
 #define inet_bind_bucket_for_each(tb, node, head) \
 	hlist_for_each_entry(tb, node, head, node)
 
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -109,7 +109,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 					hashinfo->bhash_size)];
 			spin_lock(&head->lock);
 			inet_bind_bucket_for_each(tb, node, &head->chain)
-				if (tb->ib_net == net && tb->port == rover)
+				if (ib_net(tb) == net && tb->port == rover)
 					goto next;
 			break;
 		next:
@@ -137,7 +137,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 				hashinfo->bhash_size)];
 		spin_lock(&head->lock);
 		inet_bind_bucket_for_each(tb, node, &head->chain)
-			if (tb->ib_net == net && tb->port == snum)
+			if (ib_net(tb) == net && tb->port == snum)
 				goto tb_found;
 	}
 	tb = NULL;
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -35,7 +35,9 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
 	struct inet_bind_bucket *tb = kmem_cache_alloc(cachep, GFP_ATOMIC);
 
 	if (tb != NULL) {
+#ifdef CONFIG_NET_NS
 		tb->ib_net       = hold_net(net);
+#endif
 		tb->port      = snum;
 		tb->fastreuse = 0;
 		INIT_HLIST_HEAD(&tb->owners);
@@ -51,7 +53,7 @@ void inet_bind_bucket_destroy(struct kmem_cache *cachep, struct inet_bind_bucket
 {
 	if (hlist_empty(&tb->owners)) {
 		__hlist_del(&tb->node);
-		release_net(tb->ib_net);
+		release_net(ib_net(tb));
 		kmem_cache_free(cachep, tb);
 	}
 }
@@ -449,7 +451,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
 			 * unique enough.
 			 */
 			inet_bind_bucket_for_each(tb, node, &head->chain) {
-				if (tb->ib_net == net && tb->port == port) {
+				if (ib_net(tb) == net && tb->port == port) {
 					WARN_ON(hlist_empty(&tb->owners));
 					if (tb->fastreuse >= 0)
 						goto next_port;

Re: [PATCH v2] net: #ifdef inet_bind_bucket::ib_net

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Tue, 11 Nov 2008 14:19:46 +0300

> @@ -35,7 +35,9 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
>  	struct inet_bind_bucket *tb = kmem_cache_alloc(cachep, GFP_ATOMIC);
>  
>  	if (tb != NULL) {
> +#ifdef CONFIG_NET_NS
>  		tb->ib_net       = hold_net(net);
> +#endif
>  		tb->port      = snum;
>  		tb->fastreuse = 0;
>  		INIT_HLIST_HEAD(&tb->owners);

No, this is exactly what we don't want.

If you have to add ifdefs to core C files, you're doing something
wrong.

All the details of ifdef this or ifdef that should be hidden in the
header files.

You cited an example where there are a ton of ifdefs in some header
fule inline function, but that is EXACTLY how this stuff should be
done.  Those header files are where such ugly implementation details
belong.

When people read actual code, they should be concerning themselves
with control flow, what the code is trying to do, etc. rather then
being continually interrupted with ifdef this and ifdef that.

Re: [PATCH v2] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Tue, Nov 11, 2008 at 04:45:54PM -0800, David Miller wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> Date: Tue, 11 Nov 2008 14:19:46 +0300
> 
> > @@ -35,7 +35,9 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
> >  	struct inet_bind_bucket *tb = kmem_cache_alloc(cachep, GFP_ATOMIC);
> >  
> >  	if (tb != NULL) {
> > +#ifdef CONFIG_NET_NS
> >  		tb->ib_net       = hold_net(net);
> > +#endif
> >  		tb->port      = snum;
> >  		tb->fastreuse = 0;
> >  		INIT_HLIST_HEAD(&tb->owners);
> 
> No, this is exactly what we don't want.
> 
> If you have to add ifdefs to core C files, you're doing something
> wrong.

It depends.

> All the details of ifdef this or ifdef that should be hidden in the
> header files.

It depends.

> You cited an example where there are a ton of ifdefs in some header
> fule inline function, but that is EXACTLY how this stuff should be
> done.  Those header files are where such ugly implementation details
> belong.
> 
> When people read actual code, they should be concerning themselves
> with control flow, what the code is trying to do, etc. rather then
> being continually interrupted with ifdef this and ifdef that.

On the other hand, people are interrupted with ctags jumping when suddenly
whole new file appears, so loss of context is even more. Distance between
static inline pair tend to increase as people add more stuff in between.

And how this one line ifdef (in one place -- allocation) can interrupt
control flow when you see it's start and immediately see it's end? Is it
because ifdef starts at column one, and code on average is two-three tabs
indented, so eye jumps to column one?

Whey you are suspicious of the code (say, look for a bug) these wrappers
are nuisaince, because some very smart one may do completely unexpected
thing wrt it's innocent name. And you check them all because you're
suspicious.

Netdevices use dev_net_set(). Add ib_net_set() and forget about this hungarian
pnet thing. const qualifier is also pointless, there maybe nothing wrong with
it, be there is nothing right as well.

Well, yes, Linus starts blogging and hungarian notation in core networking. :-)

Re: [PATCH v2] net: #ifdef inet_bind_bucket::ib_net

[Eric Dumazet]

Alexey Dobriyan a écrit :
> On Tue, Nov 11, 2008 at 04:45:54PM -0800, David Miller wrote:
>> From: Alexey Dobriyan <adobriyan@gmail.com>
>> Date: Tue, 11 Nov 2008 14:19:46 +0300
>>
>>> @@ -35,7 +35,9 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
>>>  	struct inet_bind_bucket *tb = kmem_cache_alloc(cachep, GFP_ATOMIC);
>>>  
>>>  	if (tb != NULL) {
>>> +#ifdef CONFIG_NET_NS
>>>  		tb->ib_net       = hold_net(net);
>>> +#endif
>>>  		tb->port      = snum;
>>>  		tb->fastreuse = 0;
>>>  		INIT_HLIST_HEAD(&tb->owners);
>> No, this is exactly what we don't want.
>>
>> If you have to add ifdefs to core C files, you're doing something
>> wrong.
> 
> It depends.
> 
>> All the details of ifdef this or ifdef that should be hidden in the
>> header files.
> 
> It depends.
> 
>> You cited an example where there are a ton of ifdefs in some header
>> fule inline function, but that is EXACTLY how this stuff should be
>> done.  Those header files are where such ugly implementation details
>> belong.
>>
>> When people read actual code, they should be concerning themselves
>> with control flow, what the code is trying to do, etc. rather then
>> being continually interrupted with ifdef this and ifdef that.
> 
> On the other hand, people are interrupted with ctags jumping when suddenly
> whole new file appears, so loss of context is even more. Distance between
> static inline pair tend to increase as people add more stuff in between.
> 
> And how this one line ifdef (in one place -- allocation) can interrupt
> control flow when you see it's start and immediately see it's end? Is it
> because ifdef starts at column one, and code on average is two-three tabs
> indented, so eye jumps to column one?
> 
> Whey you are suspicious of the code (say, look for a bug) these wrappers
> are nuisaince, because some very smart one may do completely unexpected
> thing wrt it's innocent name. And you check them all because you're
> suspicious.
> 
> Netdevices use dev_net_set(). Add ib_net_set() and forget about this hungarian
> pnet thing. const qualifier is also pointless, there maybe nothing wrong with
> it, be there is nothing right as well.
> 
> Well, yes, Linus starts blogging and hungarian notation in core networking. :-)

Take a look at include/net/net_namespace.h 
I made my best to choose a notation and found :

copy_net_ns(), __put_net(), net_alive(), get_net(), maybe_get_net(), 
put_net(), net_eq(), hold_net(), release_net()

What a mess.

I then decided to name my two helpers read_pnet() and write_pnet(), not
because I love hungarion notaion, but to keep existing practice in this
file.

I have no problem to clean the whole file and have a consistent notation.

I have no problem you take care of this.

Thank you

[PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[Alexey Dobriyan]

On Wed, Nov 12, 2008 at 11:50:29AM +0100, Eric Dumazet wrote:
> I have no problem you take care of this.

OK, here is somethiing that hopefully satisfies everyone.
It depends on pnet stuff being dropped.



[PATCH v3] net: #ifdef inet_bind_bucket::ib_net

Save one pointer in every inet_bind_bucket in NET_NS=n case.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 include/net/inet_hashtables.h   |   18 ++++++++++++++++++
 net/ipv4/inet_connection_sock.c |    4 ++--
 net/ipv4/inet_hashtables.c      |    6 +++---
 3 files changed, 23 insertions(+), 5 deletions(-)

--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -77,13 +77,31 @@ struct inet_ehash_bucket {
  * ports are created in O(1) time?  I thought so. ;-)	-DaveM
  */
 struct inet_bind_bucket {
+#ifdef CONFIG_NET_NS
 	struct net		*ib_net;
+#endif
 	unsigned short		port;
 	signed short		fastreuse;
 	struct hlist_node	node;
 	struct hlist_head	owners;
 };
 
+static inline struct net *ib_net(struct inet_bind_bucket *ib)
+{
+#ifdef CONFIG_NET_NS
+	return ib->ib_net;
+#else
+	return &init_net;
+#endif
+}
+
+static inline void ib_net_set(struct inet_bind_bucket *ib, struct net *net)
+{
+#ifdef CONFIG_NET_NS
+	ib->ib_net = net;
+#endif
+}
+
 #define inet_bind_bucket_for_each(tb, node, head) \
 	hlist_for_each_entry(tb, node, head, node)
 
--- a/net/ipv4/inet_connection_sock.c
+++ b/net/ipv4/inet_connection_sock.c
@@ -109,7 +109,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 					hashinfo->bhash_size)];
 			spin_lock(&head->lock);
 			inet_bind_bucket_for_each(tb, node, &head->chain)
-				if (tb->ib_net == net && tb->port == rover)
+				if (ib_net(tb) == net && tb->port == rover)
 					goto next;
 			break;
 		next:
@@ -137,7 +137,7 @@ int inet_csk_get_port(struct sock *sk, unsigned short snum)
 				hashinfo->bhash_size)];
 		spin_lock(&head->lock);
 		inet_bind_bucket_for_each(tb, node, &head->chain)
-			if (tb->ib_net == net && tb->port == snum)
+			if (ib_net(tb) == net && tb->port == snum)
 				goto tb_found;
 	}
 	tb = NULL;
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -35,7 +35,7 @@ struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep,
 	struct inet_bind_bucket *tb = kmem_cache_alloc(cachep, GFP_ATOMIC);
 
 	if (tb != NULL) {
-		tb->ib_net       = hold_net(net);
+		ib_net_set(tb, hold_net(net));
 		tb->port      = snum;
 		tb->fastreuse = 0;
 		INIT_HLIST_HEAD(&tb->owners);
@@ -51,7 +51,7 @@ void inet_bind_bucket_destroy(struct kmem_cache *cachep, struct inet_bind_bucket
 {
 	if (hlist_empty(&tb->owners)) {
 		__hlist_del(&tb->node);
-		release_net(tb->ib_net);
+		release_net(ib_net(tb));
 		kmem_cache_free(cachep, tb);
 	}
 }
@@ -449,7 +449,7 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
 			 * unique enough.
 			 */
 			inet_bind_bucket_for_each(tb, node, &head->chain) {
-				if (tb->ib_net == net && tb->port == port) {
+				if (ib_net(tb) == net && tb->port == port) {
 					WARN_ON(hlist_empty(&tb->owners));
 					if (tb->fastreuse >= 0)
 						goto next_port;

Re: [PATCH v3] net: #ifdef inet_bind_bucket::ib_net

[David Miller]

From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Wed, 12 Nov 2008 15:24:48 +0300

> +static inline void ib_net_set(struct inet_bind_bucket *ib, struct net *net)
> +{
> +#ifdef CONFIG_NET_NS
> +	ib->ib_net = net;
> +#endif
> +}
> +

It's basically read_pnet() hidden behind another name.
And you'll add new "aliases" for read_pnet() over and over
again.

That makes no sense to me.

I like read_pnet() much better, and the only thing that
can be improved is read_pnet()'d name (along with the
name of every other similar interface already in
net_namespace.h)