| Message ID | 20081111111946.GD3665@x200.localdomain |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
--- 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;
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