On Wed, Sep 04, 2013 at 06:27:36PM +0200, Simon Wunderlich wrote:
@@ -897,22 +904,39 @@ static void batadv_iv_ogm_schedule(struct batadv_hard_iface *hard_iface) batadv_hardif_free_ref(primary_if); }
+/**
- batadv_iv_ogm_orig_update - use OGM to update corresponding data in an
- originator
- @bat_priv: the bat priv with all the soft interface information
- @orig_node: the orig node who originally emitted the ogm packet
- @orig_node_ifinfo: ifinfo for the according outgoing interface
where is this argument in the function declaration?
- @ethhdr: Ethernet header of the OGM
- @@batadv_ogm_packet: the ogm packet
too many '@' :)
- @if_incoming: interface where the packet was received
- @if_outgoing: interface for which the retransmission should be considered
- @tt_buff: pointer to the tt buffer
- @dup_status: the duplicate status of this ogm packet.
- */
static void batadv_iv_ogm_orig_update(struct batadv_priv *bat_priv, struct batadv_orig_node *orig_node, const struct ethhdr *ethhdr, const struct batadv_ogm_packet *batadv_ogm_packet, struct batadv_hard_iface *if_incoming,
struct batadv_hard_iface *if_outgoing, const unsigned char *tt_buff, enum batadv_dup_status dup_status){ struct batadv_neigh_node *neigh_node = NULL, *tmp_neigh_node = NULL; struct batadv_neigh_node *router = NULL; struct batadv_orig_node *orig_node_tmp;
- struct batadv_neigh_node_ifinfo *neigh_ifinfo = NULL,
*tmp_neigh_ifinfo,*router_ifinfo;
we have never used this kind of style for declaration. I'd prefer you to re-write the variable type on each line, please.
int if_num; uint8_t sum_orig, sum_neigh; uint8_t *neigh_addr;
- uint8_t tq_avg;
uint8_t tq_avg, *tq_recv, *tq_index;
batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "update_originator(): Searching and updating originator entry of received packet\n");
[...]
@@ -1134,17 +1193,20 @@ out:
- @ethhdr: ethernet header of the packet
- @batadv_ogm_packet: OGM packet to be considered
- @if_incoming: interface on which the OGM packet was received
*/
- @if_outgoing: interface for which the retransmission should be considered
- Returns duplicate status as enum batadv_dup_status
static enum batadv_dup_status batadv_iv_ogm_update_seqnos(const struct ethhdr *ethhdr, const struct batadv_ogm_packet *batadv_ogm_packet,
const struct batadv_hard_iface *if_incoming)
const struct batadv_hard_iface *if_incoming,struct batadv_hard_iface *if_outgoing)
why isn't this const as well? It is not going to be changed or what, so keeping the const qualifier is good imho.
{ struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface); struct batadv_orig_node *orig_node;
- struct batadv_neigh_node *tmp_neigh_node;
- struct batadv_neigh_node *neigh_node;
- struct batadv_neigh_node_ifinfo *neigh_ifinfo; int is_dup; int32_t seq_diff; int need_update = 0;
@@ -1171,15 +1233,20 @@ batadv_iv_ogm_update_seqnos(const struct ethhdr *ethhdr, }
rcu_read_lock();
- hlist_for_each_entry_rcu(tmp_neigh_node,
- hlist_for_each_entry_rcu(neigh_node, &orig_node->neigh_list, list) {
maybe this line can be rearranged now that the first parameter is shorter than before?
neigh_addr = tmp_neigh_node->addr;is_dup = batadv_test_bit(tmp_neigh_node->bat_iv.real_bits,
neigh_ifinfo = batadv_neigh_node_get_ifinfo(neigh_node,if_outgoing);if (WARN_ON(!neigh_ifinfo))continue;neigh_addr = neigh_node->addr;is_dup = batadv_test_bit(neigh_ifinfo->bat_iv.real_bits, orig_node->last_real_seqno, seqno);if (batadv_compare_eth(neigh_addr, ethhdr->h_source) &&
tmp_neigh_node->if_incoming == if_incoming) {
neigh_node->if_incoming == if_incoming) { set_mark = 1; if (is_dup) ret = BATADV_NEIGH_DUP;
[...]
@@ -1597,34 +1677,50 @@ next:
- batadv_iv_ogm_neigh_cmp - compare the metrics of two neighbors
- @neigh1: the first neighbor object of the comparison
- @neigh2: the second neighbor object of the comparison
*/
- @if_outgoing: outgoing interface for the comparison
- Returns a value less, equal to or greater than 0 if the metric via neigh1 is
- lower, the same as or higher than the metric via neigh2
static int batadv_iv_ogm_neigh_cmp(struct batadv_neigh_node *neigh1,
struct batadv_neigh_node *neigh2)
struct batadv_neigh_node *neigh2,struct batadv_hard_iface *if_outgoing)
To make this API really generic, wouldn't it better to specify an if_outgoing1 and an if_outgoing2 (one per neigh)? The use cases we have now would invoke this function passing the same iface as both arguments, but if we want to make this API generic enough and avoid changing it in the future I'd suggest this change.
Theoretically we could use this function by passing the same neigh and two different interfaces...no? In this way we decide which is the best outgoing interface (what I described does not sound like the best approach...I think I will understand how you do this in the next patches...)
[...]
/**
- batadv_iv_ogm_neigh_is_eob - check if neigh1 is equally good or better than
- neigh2 from the metric prospective
- @neigh1: the first neighbor object of the comparison
- @neigh2: the second neighbor object of the comparison
- batadv_iv_ogm_neigh_is_eob - check if neigh1_ifinfo is equally good or\
- better than neigh2_ifinfo from the metric prospective
- @neigh1_ifinfo: the first neighbor ifinfo object of the comparison
- @neigh2_ifinfo: the second neighbor ifinfo object of the comparison
- Returns true if the metric via neigh1 is equally good or better than the
- metric via neigh2, false otherwise.
- Returns true if the metric via neigh1_ifinfo is equally good or better than
*/
- the metric via neigh2_ifinfo, false otherwise.
-static bool batadv_iv_ogm_neigh_is_eob(struct batadv_neigh_node *neigh1,
struct batadv_neigh_node *neigh2)+static bool +batadv_iv_ogm_neigh_is_eob(struct batadv_neigh_node_ifinfo *neigh1_ifinfo,
struct batadv_neigh_node_ifinfo *neigh2_ifinfo)
We could do the same as the previous function (..neigh1, iface1, neigh2, iface2). In this way we would not need to get the ifinfo objects outside but we can leave the duty to this function (and reduce code).
[...]
@@ -287,8 +303,11 @@ void batadv_gw_check_election(struct batadv_priv *bat_priv, { struct batadv_orig_node *curr_gw_orig; struct batadv_neigh_node *router_gw = NULL, *router_orig = NULL;
- struct batadv_neigh_node_ifinfo *router_gw_tq = NULL,
*router_orig_tq = NULL;
as before.
[...]
@@ -173,6 +191,55 @@ batadv_orig_node_get_router(struct batadv_orig_node *orig_node) }
/**
- batadv_neigh_node_get_ifinfo - gets the ifinfo from an neigh_node
- @neigh_node: the neigh node to be queried
- @if_outgoing: the interface for which the ifinfo should be acquired
- Note: this function must be called under rcu lock
- Returns the requested neigh_node_ifinfo or NULL if not found
- */
+struct batadv_neigh_node_ifinfo * +batadv_neigh_node_get_ifinfo(struct batadv_neigh_node *neigh,
struct batadv_hard_iface *if_outgoing)+{
- struct batadv_neigh_node_ifinfo *neigh_ifinfo = NULL,
*tmp_neigh_ifinfo;- rcu_read_lock();
- hlist_for_each_entry_rcu(tmp_neigh_ifinfo, &neigh->ifinfo_list,
list) {if (tmp_neigh_ifinfo->if_outgoing != if_outgoing)continue;neigh_ifinfo = tmp_neigh_ifinfo;- }
- if (neigh_ifinfo)
goto out;- neigh_ifinfo = kzalloc(sizeof(*neigh_ifinfo), GFP_ATOMIC);
- if (!neigh_ifinfo)
goto out;- if (if_outgoing && !atomic_inc_not_zero(&if_outgoing->refcount)) {
kfree(neigh_ifinfo);neigh_ifinfo = NULL;goto out;- }
- INIT_HLIST_NODE(&neigh_ifinfo->list);
- neigh_ifinfo->if_outgoing = if_outgoing;
- spin_lock_bh(&neigh->bat_iv.lq_update_lock);
- hlist_add_head_rcu(&neigh_ifinfo->list, &neigh->ifinfo_list);
- spin_unlock_bh(&neigh->bat_iv.lq_update_lock);
These batman iv attributes should be initialised in a batman-iv-private function, otherwise we are back with to code (something that has been partly solved by my previous patchset)...
[..]
- bool (*bat_neigh_ifinfo_is_equiv_or_better)
(struct batadv_neigh_node_ifinfo *neigh1_ifinfo,struct batadv_neigh_node_ifinfo *neigh2_ifinfo);
really ugly! but I don't know how we should re-arrange this... :(
Cheers,