Per-net and on-demand link indices (and related) v2
Subsystems (4)
cover Per-net and on-demand link indices (and related) v2
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
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.
--- include/linux/hash.h | 10 ++++++++++ include/net/netns/hash.h | 9 ++------- 2 files changed, 12 insertions(+), 7 deletions(-)
include/linux/hash.h +10 -0
@@ -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 */
include/net/netns/hash.h +2 -7
@@ -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
Signature · 1 line
1.7.6.5
David Miller
maintainerRe: [PATCH 1/6] hash: Introduce ptr_hash_mix routine
From: Pavel Emelyanov <[email protected]>
Date: Mon, 06 Aug 2012 18:13:47 +0400
Quoted 4 lines
include/linux/hash.h +0
-0@@ -67,4 +68,13 @@ static inline unsigned long hash_ptr(const void *ptr, unsigned int bits) {return hash_long ((unsigned long )ptr, bits); }
include/linux/hash.h static inline unsigned long hash_ptr(const void *ptr, unsigned int bits) · line 80
Quoted 10 lines
include/linux/hash.h +9
-0+ +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
Quoted 27 lines · David Miller
From: Pavel Emelyanov <[email protected]>
Date: Mon, 06 Aug 2012 18:13:47 +0400Quoted 4 lines
include/linux/hash.h +0
-0@@ -67,4 +68,13 @@ static inline unsigned long hash_ptr(const void *ptr, unsigned int bits) {return hash_long ((unsigned long )ptr, bits); }include/linux/hash.h static inline unsigned long hash_ptr(const void *ptr, unsigned int bits) · line 80
Quoted 10 lines
include/linux/hash.h +9
-0+ +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)
Quoted 8 lines
for the 64-bit case.
Also, that
L1_CACHE_SHIFTis 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
Quoted 72 lines · Pavel Emelyanov
Quoted 27 lines · David Miller
From: Pavel Emelyanov <[email protected]>
Date: Mon, 06 Aug 2012 18:13:47 +0400Quoted 4 lines
include/linux/hash.h +0
-0@@ -67,4 +68,13 @@ static inline unsigned long hash_ptr(const void *ptr, unsigned int bits) {return hash_long ((unsigned long )ptr, bits); }include/linux/hash.h static inline unsigned long hash_ptr(const void *ptr, unsigned int bits) · line 80
Quoted 10 lines
include/linux/hash.h +9
-0+ +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)Quoted 8 lines
for the 64-bit case.
Also, that
L1_CACHE_SHIFTis 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->hashwork more significantly then? Currently it is used likehash = 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 thenet_devicepointer 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, u32hash_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); }
Quoted 1 line
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
Quoted 8 lines
I think we should avoid
hash_ptr()because its quite expensiveDavid suggested to not use the
L1_CACHE_SHIFTand instead do a plain :static inline u32
ptr_hash_mix(const void *ptr){ unsigned long val = (unsigned long)ptr;
Quoted 7 lines
#if BITS_PER_LONG == 64 val ^= (val >> 32); #endif return (u32)val; }By the way we could name this
hash32_ptr()instead ofptr_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 <[email protected]>
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!
--- include/linux/hash.h | 10 ++++++++++ include/net/arp.h | 3 ++- include/net/ndisc.h | 3 ++- 3 files changed, 14 insertions(+), 2 deletions(-)
include/linux/hash.h +10 -0
@@ -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 */
include/net/arp.h +2 -1
@@ -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; }
include/net/ndisc.h +2 -1
@@ -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 ]));
Signature · 1 line
1.7.6.5
Re: [PATCH 1/6] hash: Introduce ptr_hash_mix routine
Quoted 20 lines · Pavel Emelyanov
OK. I was under impression, that
hash_ptrwas 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,
PavelFrom: Pavel Emelyanov <[email protected]>
Subject: [PATCH 1/5] net: Dont use ifindices in hash fnsEric 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_deviceaddress 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.
Quoted 8 lines
Many thanks to David and Eric for the
hash32_ptrimplementation!Signed-off-by Pavel Emelyanov <[email protected]>--- include/linux/hash.h | 10 ++++++++++ include/net/arp.h | 3 ++- include/net/ndisc.h | 3 ++- 3 files changed, 14 insertions(+), 2 deletions(-)
You should resend other patches, since they are no more on
http://patchwork.ozlabs.org/project/netdev/list/
David Miller
maintainerRe: [PATCH 1/6] hash: Introduce ptr_hash_mix routine
In reply to Pavel EmelyanovFrom: Pavel Emelyanov <[email protected]>
Date: Tue, 07 Aug 2012 13:11:41 +0400
Quoted 7 lines
Maybe it would be better to change the way
neigh_table->hashwork more significantly then? Currently it is used likehash = 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.
PATCH 2/6 net: Dont use ifindices in hash fns
In reply to Pavel EmelyanovEric 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.
--- include/net/arp.h | 3 ++- include/net/ndisc.h | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
include/net/arp.h +2 -1
@@ -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; }
include/net/ndisc.h +2 -1
@@ -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 ]));
Signature · 1 line
1.7.6.5
PATCH 3/6 net: Allow to create links with given ifindex
In reply to Pavel EmelyanovCurrently 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).
--- net/core/dev.c | 7 ++++++- net/core/rtnetlink.c | 12 +++++++----- 2 files changed, 13 insertions(+), 6 deletions(-)
net/core/dev.c +6 -1
@@ -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;
net/core/rtnetlink.c +7 -5
@@ -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);
Signature · 1 line
1.7.6.5
PATCH 2/5 net: Allow to create links with given ifindex
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).
--- net/core/dev.c | 7 ++++++- net/core/rtnetlink.c | 12 +++++++----- 2 files changed, 13 insertions(+), 6 deletions(-)
net/core/dev.c +6 -1
@@ -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;
net/core/rtnetlink.c +7 -5
@@ -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);
Signature · 1 line
1.7.6.5
Re: [PATCH 2/5 (resend)] net: Allow to create links with given ifindex
Quoted 6 lines · Pavel Emelyanov
Currently the
RTM_NEWLINKresults in-EOPNOTSUPPif theifinfomsg->ifi_indexis 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 <[email protected]>
David Miller
maintainerRe: [PATCH 2/5 (resend)] net: Allow to create links with given ifindex
In reply to Pavel EmelyanovWhere 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.
PATCH 4/6 veth: Allow to create peer link with given ifindex
In reply to Pavel EmelyanovThe ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago), so take the given ifidex and register netdev with it.
--- drivers/net/veth.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/net/veth.c +3 -0
@@ -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 ;
Signature · 1 line
1.7.6.5
PATCH 3/5 veth: Allow to create peer link with given ifindex
The ifinfomsg is in there (thanks kaber@ for foreseeing this long time ago), so take the given ifidex and register netdev with it.
--- drivers/net/veth.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/net/veth.c +3 -0
@@ -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 ;
Signature · 1 line
1.7.6.5
Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex
Quoted 7 lines · 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 <[email protected]>--- drivers/net/veth.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex
In reply to Pavel EmelyanovQuoted 13 lines · 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 <[email protected]>--- drivers/net/veth.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/net/veth.c +0
-0
drivers/net/veth.c static int veth_newlink(struct net *src_net, struct net_device *dev, · line 356
Quoted 9 lines
drivers/net/veth.c +3
-0@@ -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.
Signature · 3 lines
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
Quoted 17 lines · Ben Hutchings
Quoted 16 lines · 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 <[email protected]>--- drivers/net/veth.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/net/veth.c +0
-0@@ -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);
Quoted 10 lines
drivers/net/veth.c static int veth_newlink(struct net *src_net, struct net_device *dev, · line 356
Quoted 6 lines
drivers/net/veth.c +3
-0+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?
Quoted 2 lines
Ben.
Thanks,
Pavel
Re: [PATCH 3/5 (resend)] veth: Allow to create peer link with given ifindex
Quoted 35 lines · Pavel Emelyanov
Quoted 17 lines · Ben Hutchings
Quoted 16 lines · 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 <[email protected]>--- drivers/net/veth.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)drivers/net/veth.c +0
-0@@ -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);Quoted 10 lines
drivers/net/veth.c static int veth_newlink(struct net *src_net, struct net_device *dev, · line 356
Quoted 6 lines
drivers/net/veth.c +3
-0+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_indexfor 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:
Quoted 4 lines
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.
Signature · 3 lines
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
Quoted 6 lines
Quoted 6 lines
You're right, I've missed that fact :( The good news is that we still can use the
ifmp->ifi_indexfor 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:
Quoted 7 lines
Quoted 4 lines
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!
Quoted 2 lines
Ben.
Thanks,
Pavel
PATCH 5/6 net: Make ifindex generation per-net namespace
In reply to Pavel EmelyanovStrictly 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.
--- include/net/net_namespace.h | 1 + net/core/dev.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)
include/net/net_namespace.h +1 -0
@@ -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;
net/core/dev.c +2 -2
@@ -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; } }
Signature · 1 line
1.7.6.5
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.
--- include/net/net_namespace.h | 1 + net/core/dev.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)
include/net/net_namespace.h +1 -0
@@ -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;
net/core/dev.c +2 -2
@@ -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; } }
Signature · 1 line
1.7.6.5
Re: [PATCH 4/5 (resend)] net: Make ifindex generation per-net namespace
Quoted 23 lines · 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_netonly.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 <[email protected]>--- include/net/net_namespace.h | 1 + net/core/dev.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)include/net/net_namespace.h +0
-0
include/net/net_namespace.h struct net { · line 65
Quoted 4 lines
include/net/net_namespace.h +1
-0@@ -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()
Quoted 22 lines
include/net/net_namespace.h +0
-0struct list_head dev_base_head;struct hlist_head *dev_name_head;struct hlist_head *dev_index_head;net/core/dev.c +2
-2@@ -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 net: Make ifindex generation per-net namespace (v2)
Quoted 9 lines
Review struct net { · line 65
Quoted 5 lines
(quoted diff) +1
-0@@ -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 <[email protected]>
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()
--- include/net/net_namespace.h | 1 + net/core/dev.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-)
include/net/net_namespace.h +1 -0
@@ -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;
net/core/dev.c +2 -2
@@ -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; } }
Signature · 1 line
1.7.6.5
Re: [PATCH 4/5 (resend)] net: Make ifindex generation per-net namespace (v2)
Quoted 27 lines · Pavel Emelyanov
Quoted 9 lines
Review struct net { · line 65
Quoted 5 lines
(quoted diff) +1
-0@@ -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 <[email protected]>
Subject: [PATCH 4/5] net: Make ifindex generation per-net namespaceStrictly 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_netonly.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.
Quoted 5 lines
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 <[email protected]>---
PATCH 6/6 net: Loopback ifindex is constant now
In reply to Pavel EmelyanovAs 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.
--- 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(-)
drivers/net/loopback.c +1 -0
@@ -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 ;
include/net/net_namespace.h +7 -0
@@ -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>
net/decnet/dn_route.c +3 -3
@@ -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; }
net/ipv4/fib_frontend.c +1 -1
@@ -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);
net/ipv4/ipmr.c +1 -1
@@ -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, };
net/ipv4/netfilter/ipt_rpfilter.c +1 -1
@@ -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;
net/ipv4/route.c +3 -3
@@ -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;
net/ipv6/route.c +1 -1
@@ -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 ;
Signature · 1 line
1.7.6.5
PATCH 5/5 net: Loopback ifindex is constant now
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.
--- 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(-)
drivers/net/loopback.c +1 -0
@@ -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 ;
include/net/net_namespace.h +7 -0
@@ -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>
net/decnet/dn_route.c +3 -3
@@ -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; }
net/ipv4/fib_frontend.c +1 -1
@@ -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);
net/ipv4/ipmr.c +1 -1
@@ -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, };
net/ipv4/netfilter/ipt_rpfilter.c +1 -1
@@ -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;
net/ipv4/route.c +3 -3
@@ -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;
net/ipv6/route.c +1 -1
@@ -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 ;
Signature · 1 line
1.7.6.5
Re: [PATCH 5/5 (resend)] net: Loopback ifindex is constant now
Quoted 6 lines · Pavel Emelyanov
As pointed out, there are places, that access
net->loopback_dev->ifindexand after ifindex generation is made per-net this value becomes constant equals 1. So go ahead and introduce theLOOPBACK_IFINDEXconstant and use it where appropriate.Signed-off-by Pavel Emelyanov <[email protected]>
Quoted 10 lines
--- 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(-)