Skip to contentSkip to conversation

reordered rcu (part II)

B.A.T.M.A.N. In review Marek Lindner 2 participants 9 messages Feb 9, 2011 – Feb 18, 2011

lore
Share & tools
lore
#0 lore raw

Hi,

here comes the next batch of changes to make the code "rcu safe". It won't be the last one but should make all follow-ups much easier.

Regards,
Marek

#1 lore raw

PATCH 1/2 batman-adv: separate ethernet comparing calls from hash functions

Note: The function compare_ether_addr() provided by the Linux kernel requires aligned memory.

---
 batman-adv/hard-interface.c    |    2 +-
 batman-adv/main.c              |    2 +-
 batman-adv/main.h              |   10 +++++++
 batman-adv/routing.c           |   55 +++++++++++++++++++--------------------
 batman-adv/send.c              |    2 +-
 batman-adv/soft-interface.c    |    2 +-
 batman-adv/translation-table.c |    2 +-
 batman-adv/vis.c               |   18 ++++++------
 8 files changed, 51 insertions(+), 42 deletions(-)
batman-adv/hard-interface.c +1 -1
index 8982485..de9bd36 100644
@@ -187,7 +187,7 @@ static void check_known_mac_addr(struct net_device *net_dev)
 		if (batman_if->net_dev == net_dev)
 			continue;
 
-		if (!compare_orig(batman_if->net_dev->dev_addr,
+		if (!compare_eth(batman_if->net_dev->dev_addr,
 				  net_dev->dev_addr))
 			continue;
 
batman-adv/main.c +1 -1
index 658ad5a..09c21f2 100644
@@ -160,7 +160,7 @@ int is_my_mac(uint8_t *addr)
 		if (batman_if->if_status != IF_ACTIVE)
 			continue;
 
-		if (compare_orig(batman_if->net_dev->dev_addr, addr)) {
+		if (compare_eth(batman_if->net_dev->dev_addr, addr)) {
 			rcu_read_unlock();
 			return 1;
 		}
batman-adv/main.h +10 -0
index bd8470d..57f0d24 100644
@@ -167,4 +167,14 @@ static inline void bat_dbg(char type __always_unused,
 		pr_err("%s: " fmt, _netdev->name, ## arg);		\
 	} while (0)
 
+/**
+ * returns 1 if they are the same ethernet addr
+ *
+ * note: can't use compare_ether_addr() as it requires aligned memory
+ */
+static inline int compare_eth(void *data1, void *data2)
+{
+	return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0);
+}
+
 #endif /* _NET_BATMAN_ADV_MAIN_H_ */
batman-adv/routing.c +27 -28

132 lines not on this page · raw message

batman-adv/send.c +1 -1
index 8314276..f0232ad 100644
@@ -326,7 +326,7 @@ void schedule_forward_packet(struct orig_node *orig_node,
 	if ((orig_node->router) && (orig_node->router->tq_avg != 0)) {
 
 		/* rebroadcast ogm of best ranking neighbor as is */
-		if (!compare_orig(orig_node->router->addr, ethhdr->h_source)) {
+		if (!compare_eth(orig_node->router->addr, ethhdr->h_source)) {
 			batman_packet->tq = orig_node->router->tq_avg;
 
 			if (orig_node->router->last_ttl)
batman-adv/soft-interface.c +1 -1
index bd8b539..41f9f57 100644
@@ -134,7 +134,7 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
 	rcu_read_lock();
 	hlist_for_each_entry_rcu(softif_neigh, node,
 				 &bat_priv->softif_neigh_list, list) {
-		if (memcmp(softif_neigh->addr, addr, ETH_ALEN) != 0)
+		if (!compare_eth(softif_neigh->addr, addr))
 			continue;
 
 		if (softif_neigh->vid != vid)
batman-adv/translation-table.c +1 -1
index 28a212a..b323c24 100644
@@ -101,7 +101,7 @@ void hna_local_add(struct net_device *soft_iface, uint8_t *addr)
 	hna_local_entry->last_seen = jiffies;
 
 	/* the batman interface mac address should never be purged */
-	if (compare_orig(addr, soft_iface->dev_addr))
+	if (compare_eth(addr, soft_iface->dev_addr))
 		hna_local_entry->never_purge = 1;
 	else
 		hna_local_entry->never_purge = 0;
batman-adv/vis.c +9 -9
index c1c3258..6435886 100644
@@ -76,7 +76,7 @@ static int vis_info_cmp(void *data1, void *data2)
 	d2 = data2;
 	p1 = (struct vis_packet *)d1->skb_packet->data;
 	p2 = (struct vis_packet *)d2->skb_packet->data;
-	return compare_orig(p1->vis_orig, p2->vis_orig);
+	return compare_eth(p1->vis_orig, p2->vis_orig);
 }
 
 /* hash function to choose an entry in a hash table of given size */
@@ -114,7 +114,7 @@ static void vis_data_insert_interface(const uint8_t *interface,
 	struct hlist_node *pos;
 
 	hlist_for_each_entry(entry, pos, if_list, list) {
-		if (compare_orig(entry->addr, (void *)interface))
+		if (compare_eth(entry->addr, (void *)interface))
 			return;
 	}
 
@@ -166,7 +166,7 @@ static ssize_t vis_data_read_entry(char *buff, struct vis_info_entry *entry,
 	/* maximal length: max(4+17+2, 3+17+1+3+2) == 26 */
 	if (primary && entry->quality == 0)
 		return sprintf(buff, "HNA %pM, ", entry->dest);
-	else if (compare_orig(entry->src, src))
+	else if (compare_eth(entry->src, src))
 		return sprintf(buff, "TQ %pM %d, ", entry->dest,
 			       entry->quality);
 
@@ -213,7 +213,7 @@ int vis_seq_print_text(struct seq_file *seq, void *offset)
 				if (entries[j].quality == 0)
 					continue;
 				compare =
-				 compare_orig(entries[j].src, packet->vis_orig);
+				 compare_eth(entries[j].src, packet->vis_orig);
 				vis_data_insert_interface(entries[j].src,
 							  &vis_if_list,
 							  compare);
@@ -223,7 +223,7 @@ int vis_seq_print_text(struct seq_file *seq, void *offset)
 				buf_size += 18 + 26 * packet->entries;
 
 				/* add primary/secondary records */
-				if (compare_orig(entry->addr, packet->vis_orig))
+				if (compare_eth(entry->addr, packet->vis_orig))
 					buf_size +=
 					  vis_data_count_prim_sec(&vis_if_list);
 
@@ -259,7 +259,7 @@ int vis_seq_print_text(struct seq_file *seq, void *offset)
 				if (entries[j].quality == 0)
 					continue;
 				compare =
-				 compare_orig(entries[j].src, packet->vis_orig);
+				 compare_eth(entries[j].src, packet->vis_orig);
 				vis_data_insert_interface(entries[j].src,
 							  &vis_if_list,
 							  compare);
@@ -277,7 +277,7 @@ int vis_seq_print_text(struct seq_file *seq, void *offset)
 							entry->primary);
 
 				/* add primary/secondary records */
-				if (compare_orig(entry->addr, packet->vis_orig))
+				if (compare_eth(entry->addr, packet->vis_orig))
 					buff_pos +=
 					 vis_data_read_prim_sec(buff + buff_pos,
 								&vis_if_list);
@@ -345,7 +345,7 @@ static int recv_list_is_in(struct bat_priv *bat_priv,
 
 	spin_lock_bh(&bat_priv->vis_list_lock);
 	list_for_each_entry(entry, recv_list, list) {
-		if (memcmp(entry->mac, mac, ETH_ALEN) == 0) {
+		if (compare_eth(entry->mac, mac)) {
 			spin_unlock_bh(&bat_priv->vis_list_lock);
 			return 1;
 		}
@@ -615,7 +615,7 @@ static int generate_vis_packet(struct bat_priv *bat_priv)
 			if (!neigh_node)
 				continue;
 
-			if (!compare_orig(neigh_node->addr, orig_node->orig))
+			if (!compare_eth(neigh_node->addr, orig_node->orig))
 				continue;
 
 			if (neigh_node->if_incoming->if_status != IF_ACTIVE)
Signature · 1 line
1.7.2.3
#2 lore raw

PATCH 2/2 batman-adv: remove extra layer between hash and hash element - hash bucket

In reply to Marek Lindner
---
 batman-adv/compat.h            |    8 ++
 batman-adv/hash.c              |    8 --
 batman-adv/hash.h              |   95 +++++--------------
 batman-adv/icmp_socket.c       |    5 +-
 batman-adv/originator.c        |   71 ++++----------
 batman-adv/originator.h        |   34 +++++++-
 batman-adv/routing.c           |   41 +++-----
 batman-adv/translation-table.c |  208 +++++++++++++++++++++++++--------------
 batman-adv/types.h             |    4 +
 batman-adv/unicast.c           |   12 +--
 batman-adv/vis.c               |  102 +++++++++++---------
 11 files changed, 296 insertions(+), 292 deletions(-)
batman-adv/compat.h +8 -0
index a76d0be..29a3203 100644
@@ -270,4 +270,12 @@ int bat_seq_printf(struct seq_file *m, const char *f, ...);
 
 #endif /* < KERNEL_VERSION(2, 6, 33) */
 
+#define hlist_first_rcu(head) (*((struct hlist_node **)(&(head)->first)))
+#define hlist_next_rcu(node) (*((struct hlist_node **)(&(node)->next)))
+
+#define __hlist_for_each_rcu(pos, head) \
+	for (pos = rcu_dereference(hlist_first_rcu(head)); \
+	pos && ({ prefetch(pos->next); 1; }); \
+	pos = rcu_dereference(hlist_next_rcu(pos)))
+
 #endif /* _NET_BATMAN_ADV_COMPAT_H_ */
batman-adv/hash.c +0 -8
index 0265366..c5213d8 100644
@@ -68,11 +68,3 @@ free_hash:
 	kfree(hash);
 	return NULL;
 }
-
-void bucket_free_rcu(struct rcu_head *rcu)
-{
-	struct element_t *bucket;
-
-	bucket = container_of(rcu, struct element_t, rcu);
-	kfree(bucket);
-}
batman-adv/hash.h +24 -71

161 lines not on this page · raw message

batman-adv/icmp_socket.c +1 -4
index 9f150bf..7159c81 100644
@@ -219,14 +219,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
 		goto dst_unreach;
 
 	rcu_read_lock();
-	orig_node = ((struct orig_node *)hash_find(bat_priv->orig_hash,
-						   compare_orig, choose_orig,
-						   icmp_packet->dst));
+	orig_node = orig_hash_find(bat_priv->orig_hash, icmp_packet->dst);
 
 	if (!orig_node)
 		goto unlock;
 
-	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
 	if (!neigh_node)
batman-adv/originator.c +19 -52

165 lines not on this page · raw message

batman-adv/originator.h +33 -1
index 84d96e2..dc087fd 100644
@@ -22,6 +22,8 @@
 #ifndef _NET_BATMAN_ADV_ORIGINATOR_H_
 #define _NET_BATMAN_ADV_ORIGINATOR_H_
 
+#include "hash.h"
+
 int originator_init(struct bat_priv *bat_priv);
 void originator_free(struct bat_priv *bat_priv);
 void purge_orig_ref(struct bat_priv *bat_priv);
@@ -38,8 +40,10 @@ int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
 
 
 /* returns 1 if they are the same originator */
-static inline int compare_orig(void *data1, void *data2)
+static inline int compare_orig(struct hlist_node *node, void *data2)
 {
+	void *data1 = container_of(node, struct orig_node, hash_entry);
+
 	return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0);
 }
 
@@ -64,4 +68,32 @@ static inline int choose_orig(void *data, int32_t size)
 	return hash % size;
 }
 
+static inline struct orig_node *orig_hash_find(struct hashtable_t *hash,
+					       void *data)
+{
+	int index;
+	struct hlist_head *head;
+	struct hlist_node *node;
+	struct orig_node *orig_node, *orig_node_tmp = NULL;
+
+	if (!hash)
+		return NULL;
+
+	index = choose_orig(data, hash->size);
+	head = &hash->table[index];
+
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(orig_node, node, head, hash_entry) {
+		if (!compare_eth(orig_node, data))
+			continue;
+
+		orig_node_tmp = orig_node;
+		kref_get(&orig_node_tmp->refcount);
+		break;
+	}
+	rcu_read_unlock();
+
+	return orig_node_tmp;
+}
+
 #endif /* _NET_BATMAN_ADV_ORIGINATOR_H_ */
batman-adv/routing.c +14 -27

109 lines not on this page · raw message

batman-adv/translation-table.c +133 -75

403 lines not on this page · raw message

batman-adv/types.h +4 -0
index ee77d48..171b780 100644
@@ -85,6 +85,7 @@ struct orig_node {
 	struct list_head frag_list;
 	spinlock_t neigh_list_lock; /* protects neighbor list */
 	struct kref refcount;
+	struct hlist_node hash_entry;
 	struct bat_priv *bat_priv;
 	unsigned long last_frag_packet;
 	spinlock_t ogm_cnt_lock; /* protects: bcast_own, bcast_own_sum,
@@ -193,11 +194,13 @@ struct hna_local_entry {
 	uint8_t addr[ETH_ALEN];
 	unsigned long last_seen;
 	char never_purge;
+	struct hlist_node hash_entry;
 };
 
 struct hna_global_entry {
 	uint8_t addr[ETH_ALEN];
 	struct orig_node *orig_node;
+	struct hlist_node hash_entry;
 };
 
 /**
@@ -247,6 +250,7 @@ struct vis_info {
 			     * from.  we should not reply to them. */
 	struct list_head send_list;
 	struct kref refcount;
+	struct hlist_node hash_entry;
 	struct bat_priv *bat_priv;
 	/* this packet might be part of the vis send queue. */
 	struct sk_buff *skb_packet;
batman-adv/unicast.c +2 -10
index ddd6852..c4616d2 100644
@@ -184,15 +184,9 @@ int frag_reassemble_skb(struct sk_buff *skb, struct bat_priv *bat_priv,
 
 	*new_skb = NULL;
 
-	rcu_read_lock();
-	orig_node = ((struct orig_node *)
-		    hash_find(bat_priv->orig_hash, compare_orig, choose_orig,
-			      unicast_packet->orig));
+	orig_node = orig_hash_find(bat_priv->orig_hash, unicast_packet->orig);
 	if (!orig_node)
-		goto unlock;
-
-	kref_get(&orig_node->refcount);
-	rcu_read_unlock();
+		goto out;
 
 	orig_node->last_frag_packet = jiffies;
 
@@ -219,8 +213,6 @@ int frag_reassemble_skb(struct sk_buff *skb, struct bat_priv *bat_priv,
 
 	goto out;
 
-unlock:
-	rcu_read_unlock();
 out:
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
batman-adv/vis.c +58 -44

248 lines not on this page · raw message

Signature · 1 line
1.7.2.3
#3 lore raw

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: remove extra layer between hash and hash element - hash bucket

Quoted 7 lines
batman-adv/compat.h +0 -0
index a76d0be..29a3203 100644
@@ -270,4 +270,12 @@ int bat_seq_printf(struct seq_file *m, const char *f, ...);
 
 #endif /* < KERNEL_VERSION(2, 6, 33) */

batman-adv/compat.h int bat_seq_printf(struct seq_file *m, const char *f, ...); · line 281

Quoted 10 lines
batman-adv/compat.h +8 -0
 
+#define hlist_first_rcu(head) (*((struct hlist_node **)(&(head)->first)))
+#define hlist_next_rcu(node) (*((struct hlist_node **)(&(node)->next)))
+
+#define __hlist_for_each_rcu(pos, head) \
+	for (pos = rcu_dereference(hlist_first_rcu(head)); \
+	pos && ({ prefetch(pos->next); 1; }); \
+	pos = rcu_dereference(hlist_next_rcu(pos)))
+
 #endif /* _NET_BATMAN_ADV_COMPAT_H_ */

Isn't there a #define with < KERNEL_VERSION(2, 6, 37) missing? (Haven't checked much else about this patch yet)

Cheers, Linus

#4 lore raw

Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: remove extra layer between hash and hash element - hash bucket

Quoted 2 lines · On Thursday 10 February 2011 14:03:03 Linus Lüssing wrote:

Isn't there a #define with < KERNEL_VERSION(2, 6, 37) missing? (Haven't checked much else about this patch yet)

Why would there be a need for .37 (this functionality came with .34) ? Nevertheless, you are right - it is missing here.

Regards,
Marek

#5 lore raw

PATCH v2 2/4 batman-adv: remove extra layer between hash and hash element - hash bucket

In reply to Marek Lindner

---
Rebase on trunk, checking kernel version in compat.h and minor hash_find() API change.

 batman-adv/compat.h            |   12 +++
 batman-adv/hash.c              |    8 --
 batman-adv/hash.h              |   95 +++++--------------
 batman-adv/icmp_socket.c       |    5 +-
 batman-adv/originator.c        |   71 ++++----------
 batman-adv/originator.h        |   35 +++++++-
 batman-adv/routing.c           |   41 +++-----
 batman-adv/translation-table.c |  204 +++++++++++++++++++++++++---------------
 batman-adv/types.h             |    4 +
 batman-adv/unicast.c           |   12 +--
 batman-adv/vis.c               |  103 ++++++++++++---------
 11 files changed, 298 insertions(+), 292 deletions(-)
batman-adv/compat.h +12 -0
index 4e89049..0c5ad82 100644
@@ -270,6 +270,18 @@ int bat_seq_printf(struct seq_file *m, const char *f, ...);
 
 #endif /* < KERNEL_VERSION(2, 6, 33) */
 
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 34)
+
+#define hlist_first_rcu(head) (*((struct hlist_node **)(&(head)->first)))
+#define hlist_next_rcu(node) (*((struct hlist_node **)(&(node)->next)))
+
+#define __hlist_for_each_rcu(pos, head) \
+	for (pos = rcu_dereference(hlist_first_rcu(head)); \
+	pos && ({ prefetch(pos->next); 1; }); \
+	pos = rcu_dereference(hlist_next_rcu(pos)))
+
+#endif /* < KERNEL_VERSION(2, 6, 34) */
+
 #if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 36)
 
 #define __rcu
batman-adv/hash.c +0 -8
index 0265366..c5213d8 100644
@@ -68,11 +68,3 @@ free_hash:
 	kfree(hash);
 	return NULL;
 }
-
-void bucket_free_rcu(struct rcu_head *rcu)
-{
-	struct element_t *bucket;
-
-	bucket = container_of(rcu, struct element_t, rcu);
-	kfree(bucket);
-}
batman-adv/hash.h +24 -71

161 lines not on this page · raw message

batman-adv/icmp_socket.c +1 -4
index 9f150bf..17792f3 100644
@@ -219,14 +219,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
 		goto dst_unreach;
 
 	rcu_read_lock();
-	orig_node = ((struct orig_node *)hash_find(bat_priv->orig_hash,
-						   compare_orig, choose_orig,
-						   icmp_packet->dst));
+	orig_node = orig_hash_find(bat_priv, icmp_packet->dst);
 
 	if (!orig_node)
 		goto unlock;
 
-	kref_get(&orig_node->refcount);
 	neigh_node = orig_node->router;
 
 	if (!neigh_node)
batman-adv/originator.c +19 -52

165 lines not on this page · raw message

batman-adv/originator.h +34 -1
index 84d96e2..b4b9a09 100644
@@ -22,6 +22,8 @@
 #ifndef _NET_BATMAN_ADV_ORIGINATOR_H_
 #define _NET_BATMAN_ADV_ORIGINATOR_H_
 
+#include "hash.h"
+
 int originator_init(struct bat_priv *bat_priv);
 void originator_free(struct bat_priv *bat_priv);
 void purge_orig_ref(struct bat_priv *bat_priv);
@@ -38,8 +40,10 @@ int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
 
 
 /* returns 1 if they are the same originator */
-static inline int compare_orig(void *data1, void *data2)
+static inline int compare_orig(struct hlist_node *node, void *data2)
 {
+	void *data1 = container_of(node, struct orig_node, hash_entry);
+
 	return (memcmp(data1, data2, ETH_ALEN) == 0 ? 1 : 0);
 }
 
@@ -64,4 +68,33 @@ static inline int choose_orig(void *data, int32_t size)
 	return hash % size;
 }
 
+static inline struct orig_node *orig_hash_find(struct bat_priv *bat_priv,
+					       void *data)
+{
+	struct hashtable_t *hash = bat_priv->orig_hash;
+	struct hlist_head *head;
+	struct hlist_node *node;
+	struct orig_node *orig_node, *orig_node_tmp = NULL;
+	int index;
+
+	if (!hash)
+		return NULL;
+
+	index = choose_orig(data, hash->size);
+	head = &hash->table[index];
+
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(orig_node, node, head, hash_entry) {
+		if (!compare_eth(orig_node, data))
+			continue;
+
+		orig_node_tmp = orig_node;
+		kref_get(&orig_node_tmp->refcount);
+		break;
+	}
+	rcu_read_unlock();
+
+	return orig_node_tmp;
+}
+
 #endif /* _NET_BATMAN_ADV_ORIGINATOR_H_ */
batman-adv/routing.c +14 -27

109 lines not on this page · raw message

batman-adv/translation-table.c +129 -75

399 lines not on this page · raw message

batman-adv/types.h +4 -0
index b6acb12..cc10843 100644
@@ -85,6 +85,7 @@ struct orig_node {
 	struct list_head frag_list;
 	spinlock_t neigh_list_lock; /* protects neighbor list */
 	struct kref refcount;
+	struct hlist_node hash_entry;
 	struct bat_priv *bat_priv;
 	unsigned long last_frag_packet;
 	spinlock_t ogm_cnt_lock; /* protects: bcast_own, bcast_own_sum,
@@ -193,11 +194,13 @@ struct hna_local_entry {
 	uint8_t addr[ETH_ALEN];
 	unsigned long last_seen;
 	char never_purge;
+	struct hlist_node hash_entry;
 };
 
 struct hna_global_entry {
 	uint8_t addr[ETH_ALEN];
 	struct orig_node *orig_node;
+	struct hlist_node hash_entry;
 };
 
 /**
@@ -247,6 +250,7 @@ struct vis_info {
 			     * from.  we should not reply to them. */
 	struct list_head send_list;
 	struct kref refcount;
+	struct hlist_node hash_entry;
 	struct bat_priv *bat_priv;
 	/* this packet might be part of the vis send queue. */
 	struct sk_buff *skb_packet;
batman-adv/unicast.c +2 -10
index a7aacad..af81008 100644
@@ -184,15 +184,9 @@ int frag_reassemble_skb(struct sk_buff *skb, struct bat_priv *bat_priv,
 
 	*new_skb = NULL;
 
-	rcu_read_lock();
-	orig_node = ((struct orig_node *)
-		    hash_find(bat_priv->orig_hash, compare_orig, choose_orig,
-			      unicast_packet->orig));
+	orig_node = orig_hash_find(bat_priv, unicast_packet->orig);
 	if (!orig_node)
-		goto unlock;
-
-	kref_get(&orig_node->refcount);
-	rcu_read_unlock();
+		goto out;
 
 	orig_node->last_frag_packet = jiffies;
 
@@ -219,8 +213,6 @@ int frag_reassemble_skb(struct sk_buff *skb, struct bat_priv *bat_priv,
 
 	goto out;
 
-unlock:
-	rcu_read_unlock();
 out:
 	if (orig_node)
 		kref_put(&orig_node->refcount, orig_node_free_ref);
batman-adv/vis.c +59 -44

249 lines not on this page · raw message

Signature · 1 line
1.7.2.3
#6 lore raw

PATCH 3/4 batman-adv: Correct rcu refcounting for orig_node

In reply to Marek Lindner

It might be possible that 2 threads access the same data in the same rcu grace period. The first thread calls call_rcu() to decrement the refcount and free the data while the second thread increases the refcount to use the data. To avoid this race condition all refcount operations have to be atomic.

---
 batman-adv/gateway_client.c    |    6 ++++--
 batman-adv/icmp_socket.c       |    2 +-
 batman-adv/originator.c        |   18 ++++++++++++------
 batman-adv/originator.h        |    6 ++++--
 batman-adv/routing.c           |   22 +++++++++++-----------
 batman-adv/translation-table.c |   15 +++++++++------
 batman-adv/types.h             |    3 ++-
 batman-adv/unicast.c           |    6 ++----
 batman-adv/vis.c               |    2 +-
 9 files changed, 46 insertions(+), 34 deletions(-)
batman-adv/gateway_client.c +4 -2
index 3486854..ba6e4f2 100644
@@ -53,9 +53,11 @@ void *gw_get_selected(struct bat_priv *bat_priv)
 		goto out;
 
 	orig_node = curr_gateway_tmp->orig_node;
+	if (!orig_node)
+		goto out;
 
-	if (orig_node)
-		kref_get(&orig_node->refcount);
+	if (!atomic_inc_not_zero(&orig_node->refcount))
+		orig_node = NULL;
 
 out:
 	rcu_read_unlock();
batman-adv/icmp_socket.c +1 -1
index 17792f3..34ce56c 100644
@@ -263,7 +263,7 @@ out:
 	if (neigh_node)
 		neigh_node_free_ref(neigh_node);
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	return len;
 }
 
batman-adv/originator.c +12 -6
index c9ba013..f6be7a1 100644
@@ -99,13 +99,13 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
 	return neigh_node;
 }
 
-void orig_node_free_ref(struct kref *refcount)
+static void orig_node_free_rcu(struct rcu_head *rcu)
 {
 	struct hlist_node *node, *node_tmp;
 	struct neigh_node *neigh_node, *tmp_neigh_node;
 	struct orig_node *orig_node;
 
-	orig_node = container_of(refcount, struct orig_node, refcount);
+	orig_node = container_of(rcu, struct orig_node, rcu);
 
 	spin_lock_bh(&orig_node->neigh_list_lock);
 
@@ -134,6 +134,12 @@ void orig_node_free_ref(struct kref *refcount)
 	kfree(orig_node);
 }
 
+void orig_node_free_ref(struct orig_node *orig_node)
+{
+	if (atomic_dec_and_test(&orig_node->refcount))
+		call_rcu(&orig_node->rcu, orig_node_free_rcu);
+}
+
 void originator_free(struct bat_priv *bat_priv)
 {
 	struct hashtable_t *hash = bat_priv->orig_hash;
@@ -159,7 +165,7 @@ void originator_free(struct bat_priv *bat_priv)
 					  head, hash_entry) {
 
 			hlist_del_rcu(node);
-			kref_put(&orig_node->refcount, orig_node_free_ref);
+			orig_node_free_ref(orig_node);
 		}
 		spin_unlock_bh(list_lock);
 	}
@@ -191,7 +197,9 @@ struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr)
 	spin_lock_init(&orig_node->ogm_cnt_lock);
 	spin_lock_init(&orig_node->bcast_seqno_lock);
 	spin_lock_init(&orig_node->neigh_list_lock);
-	kref_init(&orig_node->refcount);
+
+	/* extra reference for return */
+	atomic_set(&orig_node->refcount, 2);
 
 	orig_node->bat_priv = bat_priv;
 	memcpy(orig_node->orig, addr, ETH_ALEN);
@@ -224,8 +232,6 @@ struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr)
 	if (hash_added < 0)
 		goto free_bcast_own_sum;
 
-	/* extra reference for return */
-	kref_get(&orig_node->refcount);
 	return orig_node;
 free_bcast_own_sum:
 	kfree(orig_node->bcast_own_sum);
batman-adv/originator.h +4 -2
index b4b9a09..3d7a39d 100644
@@ -27,7 +27,7 @@
 int originator_init(struct bat_priv *bat_priv);
 void originator_free(struct bat_priv *bat_priv);
 void purge_orig_ref(struct bat_priv *bat_priv);
-void orig_node_free_ref(struct kref *refcount);
+void orig_node_free_ref(struct orig_node *orig_node);
 struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
 struct neigh_node *create_neighbor(struct orig_node *orig_node,
 				   struct orig_node *orig_neigh_node,
@@ -88,8 +88,10 @@ static inline struct orig_node *orig_hash_find(struct bat_priv *bat_priv,
 		if (!compare_eth(orig_node, data))
 			continue;
 
+		if (!atomic_inc_not_zero(&orig_node->refcount))
+			continue;
+
 		orig_node_tmp = orig_node;
-		kref_get(&orig_node_tmp->refcount);
 		break;
 	}
 	rcu_read_unlock();
batman-adv/routing.c +11 -11
index d381cbc..dc24871 100644
@@ -416,7 +416,7 @@ static void update_orig(struct bat_priv *bat_priv,
 		neigh_node = create_neighbor(orig_node, orig_tmp,
 					     ethhdr->h_source, if_incoming);
 
-		kref_put(&orig_tmp->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_tmp);
 		if (!neigh_node)
 			goto unlock;
 
@@ -600,7 +600,7 @@ static char count_real_packets(struct ethhdr *ethhdr,
 
 out:
 	spin_unlock_bh(&orig_node->ogm_cnt_lock);
-	kref_put(&orig_node->refcount, orig_node_free_ref);
+	orig_node_free_ref(orig_node);
 	return ret;
 }
 
@@ -726,7 +726,7 @@ void receive_bat_packet(struct ethhdr *ethhdr,
 
 		bat_dbg(DBG_BATMAN, bat_priv, "Drop packet: "
 			"originator packet from myself (via neighbor)\n");
-		kref_put(&orig_neigh_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_neigh_node);
 		return;
 	}
 
@@ -832,9 +832,9 @@ void receive_bat_packet(struct ethhdr *ethhdr,
 
 out_neigh:
 	if ((orig_neigh_node) && (!is_single_hop_neigh))
-		kref_put(&orig_neigh_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_neigh_node);
 out:
-	kref_put(&orig_node->refcount, orig_node_free_ref);
+	orig_node_free_ref(orig_node);
 }
 
 int recv_bat_packet(struct sk_buff *skb, struct batman_if *batman_if)
@@ -935,7 +935,7 @@ out:
 	if (neigh_node)
 		neigh_node_free_ref(neigh_node);
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	return ret;
 }
 
@@ -1001,7 +1001,7 @@ out:
 	if (neigh_node)
 		neigh_node_free_ref(neigh_node);
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	return ret;
 }
 
@@ -1097,7 +1097,7 @@ out:
 	if (neigh_node)
 		neigh_node_free_ref(neigh_node);
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	return ret;
 }
 
@@ -1152,7 +1152,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
 		if (!primary_orig_node)
 			goto return_router;
 
-		kref_put(&primary_orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(primary_orig_node);
 	}
 
 	/* with less than 2 candidates, we can't do any
@@ -1354,7 +1354,7 @@ out:
 	if (neigh_node)
 		neigh_node_free_ref(neigh_node);
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	return ret;
 }
 
@@ -1492,7 +1492,7 @@ spin_unlock:
 	spin_unlock_bh(&orig_node->bcast_seqno_lock);
 out:
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	return ret;
 }
 
batman-adv/translation-table.c +9 -6
index cd8a583..8d15b48 100644
@@ -589,17 +589,20 @@ void hna_global_free(struct bat_priv *bat_priv)
 struct orig_node *transtable_search(struct bat_priv *bat_priv, uint8_t *addr)
 {
 	struct hna_global_entry *hna_global_entry;
+	struct orig_node *orig_node = NULL;
 
 	spin_lock_bh(&bat_priv->hna_ghash_lock);
 	hna_global_entry = hna_global_hash_find(bat_priv, addr);
 
-	if (hna_global_entry)
-		kref_get(&hna_global_entry->orig_node->refcount);
+	if (!hna_global_entry)
+		goto out;
 
-	spin_unlock_bh(&bat_priv->hna_ghash_lock);
+	if (!atomic_inc_not_zero(&hna_global_entry->orig_node->refcount))
+		goto out;
 
-	if (!hna_global_entry)
-		return NULL;
+	orig_node = hna_global_entry->orig_node;
 
-	return hna_global_entry->orig_node;
+out:
+	spin_unlock_bh(&bat_priv->hna_ghash_lock);
+	return orig_node;
 }
batman-adv/types.h +2 -1
index cc10843..a9bf186 100644
@@ -84,7 +84,8 @@ struct orig_node {
 	struct hlist_head neigh_list;
 	struct list_head frag_list;
 	spinlock_t neigh_list_lock; /* protects neighbor list */
-	struct kref refcount;
+	atomic_t refcount;
+	struct rcu_head rcu;
 	struct hlist_node hash_entry;
 	struct bat_priv *bat_priv;
 	unsigned long last_frag_packet;
batman-adv/unicast.c +2 -4
index af81008..6bfb573 100644
@@ -211,11 +211,9 @@ int frag_reassemble_skb(struct sk_buff *skb, struct bat_priv *bat_priv,
 	if (*new_skb)
 		ret = NET_RX_SUCCESS;
 
-	goto out;
-
 out:
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	return ret;
 }
 
@@ -344,7 +342,7 @@ out:
 	if (neigh_node)
 		neigh_node_free_ref(neigh_node);
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	if (ret == 1)
 		kfree_skb(skb);
 	return ret;
batman-adv/vis.c +1 -1
index 2e8fc4d..074feba 100644
@@ -809,7 +809,7 @@ out:
 	if (neigh_node)
 		neigh_node_free_ref(neigh_node);
 	if (orig_node)
-		kref_put(&orig_node->refcount, orig_node_free_ref);
+		orig_node_free_ref(orig_node);
 	return;
 }
 
Signature · 1 line
1.7.2.3
#7 lore raw

PATCH 4/4 batman-adv: increase refcount in create_neighbor to be consistent

In reply to Marek Lindner
---
 batman-adv/originator.c |    4 ++-
 batman-adv/routing.c    |   63 ++++++++++++++++++++--------------------------
 2 files changed, 30 insertions(+), 37 deletions(-)
batman-adv/originator.c +3 -1
index f6be7a1..c2017b2 100644
@@ -91,7 +91,9 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
 	memcpy(neigh_node->addr, neigh, ETH_ALEN);
 	neigh_node->orig_node = orig_neigh_node;
 	neigh_node->if_incoming = if_incoming;
-	atomic_set(&neigh_node->refcount, 1);
+
+	/* extra reference for return */
+	atomic_set(&neigh_node->refcount, 2);
 
 	spin_lock_bh(&orig_node->neigh_list_lock);
 	hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
batman-adv/routing.c +27 -36

109 lines not on this page · raw message

Signature · 1 line
1.7.2.3
author Applied
#8 lore raw
In reply to Marek Lindner
Quoted 2 lines · On Wednesday 09 February 2011 19:18:41 Marek Lindner wrote:

here comes the next batch of changes to make the code "rcu safe". It won't be the last one but should make all follow-ups much easier.

Applied in revision 1947-1950.

Regards,
Marek