Skip to contentSkip to conversation

net: introduce read_pnet() and write_pnet() functions

PATCH In review Eric Dumazet 3 participants 20 messages Nov 7, 2008 – Nov 12, 2008

Subsystems (7)
Networking Network drivers netfilter IPv4/IPv6 +3 more subsystems
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 net: introduce read_pnet() and write_pnet() functions

CONFIG_NET_NS is not a widespread option, we can reduce kernel size not declaring useless "struct net" pointers in several structures.

This patch declares three helper to clean various "ifdef CONFIG_NET_NS" that we have in many places.

#ifdef CONFIG_NET_NS

#define DECLARE_PNET(name) struct net *name;
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)

<net/net_namespace.h> doesnt automatically includes <linux/seq_file_net.h> anymore.

Signed-off-by Eric Dumazet <[email protected]>

---

fs/proc/proc_net.c                                    |    1
include/linux/netdevice.h                             |   21 ++++------
include/linux/seq_file_net.h                          |   13 ------
include/net/dst.h                                     |    4 -
include/net/inet_hashtables.h                         |    2
include/net/inet_timewait_sock.h                      |   10 ----
include/net/ip_fib.h                                  |    2
include/net/neighbour.h                               |   21 ++--------
include/net/net_namespace.h                           |   20 +++++++++
include/net/netfilter/nf_conntrack.h                  |   10 ----
include/net/netfilter/nf_conntrack_expect.h           |    6 --
include/net/sock.h                                    |   14 +-----
net/8021q/vlanproc.c                                  |    1
net/core/neighbour.c                                  |   12 +----
net/ipv4/fib_semantics.c                              |    8 +--
net/ipv4/inet_connection_sock.c                       |    6 +-
net/ipv4/inet_hashtables.c                            |    7 +--
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c |    1
net/ipv6/addrlabel.c                                  |   18 +-------
net/ipv6/route.c                                      |    8 +--
net/netfilter/nf_conntrack_acct.c                     |    1
net/netfilter/nf_conntrack_core.c                     |    8 ---
net/netfilter/nf_conntrack_expect.c                   |    1
net/netfilter/nf_conntrack_standalone.c               |    1
net/netfilter/x_tables.c                              |    1
25 files changed, 79 insertions(+), 118 deletions(-)
fs/proc/proc_net.c +1 -0
index 7bc296f..0a91454 100644
@@ -23,6 +23,7 @@
 #include <linux/nsproxy.h>
 #include <net/net_namespace.h>
 #include <linux/seq_file.h>
+#include <linux/seq_file_net.h>
 
 #include "internal.h"
include/linux/netdevice.h +9 -12
index 12d7f44..3ea874f 100644
@@ -734,10 +734,8 @@ struct net_device
 	u16			(*select_queue)(struct net_device *dev,
 						struct sk_buff *skb);
 
-#ifdef CONFIG_NET_NS
 	/* Network namespace this network device is inside */
-	struct net		*nd_net;
-#endif
+	DECLARE_PNET(nd_net)
 
 	/* mid-layer private */
 	void			*ml_priv;

@@ -794,20 +792,19 @@ static inline void netdev_for_each_tx_queue(struct net_device *dev,
 static inline
 struct net *dev_net(const struct net_device *dev)
 {
-#ifdef CONFIG_NET_NS
-	return dev->nd_net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&dev->nd_net);
 }
 
 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
+	/*
+	 * hold this one before release old one
+	 */
+	struct net *new_net = hold_net(net);
+
+	release_net(read_pnet(&dev->nd_net));
+	write_pnet(&dev->nd_net, new_net);
 }
 
 static inline bool netdev_uses_dsa_tags(struct net_device *dev)
include/linux/seq_file_net.h +2 -11
index 32c89bb..892f86d 100644
@@ -3,13 +3,8 @@
 
 #include <linux/seq_file.h>
 
-struct net;
-extern struct net init_net;
-
 struct seq_net_private {
-#ifdef CONFIG_NET_NS
-	struct net *net;
-#endif
+	DECLARE_PNET(net)
 };
 
 int seq_open_net(struct inode *, struct file *,

@@ -20,11 +15,7 @@ int seq_release_net(struct inode *, struct file *);
 int single_release_net(struct inode *, struct file *);
 static inline struct net *seq_file_net(struct seq_file *seq)
 {
-#ifdef CONFIG_NET_NS
-	return ((struct seq_net_private *)seq->private)->net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&((struct seq_net_private *)seq->private)->net);
 }
 
 #endif
include/net/dst.h +2 -2
index f96c4ba..7ae79a6 100644
@@ -107,8 +107,8 @@ struct dst_ops
 	int			entry_size;
 
 	atomic_t		entries;
-	struct kmem_cache 		*kmem_cachep;
-	struct net              *dst_net;
+	struct kmem_cache 	*kmem_cachep;
+	DECLARE_PNET(dst_net)
 };
 
 #ifdef __KERNEL__
include/net/inet_hashtables.h +1 -1
index 5cc182f..47f64d0 100644
@@ -77,7 +77,7 @@ struct inet_ehash_bucket {
  * ports are created in O(1) time?  I thought so. ;-)	-DaveM
  */
 struct inet_bind_bucket {
-	struct net		*ib_net;
+	DECLARE_PNET(ib_net)
 	unsigned short		port;
 	signed short		fastreuse;
 	struct hlist_node	node;
include/net/inet_timewait_sock.h +2 -8
index 80e4977..5d2c232 100644
@@ -215,18 +215,12 @@ extern void inet_twsk_purge(struct net *net, struct inet_hashinfo *hashinfo,
 static inline
 struct net *twsk_net(const struct inet_timewait_sock *twsk)
 {
-#ifdef CONFIG_NET_NS
-	return twsk->tw_net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&twsk->tw_net);
 }
 
 static inline
 void twsk_net_set(struct inet_timewait_sock *twsk, struct net *net)
 {
-#ifdef CONFIG_NET_NS
-	twsk->tw_net = net;
-#endif
+	write_pnet(&twsk->tw_net, net);
 }
 #endif	/* _INET_TIMEWAIT_SOCK_ */
include/net/ip_fib.h +1 -1
index 8b12667..f872f11 100644
@@ -69,7 +69,7 @@ struct fib_nh {
 struct fib_info {
 	struct hlist_node	fib_hash;
 	struct hlist_node	fib_lhash;
-	struct net		*fib_net;
+	DECLARE_PNET(fib_net)
 	int			fib_treeref;
 	atomic_t		fib_clntref;
 	int			fib_dead;
include/net/neighbour.h +5 -16
index aa4b708..77dbad7 100644
@@ -25,6 +25,7 @@
 #include <linux/err.h>
 #include <linux/sysctl.h>
 #include <net/rtnetlink.h>
+#include <linux/seq_file_net.h>
 
 /*
  * NUD stands for "neighbor unreachability detection"

@@ -38,9 +39,7 @@ struct neighbour;
 
 struct neigh_parms
 {
-#ifdef CONFIG_NET_NS
-	struct net *net;
-#endif
+	DECLARE_PNET(net)
 	struct net_device *dev;
 	struct neigh_parms *next;
 	int	(*neigh_setup)(struct neighbour *);

@@ -135,9 +134,7 @@ struct neigh_ops
 struct pneigh_entry
 {
 	struct pneigh_entry	*next;
-#ifdef CONFIG_NET_NS
-	struct net		*net;
-#endif
+	DECLARE_PNET(net)
 	struct net_device	*dev;
 	u8			flags;
 	u8			key[0];

@@ -223,11 +220,7 @@ extern void			neigh_parms_release(struct neigh_table *tbl, struct neigh_parms *p
 static inline
 struct net			*neigh_parms_net(const struct neigh_parms *parms)
 {
-#ifdef CONFIG_NET_NS
-	return parms->net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&parms->net);
 }
 
 extern unsigned long		neigh_rand_reach_time(unsigned long base);

@@ -244,11 +237,7 @@ extern int			pneigh_delete(struct neigh_table *tbl, struct net *net, const void
 static inline
 struct net			*pneigh_net(const struct pneigh_entry *pneigh)
 {
-#ifdef CONFIG_NET_NS
-	return pneigh->net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&pneigh->net);
 }
 
 extern void neigh_app_ns(struct neighbour *n);
include/net/net_namespace.h +19 -1
index 700c53a..5aef6c1 100644
@@ -78,7 +78,6 @@ struct net {
 };
 
 
-#include <linux/seq_file_net.h>
 
 /* Init's network namespace */
 extern struct net init_net;

@@ -192,6 +191,25 @@ static inline void release_net(struct net *net)
 }
 #endif
 
+#ifdef CONFIG_NET_NS
+
+#define DECLARE_PNET(name) struct net *name;
+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
 
 #define for_each_net(VAR)				\
 	list_for_each_entry(VAR, &net_namespace_list, list)
include/net/netfilter/nf_conntrack.h +2 -8
index b76a868..373f801 100644
@@ -123,9 +123,7 @@ struct nf_conn
 
 	/* Extensions */
 	struct nf_ct_ext *ext;
-#ifdef CONFIG_NET_NS
-	struct net *ct_net;
-#endif
+	DECLARE_PNET(ct_net);
 	struct rcu_head rcu;
 };
 

@@ -153,11 +151,7 @@ extern struct net init_net;
 
 static inline struct net *nf_ct_net(const struct nf_conn *ct)
 {
-#ifdef CONFIG_NET_NS
-	return ct->ct_net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&ct->ct_net);
 }
 
 /* Alter reply tuple (maybe alter helper). */
include/net/netfilter/nf_conntrack_expect.h +1 -5
index 37a7fc1..1612b1f 100644
@@ -57,11 +57,7 @@ struct nf_conntrack_expect
 
 static inline struct net *nf_ct_exp_net(struct nf_conntrack_expect *exp)
 {
-#ifdef CONFIG_NET_NS
-	return exp->master->ct_net;	/* by definition */
-#else
-	return &init_net;
-#endif
+	return read_pnet(&exp->master->ct_net);	/* by definition */
 }
 
 struct nf_conntrack_expect_policy
include/net/sock.h +3 -11
index 08291c1..c5760e8 100644
@@ -125,9 +125,7 @@ struct sock_common {
 	atomic_t		skc_refcnt;
 	unsigned int		skc_hash;
 	struct proto		*skc_prot;
-#ifdef CONFIG_NET_NS
-	struct net	 	*skc_net;
-#endif
+	DECLARE_PNET(skc_net)
 };
 
 /**

@@ -1340,19 +1338,13 @@ static inline void sk_eat_skb(struct sock *sk, struct sk_buff *skb, int copied_e
 static inline
 struct net *sock_net(const struct sock *sk)
 {
-#ifdef CONFIG_NET_NS
-	return sk->sk_net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&sk->sk_net);
 }
 
 static inline
 void sock_net_set(struct sock *sk, struct net *net)
 {
-#ifdef CONFIG_NET_NS
-	sk->sk_net = net;
-#endif
+	write_pnet(&sk->sk_net, net);
 }
 
 /*
net/8021q/vlanproc.c +1 -0
index 0feefa4..389be65 100644
@@ -28,6 +28,7 @@
 #include <linux/if_vlan.h>
 #include <net/net_namespace.h>
 #include <net/netns/generic.h>
+#include <linux/seq_file_net.h>
 #include "vlanproc.h"
 #include "vlan.h"
net/core/neighbour.c +3 -9
index d9bbe01..7a7e5a1 100644
@@ -531,9 +531,7 @@ struct pneigh_entry * pneigh_lookup(struct neigh_table *tbl,
 	if (!n)
 		goto out;
 
-#ifdef CONFIG_NET_NS
-	n->net = hold_net(net);
-#endif
+	write_pnet(&n->net, hold_net(net));
 	memcpy(n->key, pkey, key_len);
 	n->dev = dev;
 	if (dev)

@@ -1350,9 +1348,7 @@ struct neigh_parms *neigh_parms_alloc(struct net_device *dev,
 
 		dev_hold(dev);
 		p->dev = dev;
-#ifdef CONFIG_NET_NS
-		p->net = hold_net(net);
-#endif
+		write_pnet(&p->net, hold_net(net));
 		p->sysctl_table = NULL;
 		write_lock_bh(&tbl->lock);
 		p->next		= tbl->parms.next;

@@ -1407,9 +1403,7 @@ void neigh_table_init_no_netlink(struct neigh_table *tbl)
 	unsigned long now = jiffies;
 	unsigned long phsize;
 
-#ifdef CONFIG_NET_NS
-	tbl->parms.net = &init_net;
-#endif
+	write_pnet(&tbl->parms.net, &init_net);
 	atomic_set(&tbl->parms.refcnt, 1);
 	tbl->parms.reachable_time =
 			  neigh_rand_reach_time(tbl->parms.base_reachable_time);
net/ipv4/fib_semantics.c +4 -4
index 4817dea..1854b25 100644
@@ -150,7 +150,7 @@ void free_fib_info(struct fib_info *fi)
 		nh->nh_dev = NULL;
 	} endfor_nexthops(fi);
 	fib_info_cnt--;
-	release_net(fi->fib_net);
+	release_net(read_pnet(&fi->fib_net));
 	kfree(fi);
 }
 

@@ -228,7 +228,7 @@ static struct fib_info *fib_find_info(const struct fib_info *nfi)
 	head = &fib_info_hash[hash];
 
 	hlist_for_each_entry(fi, node, head, fib_hash) {
-		if (fi->fib_net != nfi->fib_net)
+		if (!net_eq(read_pnet(&fi->fib_net),read_pnet(&nfi->fib_net)))
 			continue;
 		if (fi->fib_nhs != nfi->fib_nhs)
 			continue;

@@ -729,7 +729,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
 		goto failure;
 	fib_info_cnt++;
 
-	fi->fib_net = hold_net(net);
+	write_pnet(&fi->fib_net, hold_net(net));
 	fi->fib_protocol = cfg->fc_protocol;
 	fi->fib_flags = cfg->fc_flags;
 	fi->fib_priority = cfg->fc_priority;

@@ -1047,7 +1047,7 @@ int fib_sync_down_addr(struct net *net, __be32 local)
 		return 0;
 
 	hlist_for_each_entry(fi, node, head, fib_lhash) {
-		if (fi->fib_net != net)
+		if (!net_eq(read_pnet(&fi->fib_net),net))
 			continue;
 		if (fi->fib_prefsrc == local) {
 			fi->fib_flags |= RTNH_F_DEAD;
net/ipv4/inet_connection_sock.c +4 -2
index 36f4cbc..0c61f69 100644
@@ -109,7 +109,8 @@ 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 (net_eq(read_pnet(&tb->ib_net), net) &&
+					tb->port == rover)
 					goto next;
 			break;
 		next:

@@ -137,7 +138,8 @@ 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 (net_eq(read_pnet(&tb->ib_net), net) &&
+				tb->port == snum)
 				goto tb_found;
 	}
 	tb = NULL;
net/ipv4/inet_hashtables.c +4 -3
index 4498190..30aa9ff 100644
@@ -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);
+		write_pnet(&tb->ib_net, 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(read_pnet(&tb->ib_net));
 		kmem_cache_free(cachep, tb);
 	}
 }

@@ -449,7 +449,8 @@ 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 (net_eq(read_pnet(&tb->ib_net), net) &&
+					tb->port == port) {
 					WARN_ON(hlist_empty(&tb->owners));
 					if (tb->fastreuse >= 0)
 						goto next_port;
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c +1 -0
index 313ebf0..6ba76ce 100644
@@ -12,6 +12,7 @@
 #include <linux/seq_file.h>
 #include <linux/percpu.h>
 #include <net/net_namespace.h>
+#include <linux/seq_file_net.h>
 
 #include <linux/netfilter.h>
 #include <net/netfilter/nf_conntrack_core.h>
net/ipv6/addrlabel.c +4 -14
index 6ff73c4..f021bed 100644
@@ -29,9 +29,7 @@
  */
 struct ip6addrlbl_entry
 {
-#ifdef CONFIG_NET_NS
-	struct net *lbl_net;
-#endif
+	DECLARE_PNET(lbl_net)
 	struct in6_addr prefix;
 	int prefixlen;
 	int ifindex;

@@ -52,11 +50,7 @@ static struct ip6addrlbl_table
 static inline
 struct net *ip6addrlbl_net(const struct ip6addrlbl_entry *lbl)
 {
-#ifdef CONFIG_NET_NS
-	return lbl->lbl_net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&lbl->lbl_net);
 }
 
 /*

@@ -121,9 +115,7 @@ static const __net_initdata struct ip6addrlbl_init_table
 /* Object management */
 static inline void ip6addrlbl_free(struct ip6addrlbl_entry *p)
 {
-#ifdef CONFIG_NET_NS
-	release_net(p->lbl_net);
-#endif
+	release_net(read_pnet(&p->lbl_net));
 	kfree(p);
 }
 

@@ -233,9 +225,7 @@ static struct ip6addrlbl_entry *ip6addrlbl_alloc(struct net *net,
 	newp->addrtype = addrtype;
 	newp->label = label;
 	INIT_HLIST_NODE(&newp->list);
-#ifdef CONFIG_NET_NS
-	newp->lbl_net = hold_net(net);
-#endif
+	write_pnet(&newp->lbl_net, hold_net(net));
 	atomic_set(&newp->refcnt, 1);
 	return newp;
 }
net/ipv6/route.c +4 -4
index 4d40dc2..5820738 100644
@@ -1025,7 +1025,7 @@ static void icmp6_clean_all(int (*func)(struct rt6_info *rt, void *arg),
 static int ip6_dst_gc(struct dst_ops *ops)
 {
 	unsigned long now = jiffies;
-	struct net *net = ops->dst_net;
+	struct net *net = read_pnet(&ops->dst_net);
 	int rt_min_interval = net->ipv6.sysctl.ip6_rt_gc_min_interval;
 	int rt_max_size = net->ipv6.sysctl.ip6_rt_max_size;
 	int rt_elasticity = net->ipv6.sysctl.ip6_rt_gc_elasticity;

@@ -2616,7 +2616,7 @@ static int ip6_route_net_init(struct net *net)
 					GFP_KERNEL);
 	if (!net->ipv6.ip6_dst_ops)
 		goto out;
-	net->ipv6.ip6_dst_ops->dst_net = hold_net(net);
+	write_pnet(&net->ipv6.ip6_dst_ops->dst_net, hold_net(net));
 
 	net->ipv6.ip6_null_entry = kmemdup(&ip6_null_entry_template,
 					   sizeof(*net->ipv6.ip6_null_entry),

@@ -2673,7 +2673,7 @@ out_ip6_null_entry:
 	kfree(net->ipv6.ip6_null_entry);
 #endif
 out_ip6_dst_ops:
-	release_net(net->ipv6.ip6_dst_ops->dst_net);
+	release_net(read_pnet(&net->ipv6.ip6_dst_ops->dst_net));
 	kfree(net->ipv6.ip6_dst_ops);
 	goto out;
 }

@@ -2689,7 +2689,7 @@ static void ip6_route_net_exit(struct net *net)
 	kfree(net->ipv6.ip6_prohibit_entry);
 	kfree(net->ipv6.ip6_blk_hole_entry);
 #endif
-	release_net(net->ipv6.ip6_dst_ops->dst_net);
+	release_net(read_pnet(&net->ipv6.ip6_dst_ops->dst_net));
 	kfree(net->ipv6.ip6_dst_ops);
 }
net/netfilter/nf_conntrack_acct.c +1 -0
index 9fe8982..4d73477 100644
@@ -15,6 +15,7 @@
 #include <net/netfilter/nf_conntrack.h>
 #include <net/netfilter/nf_conntrack_extend.h>
 #include <net/netfilter/nf_conntrack_acct.h>
+#include <linux/seq_file_net.h>
 
 #ifdef CONFIG_NF_CT_ACCT
 #define NF_CT_ACCT_DEFAULT 1
net/netfilter/nf_conntrack_core.c +2 -6
index 622d7c6..b210d4c 100644
@@ -505,9 +505,7 @@ struct nf_conn *nf_conntrack_alloc(struct net *net,
 	ct->tuplehash[IP_CT_DIR_REPLY].tuple = *repl;
 	/* Don't set timer yet: wait for confirmation */
 	setup_timer(&ct->timeout, death_by_timeout, (unsigned long)ct);
-#ifdef CONFIG_NET_NS
-	ct->ct_net = net;
-#endif
+	write_pnet(&ct->ct_net, net);
 	INIT_RCU_HEAD(&ct->rcu);
 
 	return ct;

@@ -1232,9 +1230,7 @@ static int nf_conntrack_init_net(struct net *net)
 
 	/* Set up fake conntrack:
 	    - to never be deleted, not in any hashes */
-#ifdef CONFIG_NET_NS
-	nf_conntrack_untracked.ct_net = &init_net;
-#endif
+	write_pnet(&nf_conntrack_untracked.ct_net, &init_net);
 	atomic_set(&nf_conntrack_untracked.ct_general.use, 1);
 	/*  - and look it like as a confirmed connection */
 	set_bit(IPS_CONFIRMED_BIT, &nf_conntrack_untracked.status);
net/netfilter/nf_conntrack_expect.c +1 -0
index 37a703b..8cd2694 100644
@@ -422,6 +422,7 @@ out:
 EXPORT_SYMBOL_GPL(nf_ct_expect_related);
 
 #ifdef CONFIG_PROC_FS
+#include <linux/seq_file_net.h>
 struct ct_expect_iter_state {
 	struct seq_net_private p;
 	unsigned int bucket;
net/netfilter/nf_conntrack_standalone.c +1 -0
index f37b9b7..6ae6494 100644
@@ -15,6 +15,7 @@
 #include <linux/percpu.h>
 #include <linux/netdevice.h>
 #include <net/net_namespace.h>
+#include <linux/seq_file_net.h>
 #ifdef CONFIG_SYSCTL
 #include <linux/sysctl.h>
 #endif
net/netfilter/x_tables.c +1 -0
index 89837a4..a8313e1 100644
@@ -749,6 +749,7 @@ void *xt_unregister_table(struct xt_table *table)
 EXPORT_SYMBOL_GPL(xt_unregister_table);
 
 #ifdef CONFIG_PROC_FS
+#include <linux/seq_file_net.h>
 struct xt_names_priv {
 	struct seq_net_private p;
 	u_int8_t af;
#1 lore raw
Quoted 2 lines · Eric Dumazet

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

Quoted 2 lines

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:

	#ifdef CONFIG_NET_NS
		struct net *ct_net;
	#endif
Quoted 3 lines

#ifdef CONFIG_NET_NS

#define DECLARE_PNET(name) struct net *name;

One more macro, instead of immediately understandable thing.

Quoted 11 lines

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

Quoted 8 lines

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

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.

author
#2 lore raw
Quoted 5 lines · Alexey Dobriyan
Quoted 2 lines · Eric Dumazet

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.

Quoted 6 lines
Quoted 3 lines

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.

Quoted 10 lines
	#ifdef CONFIG_NET_NS
		struct net *ct_net;
	#endif
Quoted 3 lines

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

Quoted 16 lines
Quoted 16 lines

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)

Quoted 6 lines
Quoted 4 lines

#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 ?

Quoted 7 lines

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.

#3 lore raw
Quoted 14 lines · Eric Dumazet
Quoted 5 lines · Alexey Dobriyan
Quoted 2 lines · Eric Dumazet

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.

Quoted 4 lines
Quoted 3 lines

This patch declares three helper to clean various "ifdef CONFIG_NET_NS" that we have in many places.

Quoted 6 lines
Quoted 2 lines

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
	}
Quoted 6 lines
Quoted 6 lines
	#ifdef CONFIG_NET_NS
		struct net *ct_net;
	#endif
Quoted 2 lines

#ifdef CONFIG_NET_NS

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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

Quoted 15 lines
Quoted 15 lines
Quoted 15 lines

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)

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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

Quoted 10 lines
Quoted 6 lines

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 :-)

maintainer
#4 lore raw

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.

#5 lore raw

PATCH 1/1 net: #ifdef inet_bind_bucket::ib_net

Signed-off-by Alexey Dobriyan <[email protected]>

---

 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(-)
include/net/inet_hashtables.h +11 -0
@@ -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)
net/ipv4/inet_connection_sock.c +2 -2
@@ -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;
net/ipv4/inet_hashtables.c +5 -1
@@ -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;
#6 lore raw

PATCH v2 1/1 net: #ifdef inet_bind_bucket::ib_net

Signed-off-by Alexey Dobriyan <[email protected]>

---

 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(-)
include/net/inet_hashtables.h +11 -0
@@ -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)
net/ipv4/inet_connection_sock.c +2 -2
@@ -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;
net/ipv4/inet_hashtables.c +4 -2
@@ -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;
maintainer
#7 lore raw

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

From: Alexey Dobriyan <[email protected]>
Date: Tue, 11 Nov 2008 14:19:46 +0300

net/ipv4/inet_hashtables.c struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep, · line 43

Quoted 10 lines
net/ipv4/inet_hashtables.c +2 -0
@@ -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.

author
#8 lore raw

PATCH 1/1 nets: Introduce read_pnet() and write_pnet() helpers

So in this version I splited the patches to make them clear

I removed the DECLARE_PNET since it was apparently using too many capital letters :)

Thanks

[PATCH] nets: Introduce read_pnet() and write_pnet() helpers

This patch introduces two helpers that deal with reading and writing struct net pointers in various network structures.

Their implementation depends on CONFIG_NET_NS

For symmetry, both functions work with "struct net **pnet".

Their usage should reduce the number of #ifdef CONFIG_NET_NS, without adding many helpers for each network structure that hold a "struct net *pointer"

Signed-off-by Eric Dumazet <[email protected]>

---
include/net/net_namespace.h | 18 ++++++++++++++++++

1 files changed, 18 insertions(+)
include/net/net_namespace.h +18 -0
index 700c53a..3195577 100644
@@ -192,6 +192,24 @@ static inline void release_net(struct net *net)
 }
 #endif
 
+#ifdef CONFIG_NET_NS
+
+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 write_pnet(pnet, net)	do { (void)(net);} while (0)
+#define read_pnet(pnet)		(&init_net)
+
+#endif
 
 #define for_each_net(VAR)				\
 	list_for_each_entry(VAR, &net_namespace_list, list)
author
#9 lore raw

PATCH 1/1 net: Cleanup of neighbour code

Using read_pnet() and write_pnet() in neighbour code ease the reading of code.

Signed-off-by Eric Dumazet <[email protected]>

---

include/net/neighbour.h |   12 ++----------
net/core/neighbour.c    |   12 +++---------
2 files changed, 5 insertions(+), 19 deletions(-)
include/net/neighbour.h +2 -10
index 365b5e2..d8d790e 100644
@@ -220,11 +220,7 @@ extern void			neigh_parms_release(struct neigh_table *tbl, struct neigh_parms *p
 static inline
 struct net			*neigh_parms_net(const struct neigh_parms *parms)
 {
-#ifdef CONFIG_NET_NS
-	return parms->net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&parms->net);
 }
 
 extern unsigned long		neigh_rand_reach_time(unsigned long base);

@@ -241,11 +237,7 @@ extern int			pneigh_delete(struct neigh_table *tbl, struct net *net, const void
 static inline
 struct net			*pneigh_net(const struct pneigh_entry *pneigh)
 {
-#ifdef CONFIG_NET_NS
-	return pneigh->net;
-#else
-	return &init_net;
-#endif
+	return read_pnet(&pneigh->net);
 }
 
 extern void neigh_app_ns(struct neighbour *n);
net/core/neighbour.c +3 -9
index 500c243..cca6a55 100644
@@ -531,9 +531,7 @@ struct pneigh_entry * pneigh_lookup(struct neigh_table *tbl,
 	if (!n)
 		goto out;
 
-#ifdef CONFIG_NET_NS
-	n->net = hold_net(net);
-#endif
+	write_pnet(&n->net, hold_net(net));
 	memcpy(n->key, pkey, key_len);
 	n->dev = dev;
 	if (dev)

@@ -1350,9 +1348,7 @@ struct neigh_parms *neigh_parms_alloc(struct net_device *dev,
 
 		dev_hold(dev);
 		p->dev = dev;
-#ifdef CONFIG_NET_NS
-		p->net = hold_net(net);
-#endif
+		write_pnet(&p->net, hold_net(net));
 		p->sysctl_table = NULL;
 		write_lock_bh(&tbl->lock);
 		p->next		= tbl->parms.next;

@@ -1407,9 +1403,7 @@ void neigh_table_init_no_netlink(struct neigh_table *tbl)
 	unsigned long now = jiffies;
 	unsigned long phsize;
 
-#ifdef CONFIG_NET_NS
-	tbl->parms.net = &init_net;
-#endif
+	write_pnet(&tbl->parms.net, &init_net);
 	atomic_set(&tbl->parms.refcnt, 1);
 	tbl->parms.reachable_time =
 			  neigh_rand_reach_time(tbl->parms.base_reachable_time);
maintainer
#10 lore raw

Re: [PATCH] net: Cleanup of neighbour code

From: Eric Dumazet <[email protected]>
Date: Wed, 12 Nov 2008 07:31:58 +0100

Quoted 3 lines

Using read_pnet() and write_pnet() in neighbour code ease the reading of code.

Signed-off-by Eric Dumazet <[email protected]>

Happily applied.

Now that's what I'm talking about! Put that ifdef shit in the header files!

Whoot!

maintainer Applied
#11 lore raw

Re: [PATCH] nets: Introduce read_pnet() and write_pnet() helpers

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Wed, 12 Nov 2008 07:20:46 +0100

Quoted 7 lines

[PATCH] nets: Introduce read_pnet() and write_pnet() helpers

This patch introduces two helpers that deal with reading and writing struct net pointers in various network structures.

Their implementation depends on CONFIG_NET_NS

Quoted 7 lines

For symmetry, both functions work with "struct net **pnet".

Their usage should reduce the number of #ifdef CONFIG_NET_NS, without adding many helpers for each network structure that hold a "struct net *pointer"

Signed-off-by Eric Dumazet <[email protected]>

Applied.

author
#12 lore raw

PATCH 1/1 net: ib_net pointer should depends on CONFIG_NET_NS

In reply to David Miller

We can shrink size of "struct inet_bind_bucket" by 50%, using read_pnet() and write_pnet()

Signed-off-by Eric Dumazet <[email protected]>

---

include/net/inet_hashtables.h   |    7 +++++++
net/ipv4/inet_connection_sock.c |    4 ++--
net/ipv4/inet_hashtables.c      |    6 +++---
3 files changed, 12 insertions(+), 5 deletions(-)
include/net/inet_hashtables.h +7 -0
index 5cc182f..cb31fbf 100644
@@ -77,13 +77,20 @@ 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)
+{
+	return read_pnet(&ib->ib_net);
+}
+
 #define inet_bind_bucket_for_each(tb, node, head) \
 	hlist_for_each_entry(tb, node, head, node)
net/ipv4/inet_connection_sock.c +2 -2
index 36f4cbc..05af807 100644
@@ -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;
net/ipv4/inet_hashtables.c +3 -3
index 4498190..be41ebb 100644
@@ -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);
+		write_pnet(&tb->ib_net, 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;
maintainer Applied
#13 lore raw

Re: [PATCH] net: ib_net pointer should depends on CONFIG_NET_NS

From: Eric Dumazet <[email protected]>
Date: Wed, 12 Nov 2008 07:21:21 +0100

Quoted 3 lines

We can shrink size of "struct inet_bind_bucket" by 50%, using read_pnet() and write_pnet()

Signed-off-by Eric Dumazet <[email protected]>

Applied!

#14 lore raw

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

In reply to David Miller
Quoted 16 lines · David Miller

From: Alexey Dobriyan <[email protected]>
Date: Tue, 11 Nov 2008 14:19:46 +0300

net/ipv4/inet_hashtables.c struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep, · line 43

Quoted 10 lines
net/ipv4/inet_hashtables.c +2 -0
@@ -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.

Quoted 2 lines

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

It depends.

Quoted 2 lines

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

It depends.

Quoted 8 lines

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

author
#15 lore raw

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

Quoted 49 lines · Alexey Dobriyan
Quoted 15 lines · David Miller

From: Alexey Dobriyan <[email protected]>
Date: Tue, 11 Nov 2008 14:19:46 +0300

net/ipv4/inet_hashtables.c struct inet_bind_bucket *inet_bind_bucket_create(struct kmem_cache *cachep, · line 43

Quoted 10 lines
net/ipv4/inet_hashtables.c +2 -0
@@ -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.

Quoted 2 lines

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

It depends.

Quoted 2 lines

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

It depends.

Quoted 8 lines

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.

Quoted 5 lines

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

#16 lore raw

PATCH v3 1/1 net: #ifdef inet_bind_bucket::ib_net

Quoted 1 line · Eric Dumazet

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 <[email protected]>

---

 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(-)
include/net/inet_hashtables.h +18 -0
@@ -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)
net/ipv4/inet_connection_sock.c +2 -2
@@ -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;
net/ipv4/inet_hashtables.c +3 -3
@@ -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;
maintainer
#17 lore raw

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

From: Alexey Dobriyan <[email protected]>
Date: Wed, 12 Nov 2008 15:24:48 +0300

include/net/inet_hashtables.h struct inet_ehash_bucket { · line 104

Quoted 7 lines
include/net/inet_hashtables.h +7 -0
+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)

author
#18 lore raw

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

In reply to Alexey Dobriyan
Quoted 4 lines · Alexey Dobriyan

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);
}
maintainer
#19 lore raw

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

In reply to Alexey Dobriyan

From: Alexey Dobriyan <[email protected]>
Date: Wed, 12 Nov 2008 13:44:39 +0300

Quoted 2 lines

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