Re: [B.A.T.M.A.N.] [PATCHv2 2/8] batman-adv: split tq information in neigh_node struct

[Simon Wunderlich <sw@simonwunderlich.de>]

Hey Linus,

Thanks for the comments! I applied your proposed changes/fixes and will include 
them in the next patchset, a few comments below:

> > @@ -1171,15 +1232,19 @@ batadv_iv_ogm_update_seqnos(const struct ethhdr
> > *ethhdr,
> > 
> >  	}
> >  	
> >  	rcu_read_lock();
> > 
> > -	hlist_for_each_entry_rcu(tmp_neigh_node,
> > -				 &orig_node->neigh_list, list) {
> > -		neigh_addr = tmp_neigh_node->addr;
> > -		is_dup = batadv_test_bit(tmp_neigh_node->bat_iv.real_bits,
> > +	hlist_for_each_entry_rcu(neigh_node, &orig_node->neigh_list, list) {
> > +		neigh_ifinfo = batadv_neigh_ifinfo_new(neigh_node,
> > +						       if_outgoing);
> 
> hm, not quite sure, but would a batadv_neigh_ifinfo_get() be
> sufficient here?

I don't think so. batadv_iv_ogm_update_seqnos() is called before 
batadv_iv_ogm_orig_update() which is the other function to call _new(). We 
need batadv_iv_ogm_update_seqnos() to call _new to create the ifinfo and put 
the first mark in the bitfield. 

> > +static void batadv_neigh_node_free_rcu(struct rcu_head *rcu)
> > +{
> > +	struct hlist_node *node_tmp;
> > +	struct batadv_neigh_node *neigh_node;
> > +	struct batadv_neigh_ifinfo *neigh_ifinfo;
> > +
> > +	neigh_node = container_of(rcu, struct batadv_neigh_node, rcu);
> > +
> > +	hlist_for_each_entry_safe(neigh_ifinfo, node_tmp,
> > +				  &neigh_node->ifinfo_list, list) {
> > +		batadv_neigh_ifinfo_free_ref_now(neigh_ifinfo);
> > +	}
> 
> two things are missing here: orig_node and if_incoming refcounters
> need to be decreased
> 

This appears to be broken in the current code already:
 * batadv_iv_ogm_neigh_new() calls batadv_neigh_node_new() and increases the 
if_incoming refcount (shouldn't be that in batadv_neigh_node_new() instead?)
 * orig_node does not get its refcounter increased in both functions
 * both functions set hard_iface and orig_node within the neigh_node struct 
(that seems to be redundant)
 * the current code does not free_ref neigh->if_incoming

As we have a cyclic dependency here (neigh_node -> orig_node and vice versa), 
I'm not sure if we add more problems by adding refcounting to the orig_node 
here (when cleaning up, that is).

I'll therefore add the free_ref for if_incoming and keep the orig_node 
refcounting as it is.

> 
> why not simply using a "break" within the if-case here instead of the
> "!best" check and continuing to iterate over the list?
> 

because we could have found a better neighbor then the previous "best", but 
not yet the "best" of the whole list. We need to check the whole list to find 
the best. 

Thanks,
     Simon