Re: [B.A.T.M.A.N.] [RFCv2 3/6] batman-adv: split out router from orig_node

On Wed, Sep 04, 2013 at 06:27:37PM +0200, Simon Wunderlich wrote:

From: Simon Wunderlich simon@open-mesh.com

For the network wide multi interface optimization there are different routers for each outgoing interface (outgoing from the OGM perspective, incoming for payload traffic). To reflect this, change the router and associated data to a list of routers.

Signed-off-by: Simon Wunderlich simon@open-mesh.com

[...]

+static void +batadv_iv_ogm_process_per_outif(const struct ethhdr *ethhdr,

• 		struct batadv_orig_node *orig_node,
  

• 		struct batadv_ogm_packet *batadv_ogm_packet,
  

• 		const unsigned char *tt_buff,
  

• 		struct batadv_hard_iface *if_incoming,
  

• 		struct batadv_hard_iface *if_outgoing)
  

{ struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);

• struct batadv_hard_iface *hard_iface;
• struct batadv_orig_node *orig_neigh_node, *orig_node, *orig_node_tmp; struct batadv_neigh_node *router = NULL, *router_router = NULL; struct batadv_neigh_node *orig_neigh_router = NULL; struct batadv_neigh_node_ifinfo *router_ifinfo = NULL;
• int has_directlink_flag;
• int is_my_addr = 0, is_my_orig = 0, is_my_oldorig = 0;
• int is_bidirect;
• bool is_single_hop_neigh = false;
• bool is_from_best_next_hop = false;
• int sameseq, similar_ttl;

• struct batadv_orig_node *orig_neigh_node, *orig_node_tmp;
• struct batadv_orig_node_ifinfo *orig_node_ifinfo;
• int is_bidirect, sameseq, similar_ttl; enum batadv_dup_status dup_status;

• uint32_t if_incoming_seqno; uint8_t *prev_sender;

• struct batadv_ogm_packet ogm_packet_backup;
• bool is_from_best_next_hop = false;
• bool is_single_hop_neigh = false;

Since you have the opportunity, please sort these new declarations by line length (like you have done 6 lines above).

[...]

• sameseq = orig_node_ifinfo->last_real_seqno ==

•    ntohl(batadv_ogm_packet->seqno));
  

• similar_ttl = (orig_node_ifinfo->last_ttl - 3) <=

•       batadv_ogm_packet->header.ttl;
  

you know this is very ugly, right? :-)

you could try to remove sameseq and similar_ttl and use temporary variables for last_real_seqno, seqno, last_ttl and header.ttl.....

• if (is_bidirect && ((dup_status == BATADV_NO_DUP) ||

• 	    (sameseq && similar_ttl)))
  

...then you can try to make them fit this 'if' directly.

• batadv_iv_ogm_orig_update(bat_priv, orig_node,
  

• 			  orig_node_ifinfo, ethhdr,
  

• 			  batadv_ogm_packet, if_incoming,
  

• 			  if_outgoing, tt_buff, dup_status);
  

• rcu_read_unlock();

[..]

+static void batadv_iv_ogm_process(const struct ethhdr *ethhdr,

• 		  struct batadv_ogm_packet *batadv_ogm_packet,
  

• 		  const unsigned char *tt_buff,
  

• 		  struct batadv_hard_iface *if_incoming)
  

+{

• struct batadv_priv *bat_priv = netdev_priv(if_incoming->soft_iface);
• struct batadv_hard_iface *hard_iface;
• struct batadv_orig_node *orig_neigh_node, *orig_node;
• uint32_t if_incoming_seqno;
• int has_directlink_flag;
• int is_my_addr = 0, is_my_orig = 0, is_my_oldorig = 0;

Please sort the declarations by line length.

[...]

diff --git a/network-coding.c b/network-coding.c index 173a96e..6d1b659 100644 --- a/network-coding.c +++ b/network-coding.c @@ -1010,12 +1010,17 @@ static bool batadv_nc_code_packets(struct batadv_priv *bat_priv, int coded_size = sizeof(*coded_packet); int header_add = coded_size - unicast_size;

• router_neigh = batadv_orig_node_get_router(neigh_node->orig_node);

• /* TODO: do we need to consider the outgoing interface for

• * coded packets?
  

• */
  

• router_neigh = batadv_orig_node_get_router(neigh_node->orig_node,

• 				   NULL);
  

Perhaps we can consider them like "locally-generated". I think we should ask Martin to give us his feedback :-)

[...]

/**

• • batadv_orig_node_get_ifinfo - gets the ifinfo from an orig_node
• • @orig_node: the orig node to be queried
• • @if_received: the interface for which the ifinfo should be acquired

Put a blank line here.

• • Returns: the requested orig_node_ifinfo or NULL if not found

we never use the ':' after Returns (and a . is missing at the end of the sentence). Please make sure that all the kernel doc you added respect those guidelines.

[...]

+static void batadv_orig_node_ifinfo_free_rcu(struct rcu_head *rcu)

kernel doc?

+{

• struct batadv_orig_node_ifinfo *orig_node_ifinfo;
• orig_node_ifinfo = container_of(rcu, struct batadv_orig_node_ifinfo,

• 			rcu);
  

• if (orig_node_ifinfo->if_outgoing)

• batadv_hardif_free_ref(orig_node_ifinfo->if_outgoing);
  

Please, be sure this is not creating any race condition during the cleanup phase. We had problems int the past with a RCU callback scheduling another callback too.

The cleanup code has an rcu_barrier() to ensure everything is done and then safely free the various structures. What you do here may violate this assumption and the scheduled callback may generate a General Protection Fault.

[...]

@@ -291,6 +366,12 @@ static void batadv_orig_node_free_rcu(struct rcu_head *rcu) batadv_neigh_node_free_ref(neigh_node); }

• hlist_for_each_entry_safe(orig_node_ifinfo, node_tmp,

• 		  &orig_node->ifinfo_list, list) {
  

• hlist_del_rcu(&orig_node_ifinfo->list);
  

• call_rcu(&orig_node_ifinfo->rcu,
  

• 	 batadv_orig_node_ifinfo_free_rcu);
  

Same here. Please ensure everything is ok when scheduling a callback from another callback.

[...]

/**

• • batadv_purge_orig_ifinfo - purge ifinfo entries from originator
• • @bat_priv: the bat priv with all the soft interface information
• • @orig_node: orig node which is to be checked
• • Returns: true if any ifinfo entry was purged, false otherwise
• */

+static bool +batadv_purge_orig_ifinfo(struct batadv_priv *bat_priv,

• 	 struct batadv_orig_node *orig_node)
  

+{

• struct hlist_node *node_tmp;
• struct batadv_orig_node_ifinfo *orig_node_ifinfo;
• bool ifinfo_purged = false;
• struct batadv_hard_iface *if_outgoing;

sort by line length please.

• rcu_read_lock();
• spin_lock_bh(&orig_node->neigh_list_lock);

why do you need both the rcu and the neigh_list_lock at the same time? You acquire and release the two in the same moment (maybe I am overlooking something?)

[...]

+/**

• batadv_purge_orig_neighbors - purges neighbors from originator
• @bat_priv: the bat priv with all the soft interface information
• @orig_node: orig node which is to be checked

@@ -516,6 +643,8 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv, struct batadv_orig_node *orig_node) { struct batadv_neigh_node *best_neigh_node;

• struct batadv_hard_iface *hard_iface;
• bool changed = false;

this is useless and then.....

if (batadv_has_timed_out(orig_node->last_seen, 2 * BATADV_PURGE_TIMEOUT)) { @@ -525,11 +654,32 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv, jiffies_to_msecs(orig_node->last_seen)); return true; }

• if (!batadv_purge_orig_neighbors(bat_priv, orig_node))

• changed = changed || batadv_purge_orig_ifinfo(bat_priv, orig_node);

....just do:

changed = batadv_purge_orig_ifinfo(...);

[...]

diff --git a/originator.h b/originator.h index 161c1e3..1dce5b4 100644 --- a/originator.h +++ b/originator.h @@ -36,7 +36,16 @@ batadv_neigh_node_new(struct batadv_hard_iface *hard_iface, struct batadv_orig_node *orig_node); void batadv_neigh_node_free_ref(struct batadv_neigh_node *neigh_node); struct batadv_neigh_node * -batadv_orig_node_get_router(struct batadv_orig_node *orig_node); +batadv_orig_node_get_router(struct batadv_orig_node *orig_node,

• 	    const struct batadv_hard_iface *if_received);
  

+void

why void on another line? It should fit in the next one.

+batadv_orig_node_set_router(struct batadv_orig_node *orig_node,

• 	    struct batadv_hard_iface *if_received,
  

• 	    struct batadv_neigh_node *router);
  

Cheers,