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

login
register
mail settings
Submitter Alexey Dobriyan
Date Nov. 11, 2008, 11:19 a.m.
Message ID <20081111111946.GD3665@x200.localdomain>
Download mbox | patch
Permalink /patch/8107/
State Rejected
Delegated to: David Miller
Headers show

Comments

Alexey Dobriyan - Nov. 11, 2008, 11:19 a.m.
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(-)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 12, 2008, 12:45 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexey Dobriyan - Nov. 12, 2008, 10:44 a.m.
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. :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Nov. 12, 2008, 10:50 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Nov. 12, 2008, 10:55 a.m.
Alexey Dobriyan a écrit :

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

const qualifier is needed for upcoming cleanup patches.

Some functions in network stack do use const qualifiers, mind you.

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 12, 2008, 11:16 a.m.
From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Wed, 12 Nov 2008 13:44:39 +0300

> On the other hand, people are interrupted with ctags jumping when suddenly
> whole new file appears, so loss of context is even more.

I can't remember ever reading a patch using ctags, nor actual code for
that matter.  It's not the best facility.

People read straight code.  And inline functions defined somewhere had
damn well better be named well enough to have their basic operation
or side effect be understood.

If you absolutely, positively, must know the implementation details,
you go look it up (using 'git grep' or plain 'grep', not ctags).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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