Re: [B.A.T.M.A.N.] [PATCHv3] batman-adv: postpone OGM preparation to sending time

[Antonio Quartulli <ordex@autistici.org>]

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

On Wed, Dec 26, 2012 at 06:08:50 +0800, Marek Lindner wrote:
> On Saturday, December 15, 2012 19:14:42 Antonio Quartulli wrote:
> > OGMs are currently prepared 1 originator interval in advance
> > then the time they are used to be sent.
> > This means that once in the air they carry old information (like TT
> > announcements and possibly other flags). To fix this, postpone the OGM
> > creation to the same time of sending, in this way the OGM is first created
> > and then immediately sent.
> 
> How about:
> OGMs are currently assembled and enqueued one originator interval before they 
> are sent. This means they might carry information which was outdated while the 
> OGM waited in the outgoing packet queue (like TT announcements and possibly 
> other flags).
> The OGM assembly has to be postponed to the latest possible moment before 
> sending in order to minimize the gap between gathering the data and flooding 
> it to the network.

Sounds much better. Thanks!

> 
> 
> > -/* when do we schedule our own ogm to be sent */
> > +/**
> > + * batadv_iv_ogm_emit_wait_time - compute the OGM preparation waiting time
> > + * @bat_priv: the bat priv with all the soft interface information
> > + *
> > + * Returns the amount of jiffies to wait before preparing and sending the
> > next + * own OGM
> > + */
> 
> I'd say "Returns the number of jiffies [..]".
> 

Yeah

> 
> > @@ -468,6 +442,7 @@ static void batadv_iv_ogm_aggregate(struct
> > batadv_forw_packet *forw_packet_aggr, memcpy(skb_buff, packet_buff,
> > packet_len);
> >  	forw_packet_aggr->packet_len += packet_len;
> >  	forw_packet_aggr->num_packets++;
> > +	forw_packet_aggr->own |= own_packet;
> > 
> >  	/* save packet direct link flag status */
> >  	if (direct_link) {
> 
> Using "|= own_packet" isn't strictly necessary because "forw_packet_aggr->own" 
> isn't a bit field.

Well the point is that here own_packet could be false, but forw_packet_aggr->own
might already be true, so I didn't want to destroy the original value.

> Did you vigorously test this code ? Especially, multi-node with multiple 
> interface setups are of interest. Also use different orig intervals to ensure 
> it still works everywhere.
> 

I will try more topologies and in particular different orig intervals as soon as
I have the possibility


> The thing is: Throughout the code you can find the implicite assumption of the 
> first aggregated packet being an "own packet" (if forw_packet_aggr->own is 
> set). Therefore, you have to be very careful changing that logic. One function 
> you definitely overlooked is batadv_iv_ogm_send_to_if() but there might be 
> others.
> 
> 
> > @@ -498,8 +473,7 @@ static void batadv_iv_ogm_queue_add(struct batadv_priv
> > *bat_priv,
> > 
> >  	/* find position for the packet in the forward queue */
> >  	spin_lock_bh(&bat_priv->forw_bat_list_lock);
> > -	/* own packets are not to be aggregated */
> > -	if ((atomic_read(&bat_priv->aggregated_ogms)) && (!own_packet)) {
> > +	if ((atomic_read(&bat_priv->aggregated_ogms))) {
> >  		hlist_for_each_entry(forw_packet_pos, tmp_node,
> >  				     &bat_priv->forw_bat_list, list) {
> >  			if (batadv_iv_ogm_can_aggregate(batadv_ogm_packet,
> 
> Now it gets interesting! Did you review the impact of 
> batadv_iv_ogm_can_aggregate() before you bravely removed the exclusion of own 
> packets ? That is quite a beast ...


well, the poor existing doc didn't point out all the possible threats :-P I'll
investigate more. Thanks :)

> 
> 
> > +/**
> > + * batadv_iv_ogm_send - prepare an send an own OGM
> > + * @work: kernel work struct
> > + *
> > + * Prepare the OGM and immediately enqueue it for sending
> > + */
> 
> prepare and send an own OGM
>         ^^^
> 

well, technically it is enqueued, not sent..

> 
> > +static void batadv_iv_ogm_send(struct work_struct *work)
> 
> The naming is less than optimal. bat_iv_ogm.c now has:
>  * batadv_iv_ogm_send
>  * batadv_iv_ogm_emit

mh..ok, I will propose something better :)


> 
> 
> > --- a/types.h
> > +++ b/types.h
> > @@ -49,6 +49,7 @@ struct batadv_hard_iface_bat_iv {
> >  	unsigned char *ogm_buff;
> >  	int ogm_buff_len;
> >  	atomic_t ogm_seqno;
> > +	struct delayed_work work;
> >  };
> 
> Guess we need some kernel doc for this change too. Since types.h now is fully 
> documented ...
> 

only because you are commenting this patch after having merged your full-kernel-doc-patch :-D
Ok, I will fix it too.

Thank you very much for your precious feedback.
Greetings from down under!

Cheers,

-- 
Antonio Quartulli

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

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