Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: Fix rcu_barrier() miss due to double call_rcu() in TT code

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 3009 bytes --]

On Sun, Mar 17, 2013 at 05:44:57AM +0100, Linus Lüssing wrote:
> rcu_barrier() only waits for the currently scheduled rcu functions
> to finish - it won't wait for any function scheduled via another
> call_rcu() within an rcu scheduled function.
> 
> Unfortunately our batadv_tt_orig_list_entry_free_ref() does just that,
> via a batadv_orig_node_free_ref() call, leading to our rcu_barrier()
> call potentially missing such a batadv_orig_node_free_ref().
> 
> This patch fixes this issue by calling the batadv_orig_node_free_rcu()
> directly from the rcu callback, removing the unnecessary, additional
> call_rcu() layer here.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> 
> diff --git a/originator.c b/originator.c
> index 585e684..013c7d0 100644
> --- a/originator.c
> +++ b/originator.c
> @@ -117,7 +117,7 @@ out:
>  	return neigh_node;
>  }
>  
> -static void batadv_orig_node_free_rcu(struct rcu_head *rcu)
> +void batadv_orig_node_free_rcu(struct rcu_head *rcu)
>  {
>  	struct hlist_node *node_tmp;
>  	struct batadv_neigh_node *neigh_node, *tmp_neigh_node;
> diff --git a/originator.h b/originator.h
> index 7df48fa..4f9f88b 100644
> --- a/originator.h
> +++ b/originator.h
> @@ -25,6 +25,7 @@
>  int batadv_originator_init(struct batadv_priv *bat_priv);
>  void batadv_originator_free(struct batadv_priv *bat_priv);
>  void batadv_purge_orig_ref(struct batadv_priv *bat_priv);
> +void batadv_orig_node_free_rcu(struct rcu_head *rcu);
>  void batadv_orig_node_free_ref(struct batadv_orig_node *orig_node);
>  struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv,
>  					      const uint8_t *addr);
> diff --git a/translation-table.c b/translation-table.c
> index 9322320..ee91cc1 100644
> --- a/translation-table.c
> +++ b/translation-table.c
> @@ -144,7 +144,10 @@ static void batadv_tt_orig_list_entry_free_rcu(struct rcu_head *rcu)
>  	struct batadv_tt_orig_list_entry *orig_entry;
>  
>  	orig_entry = container_of(rcu, struct batadv_tt_orig_list_entry, rcu);
> -	batadv_orig_node_free_ref(orig_entry->orig_node);
> +
> +	if (atomic_dec_and_test(&orig_entry->orig_node->refcount))
> +		batadv_orig_node_free_rcu(&orig_entry->orig_node->rcu);
> +
>  	kfree(orig_entry);
>  }

Hi Linus,

I was just re-reading this patch: the code can be beautified later (I think it
would be worth defining a new function, e.g. batadv_orig_node_free(orig_node),
which can then be invoked both in batadv_orig_node_free_rcu() and here - in this
way we avoid to export batadv_orig_node_free_rcu() which should remain private
to the originator.c module and we avoid this forced fake rcu invocation), but I
think that putting a comment here to explain why we don't invoke call_rcu is
definitely needed. In the future somebody else may ask why we don't use it and
will try to re-add it.

The rest looks good.

Thanks!


-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]