[PATCH net-next 0/6] Per-net and on-demand link indices (and related) v2

[PATCH net-next 0/6] Per-net and on-demand link indices (and related) v2

[Pavel Emelyanov]

Hi!

This set tries to summarize the recent discussion of making ifindices friendly
to checkpoint-restore and consists of:

1. Prepare hash function to non-unique ifindices
2. Allow for specifying the desired ifindex on net link creation
3. Make ifindex generation per-net
4. Simplify loopback device ifindex access

Changes since v1:

* Fixed a stupid mistake with a pointer bits shift
* Turned the netdev_hash_mix routine into the generic ptr_hash_mix one
* Added a comment describing why loopback index is always 1 after the patch

Thanks,
Pavel

[PATCH 1/6] hash: Introduce ptr_hash_mix routine

[Pavel Emelyanov]

This one is used to make a salt out of a pointer to be mixed to some
hash function later. Idea and implementation are proposed by Eric Dumazet.

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 include/linux/hash.h     |   10 ++++++++++
 include/net/netns/hash.h |    9 ++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/include/linux/hash.h b/include/linux/hash.h
index b80506b..1bd0ab1 100644
--- a/include/linux/hash.h
+++ b/include/linux/hash.h
@@ -14,6 +14,7 @@
  * machines where multiplications are slow.
  */
 
+#include <linux/cache.h>
 #include <asm/types.h>
 
 /* 2^31 + 2^29 - 2^25 + 2^22 - 2^19 - 2^16 + 1 */
@@ -67,4 +68,13 @@ static inline unsigned long hash_ptr(const void *ptr, unsigned int bits)
 {
 	return hash_long((unsigned long)ptr, bits);
 }
+
+static inline u32 ptr_hash_mix(const void *ptr)
+{
+#if BITS_PER_LONG == 32
+	return (u32)(unsigned long)ptr;
+#else
+	return (u32)((unsigned long)ptr >> L1_CACHE_SHIFT);
+#endif
+}
 #endif /* _LINUX_HASH_H */
diff --git a/include/net/netns/hash.h b/include/net/netns/hash.h
index c06ac58..bcdabe0 100644
--- a/include/net/netns/hash.h
+++ b/include/net/netns/hash.h
@@ -1,19 +1,14 @@
 #ifndef __NET_NS_HASH_H__
 #define __NET_NS_HASH_H__
 
-#include <asm/cache.h>
+#include <linux/hash.h>
 
 struct net;
 
 static inline unsigned int net_hash_mix(struct net *net)
 {
 #ifdef CONFIG_NET_NS
-	/*
-	 * shift this right to eliminate bits, that are
-	 * always zeroed
-	 */
-
-	return (unsigned)(((unsigned long)net) >> L1_CACHE_SHIFT);
+	return ptr_hash_mix(net);
 #else
 	return 0;
 #endif
-- 
1.7.6.5

[PATCH 2/6] net: Dont use ifindices in hash fns

[Pavel Emelyanov]

Eric noticed, that when there will be devices with equal indices, some
hash functions that use them will become less effective as they could.

Fix this in advance by taking the net_device address into calculations
instead of the device index. Since the net_device is always aligned in
memory, shift the pointer to eliminate always zero bits (like we do it
in net_hash_mix).

This is true for arp and ndisc hash fns. The netlabel, can and llc ones
are also ifindex-based, but that three are init_net-only, thus will not
be affected.

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 include/net/arp.h   |    3 ++-
 include/net/ndisc.h |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/net/arp.h b/include/net/arp.h
index 7f7df93..b9ce3a9 100644
--- a/include/net/arp.h
+++ b/include/net/arp.h
@@ -3,6 +3,7 @@
 #define _ARP_H
 
 #include <linux/if_arp.h>
+#include <linux/hash.h>
 #include <net/neighbour.h>
 
 
@@ -10,7 +11,7 @@ extern struct neigh_table arp_tbl;
 
 static inline u32 arp_hashfn(u32 key, const struct net_device *dev, u32 hash_rnd)
 {
-	u32 val = key ^ dev->ifindex;
+	u32 val = key ^ ptr_hash_mix(dev);
 
 	return val * hash_rnd;
 }
diff --git a/include/net/ndisc.h b/include/net/ndisc.h
index 96a3b5c..a9b55b2 100644
--- a/include/net/ndisc.h
+++ b/include/net/ndisc.h
@@ -49,6 +49,7 @@ enum {
 #include <linux/types.h>
 #include <linux/if_arp.h>
 #include <linux/netdevice.h>
+#include <linux/hash.h>
 
 #include <net/neighbour.h>
 
@@ -134,7 +135,7 @@ static inline u32 ndisc_hashfn(const void *pkey, const struct net_device *dev, _
 {
 	const u32 *p32 = pkey;
 
-	return (((p32[0] ^ dev->ifindex) * hash_rnd[0]) +
+	return (((p32[0] ^ ptr_hash_mix(dev)) * hash_rnd[0]) +
 		(p32[1] * hash_rnd[1]) +
 		(p32[2] * hash_rnd[2]) +
 		(p32[3] * hash_rnd[3]));
-- 
1.7.6.5

[PATCH 3/6] net: Allow to create links with given ifindex

[Pavel Emelyanov]

Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container).

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 net/core/dev.c       |    7 ++++++-
 net/core/rtnetlink.c |   12 +++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f91abf8..3ca300d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5579,7 +5579,12 @@ int register_netdevice(struct net_device *dev)
 		}
 	}
 
-	dev->ifindex = dev_new_index(net);
+	ret = -EBUSY;
+	if (!dev->ifindex)
+		dev->ifindex = dev_new_index(net);
+	else if (__dev_get_by_index(net, dev->ifindex))
+		goto err_uninit;
+
 	if (dev->iflink == -1)
 		dev->iflink = dev->ifindex;
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 2c5a0a0..1aa1456 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1812,8 +1812,6 @@ replay:
 			return -ENODEV;
 		}
 
-		if (ifm->ifi_index)
-			return -EOPNOTSUPP;
 		if (tb[IFLA_MAP] || tb[IFLA_MASTER] || tb[IFLA_PROTINFO])
 			return -EOPNOTSUPP;
 
@@ -1839,10 +1837,14 @@ replay:
 			return PTR_ERR(dest_net);
 
 		dev = rtnl_create_link(net, dest_net, ifname, ops, tb);
-
-		if (IS_ERR(dev))
+		if (IS_ERR(dev)) {
 			err = PTR_ERR(dev);
-		else if (ops->newlink)
+			goto out;
+		}
+
+		dev->ifindex = ifm->ifi_index;
+
+		if (ops->newlink)
 			err = ops->newlink(net, dev, tb, data);
 		else
 			err = register_netdevice(dev);
-- 
1.7.6.5

[PATCH 4/6] veth: Allow to create peer link with given ifindex

[Pavel Emelyanov]

The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 drivers/net/veth.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 5852361..496c026 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	if (tbp[IFLA_ADDRESS] == NULL)
 		eth_hw_addr_random(peer);
 
+	if (ifmp)
+		peer->ifindex = ifmp->ifi_index;
+
 	err = register_netdevice(peer);
 	put_net(net);
 	net = NULL;
-- 
1.7.6.5

[PATCH 5/6] net: Make ifindex generation per-net namespace

[Pavel Emelyanov]

Strictly speaking this is only _really_ required for checkpoint-restore to
make loopback device always have the same index.

This change appears to be safe wrt "ifindex should be unique per-system"
concept, as all the ifindex usage is either already made per net namespace
of is explicitly limited with init_net only.

There are two cool side effects of this. The first one -- ifindices of
devices in container are always small, regardless of how many containers
we've started (and re-started) so far. The second one is -- we can speed
up the loopback ifidex access as shown in the next patch.

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 include/net/net_namespace.h |    1 +
 net/core/dev.c              |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index ae1cd6c..c5fbebf 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -62,6 +62,7 @@ struct net {
 	struct sock 		*rtnl;			/* rtnetlink socket */
 	struct sock		*genl_sock;
 
+	int			ifindex;
 	struct list_head 	dev_base_head;
 	struct hlist_head 	*dev_name_head;
 	struct hlist_head	*dev_index_head;
diff --git a/net/core/dev.c b/net/core/dev.c
index 3ca300d..1f06df8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5221,12 +5221,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
  */
 static int dev_new_index(struct net *net)
 {
-	static int ifindex;
+	int ifindex = net->ifindex;
 	for (;;) {
 		if (++ifindex <= 0)
 			ifindex = 1;
 		if (!__dev_get_by_index(net, ifindex))
-			return ifindex;
+			return net->ifindex = ifindex;
 	}
 }
 
-- 
1.7.6.5

[PATCH 6/6] net: Loopback ifindex is constant now

[Pavel Emelyanov]

As pointed out, there are places, that access net->loopback_dev->ifindex
and after ifindex generation is made per-net this value becomes constant
equals 1. So go ahead and introduce the LOOPBACK_IFINDEX constant and use
it where appropriate.

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 drivers/net/loopback.c            |    1 +
 include/net/net_namespace.h       |    7 +++++++
 net/decnet/dn_route.c             |    6 +++---
 net/ipv4/fib_frontend.c           |    2 +-
 net/ipv4/ipmr.c                   |    2 +-
 net/ipv4/netfilter/ipt_rpfilter.c |    2 +-
 net/ipv4/route.c                  |    6 +++---
 net/ipv6/route.c                  |    2 +-
 8 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index e2a06fd..4a075ba 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -197,6 +197,7 @@ static __net_init int loopback_net_init(struct net *net)
 	if (err)
 		goto out_free_netdev;
 
+	BUG_ON(dev->ifindex != LOOPBACK_IFINDEX);
 	net->loopback_dev = dev;
 	return 0;
 
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index c5fbebf..29a2370 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -105,6 +105,13 @@ struct net {
 	struct sock		*diag_nlsk;
 };
 
+/*
+ * ifindex generation is per-net namespace, and loopback is
+ * always the 1st device in ns (see net_dev_init), thus any
+ * loopback device should get ifindex 1
+ */
+
+#define LOOPBACK_IFINDEX	1
 
 #include <linux/seq_file_net.h>
 
diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index 85a3604..c855e8d 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -961,7 +961,7 @@ static int dn_route_output_slow(struct dst_entry **pprt, const struct flowidn *o
 		.saddr = oldflp->saddr,
 		.flowidn_scope = RT_SCOPE_UNIVERSE,
 		.flowidn_mark = oldflp->flowidn_mark,
-		.flowidn_iif = init_net.loopback_dev->ifindex,
+		.flowidn_iif = LOOPBACK_IFINDEX,
 		.flowidn_oif = oldflp->flowidn_oif,
 	};
 	struct dn_route *rt = NULL;
@@ -979,7 +979,7 @@ static int dn_route_output_slow(struct dst_entry **pprt, const struct flowidn *o
 		       "dn_route_output_slow: dst=%04x src=%04x mark=%d"
 		       " iif=%d oif=%d\n", le16_to_cpu(oldflp->daddr),
 		       le16_to_cpu(oldflp->saddr),
-		       oldflp->flowidn_mark, init_net.loopback_dev->ifindex,
+		       oldflp->flowidn_mark, LOOPBACK_IFINDEX,
 		       oldflp->flowidn_oif);
 
 	/* If we have an output interface, verify its a DECnet device */
@@ -1042,7 +1042,7 @@ source_ok:
 			if (!fld.daddr)
 				goto out;
 		}
-		fld.flowidn_oif = init_net.loopback_dev->ifindex;
+		fld.flowidn_oif = LOOPBACK_IFINDEX;
 		res.type = RTN_LOCAL;
 		goto make_route;
 	}
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index c43ae3f..7f073a3 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -218,7 +218,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
 	scope = RT_SCOPE_UNIVERSE;
 	if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
 		fl4.flowi4_oif = 0;
-		fl4.flowi4_iif = net->loopback_dev->ifindex;
+		fl4.flowi4_iif = LOOPBACK_IFINDEX;
 		fl4.daddr = ip_hdr(skb)->saddr;
 		fl4.saddr = 0;
 		fl4.flowi4_tos = RT_TOS(ip_hdr(skb)->tos);
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 8eec8f4..3a57570 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1798,7 +1798,7 @@ static struct mr_table *ipmr_rt_fib_lookup(struct net *net, struct sk_buff *skb)
 		.flowi4_oif = (rt_is_output_route(rt) ?
 			       skb->dev->ifindex : 0),
 		.flowi4_iif = (rt_is_output_route(rt) ?
-			       net->loopback_dev->ifindex :
+			       LOOPBACK_IFINDEX :
 			       skb->dev->ifindex),
 		.flowi4_mark = skb->mark,
 	};
diff --git a/net/ipv4/netfilter/ipt_rpfilter.c b/net/ipv4/netfilter/ipt_rpfilter.c
index 31371be..c301300 100644
--- a/net/ipv4/netfilter/ipt_rpfilter.c
+++ b/net/ipv4/netfilter/ipt_rpfilter.c
@@ -85,7 +85,7 @@ static bool rpfilter_mt(const struct sk_buff *skb, struct xt_action_param *par)
 			return ipv4_is_local_multicast(iph->daddr) ^ invert;
 		flow.flowi4_iif = 0;
 	} else {
-		flow.flowi4_iif = dev_net(par->in)->loopback_dev->ifindex;
+		flow.flowi4_iif = LOOPBACK_IFINDEX;
 	}
 
 	flow.daddr = iph->saddr;
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 21ad369..c581373 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1619,7 +1619,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 
 	if (res.type == RTN_LOCAL) {
 		err = fib_validate_source(skb, saddr, daddr, tos,
-					  net->loopback_dev->ifindex,
+					  LOOPBACK_IFINDEX,
 					  dev, in_dev, &itag);
 		if (err < 0)
 			goto martian_source_keep_err;
@@ -1895,7 +1895,7 @@ struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *fl4)
 
 	orig_oif = fl4->flowi4_oif;
 
-	fl4->flowi4_iif = net->loopback_dev->ifindex;
+	fl4->flowi4_iif = LOOPBACK_IFINDEX;
 	fl4->flowi4_tos = tos & IPTOS_RT_MASK;
 	fl4->flowi4_scope = ((tos & RTO_ONLINK) ?
 			 RT_SCOPE_LINK : RT_SCOPE_UNIVERSE);
@@ -1984,7 +1984,7 @@ struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *fl4)
 		if (!fl4->daddr)
 			fl4->daddr = fl4->saddr = htonl(INADDR_LOOPBACK);
 		dev_out = net->loopback_dev;
-		fl4->flowi4_oif = net->loopback_dev->ifindex;
+		fl4->flowi4_oif = LOOPBACK_IFINDEX;
 		res.type = RTN_LOCAL;
 		flags |= RTCF_LOCAL;
 		goto make_route;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 8e80fd2..0ddf2d1 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -965,7 +965,7 @@ struct dst_entry * ip6_route_output(struct net *net, const struct sock *sk,
 {
 	int flags = 0;
 
-	fl6->flowi6_iif = net->loopback_dev->ifindex;
+	fl6->flowi6_iif = LOOPBACK_IFINDEX;
 
 	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl6->daddr))
 		flags |= RT6_LOOKUP_F_IFACE;
-- 
1.7.6.5

Re: [PATCH 1/6] hash: Introduce ptr_hash_mix routine

[David Miller]

From: Pavel Emelyanov <xemul@parallels.com>
Date: Mon, 06 Aug 2012 18:13:47 +0400

> @@ -67,4 +68,13 @@ static inline unsigned long hash_ptr(const void *ptr, unsigned int bits)
>  {
>  	return hash_long((unsigned long)ptr, bits);
>  }
> +
> +static inline u32 ptr_hash_mix(const void *ptr)
> +{
> +#if BITS_PER_LONG == 32
> +	return (u32)(unsigned long)ptr;
> +#else
> +	return (u32)((unsigned long)ptr >> L1_CACHE_SHIFT);
> +#endif
> +}
>  #endif /* _LINUX_HASH_H */

This doesn't make much sense to me.

If the whole 32-bits of the pointer is useful for entropy on 32-bit
why isn't the whole 64-bits useful on 64-bit?

I would, instead, expect something like:

	ptr ^ (ptr >> 32)

for the 64-bit case.

Also, that L1_CACHE_SHIFT is something callers can decide to do.

Only they know the size of their structure, the alignment used to
allocate such objects, and thus what bits are "less relevant" and
therefore profitable to elide from the bottom of the value.

Re: [PATCH 1/6] hash: Introduce ptr_hash_mix routine

[Pavel Emelyanov]

On 08/07/2012 12:44 AM, David Miller wrote:
> From: Pavel Emelyanov <xemul@parallels.com>
> Date: Mon, 06 Aug 2012 18:13:47 +0400
> 
>> @@ -67,4 +68,13 @@ static inline unsigned long hash_ptr(const void *ptr, unsigned int bits)
>>  {
>>  	return hash_long((unsigned long)ptr, bits);
>>  }
>> +
>> +static inline u32 ptr_hash_mix(const void *ptr)
>> +{
>> +#if BITS_PER_LONG == 32
>> +	return (u32)(unsigned long)ptr;
>> +#else
>> +	return (u32)((unsigned long)ptr >> L1_CACHE_SHIFT);
>> +#endif
>> +}
>>  #endif /* _LINUX_HASH_H */
> 
> This doesn't make much sense to me.
> 
> If the whole 32-bits of the pointer is useful for entropy on 32-bit
> why isn't the whole 64-bits useful on 64-bit?
> 
> I would, instead, expect something like:
> 
> 	ptr ^ (ptr >> 32)
> 
> for the 64-bit case.
> 
> Also, that L1_CACHE_SHIFT is something callers can decide to do.
> 
> Only they know the size of their structure, the alignment used to
> allocate such objects, and thus what bits are "less relevant" and
> therefore profitable to elide from the bottom of the value.
> .

Maybe it would be better to change the way neigh_table->hash work more
significantly then? Currently it is used like

	hash = tbl->hash(key, dev, tbl->rnd);
	hash >>= (32 - tbl->hash_shift);

i.e. the caller asks for u32 hash value and then trims some lower bits.
It can be changed like

	hash = tbl->hash(key, dev, tbl->rnd, tbl->hash_shift);

making the hash fn trim the bits itself. This will allow us to use the
existing (declared to be proven to be effective) hash_ptr() routine for
the net_device pointer hashing (it requires the number of bits to use).

E.g. the arp hash might look like

static u32 arp_hashfn(u32 key, struct net_device *dev, u32 hash_rnd,
		unsigned int bits)
{
	return hash_ptr(dev, bits) ^ hash_32(key * hash_rnd, bits);
}

and the ndisc one like

static u32 ndisc_hashfn(u32 *pkey, struct net_device *dev, u32 *hash_rnd,
		unsigned int bits)
{
	return hash_ptr(dev, bits) ^
		hash_32(key[0] * hash_rnd[0], bits) ^
		hash_32(key[1] * hash_rnd[1], bits) ^
		hash_32(key[2] * hash_rnd[2], bits) ^
		hash_32(key[3] * hash_rnd[3], bits);
}

What do you think?

Thanks,
Pavel

Re: [PATCH 1/6] hash: Introduce ptr_hash_mix routine

[Eric Dumazet]

On Tue, 2012-08-07 at 13:11 +0400, Pavel Emelyanov wrote:
> On 08/07/2012 12:44 AM, David Miller wrote:
> > From: Pavel Emelyanov <xemul@parallels.com>
> > Date: Mon, 06 Aug 2012 18:13:47 +0400
> > 
> >> @@ -67,4 +68,13 @@ static inline unsigned long hash_ptr(const void *ptr, unsigned int bits)
> >>  {
> >>  	return hash_long((unsigned long)ptr, bits);
> >>  }
> >> +
> >> +static inline u32 ptr_hash_mix(const void *ptr)
> >> +{
> >> +#if BITS_PER_LONG == 32
> >> +	return (u32)(unsigned long)ptr;
> >> +#else
> >> +	return (u32)((unsigned long)ptr >> L1_CACHE_SHIFT);
> >> +#endif
> >> +}
> >>  #endif /* _LINUX_HASH_H */
> > 
> > This doesn't make much sense to me.
> > 
> > If the whole 32-bits of the pointer is useful for entropy on 32-bit
> > why isn't the whole 64-bits useful on 64-bit?
> > 
> > I would, instead, expect something like:
> > 
> > 	ptr ^ (ptr >> 32)
> > 
> > for the 64-bit case.
> > 
> > Also, that L1_CACHE_SHIFT is something callers can decide to do.
> > 
> > Only they know the size of their structure, the alignment used to
> > allocate such objects, and thus what bits are "less relevant" and
> > therefore profitable to elide from the bottom of the value.
> > .
> 
> Maybe it would be better to change the way neigh_table->hash work more
> significantly then? Currently it is used like
> 
> 	hash = tbl->hash(key, dev, tbl->rnd);
> 	hash >>= (32 - tbl->hash_shift);
> 
> i.e. the caller asks for u32 hash value and then trims some lower bits.
> It can be changed like
> 
> 	hash = tbl->hash(key, dev, tbl->rnd, tbl->hash_shift);
> 
> making the hash fn trim the bits itself. This will allow us to use the
> existing (declared to be proven to be effective) hash_ptr() routine for
> the net_device pointer hashing (it requires the number of bits to use).
> 
> E.g. the arp hash might look like
> 
> static u32 arp_hashfn(u32 key, struct net_device *dev, u32 hash_rnd,
> 		unsigned int bits)
> {
> 	return hash_ptr(dev, bits) ^ hash_32(key * hash_rnd, bits);
> }
> 
> and the ndisc one like
> 
> static u32 ndisc_hashfn(u32 *pkey, struct net_device *dev, u32 *hash_rnd,
> 		unsigned int bits)
> {
> 	return hash_ptr(dev, bits) ^
> 		hash_32(key[0] * hash_rnd[0], bits) ^
> 		hash_32(key[1] * hash_rnd[1], bits) ^
> 		hash_32(key[2] * hash_rnd[2], bits) ^
> 		hash_32(key[3] * hash_rnd[3], bits);
> }
> 
> What do you think?

I think we should avoid hash_ptr() because its quite expensive

David suggested to not use the L1_CACHE_SHIFT and instead do a plain :

static inline u32 ptr_hash_mix(const void *ptr)
{
	unsigned long val = (unsigned long)ptr;

#if BITS_PER_LONG == 64
	val ^= (val >> 32);
#endif
	return (u32)val;
}

By the way we could name this hash32_ptr() instead of ptr_hash_mix()

Re: [PATCH 1/6] hash: Introduce ptr_hash_mix routine

[Pavel Emelyanov]

> I think we should avoid hash_ptr() because its quite expensive
> 
> David suggested to not use the L1_CACHE_SHIFT and instead do a plain :
> 
> static inline u32 ptr_hash_mix(const void *ptr)
> {
> 	unsigned long val = (unsigned long)ptr;
> 
> #if BITS_PER_LONG == 64
> 	val ^= (val >> 32);
> #endif
> 	return (u32)val;
> }
> 
> By the way we could name this hash32_ptr() instead of ptr_hash_mix()

OK. I was under impression, that hash_ptr was balanced from the fast/effective
perspective, but I can't argue with you in that area :) So, please, consider
the below patch instead of #1 and #2 (the rest ones remain unchanged).

Thanks,
Pavel


From: Pavel Emelyanov <xemul@parallels.com>
Subject: [PATCH 1/5] net: Dont use ifindices in hash fns

Eric noticed, that when there will be devices with equal indices, some
hash functions that use them will become less effective as they could.
Fix this in advance by mixing the net_device address into the hash value
instead of the device index.

This is true for arp and ndisc hash fns. The netlabel, can and llc ones
are also ifindex-based, but that three are init_net-only, thus will not
be affected.

Many thanks to David and Eric for the hash32_ptr implementation!

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 include/linux/hash.h |   10 ++++++++++
 include/net/arp.h    |    3 ++-
 include/net/ndisc.h  |    3 ++-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/hash.h b/include/linux/hash.h
index b80506b..24df9e7 100644
--- a/include/linux/hash.h
+++ b/include/linux/hash.h
@@ -67,4 +67,14 @@ static inline unsigned long hash_ptr(const void *ptr, unsigned int bits)
 {
 	return hash_long((unsigned long)ptr, bits);
 }
+
+static inline u32 hash32_ptr(const void *ptr)
+{
+	unsigned long val = (unsigned long)ptr;
+
+#if BITS_PER_LONG == 64
+	val ^= (val >> 32);
+#endif
+	return (u32)val;
+}
 #endif /* _LINUX_HASH_H */
diff --git a/include/net/arp.h b/include/net/arp.h
index 7f7df93..b630dae 100644
--- a/include/net/arp.h
+++ b/include/net/arp.h
@@ -3,6 +3,7 @@
 #define _ARP_H
 
 #include <linux/if_arp.h>
+#include <linux/hash.h>
 #include <net/neighbour.h>
 
 
@@ -10,7 +11,7 @@ extern struct neigh_table arp_tbl;
 
 static inline u32 arp_hashfn(u32 key, const struct net_device *dev, u32 hash_rnd)
 {
-	u32 val = key ^ dev->ifindex;
+	u32 val = key ^ hash32_ptr(dev);
 
 	return val * hash_rnd;
 }
diff --git a/include/net/ndisc.h b/include/net/ndisc.h
index 96a3b5c..980d263 100644
--- a/include/net/ndisc.h
+++ b/include/net/ndisc.h
@@ -49,6 +49,7 @@ enum {
 #include <linux/types.h>
 #include <linux/if_arp.h>
 #include <linux/netdevice.h>
+#include <linux/hash.h>
 
 #include <net/neighbour.h>
 
@@ -134,7 +135,7 @@ static inline u32 ndisc_hashfn(const void *pkey, const struct net_device *dev, _
 {
 	const u32 *p32 = pkey;
 
-	return (((p32[0] ^ dev->ifindex) * hash_rnd[0]) +
+	return (((p32[0] ^ hash32_ptr(dev)) * hash_rnd[0]) +
 		(p32[1] * hash_rnd[1]) +
 		(p32[2] * hash_rnd[2]) +
 		(p32[3] * hash_rnd[3]));
-- 
1.7.6.5

Re: [PATCH 1/6] hash: Introduce ptr_hash_mix routine

[Eric Dumazet]

On Tue, 2012-08-07 at 13:55 +0400, Pavel Emelyanov wrote:

> OK. I was under impression, that hash_ptr was balanced from the fast/effective
> perspective, but I can't argue with you in that area :) So, please, consider
> the below patch instead of #1 and #2 (the rest ones remain unchanged).
> 
> Thanks,
> Pavel
> 
> 
> From: Pavel Emelyanov <xemul@parallels.com>
> Subject: [PATCH 1/5] net: Dont use ifindices in hash fns
> 
> Eric noticed, that when there will be devices with equal indices, some
> hash functions that use them will become less effective as they could.
> Fix this in advance by mixing the net_device address into the hash value
> instead of the device index.
> 
> This is true for arp and ndisc hash fns. The netlabel, can and llc ones
> are also ifindex-based, but that three are init_net-only, thus will not
> be affected.
> 
> Many thanks to David and Eric for the hash32_ptr implementation!
> 
> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
> ---
>  include/linux/hash.h |   10 ++++++++++
>  include/net/arp.h    |    3 ++-
>  include/net/ndisc.h  |    3 ++-
>  3 files changed, 14 insertions(+), 2 deletions(-)

Signed-off-by: Eric Dumazet <edumazet@google.com>

You should resend other patches, since they are no more on
http://patchwork.ozlabs.org/project/netdev/list/

[PATCH 2/5 (resend)] net: Allow to create links with given ifindex

[Pavel Emelyanov]

Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
is not zero. I propose to allow requesting ifindices on link creation. This
is required by the checkpoint-restore to correctly restore a net namespace
(i.e. -- a container).

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 net/core/dev.c       |    7 ++++++-
 net/core/rtnetlink.c |   12 +++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f91abf8..3ca300d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5579,7 +5579,12 @@ int register_netdevice(struct net_device *dev)
 		}
 	}
 
-	dev->ifindex = dev_new_index(net);
+	ret = -EBUSY;
+	if (!dev->ifindex)
+		dev->ifindex = dev_new_index(net);
+	else if (__dev_get_by_index(net, dev->ifindex))
+		goto err_uninit;
+
 	if (dev->iflink == -1)
 		dev->iflink = dev->ifindex;
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 2c5a0a0..1aa1456 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1812,8 +1812,6 @@ replay:
 			return -ENODEV;
 		}
 
-		if (ifm->ifi_index)
-			return -EOPNOTSUPP;
 		if (tb[IFLA_MAP] || tb[IFLA_MASTER] || tb[IFLA_PROTINFO])
 			return -EOPNOTSUPP;
 
@@ -1839,10 +1837,14 @@ replay:
 			return PTR_ERR(dest_net);
 
 		dev = rtnl_create_link(net, dest_net, ifname, ops, tb);
-
-		if (IS_ERR(dev))
+		if (IS_ERR(dev)) {
 			err = PTR_ERR(dev);
-		else if (ops->newlink)
+			goto out;
+		}
+
+		dev->ifindex = ifm->ifi_index;
+
+		if (ops->newlink)
 			err = ops->newlink(net, dev, tb, data);
 		else
 			err = register_netdevice(dev);
-- 
1.7.6.5

[PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex

[Pavel Emelyanov]

The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
so take the given ifidex and register netdev with it.

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 drivers/net/veth.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 5852361..496c026 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	if (tbp[IFLA_ADDRESS] == NULL)
 		eth_hw_addr_random(peer);
 
+	if (ifmp)
+		peer->ifindex = ifmp->ifi_index;
+
 	err = register_netdevice(peer);
 	put_net(net);
 	net = NULL;
-- 
1.7.6.5

[PATCH 4/5 (resend)] net: Make ifindex generation per-net namespace

[Pavel Emelyanov]

Strictly speaking this is only _really_ required for checkpoint-restore to
make loopback device always have the same index.

This change appears to be safe wrt "ifindex should be unique per-system"
concept, as all the ifindex usage is either already made per net namespace
of is explicitly limited with init_net only.

There are two cool side effects of this. The first one -- ifindices of
devices in container are always small, regardless of how many containers
we've started (and re-started) so far. The second one is -- we can speed
up the loopback ifidex access as shown in the next patch.

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 include/net/net_namespace.h |    1 +
 net/core/dev.c              |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index ae1cd6c..c5fbebf 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -62,6 +62,7 @@ struct net {
 	struct sock 		*rtnl;			/* rtnetlink socket */
 	struct sock		*genl_sock;
 
+	int			ifindex;
 	struct list_head 	dev_base_head;
 	struct hlist_head 	*dev_name_head;
 	struct hlist_head	*dev_index_head;
diff --git a/net/core/dev.c b/net/core/dev.c
index 3ca300d..1f06df8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5221,12 +5221,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
  */
 static int dev_new_index(struct net *net)
 {
-	static int ifindex;
+	int ifindex = net->ifindex;
 	for (;;) {
 		if (++ifindex <= 0)
 			ifindex = 1;
 		if (!__dev_get_by_index(net, ifindex))
-			return ifindex;
+			return net->ifindex = ifindex;
 	}
 }
 
-- 
1.7.6.5

[PATCH 5/5 (resend)] net: Loopback ifindex is constant now

[Pavel Emelyanov]

As pointed out, there are places, that access net->loopback_dev->ifindex
and after ifindex generation is made per-net this value becomes constant
equals 1. So go ahead and introduce the LOOPBACK_IFINDEX constant and use
it where appropriate.

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 drivers/net/loopback.c            |    1 +
 include/net/net_namespace.h       |    7 +++++++
 net/decnet/dn_route.c             |    6 +++---
 net/ipv4/fib_frontend.c           |    2 +-
 net/ipv4/ipmr.c                   |    2 +-
 net/ipv4/netfilter/ipt_rpfilter.c |    2 +-
 net/ipv4/route.c                  |    6 +++---
 net/ipv6/route.c                  |    2 +-
 8 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index e2a06fd..4a075ba 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -197,6 +197,7 @@ static __net_init int loopback_net_init(struct net *net)
 	if (err)
 		goto out_free_netdev;
 
+	BUG_ON(dev->ifindex != LOOPBACK_IFINDEX);
 	net->loopback_dev = dev;
 	return 0;
 
diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index c5fbebf..29a2370 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -105,6 +105,13 @@ struct net {
 	struct sock		*diag_nlsk;
 };
 
+/*
+ * ifindex generation is per-net namespace, and loopback is
+ * always the 1st device in ns (see net_dev_init), thus any
+ * loopback device should get ifindex 1
+ */
+
+#define LOOPBACK_IFINDEX	1
 
 #include <linux/seq_file_net.h>
 
diff --git a/net/decnet/dn_route.c b/net/decnet/dn_route.c
index 85a3604..c855e8d 100644
--- a/net/decnet/dn_route.c
+++ b/net/decnet/dn_route.c
@@ -961,7 +961,7 @@ static int dn_route_output_slow(struct dst_entry **pprt, const struct flowidn *o
 		.saddr = oldflp->saddr,
 		.flowidn_scope = RT_SCOPE_UNIVERSE,
 		.flowidn_mark = oldflp->flowidn_mark,
-		.flowidn_iif = init_net.loopback_dev->ifindex,
+		.flowidn_iif = LOOPBACK_IFINDEX,
 		.flowidn_oif = oldflp->flowidn_oif,
 	};
 	struct dn_route *rt = NULL;
@@ -979,7 +979,7 @@ static int dn_route_output_slow(struct dst_entry **pprt, const struct flowidn *o
 		       "dn_route_output_slow: dst=%04x src=%04x mark=%d"
 		       " iif=%d oif=%d\n", le16_to_cpu(oldflp->daddr),
 		       le16_to_cpu(oldflp->saddr),
-		       oldflp->flowidn_mark, init_net.loopback_dev->ifindex,
+		       oldflp->flowidn_mark, LOOPBACK_IFINDEX,
 		       oldflp->flowidn_oif);
 
 	/* If we have an output interface, verify its a DECnet device */
@@ -1042,7 +1042,7 @@ source_ok:
 			if (!fld.daddr)
 				goto out;
 		}
-		fld.flowidn_oif = init_net.loopback_dev->ifindex;
+		fld.flowidn_oif = LOOPBACK_IFINDEX;
 		res.type = RTN_LOCAL;
 		goto make_route;
 	}
diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index c43ae3f..7f073a3 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -218,7 +218,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
 	scope = RT_SCOPE_UNIVERSE;
 	if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
 		fl4.flowi4_oif = 0;
-		fl4.flowi4_iif = net->loopback_dev->ifindex;
+		fl4.flowi4_iif = LOOPBACK_IFINDEX;
 		fl4.daddr = ip_hdr(skb)->saddr;
 		fl4.saddr = 0;
 		fl4.flowi4_tos = RT_TOS(ip_hdr(skb)->tos);
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 8eec8f4..3a57570 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1798,7 +1798,7 @@ static struct mr_table *ipmr_rt_fib_lookup(struct net *net, struct sk_buff *skb)
 		.flowi4_oif = (rt_is_output_route(rt) ?
 			       skb->dev->ifindex : 0),
 		.flowi4_iif = (rt_is_output_route(rt) ?
-			       net->loopback_dev->ifindex :
+			       LOOPBACK_IFINDEX :
 			       skb->dev->ifindex),
 		.flowi4_mark = skb->mark,
 	};
diff --git a/net/ipv4/netfilter/ipt_rpfilter.c b/net/ipv4/netfilter/ipt_rpfilter.c
index 31371be..c301300 100644
--- a/net/ipv4/netfilter/ipt_rpfilter.c
+++ b/net/ipv4/netfilter/ipt_rpfilter.c
@@ -85,7 +85,7 @@ static bool rpfilter_mt(const struct sk_buff *skb, struct xt_action_param *par)
 			return ipv4_is_local_multicast(iph->daddr) ^ invert;
 		flow.flowi4_iif = 0;
 	} else {
-		flow.flowi4_iif = dev_net(par->in)->loopback_dev->ifindex;
+		flow.flowi4_iif = LOOPBACK_IFINDEX;
 	}
 
 	flow.daddr = iph->saddr;
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 21ad369..c581373 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1619,7 +1619,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 
 	if (res.type == RTN_LOCAL) {
 		err = fib_validate_source(skb, saddr, daddr, tos,
-					  net->loopback_dev->ifindex,
+					  LOOPBACK_IFINDEX,
 					  dev, in_dev, &itag);
 		if (err < 0)
 			goto martian_source_keep_err;
@@ -1895,7 +1895,7 @@ struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *fl4)
 
 	orig_oif = fl4->flowi4_oif;
 
-	fl4->flowi4_iif = net->loopback_dev->ifindex;
+	fl4->flowi4_iif = LOOPBACK_IFINDEX;
 	fl4->flowi4_tos = tos & IPTOS_RT_MASK;
 	fl4->flowi4_scope = ((tos & RTO_ONLINK) ?
 			 RT_SCOPE_LINK : RT_SCOPE_UNIVERSE);
@@ -1984,7 +1984,7 @@ struct rtable *__ip_route_output_key(struct net *net, struct flowi4 *fl4)
 		if (!fl4->daddr)
 			fl4->daddr = fl4->saddr = htonl(INADDR_LOOPBACK);
 		dev_out = net->loopback_dev;
-		fl4->flowi4_oif = net->loopback_dev->ifindex;
+		fl4->flowi4_oif = LOOPBACK_IFINDEX;
 		res.type = RTN_LOCAL;
 		flags |= RTCF_LOCAL;
 		goto make_route;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 8e80fd2..0ddf2d1 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -965,7 +965,7 @@ struct dst_entry * ip6_route_output(struct net *net, const struct sock *sk,
 {
 	int flags = 0;
 
-	fl6->flowi6_iif = net->loopback_dev->ifindex;
+	fl6->flowi6_iif = LOOPBACK_IFINDEX;
 
 	if ((sk && sk->sk_bound_dev_if) || rt6_need_strict(&fl6->daddr))
 		flags |= RT6_LOOKUP_F_IFACE;
-- 
1.7.6.5

Re: [PATCH 4/5 (resend)] net: Make ifindex generation per-net namespace

[Eric Dumazet]

On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
> Strictly speaking this is only _really_ required for checkpoint-restore to
> make loopback device always have the same index.
> 
> This change appears to be safe wrt "ifindex should be unique per-system"
> concept, as all the ifindex usage is either already made per net namespace
> of is explicitly limited with init_net only.
> 
> There are two cool side effects of this. The first one -- ifindices of
> devices in container are always small, regardless of how many containers
> we've started (and re-started) so far. The second one is -- we can speed
> up the loopback ifidex access as shown in the next patch.
> 
> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
> ---
>  include/net/net_namespace.h |    1 +
>  net/core/dev.c              |    4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
> index ae1cd6c..c5fbebf 100644
> --- a/include/net/net_namespace.h
> +++ b/include/net/net_namespace.h
> @@ -62,6 +62,7 @@ struct net {
>  	struct sock 		*rtnl;			/* rtnetlink socket */
>  	struct sock		*genl_sock;
>  
> +	int			ifindex;

could you place ifindex right after dev_base_seq : avoid two holes
and use the same cache line, dirtied in
list_netdevice()/unlist_netdevice()

>  	struct list_head 	dev_base_head;
>  	struct hlist_head 	*dev_name_head;
>  	struct hlist_head	*dev_index_head;
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3ca300d..1f06df8 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5221,12 +5221,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
>   */
>  static int dev_new_index(struct net *net)
>  {
> -	static int ifindex;
> +	int ifindex = net->ifindex;
>  	for (;;) {
>  		if (++ifindex <= 0)
>  			ifindex = 1;
>  		if (!__dev_get_by_index(net, ifindex))
> -			return ifindex;
> +			return net->ifindex = ifindex;
>  	}
>  }
>  

[PATCH 4/5 (resend)] net: Make ifindex generation per-net namespace (v2)

[Pavel Emelyanov]

>> @@ -62,6 +62,7 @@ struct net {
>>  	struct sock 		*rtnl;			/* rtnetlink socket */
>>  	struct sock		*genl_sock;
>>  
>> +	int			ifindex;
> 
> could you place ifindex right after dev_base_seq : avoid two holes
> and use the same cache line, dirtied in
> list_netdevice()/unlist_netdevice()

Sure! Here it is:

From: Pavel Emelyanov <xemul@parallels.com>
Subject: [PATCH 4/5] net: Make ifindex generation per-net namespace

Strictly speaking this is only _really_ required for checkpoint-restore to
make loopback device always have the same index.

This change appears to be safe wrt "ifindex should be unique per-system"
concept, as all the ifindex usage is either already made per net namespace
of is explicitly limited with init_net only.

There are two cool side effects of this. The first one -- ifindices of
devices in container are always small, regardless of how many containers
we've started (and re-started) so far. The second one is -- we can speed
up the loopback ifidex access as shown in the next patch.

v2: Place ifindex right after dev_base_seq : avoid two holes and use the
    same cache line, dirtied in list_netdevice()/unlist_netdevice()

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
---
 include/net/net_namespace.h |    1 +
 net/core/dev.c              |    4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index ae1cd6c..6dc3db3 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -66,6 +66,7 @@ struct net {
 	struct hlist_head 	*dev_name_head;
 	struct hlist_head	*dev_index_head;
 	unsigned int		dev_base_seq;	/* protected by rtnl_mutex */
+	int			ifindex;
 
 	/* core fib_rules */
 	struct list_head	rules_ops;
diff --git a/net/core/dev.c b/net/core/dev.c
index 3ca300d..1f06df8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5221,12 +5221,12 @@ int dev_ioctl(struct net *net, unsigned int cmd, void __user *arg)
  */
 static int dev_new_index(struct net *net)
 {
-	static int ifindex;
+	int ifindex = net->ifindex;
 	for (;;) {
 		if (++ifindex <= 0)
 			ifindex = 1;
 		if (!__dev_get_by_index(net, ifindex))
-			return ifindex;
+			return net->ifindex = ifindex;
 	}
 }
 
-- 
1.7.6.5

Re: [PATCH 4/5 (resend)] net: Make ifindex generation per-net namespace (v2)

[Eric Dumazet]

On Tue, 2012-08-07 at 16:37 +0400, Pavel Emelyanov wrote:
> >> @@ -62,6 +62,7 @@ struct net {
> >>  	struct sock 		*rtnl;			/* rtnetlink socket */
> >>  	struct sock		*genl_sock;
> >>  
> >> +	int			ifindex;
> > 
> > could you place ifindex right after dev_base_seq : avoid two holes
> > and use the same cache line, dirtied in
> > list_netdevice()/unlist_netdevice()
> 
> Sure! Here it is:
> 
> From: Pavel Emelyanov <xemul@parallels.com>
> Subject: [PATCH 4/5] net: Make ifindex generation per-net namespace
> 
> Strictly speaking this is only _really_ required for checkpoint-restore to
> make loopback device always have the same index.
> 
> This change appears to be safe wrt "ifindex should be unique per-system"
> concept, as all the ifindex usage is either already made per net namespace
> of is explicitly limited with init_net only.
> 
> There are two cool side effects of this. The first one -- ifindices of
> devices in container are always small, regardless of how many containers
> we've started (and re-started) so far. The second one is -- we can speed
> up the loopback ifidex access as shown in the next patch.
> 
> v2: Place ifindex right after dev_base_seq : avoid two holes and use the
>     same cache line, dirtied in list_netdevice()/unlist_netdevice()
> 
> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
> ---
Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH 5/5 (resend)] net: Loopback ifindex is constant now

[Eric Dumazet]

On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
> As pointed out, there are places, that access net->loopback_dev->ifindex
> and after ifindex generation is made per-net this value becomes constant
> equals 1. So go ahead and introduce the LOOPBACK_IFINDEX constant and use
> it where appropriate.
> 
> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
> ---
>  drivers/net/loopback.c            |    1 +
>  include/net/net_namespace.h       |    7 +++++++
>  net/decnet/dn_route.c             |    6 +++---
>  net/ipv4/fib_frontend.c           |    2 +-
>  net/ipv4/ipmr.c                   |    2 +-
>  net/ipv4/netfilter/ipt_rpfilter.c |    2 +-
>  net/ipv4/route.c                  |    6 +++---
>  net/ipv6/route.c                  |    2 +-
>  8 files changed, 18 insertions(+), 10 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex

[Eric Dumazet]

On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
> The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
> so take the given ifidex and register netdev with it.
> 
> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
> ---
>  drivers/net/veth.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH 2/5 (resend)] net: Allow to create links with given ifindex

[Eric Dumazet]

On Tue, 2012-08-07 at 15:01 +0400, Pavel Emelyanov wrote:
> Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
> is not zero. I propose to allow requesting ifindices on link creation. This
> is required by the checkpoint-restore to correctly restore a net namespace
> (i.e. -- a container).
> 
> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>

Acked-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex

[Ben Hutchings]

On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
> The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
> so take the given ifidex and register netdev with it.
> 
> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
> ---
>  drivers/net/veth.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> index 5852361..496c026 100644
> --- a/drivers/net/veth.c
> +++ b/drivers/net/veth.c
> @@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>  	if (tbp[IFLA_ADDRESS] == NULL)
>  		eth_hw_addr_random(peer);
>  
> +	if (ifmp)
> +		peer->ifindex = ifmp->ifi_index;
> +
>  	err = register_netdevice(peer);
>  	put_net(net);
>  	net = NULL;

Is this safe, given that this code path previously ignored
ifmp->ifi_index?  Userland could be passing in garbage and may now fail
occasionally because the value clashes with an existing interface.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/6] hash: Introduce ptr_hash_mix routine

[David Miller]

From: Pavel Emelyanov <xemul@parallels.com>
Date: Tue, 07 Aug 2012 13:11:41 +0400

> Maybe it would be better to change the way neigh_table->hash work more
> significantly then? Currently it is used like
> 
> 	hash = tbl->hash(key, dev, tbl->rnd);
> 	hash >>= (32 - tbl->hash_shift);
> 
> i.e. the caller asks for u32 hash value and then trims some lower bits.

We do this because the hash function we use in the neigh
implementations causes the top bits to have the most entropy.

Please look at the commits that made the code this way, it's
very much intentional.

Re: [PATCH 2/5 (resend)] net: Allow to create links with given ifindex

[David Miller]

Where is patch 1/5?

You have the resend the entire series as a group, every single one,
not just then ones you think you need to.  Because when you only sent
1/5 all by itself, I tossed it.

Never take shortcuts like this.

Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex

[Pavel Emelyanov]

On 08/07/2012 10:36 PM, Ben Hutchings wrote:
> On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
>> The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
>> so take the given ifidex and register netdev with it.
>>
>> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
>> ---
>>  drivers/net/veth.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
>> index 5852361..496c026 100644
>> --- a/drivers/net/veth.c
>> +++ b/drivers/net/veth.c
>> @@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>>  	if (tbp[IFLA_ADDRESS] == NULL)
>>  		eth_hw_addr_random(peer);
>>  
>> +	if (ifmp)
>> +		peer->ifindex = ifmp->ifi_index;
>> +
>>  	err = register_netdevice(peer);
>>  	put_net(net);
>>  	net = NULL;
> 
> Is this safe, given that this code path previously ignored
> ifmp->ifi_index?  Userland could be passing in garbage and may now fail
> occasionally because the value clashes with an existing interface.

You're right, I've missed that fact :( The good news is that we still can
use the ifmp->ifi_index for the peer index configuration. We just need to
assume that if the caller specified the ifindex for the veth master device,
then it's aware of this possibility and should explicitly configure (or set
to 0) the peer's ifindex as well. Like this:

	if (ifmp && (dev->ifindex != 0))
		peer->ifindex = ifmp->ifi_index;

Does this assumption work from you POV?

> Ben.
> 

Thanks,
Pavel

Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex

[Ben Hutchings]

On Wed, 2012-08-08 at 13:00 +0400, Pavel Emelyanov wrote:
> On 08/07/2012 10:36 PM, Ben Hutchings wrote:
> > On Tue, 2012-08-07 at 15:02 +0400, Pavel Emelyanov wrote:
> >> The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago),
> >> so take the given ifidex and register netdev with it.
> >>
> >> Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
> >> ---
> >>  drivers/net/veth.c |    3 +++
> >>  1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/net/veth.c b/drivers/net/veth.c
> >> index 5852361..496c026 100644
> >> --- a/drivers/net/veth.c
> >> +++ b/drivers/net/veth.c
> >> @@ -348,6 +348,9 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
> >>  	if (tbp[IFLA_ADDRESS] == NULL)
> >>  		eth_hw_addr_random(peer);
> >>  
> >> +	if (ifmp)
> >> +		peer->ifindex = ifmp->ifi_index;
> >> +
> >>  	err = register_netdevice(peer);
> >>  	put_net(net);
> >>  	net = NULL;
> > 
> > Is this safe, given that this code path previously ignored
> > ifmp->ifi_index?  Userland could be passing in garbage and may now fail
> > occasionally because the value clashes with an existing interface.
> 
> You're right, I've missed that fact :( The good news is that we still can
> use the ifmp->ifi_index for the peer index configuration. We just need to
> assume that if the caller specified the ifindex for the veth master device,
> then it's aware of this possibility and should explicitly configure (or set
> to 0) the peer's ifindex as well. Like this:
> 
> 	if (ifmp && (dev->ifindex != 0))
> 		peer->ifindex = ifmp->ifi_index;
> 
> Does this assumption work from you POV?

Yes, that looks like a neat way to do it.  Maybe with an explanatory
comment?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex

[Pavel Emelyanov]

>> You're right, I've missed that fact :( The good news is that we still can
>> use the ifmp->ifi_index for the peer index configuration. We just need to
>> assume that if the caller specified the ifindex for the veth master device,
>> then it's aware of this possibility and should explicitly configure (or set
>> to 0) the peer's ifindex as well. Like this:
>>
>> 	if (ifmp && (dev->ifindex != 0))
>> 		peer->ifindex = ifmp->ifi_index;
>>
>> Does this assumption work from you POV?
> 
> Yes, that looks like a neat way to do it.  Maybe with an explanatory
> comment?

Yes, sure. I will resend the set shortly.
Thanks for the feedback!

> Ben.
> 

Thanks,
Pavel