Re: [B.A.T.M.A.N.] [PATCH 2/2] batman-adv: Simple (re)broadcast avoidance

[Sven Eckelmann <sven@narfation.org>]

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

Hi,

see the inline comments. Most things appear multiple times.

On Freitag, 22. Juli 2016 13:38:02 CEST Linus Lüssing wrote:
> @@ -182,6 +183,25 @@ static void batadv_v_ogm_send(struct work_struct *work)
>  		if (!kref_get_unless_zero(&hard_iface->refcount))
>  			continue;
>  
> +		ret = batadv_hardif_no_broadcast(hard_iface, NULL, NULL);
> +		if (ret) {
> +			char *type = "unknown";
> +
> +			if (ret == BATADV_HARDIF_BCAST_NORECIPIENT)
> +				type = "no neighbor";
> +
> +			if (ret == BATADV_HARDIF_BCAST_DUPFWD)
> +				type = "single neighbor is source";
> +
> +			if (ret == BATADV_HARDIF_BCAST_DUPORIG)
> +				type = "single neighbor is originator";

what about using "switch (ret)" here?

> +
> +			batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "OGM2 from ourselve on %s surpressed: %s\n",
> +				   hard_iface->net_dev->name, type);
> +
> +			continue;
> +		}
> +

Where is the put for hard_iface when continue is done after
batadv_hardif_no_broadcast? See the previous chunk for
"kref_get_unless_zero(&hard_iface->refcount)".


> @@ -716,6 +737,29 @@ static void batadv_v_ogm_process(const struct sk_buff *skb, int ogm_offset,
>  		if (!kref_get_unless_zero(&hard_iface->refcount))
>  			continue;
>  
> +		ret = batadv_hardif_no_broadcast(hard_iface,
> +						 hardif_neigh->orig_node,
> +						 ogm_packet->orig);
> +
> +		if (ret) {
> +			char *type = "unknown";
> +
> +			if (ret == BATADV_HARDIF_BCAST_NORECIPIENT)
> +				type = "no neighbor";
> +
> +			if (ret == BATADV_HARDIF_BCAST_DUPFWD)
> +				type = "single neighbor is source";
> +
> +			if (ret == BATADV_HARDIF_BCAST_DUPORIG)
> +				type = "single neighbor is originator";

what about using "switch (ret)" here?

> +
> +			batadv_dbg(BATADV_DBG_BATMAN, bat_priv, "OGM2 packet from %pM on %s surpressed: %s\n",
> +				   ogm_packet->orig, hard_iface->net_dev->name,
> +				   type);
> +
> +			continue;
> +		}
> +

Where is the put for hard_iface when continue is done after
batadv_hardif_no_broadcast? See the previous chunk for
"kref_get_unless_zero(&hard_iface->refcount)".

> @@ -517,7 +518,8 @@ batadv_neigh_node_get(const struct batadv_orig_node *orig_node,
>   */
>  static struct batadv_hardif_neigh_node *
>  batadv_hardif_neigh_create(struct batadv_hard_iface *hard_iface,
> -			   const u8 *neigh_addr)
> +			   const u8 *neigh_addr,
> +			   struct batadv_orig_node *orig_node)
>  {
>  	struct batadv_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
>  	struct batadv_hardif_neigh_node *hardif_neigh = NULL;
> @@ -534,10 +536,12 @@ batadv_hardif_neigh_create(struct batadv_hard_iface *hard_iface,
>  		goto out;
>  
>  	kref_get(&hard_iface->refcount);
> +	kref_get(&orig_node->refcount);
>  	INIT_HLIST_NODE(&hardif_neigh->list);
>  	ether_addr_copy(hardif_neigh->addr, neigh_addr);
>  	hardif_neigh->if_incoming = hard_iface;
>  	hardif_neigh->last_seen = jiffies;
> +	hardif_neigh->orig_node = orig_node;

Can you please move the kref_get near the place where new reference is used?


> @@ -634,6 +641,46 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work)
[...]
> +		if (ret) {
> +			char *type = "unknown";
> +
> +			if (ret == BATADV_HARDIF_BCAST_NORECIPIENT)
> +				type = "no neighbor";
> +
> +			if (ret == BATADV_HARDIF_BCAST_DUPFWD)
> +				type = "single neighbor is source";
> +
> +			if (ret == BATADV_HARDIF_BCAST_DUPORIG)
> +				type = "single neighbor is originator";

what about using "switch (ret)" here?

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]