Re: [B.A.T.M.A.N.] [PATCHv2 1/2] batman-adv: avoid race conditions on TT global table by counting references

[Antonio Quartulli <ordex@autistici.org>]

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

On Wed, Apr 17, 2013 at 09:05:29PM +0200, "Linus Lüssing" wrote:
> Hi Antonio,
> 
> Looks good, I like the idea of using refcounting for the tt global hash. That will indeed nicely remove the ordering dependancy I introduced with my patch earlier.
> 
> > @@ -844,7 +897,17 @@ int batadv_tt_global_add(struct batadv_priv *bat_priv,
> >  
> >  		if (unlikely(hash_added != 0)) {
> >  			/* remove the reference for the hash */
> > -			batadv_tt_global_entry_free_ref(tt_global_entry);
> > +			batadv_tt_global_entry_free_ref(bat_priv,
> > +							tt_global_entry);
> > +			goto out_remove;
> > +		}
> > +
> > +		/* increase the refcounter for this new "reference to the global
> > +		 * table represented by the global entry. If the increment
> > +		 * fails remove the entry from the hash
> > +		 */
> > +		if (!atomic_inc_not_zero(&bat_priv->tt.global_refcount)) {
> > +			hlist_del_rcu(&tt_global_entry->common.hash_entry);
> >  			goto out_remove;
> >  		}
> 
> Looks like there's a spin-lock missing, we usually need to do the rcu-list-adds/dels within such a lock.
> 
> Also the ordering looks a little different compared to what we usually do. Usually we increase all the refcounting first and add things to the lists in the end. Changing the order should remove the need for an hlist_del_rcu/spin-lock in the first place.

Hi Linus,

thank you very much for the review.
However, after having properly fixed the "RCU coordination" in the clean up
path, this patch is not needed anymore.

Cheers,

-- 
Antonio Quartulli

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

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