So, had a second look at the way the gw_node rcu-locking refcounting stuff is done and looks sane to me with the rcu_dereference/assign_pointer additions. Just one thing I noticed in gw_select() which probably is not an issue:
It's not harmful to hold a rcu_read_lock() while calling this as we are using call_rcu() gw_node_free_ref(), right? Do we need to increase the new_gw_node refcount? Of course we cannot call the whole gw_select() without any rcu locking due to the atomic_inc_not_zero(), but just wondering if the rcu_read_unlock() should be called earlier. So if some one can confirm that this is not an issue, then the gw_node rcu refcount patches get my ok for being applied to the trunk. Cheers, Linus On Fri, Feb 04, 2011 at 04:21:30PM +0100, Linus Lüssing wrote: > From: Sven Eckelmann <[email protected]> > > 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. > > Reported-by: Sven Eckelmann <[email protected]> > Signed-off-by: Marek Lindner <[email protected]> > --- > gateway_client.c | 41 ++++++++++++++++++----------------------- > types.h | 2 +- > 2 files changed, 19 insertions(+), 24 deletions(-) > > diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c > index 429a013..517e001 100644 > --- a/batman-adv/gateway_client.c > +++ b/batman-adv/gateway_client.c > @@ -28,20 +28,18 @@ > #include <linux/udp.h> > #include <linux/if_vlan.h> > > -static void gw_node_free_ref(struct kref *refcount) > -{ > - struct gw_node *gw_node; > - > - gw_node = container_of(refcount, struct gw_node, refcount); > - kfree(gw_node); > -} > - > static void gw_node_free_rcu(struct rcu_head *rcu) > { > struct gw_node *gw_node; > > gw_node = container_of(rcu, struct gw_node, rcu); > - kref_put(&gw_node->refcount, gw_node_free_ref); > + kfree(gw_node); > +} > + > +static void gw_node_free_ref(struct gw_node *gw_node) > +{ > + if (atomic_dec_and_test(&gw_node->refcount)) > + call_rcu(&gw_node->rcu, gw_node_free_rcu); > } > > void *gw_get_selected(struct bat_priv *bat_priv) > @@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv) > bat_priv->curr_gw = NULL; > > if (gw_node) > - kref_put(&gw_node->refcount, gw_node_free_ref); > + gw_node_free_ref(gw_node); > } > > -static struct gw_node *gw_select(struct bat_priv *bat_priv, > - struct gw_node *new_gw_node) > +static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node) > { > struct gw_node *curr_gw_node = bat_priv->curr_gw; > > - if (new_gw_node) > - kref_get(&new_gw_node->refcount); > + if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount)) > + new_gw_node = NULL; > > bat_priv->curr_gw = new_gw_node; > - return curr_gw_node; > + > + if (curr_gw_node) > + gw_node_free_ref(curr_gw_node); > } > > void gw_election(struct bat_priv *bat_priv) > { > struct hlist_node *node; > - struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL; > + struct gw_node *gw_node, *curr_gw_tmp = NULL; > uint8_t max_tq = 0; > uint32_t max_gw_factor = 0, tmp_gw_factor = 0; > int down, up; > @@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv) > curr_gw_tmp->orig_node->gw_flags, > curr_gw_tmp->orig_node->router->tq_avg); > > - old_gw_node = gw_select(bat_priv, curr_gw_tmp); > + gw_select(bat_priv, curr_gw_tmp); > } > > rcu_read_unlock(); > - > - /* the kfree() has to be outside of the rcu lock */ > - if (old_gw_node) > - kref_put(&old_gw_node->refcount, gw_node_free_ref); > } > > void gw_check_election(struct bat_priv *bat_priv, struct orig_node > *orig_node) > @@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv, > memset(gw_node, 0, sizeof(struct gw_node)); > INIT_HLIST_NODE(&gw_node->list); > gw_node->orig_node = orig_node; > - kref_init(&gw_node->refcount); > + atomic_set(&gw_node->refcount, 1); > > spin_lock_bh(&bat_priv->gw_list_lock); > hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list); > @@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv) > gw_deselect(bat_priv); > > hlist_del_rcu(&gw_node->list); > - call_rcu(&gw_node->rcu, gw_node_free_rcu); > + gw_node_free_ref(gw_node); > } > > > diff --git a/batman-adv/types.h b/batman-adv/types.h > index e4a0462..ca5f20a 100644 > --- a/batman-adv/types.h > +++ b/batman-adv/types.h > @@ -100,7 +100,7 @@ struct gw_node { > struct hlist_node list; > struct orig_node *orig_node; > unsigned long deleted; > - struct kref refcount; > + atomic_t refcount; > struct rcu_head rcu; > }; > > -- > 1.7.2.3 >
