diff mbox

[net-next] ip: initialize hash list

Message ID 20130720102657.768a11ea@nehalam.linuxnetplumber.net
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Stephen Hemminger July 20, 2013, 5:26 p.m. UTC 
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>


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

Comments

Joe Perches July 20, 2013, 5:46 p.m. UTC | #1 
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))


--
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
David Miller July 20, 2013, 7:20 p.m. UTC | #2 
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.
--
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
Joe Perches July 20, 2013, 8:28 p.m. UTC | #3 
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)



--
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
Stephen Hemminger July 22, 2013, 2:52 p.m. UTC | #4 
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.
--
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
Joe Perches July 22, 2013, 3:06 p.m. UTC | #5 
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.



--
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
Eric Dumazet July 22, 2013, 3:15 p.m. UTC | #6 
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 ?



--
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
Eric Dumazet July 22, 2013, 3:17 p.m. UTC | #7 
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 ?




--
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
Joe Perches July 22, 2013, 3:31 p.m. UTC | #8 
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.


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

Patch

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