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

On Wednesday 09 October 2013 15:05:33 Simon Wunderlich wrote:

@@ -1223,6 +1286,7 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr, struct batadv_orig_node *orig_neigh_node, *orig_node, *orig_node_tmp; struct batadv_neigh_node *router = NULL, *router_router = NULL; struct batadv_neigh_node *orig_neigh_router = NULL;

• struct batadv_neigh_node_ifinfo *router_ifinfo = NULL; int has_directlink_flag; int is_my_addr = 0, is_my_orig = 0, is_my_oldorig = 0; int is_bidirect;

@@ -1355,7 +1419,7 @@ static void batadv_iv_ogm_process(const struct ethhdr *ethhdr, return;

dup_status = batadv_iv_ogm_update_seqnos(ethhdr, batadv_ogm_packet,

• 				 if_incoming);
  

• 				 if_incoming, NULL);
  

Does it even make sense to call batadv_iv_ogm_update_seqnos() with outgoing_if always set to NULL ? The way you changed batadv_iv_ogm_update_seqnos() makes me wonder. If batadv_neigh_node_get_ifinfo() returns NULL if outgoing_if is NULL (as the current kernel doc states) the function does not do any duplicate check - the call is useless. If we expect batadv_neigh_node_get_ifinfo() to always return something we still haven't linked the OGM count to any specific outgoing interface. In case we want that why calling batadv_iv_ogm_update_seqnos() with an outgoing_if at all. What am I missing ? Has this code been tested ?

@@ -285,10 +301,13 @@ out: void batadv_gw_check_election(struct batadv_priv *bat_priv, struct batadv_orig_node *orig_node) {

• struct batadv_neigh_node_ifinfo *router_orig_tq = NULL;

• struct batadv_neigh_node_ifinfo *router_gw_tq = NULL; struct batadv_orig_node *curr_gw_orig; struct batadv_neigh_node *router_gw = NULL, *router_orig = NULL; uint8_t gw_tq_avg, orig_tq_avg;

• rcu_read_lock(); curr_gw_orig = batadv_gw_get_selected_orig(bat_priv); if (!curr_gw_orig) goto deselect;

@@ -297,6 +316,10 @@ void batadv_gw_check_election(struct batadv_priv *bat_priv, if (!router_gw) goto deselect;

• router_gw_tq = batadv_neigh_node_get_ifinfo(router_gw, NULL);
• if (!router_gw_tq)

• goto deselect;
  

• /* this node already is the gateway */ if (curr_gw_orig == orig_node) goto out;

@@ -305,8 +328,15 @@ void batadv_gw_check_election(struct batadv_priv *bat_priv, if (!router_orig) goto out;

• gw_tq_avg = router_gw->bat_iv.tq_avg;
• orig_tq_avg = router_orig->bat_iv.tq_avg;

• router_orig_tq = batadv_neigh_node_get_ifinfo(router_orig, NULL);
• if (!router_orig_tq) {

• batadv_gw_deselect(bat_priv);
  

• goto out;
  

• }

What hinders you to jump to the deselect goto here too ?

/**

• • 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->ifinfo_lock);
• hlist_add_head_rcu(&neigh_ifinfo->list, &neigh->ifinfo_list);
• spin_unlock_bh(&neigh->ifinfo_lock);

+out:

• rcu_read_unlock();
• return neigh_ifinfo;

+}

There seems to be no immediate need for the caller to hold rcu_lock as stated in the kernel doc. Is it an older documentation fragment that should be removed ?

Furthermore, 'returning NULL if not found' does not seem to be implemented. This is all the more interesting since you have implemented function calls that do seem to expect the 'NULL if not found' behavior. For instance, batadv_gw_get_best_gw_node(), batadv_gw_check_election(), etc. Even implictely, through batadv_iv_ogm_neigh_cmp(). Please carefully re-check your code.

+/* struct batadv_neigh_node_ifinfo - neighbor information per outgoing interface + * for BATMAN IV

• @tq_recv: ring buffer of received TQ values from this neigh node
• @tq_index: ring buffer index
• @tq_avg: averaged tq of all tq values in the ring buffer (tq_recv)
• @real_bits: bitfield containing the number of OGMs received from this

neigh - * node (relative to orig_node->last_real_seqno)

• • @real_packet_count: counted result of real_bits
• • @lq_update_lock: lock protecting tq_recv & tq_index
  */

-struct batadv_neigh_bat_iv { +struct batadv_neigh_ifinfo_bat_iv { uint8_t tq_recv[BATADV_TQ_GLOBAL_WINDOW_SIZE]; uint8_t tq_index; uint8_t tq_avg; DECLARE_BITMAP(real_bits, BATADV_TQ_LOCAL_WINDOW_SIZE); uint8_t real_packet_count;

• spinlock_t lq_update_lock; /* protects tq_recv & tq_index */

};

Documentation longer than one line should be indented by one char not 8 (second line). Half of the documentation of real_bits is removed as well as @real_packet_count though it still is defined in the struct ?

Cheers, Marek