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?



-- 
Antonio Quartulli