Re: [B.A.T.M.A.N.] [PATCH v2 5/5] batman-adv: B.A.T.M.A.N. V - implement GW selection logic

On Tuesday, May 10, 2016 18:06:09 Antonio Quartulli wrote:

+static int batadv_v_gw_throughput_get(struct batadv_gw_node *gw_node, u32 *bw)

+static struct batadv_gw_node * +batadv_v_gw_get_best_gw_node(struct batadv_priv *bat_priv)

Kernel doc ?

• struct batadv_gw_node *gw_node, *curr_gw = NULL;
• u32 max_bw = 0, bw, threshold;
• threshold = atomic_read(&bat_priv->gw.sel_class);
• rcu_read_lock();
• hlist_for_each_entry_rcu(gw_node, &bat_priv->gw.list, list) {

• if (!kref_get_unless_zero(&gw_node->refcount))
  

• 	continue;
  

• if (batadv_v_gw_throughput_get(gw_node, &bw) < 0)
  

• 	goto next;
  

• if (curr_gw && (bw <= (max_bw + threshold)))
  

• 	goto next;
  

• if (curr_gw)
  

• 	batadv_gw_node_put(curr_gw);
  

• curr_gw = gw_node;
  

• kref_get(&curr_gw->refcount);
  

• max_bw = bw;
  

+next:

• batadv_gw_node_put(gw_node);
  

• }
• rcu_read_unlock();
• return curr_gw;

+}

I don't quite follow the use of 'threshold' in this function. The threshold is meant to decide whether the switch to another gateway is worth breaking the existing stateful connections. Here, the code is retrieving the best gateway of all - no need for the threshold ? Moreover, the threshold might lead to unpredictable results. If the threshold is 5 MBit/s and the first gateway in the list offers 1 MBit/s while the second offers 5 MBit/s the better gateway is never chosen.

+static bool batadv_v_gw_is_eligible(struct batadv_priv *bat_priv,

• 		    struct batadv_orig_node *curr_gw_orig,
  

• 		    struct batadv_orig_node *orig_node)
  

+/* fails if orig_node has no router */ +static int batadv_v_gw_write_buffer_text(struct batadv_priv *bat_priv,

• 			 struct seq_file *seq,
  

• 			 const struct batadv_gw_node *gw_node)
  

+static void batadv_v_gw_print(struct batadv_priv *bat_priv,

• 	      struct seq_file *seq)
  

More room for kernel doc ..

@@ -387,7 +569,16 @@ static struct batadv_algo_ops batadv_batman_v __read_mostly = { */ int batadv_v_mesh_init(struct batadv_priv *bat_priv) {

• return batadv_v_ogm_init(bat_priv);

• int ret = 0;
• ret = batadv_v_ogm_init(bat_priv);
• if (ret < 0)

• return ret;
  

• /* set default throughput difference threshold to 5Mbps */
• atomic_set(&bat_priv->gw.sel_class, 50);
• return 0;

}

There is no need to initial 'ret' if the next line is setting a proper value. If you moved the atomic_set() above the batadv_v_ogm_init() call most of the chachacha would go away.

In batadv_softif_init_late() bat_priv->gw.sel_class is initialized for B.A.T.M.A.N. IV. Do you intend to keep it there ? If not, a per-routing algorithm init might be cleaner .. :)

Cheers, Marek