ip: initialize hash list

PATCH In review Stephen Hemminger 4 participants 9 messages Jul 20, 2013 – Jul 22, 2013

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Stephen Hemminger

author

2013-07-20 17:27 UTC #0 lore raw

PATCH 1/1 ip: initialize hash list

Rather than relying on the assumption that zero means empty on hash list head, the code should use the initialization macro. Same effect, but follows API and avoids future breakage if hlist implementation changes.

Signed-off-by Stephen Hemminger <[email protected]>

net/ipv4/ip_tunnel.c +6 -1

@@ -838,10 +838,15 @@ int ip_tunnel_init_net(struct net *net,
 {
 	struct ip_tunnel_net *itn = net_generic(net, ip_tnl_net_id);
 	struct ip_tunnel_parm parms;
+	unsigned i;
 
-	itn->tunnels = kzalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head), GFP_KERNEL);
+	itn->tunnels = kmalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head),
+			       GFP_KERNEL);
 	if (!itn->tunnels)
 		return -ENOMEM;
+
+	for (i = 0; i < IP_TNL_HASH_SIZE; i++)
+		INIT_HLIST_HEAD(&itn->tunnels[i]);
 
 	if (!ops) {
 		itn->fb_tunnel_dev = NULL;

Joe Perches

2013-07-20 17:46 UTC #1 lore raw

Quoted 4 lines · Stephen Hemminger

Rather than relying on the assumption that zero means empty on hash list head, the code should use the initialization macro. Same effect, but follows API and avoids future breakage if hlist implementation changes.

[]

Quoted 1 line

--- a/net/ipv4/ip_tunnel.c 2013-07-19 09:12:37.213529343 -0700

[]

net/ipv4/ip_tunnel.c int ip_tunnel_init_net(struct net *net, · line 849

Quoted 8 lines

net/ipv4/ip_tunnel.c +5 -1

-	itn->tunnels = kzalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head), GFP_KERNEL);
+	itn->tunnels = kmalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head),
+			       GFP_KERNEL);
 	if (!itn->tunnels)
 		return -ENOMEM;
+
+	for (i = 0; i < IP_TNL_HASH_SIZE; i++)
+		INIT_HLIST_HEAD(&itn->tunnels[i]);

Hey Stephen.

Are you doing to do just this one or submit a series?

$ git grep "alloc.*sizeof.*hlist_head"
drivers/gpu/drm/i915/i915_gem_execbuffer.c:             eb = kzalloc(count*sizeof(struct hlist_head) +
drivers/md/dm-bufio.c:  c->cache_hash = vmalloc(sizeof(struct hlist_head) << DM_BUFIO_HASH_BITS);
fs/ecryptfs/messaging.c:        ecryptfs_daemon_hash = kmalloc((sizeof(struct hlist_head)
fs/nfsd/nfscache.c:     cache_hash = kcalloc(hashsize, sizeof(struct hlist_head), GFP_KERNEL);
kernel/trace/ftrace.c:  stat->hash = kzalloc(sizeof(struct hlist_head) * size, GFP_KERNEL);
lib/lru_cache.c:        slot = kcalloc(e_count, sizeof(struct hlist_head), GFP_KERNEL);
net/ipv4/ip_tunnel.c:   itn->tunnels = kzalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head), GFP_KERNEL);
net/mac80211/mesh_pathtbl.c:    newtbl->hash_buckets = kzalloc(sizeof(struct hlist_head) *
net/mac80211/mesh_pathtbl.c:    tbl_path->known_gates = kzalloc(sizeof(struct hlist_head), GFP_ATOMIC);
net/mac80211/mesh_pathtbl.c:    tbl_mpp->known_gates = kzalloc(sizeof(struct hlist_head), GFP_ATOMIC);
net/openvswitch/datapath.c:     dp->ports = kmalloc(DP_VPORT_HASH_BUCKETS * sizeof(struct hlist_head),
net/openvswitch/flow.c: buckets = flex_array_alloc(sizeof(struct hlist_head *),
net/openvswitch/vport.c:        dev_table = kzalloc(VPORT_HASH_BUCKETS * sizeof(struct hlist_head),
net/tipc/name_table.c:  table.types = kcalloc(TIPC_NAMETBL_SIZE, sizeof(struct hlist_head),
virt/kvm/irqchip.c:     new = kzalloc(sizeof(*new) + (nr_rt_entries * sizeof(struct hlist_head))

Stephen Hemminger

author

2013-07-22 14:52 UTC #2 lore raw

Quoted 19 lines · On Sat, 20 Jul 2013 10:46:20 -0700 Joe Perches <[email protected]> wrote:

Quoted 4 lines · Stephen Hemminger
Rather than relying on the assumption that zero means empty on hash list head, the code should use the initialization macro. Same effect, but follows API and avoids future breakage if hlist implementation changes.
[]
Quoted 1 line
--- a/net/ipv4/ip_tunnel.c 2013-07-19 09:12:37.213529343 -0700
[]
net/ipv4/ip_tunnel.c int ip_tunnel_init_net(struct net *net, · line 849
Quoted 8 lines
net/ipv4/ip_tunnel.c +5 -1
-	itn->tunnels = kzalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head), GFP_KERNEL);
+	itn->tunnels = kmalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head),
+			       GFP_KERNEL);
 	if (!itn->tunnels)
 		return -ENOMEM;
+
+	for (i = 0; i < IP_TNL_HASH_SIZE; i++)
+		INIT_HLIST_HEAD(&itn->tunnels[i]);
Hey Stephen.

Quoted 1 line

Are you doing to do just this one or submit a series?

Hadn't planned on fixing more than one. And would not go outside code that I actually use.

Joe Perches

2013-07-22 15:06 UTC #3 lore raw

Quoted 2 lines · Stephen Hemminger

On Sat, 20 Jul 2013 10:46:20 -0700
Joe Perches <[email protected]> wrote:

[]

Quoted 4 lines

Quoted 1 line
Are you doing to do just this one or submit a series?
Hadn't planned on fixing more than one. And would not go outside code that I actually use.

What a pity.

I think if you identify what you believe is a
trivial defect in your own code, it's both
polite and social to fix similar defects in
other code too.

Eric Dumazet

2013-07-22 15:16 UTC #4 lore raw

Quoted 9 lines · Joe Perches

Quoted 2 lines · Stephen Hemminger
On Sat, 20 Jul 2013 10:46:20 -0700
Joe Perches <[email protected]> wrote:
[]
Quoted 4 lines
Quoted 1 line
Are you doing to do just this one or submit a series?
Hadn't planned on fixing more than one. And would not go outside code that I actually use.

Quoted 6 lines

What a pity.
I think if you identify what you believe is a
trivial defect in your own code, it's both
polite and social to fix similar defects in
other code too.

Joe, I find this quite misplaced.

So far, there is no bug.

What about fixing real bugs instead ?

Joe Perches

2013-07-22 15:31 UTC #5 lore raw

Quoted 10 lines · Eric Dumazet

Quoted 9 lines · Joe Perches
Quoted 2 lines · Stephen Hemminger
On Sat, 20 Jul 2013 10:46:20 -0700
Joe Perches <[email protected]> wrote:
[]
Quoted 4 lines
Quoted 1 line
Are you doing to do just this one or submit a series?
Hadn't planned on fixing more than one. And would not go outside code that I actually use.

Quoted 8 lines

Quoted 6 lines
What a pity.
I think if you identify what you believe is a
trivial defect in your own code, it's both
polite and social to fix similar defects in
other code too.
Joe, I find this quite misplaced.

Our opinions differ then.

What's "misplaced" about suggesting that a person that identifies and submits a patch for a code form viewed as sub-optimal could fix the other instances of that code form?

Quoted 1 line

So far, there is no bug.

I did not even suggest there was a bug. I wrote "trivial defect".

Quoted 1 line

What about fixing real bugs instead ?

That'd be good too, with the caveat that those "real bugs" can take a rather more intensive effort to identify, isolate and correct without introducing other defects.

David Miller

maintainer

2013-07-20 19:20 UTC #6 lore raw

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Sat, 20 Jul 2013 10:26:57 -0700

net/ipv4/ip_tunnel.c int ip_tunnel_init_net(struct net *net, · line 841

Quoted 1 line

net/ipv4/ip_tunnel.c +1 -0
+	unsigned i;

Please use an explicit "unsigned int", thank you.

Joe Perches

2013-07-20 20:28 UTC #7 lore raw

Quoted 6 lines · David Miller

From: Stephen Hemminger <[email protected]>
Date: Sat, 20 Jul 2013 10:26:57 -0700
Quoted 1 line
+ unsigned i;
Please use an explicit "unsigned int", thank you.

Still a dozen+ of these in net
(I removed a few false positives)

$ grep -rP  --include=*.[ch] '\bunsigned\s*+(?!(int|long|char|short|\*\/))' net
net/mac80211/ieee80211_i.h:typedef unsigned __bitwise__ ieee80211_tx_result;
net/mac80211/ieee80211_i.h:typedef unsigned __bitwise__ ieee80211_rx_result;
net/ceph/osdmap.c:	unsigned len, num;
net/ceph/osdmap.c:			(unsigned)pgid.pool;
net/core/netprio_cgroup.c:static int update_netprio(const void *v, struct file *file, unsigned n)
net/rose/rose_subr.c:int rose_parse_facilities(unsigned char *p, unsigned packet_len,
net/xfrm/xfrm_policy.c:#define XFRM_QUEUE_TMO_MIN ((unsigned)(HZ/10))
net/xfrm/xfrm_policy.c:#define XFRM_QUEUE_TMO_MAX ((unsigned)(60*HZ))
net/ipv4/netfilter/nf_nat_h323.c:static int set_h245_addr(struct sk_buff *skb, unsigned protoff,
net/ipv4/ip_vti.c:	unsigned h0 = HASH(remote);
net/ipv4/ip_vti.c:	unsigned h1 = HASH(local);
net/ipv4/ip_vti.c:	unsigned h = 0;
net/sched/cls_rsvp.h:static unsigned int gen_handle(struct tcf_proto *tp, unsigned salt)
net/sched/cls_cgroup.c:static int update_classid(const void *v, struct file *file, unsigned n)
net/mac802154/wpan.c:				   unsigned len)
net/socket.c:static long sock_ioctl(struct file *file, unsigned cmd, unsigned long arg)
net/socket.c:long __sys_sendmsg(int fd, struct msghdr __user *msg, unsigned flags)
net/socket.c:long __sys_recvmsg(int fd, struct msghdr __user *msg, unsigned flags)

Eric Dumazet

2013-07-22 15:17 UTC #8 lore raw

In reply to Stephen Hemminger

Quoted 15 lines · Stephen Hemminger

Rather than relying on the assumption that zero means empty on hash list head, the code should use the initialization macro. Same effect, but follows API and avoids future breakage if hlist implementation changes.
Signed-off-by Stephen Hemminger <[email protected]>
net/ipv4/ip_tunnel.c +1 -0
@@ -838,10 +838,15 @@ int ip_tunnel_init_net(struct net *net,
 {
 	struct ip_tunnel_net *itn = net_generic(net, ip_tnl_net_id);
 	struct ip_tunnel_parm parms;
+	unsigned i;

net/ipv4/ip_tunnel.c int ip_tunnel_init_net(struct net *net, · line 850

Quoted 10 lines

net/ipv4/ip_tunnel.c +5 -1

 
-	itn->tunnels = kzalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head), GFP_KERNEL);
+	itn->tunnels = kmalloc(IP_TNL_HASH_SIZE * sizeof(struct hlist_head),
+			       GFP_KERNEL);
 	if (!itn->tunnels)
 		return -ENOMEM;
+
+	for (i = 0; i < IP_TNL_HASH_SIZE; i++)
+		INIT_HLIST_HEAD(&itn->tunnels[i]);

What about adding a variant of INIT_HLIST_HEAD() that does nothing for already cleared memory ?