Hi Lunus,
On 21/05/18 22:48, Linus Lüssing wrote:
On Fri, May 18, 2018 at 09:47:53AM +0800, Marek Lindner wrote:
+/**
- batadv_v_elp_tp_start() - start a tp meter session for a neighbor
- @neigh: neighbor to run tp meter on
- */
+static void batadv_v_elp_tp_start(struct batadv_hardif_neigh_node *neigh) +{
- struct batadv_hard_iface *hard_iface = neigh->if_incoming;
- struct batadv_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
- neigh->bat_v.tp_meter_running = true;
- batadv_tp_start(bat_priv, neigh->addr, neigh, 10, NULL, BATADV_TP_ELP);
+}
This seems racy here? We have no ordering constraints between the assigment and batadv_tp_start() which might lead to...
@@ -152,6 +192,25 @@ static u32 batadv_v_elp_get_throughput(struct batadv_hardif_neigh_node *neigh) return throughput * 10; }
+fallback_throughput:
- last_tp_run_jiffies = jiffies - neigh->bat_v.last_tp_meter_run;
- last_tp_run_msecs = jiffies_to_msecs(last_tp_run_jiffies);
- /* check the tp_meter_running flag before checking the timestamp to
* avoid a race condition where a new tp meter session is scheduled* right after the previous tp meter session has completed*/- if (!neigh->bat_v.tp_meter_running &&
last_tp_run_msecs > BATADV_ELP_TP_RUN_INTERVAL)batadv_v_elp_tp_start(neigh);...multiple instances potentially being started at the same time here?
- batadv_tp_start() is only invoked by batadv_v_elp_get_throughput(); - batadv_v_elp_get_throughput() is only invoked by batadv_v_elp_throughput_metric_update(); - batadv_v_elp_throughput_metric_update() is a periodic function executed by a timer and we can't have concurrent executions (unless we use crazy interval values).
Maybe we should add a comment to make this more clear?