Re: [B.A.T.M.A.N.] [PATCHv5] batman-adv: make the TT CRC logic VLAN specific

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 4896 bytes --]

On Thu, Jul 04, 2013 at 05:07:05PM +0800, Marek Lindner wrote:
> >  /**
> > + * batadv_tt_prepare_tvlv_global_data - prepare the TVLV TT header to send
> > + *  within a TT Response directed to another node
> > + * @orig_node: originator for which the TT data has to be prepared
> > + * @tt_data: uninitialised pointer to the address of the TVLV buffer
> > + * @tt_len: space to reserve for the TT entries. If negative, this
> > function will + *  allocate as much space is needed for the full table
> > + *
> > + * Allocate the needed amount of memory for the entire TT TVLV and write
> > its + * header made up by one tvlv_tt_data object and a series of
> > tvlv_tt_vlan_data + * objects, one per active VLAN served by the
> > originator node.
> > + *
> > + * Return the size of the allocated buffer or 0 in case of failure.
> > + */
> > +static uint16_t
> > +batadv_tt_prepare_tvlv_global_data(struct batadv_orig_node *orig_node,
> > +				   struct batadv_tvlv_tt_data **tt_data,
> > +				   int32_t tt_len)
> > +{
> > +	struct batadv_tvlv_tt_vlan_data *tt_vlan;
> > +	struct batadv_orig_node_vlan *vlan;
> > +	uint16_t num_vlan = 0, num_entries = 0;
> > +
> > +	rcu_read_lock();
> > +	list_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) {
> > +		num_vlan++;
> > +		num_entries += atomic_read(&vlan->tt.num_entries);
> > +	}
> > +
> > +	/* if tt_len is negative, allocate the space needed by the full table */
> > +	if (tt_len < 0)
> > +		tt_len = batadv_tt_len(num_entries);
> > +
> > +	tt_len += sizeof(**tt_data);
> > +	tt_len += sizeof(*tt_vlan) * num_vlan;
> > +
> > +	*tt_data = kmalloc(tt_len, GFP_ATOMIC);
> > +	if (!*tt_data) {
> > +		tt_len = 0;
> > +		goto out;
> > +	}
> > +
> > +	(*tt_data)->flags = BATADV_NO_FLAGS;
> > +	(*tt_data)->ttvn = atomic_read(&orig_node->last_ttvn);
> > +	(*tt_data)->num_vlan = htons(num_vlan);
> > +
> > +	tt_vlan = (struct batadv_tvlv_tt_vlan_data *)(*tt_data + 1);
> > +	list_for_each_entry_rcu(vlan, &orig_node->vlan_list, list) {
> > +		tt_vlan->vid = htons(vlan->vid);
> > +		tt_vlan->crc = htonl(vlan->tt.crc);
> > +
> > +		tt_vlan++;
> > +	}
> > +out:
> > +	rcu_read_unlock();
> > +	return tt_len;
> > +}
> 
> Are you certain this is race condition free ? What if local or global node is 
> added while this function is running ?

there is no problem because of the rcu_read_lock/unlock.
The only problem I see is that we may have changes to orig_node which happen
after executing this function and before filling the tvlv.

The tvlv may contain not consistent data (crc not corresponding to the data
being sent). This will be handled by another recovery procedure which will aim
to recover the correct state.

Is this acceptable? Actually we already have this "problem" in the current
implementation.


> 
> 
> > +/**
> >   * batadv_tt_tvlv_container_update - update the translation table tvlv
> > container *  after local tt changes have been committed
> >   * @bat_priv: the bat priv with all the soft interface information
> > @@ -473,10 +679,11 @@ static void batadv_tt_tvlv_container_update(struct
> > batadv_priv *bat_priv) struct batadv_tt_change_node *entry, *safe;
> >  	struct batadv_tvlv_tt_data *tt_data;
> >  	struct batadv_tvlv_tt_change *tt_change;
> > -	int tt_diff_len = 0, tt_change_len = 0;
> > +	int tt_diff_len, tt_change_len = 0;
> >  	int tt_diff_entries_num = 0, tt_diff_entries_count = 0;
> > +	uint16_t tt_len, change_offset;
> > 
> > -	tt_diff_len += batadv_tt_len(atomic_read(&bat_priv->tt.local_changes));
> > +	tt_diff_len = batadv_tt_len(atomic_read(&bat_priv->tt.local_changes));
> > 
> >  	/* if we have too many changes for one packet don't send any
> >  	 * and wait for the tt table request which will be fragmented
> > @@ -484,13 +691,16 @@ static void batadv_tt_tvlv_container_update(struct
> > batadv_priv *bat_priv) if (tt_diff_len > bat_priv->soft_iface->mtu)
> >  		tt_diff_len = 0;
> > 
> > -	tt_data = kzalloc(sizeof(*tt_data) + tt_diff_len, GFP_ATOMIC);
> > -	if (!tt_data)
> > +	tt_len = batadv_tt_prepare_tvlv_local_data(bat_priv, &tt_data,
> > +						   tt_diff_len);
> > +	if (!tt_len)
> >  		return;
> > 
> >  	tt_data->flags = BATADV_TT_OGM_DIFF;
> > -	tt_data->ttvn = atomic_read(&bat_priv->tt.vn);
> > -	tt_data->crc = htonl(bat_priv->tt.local_crc);
> 
> If you avoided this removal you could have one function instead of 
> batadv_tt_prepare_tvlv_local_data() and batadv_tt_prepare_tvlv_global_data() 
> doing basically the same thing.

uhm, i did not get this. we have several CRCs now, that is why I deleted this
line and why I provided the prepare* functions. Doing it in a generic way (for
both local and global node) would probably be not easy..

Thanks:)

Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]