| Message ID | 501F9CDA.6040403@parallels.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2012-08-06 at 14:30 +0400, Pavel Emelyanov wrote: > 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/linux/netdevice.h | 6 ++++++ > include/net/arp.h | 2 +- > include/net/ndisc.h | 2 +- > 3 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index a9db4f3..6010b37 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1330,6 +1330,12 @@ struct net_device { > > #define NETDEV_ALIGN 32 > > +static inline unsigned int netdev_hash_mix(const struct net_device *dev) > +{ > + return (unsigned int)(((unsigned long)dev) >> > + max(L1_CACHE_BYTES, NETDEV_ALIGN)); > +} > + I guess you didnt test this patch very well ... This returns 0 as is I would define a generic pointer hash mix instead of a 'net_device thing' static inline u32 ptr_hash_mix(void *ptr) { #if BITS_PER_LONG==32 return (u32)(unsigned long)ptr; #else return (u32)((unsigned long)ptr >> L1_CACHE_SHIFT); #endif } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/06/2012 02:43 PM, Eric Dumazet wrote: > On Mon, 2012-08-06 at 14:30 +0400, Pavel Emelyanov wrote: >> 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/linux/netdevice.h | 6 ++++++ >> include/net/arp.h | 2 +- >> include/net/ndisc.h | 2 +- >> 3 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >> index a9db4f3..6010b37 100644 >> --- a/include/linux/netdevice.h >> +++ b/include/linux/netdevice.h >> @@ -1330,6 +1330,12 @@ struct net_device { >> >> #define NETDEV_ALIGN 32 >> >> +static inline unsigned int netdev_hash_mix(const struct net_device *dev) >> +{ >> + return (unsigned int)(((unsigned long)dev) >> >> + max(L1_CACHE_BYTES, NETDEV_ALIGN)); >> +} >> + > > I guess you didnt test this patch very well ... Damn :( You're right. > This returns 0 as is Well, on 64-bit no, but what it does is also not what it was supposed to. > I would define a generic pointer hash mix instead of a 'net_device > thing' > > static inline u32 ptr_hash_mix(void *ptr) > { > #if BITS_PER_LONG==32 > return (u32)(unsigned long)ptr; > #else > return (u32)((unsigned long)ptr >> L1_CACHE_SHIFT); > #endif > } OK. It will also obsolete the net_hash_mix then. Any suggestions where to put the new one? Thanks, Pavel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2012-08-06 at 15:01 +0400, Pavel Emelyanov wrote: > Damn :( You're right. > > > This returns 0 as is > > Well, on 64-bit no, but what it does is also not what it was supposed to. > Here, my L1_CACHE_BYTES is 64, but whatever > > I would define a generic pointer hash mix instead of a 'net_device > > thing' > > > > static inline u32 ptr_hash_mix(void *ptr) > > { > > #if BITS_PER_LONG==32 > > return (u32)(unsigned long)ptr; > > #else > > return (u32)((unsigned long)ptr >> L1_CACHE_SHIFT); > > #endif > > } > > OK. It will also obsolete the net_hash_mix then. Any suggestions where to put > the new one? Not obsolete net_hash_mix(), since this one can return 0 if CONFIG_NET_NS is not set. static inline unsigned int net_hash_mix(struct net *net) { #ifdef CONFIG_NET_NS return ptr_hash_mix(net); #else return 0; #endif } I dont know, you could add this to include/linux/hash.h -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index a9db4f3..6010b37 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1330,6 +1330,12 @@ struct net_device { #define NETDEV_ALIGN 32 +static inline unsigned int netdev_hash_mix(const struct net_device *dev) +{ + return (unsigned int)(((unsigned long)dev) >> + max(L1_CACHE_BYTES, NETDEV_ALIGN)); +} + static inline int netdev_get_prio_tc_map(const struct net_device *dev, u32 prio) { diff --git a/include/net/arp.h b/include/net/arp.h index 7f7df93..0305a38 100644 --- a/include/net/arp.h +++ b/include/net/arp.h @@ -10,7 +10,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 ^ netdev_hash_mix(dev); return val * hash_rnd; } diff --git a/include/net/ndisc.h b/include/net/ndisc.h index 96a3b5c..ae7c1fd 100644 --- a/include/net/ndisc.h +++ b/include/net/ndisc.h @@ -134,7 +134,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] ^ netdev_hash_mix(dev)) * hash_rnd[0]) + (p32[1] * hash_rnd[1]) + (p32[2] * hash_rnd[2]) + (p32[3] * hash_rnd[3]));
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/linux/netdevice.h | 6 ++++++ include/net/arp.h | 2 +- include/net/ndisc.h | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-)