[PATCH net-next] ip: initialize hash list

[PATCH net-next] ip: initialize hash list

[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 <stephen@networkplumber.org>


--- a/net/ipv4/ip_tunnel.c	2013-07-19 09:12:37.213529343 -0700
+++ b/net/ipv4/ip_tunnel.c	2013-07-19 09:37:00.960764421 -0700
@@ -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;

Re: [PATCH net-next] ip: initialize hash list

[Joe Perches]

On Sat, 2013-07-20 at 10:26 -0700, Stephen Hemminger wrote:
> 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.
[]
> --- a/net/ipv4/ip_tunnel.c	2013-07-19 09:12:37.213529343 -0700
[]
> -	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))

Re: [PATCH net-next] ip: initialize hash list

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Sat, 20 Jul 2013 10:26:57 -0700

> +	unsigned i;

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

Re: [PATCH net-next] ip: initialize hash list

[Joe Perches]

On Sat, 2013-07-20 at 12:20 -0700, David Miller wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Sat, 20 Jul 2013 10:26:57 -0700
> 
> > +     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)

Re: [PATCH net-next] ip: initialize hash list

[Stephen Hemminger]

On Sat, 20 Jul 2013 10:46:20 -0700
Joe Perches <joe@perches.com> wrote:

> On Sat, 2013-07-20 at 10:26 -0700, Stephen Hemminger wrote:
> > 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.
> []
> > --- a/net/ipv4/ip_tunnel.c	2013-07-19 09:12:37.213529343 -0700
> []
> > -	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?

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

Re: [PATCH net-next] ip: initialize hash list

[Joe Perches]

On Mon, 2013-07-22 at 07:52 -0700, Stephen Hemminger wrote:
> On Sat, 20 Jul 2013 10:46:20 -0700
> Joe Perches <joe@perches.com> wrote:
[]
> > 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.

Re: [PATCH net-next] ip: initialize hash list

[Eric Dumazet]

On Mon, 2013-07-22 at 08:06 -0700, Joe Perches wrote:
> On Mon, 2013-07-22 at 07:52 -0700, Stephen Hemminger wrote:
> > On Sat, 20 Jul 2013 10:46:20 -0700
> > Joe Perches <joe@perches.com> wrote:
> []
> > > 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.

Joe, I find this quite misplaced.

So far, there is no bug.

What about fixing real bugs instead ?

Re: [PATCH net-next] ip: initialize hash list

[Eric Dumazet]

On Sat, 2013-07-20 at 10:26 -0700, Stephen Hemminger wrote:
> 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 <stephen@networkplumber.org>
> 
> 
> --- a/net/ipv4/ip_tunnel.c	2013-07-19 09:12:37.213529343 -0700
> +++ b/net/ipv4/ip_tunnel.c	2013-07-19 09:37:00.960764421 -0700
> @@ -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]);
>  

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

Re: [PATCH net-next] ip: initialize hash list

[Joe Perches]

On Mon, 2013-07-22 at 08:15 -0700, Eric Dumazet wrote:
> On Mon, 2013-07-22 at 08:06 -0700, Joe Perches wrote:
> > On Mon, 2013-07-22 at 07:52 -0700, Stephen Hemminger wrote:
> > > On Sat, 20 Jul 2013 10:46:20 -0700
> > > Joe Perches <joe@perches.com> wrote:
> > []
> > > > 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.
> 
> 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?

> So far, there is no bug.

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

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