Re: [B.A.T.M.A.N.] [PATCH 6/6] batman-adv: Send multicast packets to nodes with a WANT_ALL flag

[Simon Wunderlich <sw@simonwunderlich.de>]

> > Maybe I am missing the whole point of WANT_ALL but why do we maintain a
> > list of WANT_ALL nodes to only send the packet to the first valid entry
> > in the list?
> 
> Hm, the thing is, there can be multiple nodes with that flag. But
> most of the time we only end up in this function when there's just
> a single node in this list. (when there's more than one, we'd
> usually end up in the broadcast instead of unicasting path)

As far as I see, send_skb_via_mcast() is only called if forw_mode returned 
BATADV_FORW_SINGLE, that is there is only one recipient in the list, and there 
is no "regular" listener in TT.

(I don't see any good reason for calling that "most of the time" and 
"usually")

> 
> We could remove the list entirely but then we'd have to loop over
> all orig_nodes and check their mcast_flags for every packet -
> which is too costly on the fast path.
> 
> We could replace these two lists by two variables holding a single
> originator address or orig_node each. But then we'd still have to
> loop over all originators when the numbers of nodes with such a
> flag decreases to one to find this one node to update the variable
> with.
> 
> 
> But yes, this pointer pointer to a list head is not really nice
> either... what do you think about returning a pointer pointer to
> an orig_node with batadv_mcast_want_all_count() already, instead?
> That way we'd spare checking the IP address family twice, too. Or
> the list-to-orig_node-variable substitution approach?

I think that would be good to do (returning an orig_node, that is). And also 
please return an orig_node if a TT match was found for the single case.

With this we would unify the multicast sending in batadv_interface_tx() as 
well which would make it more readable. Right now mcast packets are sent via 
batadv_send_skb_via_mcast() if the want_all_list is present or via 
batadv_send_skb_via_tt() if want_all_list is not present, which isn't exactly 
easy to understand. :)

something like

                        forw_mode = batadv_mcast_forw_mode(bat_priv, skb,
                                                           &mcast_single_orig);

and then in the unicast branch below

     } else if (mcast_single_orig) {
               ret = batadv_send_skb_unicast(bat_priv, skb, BATADV_UNICAST, 0, 
mcast_single_orig, vid);
     }

should do the trick, no?

Thanks,
    Simon