Re: [B.A.T.M.A.N.] [RFC 02/10] batman-adv: make struct batadv_orig_node algorithm agnostic

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

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

On Wed, May 29, 2013 at 12:20:41AM +0200, Antonio Quartulli wrote:
> From: Antonio Quartulli <antonio@open-mesh.com>
> 
> some of the struct batadv_orig_node members are B.A.T.M.A.N. IV
> specific and therefore they are moved in a algorithm specific
> substruct in order to make batadv_orig_node routing algorithm
> agnostic
> 
> Signed-off-by: Antonio Quartulli <antonio@open-mesh.com>
> ---
>  bat_iv_ogm.c | 108 ++++++++++++++++++++++++++++++++++++++++-------------------
>  originator.c |  83 +++++++++++++++++----------------------------
>  originator.h |   2 +-
>  routing.c    |  10 ++++--
>  types.h      |  50 +++++++++++++++------------
>  5 files changed, 142 insertions(+), 111 deletions(-)
> 
> diff --git a/originator.c b/originator.c
> index 3d312de..0225ae2 100644
> --- a/originator.c
> +++ b/originator.c
> @@ -297,18 +297,13 @@ void batadv_originator_free(struct batadv_priv *bat_priv)
>  /* this function finds or creates an originator entry for the given
>   * address if it does not exits
>   */
> -struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv,
> +struct batadv_orig_node *batadv_orig_node_new(struct batadv_priv *bat_priv,
>  					      const uint8_t *addr)
>  {
>  	struct batadv_orig_node *orig_node;
>  	struct batadv_orig_node_vlan *vlan;
> -	int size, i;
> -	int hash_added;
>  	unsigned long reset_time;
> -
> -	orig_node = batadv_orig_hash_find(bat_priv, addr);
> -	if (orig_node)
> -		return orig_node;
> +	int i, hash_added;
>  
>  	batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
>  		   "Creating new originator: %pM\n", addr);
> @@ -320,8 +315,6 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv,
>  	INIT_HLIST_HEAD(&orig_node->neigh_list);
>  	INIT_LIST_HEAD(&orig_node->bond_list);
>  	INIT_LIST_HEAD(&orig_node->vlan_list);
> -	spin_lock_init(&orig_node->ogm_cnt_lock);
> -	spin_lock_init(&orig_node->bcast_seqno_lock);
>  	spin_lock_init(&orig_node->neigh_list_lock);
>  	spin_lock_init(&orig_node->tt_buff_lock);
>  	spin_lock_init(&orig_node->vlan_list_lock);
> @@ -339,11 +332,6 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv,
>  	atomic_set(&orig_node->last_ttvn, 0);
>  	orig_node->tt_buff = NULL;
>  	orig_node->tt_buff_len = 0;
> -	reset_time = jiffies - 1 - msecs_to_jiffies(BATADV_RESET_PROTECTION_MS);
> -	orig_node->bcast_seqno_reset = reset_time;
> -	orig_node->batman_seqno_reset = reset_time;
> -
> -	atomic_set(&orig_node->bond_candidates, 0);
>  
>  	/* create a vlan object for the "untagged" LAN */
>  	vlan = batadv_orig_node_vlan_new(orig_node, BATADV_NO_FLAGS);
> @@ -352,14 +340,7 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv,
>  	/* there is nothing else to do with the vlan.. */
>  	batadv_orig_node_vlan_free_ref(vlan);
>  
> -	size = bat_priv->num_ifaces * sizeof(unsigned long) * BATADV_NUM_WORDS;
> -
> -	orig_node->bcast_own = kzalloc(size, GFP_ATOMIC);
> -	if (!orig_node->bcast_own)
> -		goto free_vlan;
> -
> -	size = bat_priv->num_ifaces * sizeof(uint8_t);
> -	orig_node->bcast_own_sum = kzalloc(size, GFP_ATOMIC);
> +	atomic_set(&orig_node->bond_candidates, 0);
>  
>  	for (i = 0; i < BATADV_FRAG_BUFFER_COUNT; i++) {
>  		INIT_HLIST_HEAD(&orig_node->fragments[i].head);
> @@ -367,22 +348,20 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv,
>  		orig_node->fragments[i].size = 0;
>  	}
>  
> -	if (!orig_node->bcast_own_sum)
> -		goto free_bcast_own;
> +	spin_lock_init(&orig_node->bcast_seqno_lock);
> +	reset_time = jiffies - 1 - msecs_to_jiffies(BATADV_RESET_PROTECTION_MS);
> +	orig_node->bcast_seqno_reset = reset_time;
> +	orig_node->batman_seqno_reset = reset_time;

Why are you moving this stuff around in the function? I don't see the purpose ... reset_time
or bcast_seqno_lock are just moved, but not changed.
>  
>  	hash_added = batadv_hash_add(bat_priv->orig_hash, batadv_compare_orig,
>  				     batadv_choose_orig, orig_node,
>  				     &orig_node->hash_entry);
> -	if (hash_added != 0)
> -		goto free_bcast_own_sum;
> +	if (hash_added != 0) {
> +		kfree(orig_node);
> +		return NULL;
> +	}
>  
>  	return orig_node;
> -free_bcast_own_sum:
> -	kfree(orig_node->bcast_own_sum);
> -free_bcast_own:
> -	kfree(orig_node->bcast_own);
> -free_vlan:
> -	batadv_orig_node_vlan_free_ref(vlan);
>  free_orig_node:
>  	kfree(orig_node);
>  	return NULL;
> diff --git a/types.h b/types.h
> index c7d5fa1..fde4d68 100644
> --- a/types.h
> +++ b/types.h
> @@ -133,18 +133,29 @@ struct batadv_orig_node_vlan {
>  };
>  
>  /**
> + * struct batadv_orig_bat_iv - B.A.T.M.A.N. IV private orig_node members
> + * @bcast_own: bitfield containing the number of our OGMs this orig_node
> + *  rebroadcasted "back" to us (relative to last_real_seqno)
> + * @bcast_own_sum: counted result of bcast_own
> + * @ogm_cnt_lock: lock protecting bcast_own, bcast_own_sum,
> + *  neigh_node->real_bits & neigh_node->real_packet_count
> + */
> +struct batadv_orig_bat_iv {
> +	unsigned long *bcast_own;
> +	uint8_t *bcast_own_sum;
> +	spinlock_t ogm_cnt_lock;
> +};
> +
> +/**
>   * struct batadv_orig_node - structure for orig_list maintaining nodes of mesh
>   * @orig: originator ethernet address
>   * @primary_addr: hosts primary interface address
>   * @router: router that should be used to reach this originator
> - * @batadv_dat_addr_t:  address of the orig node in the distributed hash
> - * @bcast_own: bitfield containing the number of our OGMs this orig_node
> - *  rebroadcasted "back" to us (relative to last_real_seqno)
> - * @bcast_own_sum: counted result of bcast_own
>   * @last_seen: time when last packet from this node was received
> - * @bcast_seqno_reset: time when the broadcast seqno window was reset
> - * @batman_seqno_reset: time when the batman seqno window was reset
> + * @last_ttl: ttl of last received packet
> + * @batadv_dat_addr_t:  address of the orig node in the distributed hash
>   * @capabilities: announced capabilities of this originator
> + * @last_real_seqno: last and best known sequence number
>   * @last_ttvn: last seen translation table version number
>   * @tt_buff: last tt changeset this node received from the orig node
>   * @tt_buff_len: length of the last tt changeset this node received from the
> @@ -152,19 +163,17 @@ struct batadv_orig_node_vlan {
>   * @tt_buff_lock: lock that protects tt_buff and tt_buff_len
>   * @tt_initialised: bool keeping track of whether or not this node have received
>   *  any translation table information from the orig node yet
> - * @last_real_seqno: last and best known sequence number
> - * @last_ttl: ttl of last received packet
>   * @bcast_bits: bitfield containing the info which payload broadcast originated
>   *  from this orig node this host already has seen (relative to
>   *  last_bcast_seqno)
>   * @last_bcast_seqno: last broadcast sequence number received by this host
> + * @bcast_seqno_lock: lock protecting bcast_bits & last_bcast_seqno
> + * @bcast_seqno_reset: time when the broadcast seqno window was reset
> + * @batman_seqno_reset: time when the batman seqno window was reset
>   * @neigh_list: list of potential next hop neighbor towards this orig node
>   * @neigh_list_lock: lock protecting neigh_list, router and bonding_list
>   * @hash_entry: hlist node for batadv_priv::orig_hash
>   * @bat_priv: pointer to soft_iface this orig node belongs to
> - * @ogm_cnt_lock: lock protecting bcast_own, bcast_own_sum,
> - *  neigh_node->real_bits & neigh_node->real_packet_count
> - * @bcast_seqno_lock: lock protecting bcast_bits & last_bcast_seqno
>   * @bond_candidates: how many candidates are available
>   * @bond_list: list of bonding candidates
>   * @refcount: number of contexts the object is used
> @@ -177,29 +186,30 @@ struct batadv_orig_node_vlan {
>   * @vlan_list: a list of orig_node_vlan structs, one per VLAN served by the
>   *  originator represented by this object
>   * @vlan_list_lock: lock protecting vlan_list
> + * @bat_iv: B.A.T.M.A.N. IV private structure
>   */
>  struct batadv_orig_node {
>  	uint8_t orig[ETH_ALEN];
>  	uint8_t primary_addr[ETH_ALEN];
>  	struct batadv_neigh_node __rcu *router; /* rcu protected pointer */
> +	unsigned long last_seen;
> +	uint8_t last_ttl;
>  #ifdef CONFIG_BATMAN_ADV_DAT
>  	batadv_dat_addr_t dat_addr;
>  #endif
> -	unsigned long *bcast_own;
> -	uint8_t *bcast_own_sum;
> -	unsigned long last_seen;
> -	unsigned long bcast_seqno_reset;
> -	unsigned long batman_seqno_reset;
>  	uint8_t capabilities;
> +	uint32_t last_real_seqno;
>  	atomic_t last_ttvn;
>  	unsigned char *tt_buff;
>  	int16_t tt_buff_len;
>  	spinlock_t tt_buff_lock; /* protects tt_buff & tt_buff_len */
>  	bool tt_initialised;
> -	uint32_t last_real_seqno;
> -	uint8_t last_ttl;
>  	DECLARE_BITMAP(bcast_bits, BATADV_TQ_LOCAL_WINDOW_SIZE);
>  	uint32_t last_bcast_seqno;
> +	/* bcast_seqno_lock protects: bcast_bits & last_bcast_seqno */
> +	spinlock_t bcast_seqno_lock;
> +	unsigned long bcast_seqno_reset;
> +	unsigned long batman_seqno_reset;
>  	struct hlist_head neigh_list;
>  	/* neigh_list_lock protects: neigh_list, router & bonding_list */
>  	spinlock_t neigh_list_lock;
> @@ -208,9 +218,6 @@ struct batadv_orig_node {
>  	/* ogm_cnt_lock protects: bcast_own, bcast_own_sum,
>  	 * neigh_node->real_bits & neigh_node->real_packet_count
>  	 */
> -	spinlock_t ogm_cnt_lock;
> -	/* bcast_seqno_lock protects: bcast_bits & last_bcast_seqno */
> -	spinlock_t bcast_seqno_lock;
>  	atomic_t bond_candidates;
>  	struct list_head bond_list;
>  	atomic_t refcount;
> @@ -224,6 +231,7 @@ struct batadv_orig_node {
>  	struct batadv_frag_table_entry fragments[BATADV_FRAG_BUFFER_COUNT];
>  	struct list_head vlan_list;
>  	spinlock_t vlan_list_lock; /* protects vlan_list */
> +	struct batadv_orig_bat_iv bat_iv;
>  };

Same here: there are various fields which are just moved around for no obvious
reason (bcast_seqno_lock, batman_seqno_reset, bcast_seqno_reset, last_seen, last_ttl,
...)


>  
>  /**
> -- 
> 1.8.1.5
> 

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