diff mbox

slowpath warning

Message ID 20100301165706.GA11578@pandem0nium
State Superseded, archived
Headers show

Commit Message

Simon Wunderlich March 1, 2010, 4:57 p.m. UTC
Hey Andrew,

all list-adds and list-removes do a kref_get or kref_put respectively,
but that probably was not very clear. As soon as a refence is created 
(by linking into the hash or a list), we should also kref_get() it.

Please find attached a new version of this patch with separate 
send_list_add/remove functions which include the put/get functions. 
I've also added the kref_put/get call to the send_list loop again.
It should look more balanced now. Tested in my 9 qemu setup again,
no memory leaks or crashes but i only did a very quick test 
(15 minutes)

best regards,
	Simon

On Mon, Mar 01, 2010 at 06:59:55AM +0100, Andrew Lunn wrote:
> Hi Simon
> 
> > @@ -503,15 +550,23 @@
> >  	unsigned long flags;
> >  
> >  	spin_lock_irqsave(&vis_hash_lock, flags);
> > +
> >  	purge_vis_packets();
> >  
> > -	if (generate_vis_packet() == 0)
> > +	if (generate_vis_packet() == 0) {
> >  		/* schedule if generation was successful */
> > +		kref_get(&my_vis_info->refcount);
> >  		list_add_tail(&my_vis_info->send_list, &send_list);
> > +	}
> >  
> >  	list_for_each_entry_safe(info, temp, &send_list, send_list) {
> > +		spin_unlock_irqrestore(&vis_hash_lock, flags);
> > +
> > +		send_vis_packet(info);
> > +
> > +		spin_lock_irqsave(&vis_hash_lock, flags);
> >  		list_del_init(&info->send_list);
> > -		send_vis_packet(info);
> > +		kref_put(&info->refcount, free_info);
> >  	}
> >  	spin_unlock_irqrestore(&vis_hash_lock, flags);
> >  	start_vis_timer();
> 
> Although you say this works, i just looks wrong. It looks
> unbalanced. Where is the kref_put for my_vis_info->refcount?  There is
> a kref_put inside what looks like a loop, but no kref_get inside the
> loop. 
> 
> Can you explain this is more detail? Can we make it look correctly
> balanced?
> 
> 	Thanks
> 		Andrew
>

Comments

Andrew Lunn March 2, 2010, 6:43 a.m. UTC | #1
On Mon, Mar 01, 2010 at 05:57:06PM +0100, Simon Wunderlich wrote:
> Hey Andrew,
> 
> all list-adds and list-removes do a kref_get or kref_put respectively,
> but that probably was not very clear. As soon as a refence is created 
> (by linking into the hash or a list), we should also kref_get() it.
> 
> Please find attached a new version of this patch with separate 
> send_list_add/remove functions which include the put/get functions. 
> I've also added the kref_put/get call to the send_list loop again.
> It should look more balanced now. Tested in my 9 qemu setup again,
> no memory leaks or crashes but i only did a very quick test 
> (15 minutes)

Hi Simon

This looks better. Did it survive longer testing? 

     Andrew
Simon Wunderlich March 2, 2010, 9:13 p.m. UTC | #2
Hello Andrew,

no, sorry, and i won't be able to do it this week as i am at the cebit
this whole week (if anyone else is there, please tell me. ;).

I would therefore like to ask you and Linus to test it as well, i will 
perform more tests on sunday.

Thanks,
	Simon

On Tue, Mar 02, 2010 at 07:43:08AM +0100, Andrew Lunn wrote:
> On Mon, Mar 01, 2010 at 05:57:06PM +0100, Simon Wunderlich wrote:
> > Hey Andrew,
> > 
> > all list-adds and list-removes do a kref_get or kref_put respectively,
> > but that probably was not very clear. As soon as a refence is created 
> > (by linking into the hash or a list), we should also kref_get() it.
> > 
> > Please find attached a new version of this patch with separate 
> > send_list_add/remove functions which include the put/get functions. 
> > I've also added the kref_put/get call to the send_list loop again.
> > It should look more balanced now. Tested in my 9 qemu setup again,
> > no memory leaks or crashes but i only did a very quick test 
> > (15 minutes)
> 
> Hi Simon
> 
> This looks better. Did it survive longer testing? 
> 
>      Andrew
>
elektra March 2, 2010, 9:26 p.m. UTC | #3
Hello Simon -

I'll be in hall 13 at the booth of Atcom E42 to present the Mesh-Potato 
router. (With B.A.T.M.A.N. inside, of course) Will arrive tomorrow afternoon 
and be there until Saturday 18:00.

Cheers,
Elektra


> no, sorry, and i won't be able to do it this week as i am at the cebit
> this whole week (if anyone else is there, please tell me. ;).
Linus L├╝ssing March 2, 2010, 9:44 p.m. UTC | #4
Hi Simon and Andrew,

I already tried both Andrew's intial and Simon's latest patch
yesterday and had both of them running for about 1h+, also with
alternate vis-intervals (1s, 50ms, 30s) but so far I wasn't able
to reproduce any crashes or leaks on two Debian stable VMs
connected with each other. I'll run Andrew's patch version again
this night for some more hours to make sure that I'm able to
reproduce the crash at all. Any other hints for reproducing it
more quickly welcome :).

Cheers, Linus

PS: With how many nodes and what kind of systems could you
reproduce the crash? Were all those nodes connected directly with
each other? How many vis-servers were running?

On Tue, Mar 02, 2010 at 10:13:24PM +0100, Simon Wunderlich wrote:
> Hello Andrew,
> 
> no, sorry, and i won't be able to do it this week as i am at the cebit
> this whole week (if anyone else is there, please tell me. ;).
> 
> I would therefore like to ask you and Linus to test it as well, i will 
> perform more tests on sunday.
> 
> Thanks,
> 	Simon
> 
> On Tue, Mar 02, 2010 at 07:43:08AM +0100, Andrew Lunn wrote:
> > On Mon, Mar 01, 2010 at 05:57:06PM +0100, Simon Wunderlich wrote:
> > > Hey Andrew,
> > > 
> > > all list-adds and list-removes do a kref_get or kref_put respectively,
> > > but that probably was not very clear. As soon as a refence is created 
> > > (by linking into the hash or a list), we should also kref_get() it.
> > > 
> > > Please find attached a new version of this patch with separate 
> > > send_list_add/remove functions which include the put/get functions. 
> > > I've also added the kref_put/get call to the send_list loop again.
> > > It should look more balanced now. Tested in my 9 qemu setup again,
> > > no memory leaks or crashes but i only did a very quick test 
> > > (15 minutes)
> > 
> > Hi Simon
> > 
> > This looks better. Did it survive longer testing? 
> > 
> >      Andrew
> >
diff mbox

Patch

taging: batman-adv: Don't have interrupts disabled while sending.

send_vis_packets() would disable interrupts before calling
dev_queue_xmit() which resulting in a backtrace in local_bh_enable().
Fix this by using kref on the vis_info object so that we can call
send_vis_packets() without holding vis_hash_lock. vis_hash_lock also
used to protect recv_list, so we now need a new lock to protect that
instead of vis_hash_lock.

Also a few checkpatch cleanups.

Reported-by: Linus Luessing <linus.luessing@web.de>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
Index: a/batman-adv-kernelland/vis.c
===================================================================
--- a/batman-adv-kernelland/vis.c	(revision 1578)
+++ a/batman-adv-kernelland/vis.c	(working copy)
@@ -30,22 +30,26 @@ 
 
 struct hashtable_t *vis_hash;
 DEFINE_SPINLOCK(vis_hash_lock);
+static DEFINE_SPINLOCK(recv_list_lock);
 static struct vis_info *my_vis_info;
 static struct list_head send_list;	/* always locked with vis_hash_lock */
 
 static void start_vis_timer(void);
 
 /* free the info */
-static void free_info(void *data)
+static void free_info(struct kref *ref)
 {
-	struct vis_info *info = data;
+	struct vis_info *info = container_of(ref, struct vis_info, refcount);
 	struct recvlist_node *entry, *tmp;
+	unsigned long flags;
 
 	list_del_init(&info->send_list);
+	spin_lock_irqsave(&recv_list_lock, flags);
 	list_for_each_entry_safe(entry, tmp, &info->recv_list, list) {
 		list_del(&entry->list);
 		kfree(entry);
 	}
+	spin_unlock_irqrestore(&recv_list_lock, flags);
 	kfree(info);
 }
 
@@ -143,36 +147,65 @@ 
 	}
 }
 
+/* add the info packet to the send list, if it was not
+ * already linked in. */
+static void send_list_add(struct vis_info *info)
+{
+	if (list_empty(&info->send_list)) {
+		kref_get(&info->refcount);
+		list_add_tail(&info->send_list, &send_list);
+	}
+}
+
+/* delete the info packet from the send list, if it was
+ * linked in. */
+static void send_list_del(struct vis_info *info)
+{
+	if (!list_empty(&info->send_list)) {
+		list_del_init(&info->send_list);
+		kref_put(&info->refcount, free_info);
+	}
+}
+
 /* tries to add one entry to the receive list. */
 static void recv_list_add(struct list_head *recv_list, char *mac)
 {
 	struct recvlist_node *entry;
+	unsigned long flags;
+
 	entry = kmalloc(sizeof(struct recvlist_node), GFP_ATOMIC);
 	if (!entry)
 		return;
 
 	memcpy(entry->mac, mac, ETH_ALEN);
+	spin_lock_irqsave(&recv_list_lock, flags);
 	list_add_tail(&entry->list, recv_list);
+	spin_unlock_irqrestore(&recv_list_lock, flags);
 }
 
 /* returns 1 if this mac is in the recv_list */
 static int recv_list_is_in(struct list_head *recv_list, char *mac)
 {
 	struct recvlist_node *entry;
+	unsigned long flags;
 
+	spin_lock_irqsave(&recv_list_lock, flags);
 	list_for_each_entry(entry, recv_list, list) {
-		if (memcmp(entry->mac, mac, ETH_ALEN) == 0)
+		if (memcmp(entry->mac, mac, ETH_ALEN) == 0) {
+			spin_unlock_irqrestore(&recv_list_lock, flags);
 			return 1;
+		}
 	}
-
+	spin_unlock_irqrestore(&recv_list_lock, flags);
 	return 0;
 }
 
 /* try to add the packet to the vis_hash. return NULL if invalid (e.g. too old,
- * broken.. ).  vis hash must be locked outside.  is_new is set when the packet
+ * broken.. ).	vis hash must be locked outside.  is_new is set when the packet
  * is newer than old entries in the hash. */
 static struct vis_info *add_packet(struct vis_packet *vis_packet,
-				   int vis_info_len, int *is_new)
+				   int vis_info_len, int *is_new,
+				   int make_broadcast)
 {
 	struct vis_info *info, *old_info;
 	struct vis_info search_elem;
@@ -199,13 +232,15 @@ 
 		}
 		/* remove old entry */
 		hash_remove(vis_hash, old_info);
-		free_info(old_info);
+		send_list_del(old_info);
+		kref_put(&old_info->refcount, free_info);
 	}
 
 	info = kmalloc(sizeof(struct vis_info) + vis_info_len, GFP_ATOMIC);
 	if (info == NULL)
 		return NULL;
 
+	kref_init(&info->refcount);
 	INIT_LIST_HEAD(&info->send_list);
 	INIT_LIST_HEAD(&info->recv_list);
 	info->first_seen = jiffies;
@@ -215,16 +250,21 @@ 
 	/* initialize and add new packet. */
 	*is_new = 1;
 
+	/* Make it a broadcast packet, if required */
+	if (make_broadcast)
+		memcpy(info->packet.target_orig, broadcastAddr, ETH_ALEN);
+
 	/* repair if entries is longer than packet. */
 	if (info->packet.entries * sizeof(struct vis_info_entry) > vis_info_len)
-		info->packet.entries = vis_info_len / sizeof(struct vis_info_entry);
+		info->packet.entries = vis_info_len /
+			sizeof(struct vis_info_entry);
 
 	recv_list_add(&info->recv_list, info->packet.sender_orig);
 
 	/* try to add it */
 	if (hash_add(vis_hash, info) < 0) {
 		/* did not work (for some reason) */
-		free_info(info);
+		kref_put(&old_info->refcount, free_info);
 		info = NULL;
 	}
 
@@ -235,22 +275,21 @@ 
 void receive_server_sync_packet(struct vis_packet *vis_packet, int vis_info_len)
 {
 	struct vis_info *info;
-	int is_new;
+	int is_new, make_broadcast;
 	unsigned long flags;
 	int vis_server = atomic_read(&vis_mode);
 
+	make_broadcast = (vis_server == VIS_TYPE_SERVER_SYNC);
+
 	spin_lock_irqsave(&vis_hash_lock, flags);
-	info = add_packet(vis_packet, vis_info_len, &is_new);
+	info = add_packet(vis_packet, vis_info_len, &is_new, make_broadcast);
 	if (info == NULL)
 		goto end;
 
 	/* only if we are server ourselves and packet is newer than the one in
 	 * hash.*/
-	if (vis_server == VIS_TYPE_SERVER_SYNC && is_new) {
-		memcpy(info->packet.target_orig, broadcastAddr, ETH_ALEN);
-		if (list_empty(&info->send_list))
-			list_add_tail(&info->send_list, &send_list);
-	}
+	if (vis_server == VIS_TYPE_SERVER_SYNC && is_new)
+		send_list_add(info);
 end:
 	spin_unlock_irqrestore(&vis_hash_lock, flags);
 }
@@ -263,31 +302,32 @@ 
 	int is_new;
 	unsigned long flags;
 	int vis_server = atomic_read(&vis_mode);
+	int are_target = 0;
 
 	/* clients shall not broadcast. */
 	if (is_bcast(vis_packet->target_orig))
 		return;
 
+	/* Are we the target for this VIS packet? */
+	if (vis_server == VIS_TYPE_SERVER_SYNC	&&
+	    is_my_mac(vis_packet->target_orig))
+		are_target = 1;
+
 	spin_lock_irqsave(&vis_hash_lock, flags);
-	info = add_packet(vis_packet, vis_info_len, &is_new);
+	info = add_packet(vis_packet, vis_info_len, &is_new, are_target);
 	if (info == NULL)
 		goto end;
 	/* note that outdated packets will be dropped at this point. */
 
 
 	/* send only if we're the target server or ... */
-	if (vis_server == VIS_TYPE_SERVER_SYNC  &&
-	    is_my_mac(info->packet.target_orig) &&
-	    is_new) {
+	if (are_target && is_new) {
 		info->packet.vis_type = VIS_TYPE_SERVER_SYNC;	/* upgrade! */
-		memcpy(info->packet.target_orig, broadcastAddr, ETH_ALEN);
-		if (list_empty(&info->send_list))
-			list_add_tail(&info->send_list, &send_list);
+		send_list_add(info);
 
 		/* ... we're not the recipient (and thus need to forward). */
 	} else if (!is_my_mac(info->packet.target_orig)) {
-		if (list_empty(&info->send_list))
-			list_add_tail(&info->send_list, &send_list);
+		send_list_add(info);
 	}
 end:
 	spin_unlock_irqrestore(&vis_hash_lock, flags);
@@ -362,14 +402,17 @@ 
 	while (hash_iterate(orig_hash, &hashit_global)) {
 		orig_node = hashit_global.bucket->data;
 		if (orig_node->router != NULL
-			&& compare_orig(orig_node->router->addr, orig_node->orig)
+			&& compare_orig(orig_node->router->addr,
+					orig_node->orig)
 			&& orig_node->batman_if
 			&& (orig_node->batman_if->if_active == IF_ACTIVE)
 		    && orig_node->router->tq_avg > 0) {
 
 			/* fill one entry into buffer. */
 			entry = &entry_array[info->packet.entries];
-			memcpy(entry->src, orig_node->batman_if->net_dev->dev_addr, ETH_ALEN);
+			memcpy(entry->src,
+			       orig_node->batman_if->net_dev->dev_addr,
+			       ETH_ALEN);
 			memcpy(entry->dest, orig_node->orig, ETH_ALEN);
 			entry->quality = orig_node->router->tq_avg;
 			info->packet.entries++;
@@ -401,6 +444,8 @@ 
 	return 0;
 }
 
+/* free old vis packets. Must be called with this vis_hash_lock
+ * held */
 static void purge_vis_packets(void)
 {
 	HASHIT(hashit);
@@ -413,7 +458,8 @@ 
 		if (time_after(jiffies,
 			       info->first_seen + (VIS_TIMEOUT*HZ)/1000)) {
 			hash_remove_bucket(vis_hash, &hashit);
-			free_info(info);
+			send_list_del(info);
+			kref_put(&info->refcount, free_info);
 		}
 	}
 }
@@ -423,6 +469,8 @@ 
 	HASHIT(hashit);
 	struct orig_node *orig_node;
 	unsigned long flags;
+	struct batman_if *batman_if;
+	uint8_t dstaddr[ETH_ALEN];
 
 	spin_lock_irqsave(&orig_hash_lock, flags);
 
@@ -431,46 +479,57 @@ 
 		orig_node = hashit.bucket->data;
 
 		/* if it's a vis server and reachable, send it. */
-		if (orig_node &&
-		    (orig_node->flags & VIS_SERVER) &&
-		    orig_node->batman_if &&
-		    orig_node->router) {
+		if ((!orig_node) || (!orig_node->batman_if) ||
+		    (!orig_node->router))
+			continue;
+		if (!(orig_node->flags & VIS_SERVER))
+			continue;
+		/* don't send it if we already received the packet from
+		 * this node. */
+		if (recv_list_is_in(&info->recv_list, orig_node->orig))
+			continue;
 
-			/* don't send it if we already received the packet from
-			 * this node. */
-			if (recv_list_is_in(&info->recv_list, orig_node->orig))
-				continue;
+		memcpy(info->packet.target_orig, orig_node->orig, ETH_ALEN);
+		batman_if = orig_node->batman_if;
+		memcpy(dstaddr, orig_node->router->addr, ETH_ALEN);
+		spin_unlock_irqrestore(&orig_hash_lock, flags);
 
-			memcpy(info->packet.target_orig,
-			       orig_node->orig, ETH_ALEN);
+		send_raw_packet((unsigned char *)&info->packet,
+				packet_length, batman_if, dstaddr);
 
-			send_raw_packet((unsigned char *) &info->packet,
-					packet_length,
-					orig_node->batman_if,
-					orig_node->router->addr);
-		}
+		spin_lock_irqsave(&orig_hash_lock, flags);
+
 	}
+	spin_unlock_irqrestore(&orig_hash_lock, flags);
 	memcpy(info->packet.target_orig, broadcastAddr, ETH_ALEN);
-	spin_unlock_irqrestore(&orig_hash_lock, flags);
 }
 
 static void unicast_vis_packet(struct vis_info *info, int packet_length)
 {
 	struct orig_node *orig_node;
 	unsigned long flags;
+	struct batman_if *batman_if;
+	uint8_t dstaddr[ETH_ALEN];
 
 	spin_lock_irqsave(&orig_hash_lock, flags);
 	orig_node = ((struct orig_node *)
 		     hash_find(orig_hash, info->packet.target_orig));
 
-	if ((orig_node != NULL) &&
-	    (orig_node->batman_if != NULL) &&
-	    (orig_node->router != NULL)) {
-		send_raw_packet((unsigned char *) &info->packet, packet_length,
-				orig_node->batman_if,
-				orig_node->router->addr);
-	}
+	if ((!orig_node) || (!orig_node->batman_if) || (!orig_node->router))
+		goto out;
+
+	/* don't lock while sending the packets ... we therefore
+	 * copy the required data before sending */
+	batman_if = orig_node->batman_if;
+	memcpy(dstaddr, orig_node->router->addr, ETH_ALEN);
 	spin_unlock_irqrestore(&orig_hash_lock, flags);
+
+	send_raw_packet((unsigned char *)&info->packet,
+			packet_length, batman_if, dstaddr);
+	return;
+
+out:
+	spin_unlock_irqrestore(&orig_hash_lock, flags);
 }
 
 /* only send one vis packet. called from send_vis_packets() */
@@ -503,15 +562,24 @@ 
 	unsigned long flags;
 
 	spin_lock_irqsave(&vis_hash_lock, flags);
+
 	purge_vis_packets();
 
-	if (generate_vis_packet() == 0)
+	if (generate_vis_packet() == 0) {
 		/* schedule if generation was successful */
-		list_add_tail(&my_vis_info->send_list, &send_list);
+		send_list_add(my_vis_info);
+	}
 
 	list_for_each_entry_safe(info, temp, &send_list, send_list) {
-		list_del_init(&info->send_list);
+
+		kref_get(&info->refcount);
+		spin_unlock_irqrestore(&vis_hash_lock, flags);
+
 		send_vis_packet(info);
+
+		spin_lock_irqsave(&vis_hash_lock, flags);
+		send_list_del(info);
+		kref_put(&info->refcount, free_info);
 	}
 	spin_unlock_irqrestore(&vis_hash_lock, flags);
 	start_vis_timer();
@@ -544,6 +612,7 @@ 
 	my_vis_info->first_seen = jiffies - atomic_read(&vis_interval);
 	INIT_LIST_HEAD(&my_vis_info->recv_list);
 	INIT_LIST_HEAD(&my_vis_info->send_list);
+	kref_init(&my_vis_info->refcount);
 	my_vis_info->packet.version = COMPAT_VERSION;
 	my_vis_info->packet.packet_type = BAT_VIS;
 	my_vis_info->packet.ttl = TTL;
@@ -557,9 +626,9 @@ 
 
 	if (hash_add(vis_hash, my_vis_info) < 0) {
 		printk(KERN_ERR
-			  "batman-adv:Can't add own vis packet into hash\n");
-		free_info(my_vis_info);	/* not in hash, need to remove it
-					 * manually. */
+		       "batman-adv:Can't add own vis packet into hash\n");
+		/* not in hash, need to remove it manually. */
+		kref_put(&my_vis_info->refcount, free_info);
 		goto err;
 	}
 
@@ -573,6 +642,15 @@ 
 	return 0;
 }
 
+/* Decrease the reference count on a hash item info */
+static void free_info_ref(void *data)
+{
+	struct vis_info *info = data;
+
+	send_list_del(info);
+	kref_put(&info->refcount, free_info);
+}
+
 /* shutdown vis-server */
 void vis_quit(void)
 {
@@ -584,7 +662,7 @@ 
 
 	spin_lock_irqsave(&vis_hash_lock, flags);
 	/* properly remove, kill timers ... */
-	hash_delete(vis_hash, free_info);
+	hash_delete(vis_hash, free_info_ref);
 	vis_hash = NULL;
 	my_vis_info = NULL;
 	spin_unlock_irqrestore(&vis_hash_lock, flags);
@@ -594,5 +672,5 @@ 
 static void start_vis_timer(void)
 {
 	queue_delayed_work(bat_event_workqueue, &vis_timer_wq,
-			   (atomic_read(&vis_interval) * HZ ) / 1000);
+			   (atomic_read(&vis_interval) * HZ) / 1000);
 }
Index: a/batman-adv-kernelland/vis.h
===================================================================
--- a/batman-adv-kernelland/vis.h	(revision 1578)
+++ a/batman-adv-kernelland/vis.h	(working copy)
@@ -29,6 +29,7 @@ 
 			    /* list of server-neighbors we received a vis-packet
 			     * from.  we should not reply to them. */
 	struct list_head send_list;
+	struct kref refcount;
 	/* this packet might be part of the vis send queue. */
 	struct vis_packet packet;
 	/* vis_info may follow here*/