[3/3] batman-adv: Modified forwarding behaviour for multicast packets

Message ID 1368293014-30742-4-git-send-email-linus.luessing@web.de (mailing list archive)
State Superseded, archived
Headers

Commit Message

Linus Lüssing May 11, 2013, 5:23 p.m. UTC
  With this patch a multicast packet is not always simply flooded anymore,
the bevahiour for the following cases is changed to reduce
unnecessary overhead:

If all nodes within the horizon of a certain node have signalized
multicast listener announcement capability
(BATADV_MCAST_LISTENER_ANNOUNCEMENT) then an IPv6 multicast packet
with a destination of IPv6 link-local scope coming from the upstream
of this node...

* ...is dropped if there is no according multicast listener in the
  translation table.
* ...is forwarded via unicast if there is a single node with interested
  multicast listeners.

Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
 multicast.c         |   42 ++++++++++++++++++++++++++++++++++++++++++
 multicast.h         |    8 ++++++++
 soft-interface.c    |   10 ++++++++++
 translation-table.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 translation-table.h |    1 +
 5 files changed, 105 insertions(+)
  

Comments

Antonio Quartulli May 11, 2013, 11:29 p.m. UTC | #1
On Sat, May 11, 2013 at 07:23:27PM +0200, Linus Lüssing wrote:
> With this patch a multicast packet is not always simply flooded anymore,
> the bevahiour for the following cases is changed to reduce
> unnecessary overhead:
> 
> If all nodes within the horizon of a certain node have signalized
> multicast listener announcement capability
> (BATADV_MCAST_LISTENER_ANNOUNCEMENT) then an IPv6 multicast packet
> with a destination of IPv6 link-local scope coming from the upstream
> of this node...
> 
> * ...is dropped if there is no according multicast listener in the
>   translation table.
> * ...is forwarded via unicast if there is a single node with interested
>   multicast listeners.
> 

othwerwise? Does it get flooded like now if there is more than one receiver?

>  
>  /**
> + * batadv_mcast_flood - Checks on how to forward a multicast packet
> + * @skb: The multicast packet to check
> + * @bat_priv: the bat priv with all the soft interface information
> + *
> + * Returns 1 if the packet should be flooded, 0 if it should be forwarded
> + * via unicast or -1 if it should be drooped.
> + */
> +int batadv_mcast_flood(struct sk_buff *skb, struct batadv_priv *bat_priv)
> +{
> +	struct ethhdr *ethhdr = (struct ethhdr *)(skb->data);
> +	struct ipv6hdr *ip6hdr;
> +	int count, ret = 1;
> +
> +	if (atomic_read(&bat_priv->mcast_group_awareness) &&
> +	    !atomic_read(&bat_priv->mcast_num_non_aware) &&
> +	    ntohs(ethhdr->h_proto) == ETH_P_IPV6) {

mh..this would not work for VLANs..did you plan to introduce support for VLANs
later? or you simply overlooked it? :)

> +		if (!pskb_may_pull(skb, sizeof(*ethhdr) + sizeof(*ip6hdr))) {
> +			ret = -1;
> +			goto out;
> +		}
> +
> +		ip6hdr = ipv6_hdr(skb);
> +
> +		/* TODO: Implement Multicast Router Discovery, then add
> +		 * scope >= IPV6_ADDR_SCOPE_LINKLOCAL, too */
> +		if (IPV6_ADDR_MC_SCOPE(&ip6hdr->daddr) !=
> +		    IPV6_ADDR_SCOPE_LINKLOCAL)
> +			goto out;
> +
> +		count = batadv_tt_global_hash_count(bat_priv, ethhdr->h_dest);
> +
> +		if (!count)
> +			ret = -1;
> +		else if (count == 1)
> +			ret = 0;

how can this function return more than one?
When there is more than one originator announcing the same MAC address then we
have _a single_ global entry having a list of orig_entry. but stil only one
global entry.

so you may want to count the orig_entries rather than the global_entries?

> diff --git a/translation-table.c b/translation-table.c
> index 37e7d47..1d2d618 100644
> --- a/translation-table.c
> +++ b/translation-table.c
> @@ -83,6 +83,40 @@ batadv_tt_hash_find(struct batadv_hashtable *hash, const void *data)
>  	return tt_common_entry_tmp;
>  }
>  
> +/**
> + * batadv_tt_hash_count - Counts the number of tt entries for the given data
> + * @hash: hash table containing the tt entries
> + * @data: The data to count entries for

One line saying what you are returning would be nice :)

> + */
> +static int batadv_tt_hash_count(struct batadv_hashtable *hash, const void *data)
> +{
> +	struct hlist_head *head;
> +	struct batadv_tt_common_entry *tt_common_entry;
> +	uint32_t index;
> +	int count = 0;
> +
> +	if (!hash)
> +		goto out;
> +
> +	index = batadv_choose_orig(data, hash->size);
> +	head = &hash->table[index];
> +
> +	rcu_read_lock();
> +	hlist_for_each_entry_rcu(tt_common_entry, head, hash_entry) {
> +		if (!batadv_compare_eth(tt_common_entry, data))
> +			continue;
> +
> +		if (!atomic_read(&tt_common_entry->refcount))
> +			continue;
> +
> +		count++;
> +	}
> +	rcu_read_unlock();
> +
> +out:
> +	return count;
> +}

as I asked before: this function cannot return >1 because the same address is
never stored twice.


Nice job so far!
Thanks for working on this cool feature!

Cheers,
  
Linus Lüssing May 16, 2013, 6:22 p.m. UTC | #2
On Sun, May 12, 2013 at 01:29:29AM +0200, Antonio Quartulli wrote:
> On Sat, May 11, 2013 at 07:23:27PM +0200, Linus Lüssing wrote:
> > With this patch a multicast packet is not always simply flooded anymore,
> > the bevahiour for the following cases is changed to reduce
> > unnecessary overhead:
> > 
> > If all nodes within the horizon of a certain node have signalized
> > multicast listener announcement capability
> > (BATADV_MCAST_LISTENER_ANNOUNCEMENT) then an IPv6 multicast packet
> > with a destination of IPv6 link-local scope coming from the upstream
> > of this node...
> > 
> > * ...is dropped if there is no according multicast listener in the
> >   translation table.
> > * ...is forwarded via unicast if there is a single node with interested
> >   multicast listeners.
> > 
> 
> othwerwise? Does it get flooded like now if there is more than one receiver?

Yes, it will get flooded. Thought it was clear because I was
saying "for the following cases is changed", trying to imply that
for anything else it'll still get flooded. But ok, I can make it
more explicit.

> 
> >  
> >  /**
> > + * batadv_mcast_flood - Checks on how to forward a multicast packet
> > + * @skb: The multicast packet to check
> > + * @bat_priv: the bat priv with all the soft interface information
> > + *
> > + * Returns 1 if the packet should be flooded, 0 if it should be forwarded
> > + * via unicast or -1 if it should be drooped.
> > + */
> > +int batadv_mcast_flood(struct sk_buff *skb, struct batadv_priv *bat_priv)
> > +{
> > +	struct ethhdr *ethhdr = (struct ethhdr *)(skb->data);
> > +	struct ipv6hdr *ip6hdr;
> > +	int count, ret = 1;
> > +
> > +	if (atomic_read(&bat_priv->mcast_group_awareness) &&
> > +	    !atomic_read(&bat_priv->mcast_num_non_aware) &&
> > +	    ntohs(ethhdr->h_proto) == ETH_P_IPV6) {
> 
> mh..this would not work for VLANs..did you plan to introduce support for VLANs
> later? or you simply overlooked it? :)

I guess I overlooked, will try to add that in the same patch, too.

> 
> > +		if (!pskb_may_pull(skb, sizeof(*ethhdr) + sizeof(*ip6hdr))) {
> > +			ret = -1;
> > +			goto out;
> > +		}
> > +
> > +		ip6hdr = ipv6_hdr(skb);
> > +
> > +		/* TODO: Implement Multicast Router Discovery, then add
> > +		 * scope >= IPV6_ADDR_SCOPE_LINKLOCAL, too */
> > +		if (IPV6_ADDR_MC_SCOPE(&ip6hdr->daddr) !=
> > +		    IPV6_ADDR_SCOPE_LINKLOCAL)
> > +			goto out;
> > +
> > +		count = batadv_tt_global_hash_count(bat_priv, ethhdr->h_dest);
> > +
> > +		if (!count)
> > +			ret = -1;
> > +		else if (count == 1)
> > +			ret = 0;
> 
> how can this function return more than one?
> When there is more than one originator announcing the same MAC address then we
> have _a single_ global entry having a list of orig_entry. but stil only one
> global entry.
> 
> so you may want to count the orig_entries rather than the global_entries?

You are right, looks like I only tested dropping vs. unicast, not
unicast vs. flooding behaviour in my VMs.

> 
> > diff --git a/translation-table.c b/translation-table.c
> > index 37e7d47..1d2d618 100644
> > --- a/translation-table.c
> > +++ b/translation-table.c
> > @@ -83,6 +83,40 @@ batadv_tt_hash_find(struct batadv_hashtable *hash, const void *data)
> >  	return tt_common_entry_tmp;
> >  }
> >  
> > +/**
> > + * batadv_tt_hash_count - Counts the number of tt entries for the given data
> > + * @hash: hash table containing the tt entries
> > + * @data: The data to count entries for
> 
> One line saying what you are returning would be nice :)

ok

> 
> > + */
> > +static int batadv_tt_hash_count(struct batadv_hashtable *hash, const void *data)
> > +{
> > +	struct hlist_head *head;
> > +	struct batadv_tt_common_entry *tt_common_entry;
> > +	uint32_t index;
> > +	int count = 0;
> > +
> > +	if (!hash)
> > +		goto out;
> > +
> > +	index = batadv_choose_orig(data, hash->size);
> > +	head = &hash->table[index];
> > +
> > +	rcu_read_lock();
> > +	hlist_for_each_entry_rcu(tt_common_entry, head, hash_entry) {
> > +		if (!batadv_compare_eth(tt_common_entry, data))
> > +			continue;
> > +
> > +		if (!atomic_read(&tt_common_entry->refcount))
> > +			continue;
> > +
> > +		count++;
> > +	}
> > +	rcu_read_unlock();
> > +
> > +out:
> > +	return count;
> > +}
> 
> as I asked before: this function cannot return >1 because the same address is
> never stored twice.
> 
> 
> Nice job so far!
> Thanks for working on this cool feature!
> 
> Cheers,
> 
> -- 
> Antonio Quartulli
> 
> ..each of us alone is worth nothing..
> Ernesto "Che" Guevara

Cheers, Linus
  
Antonio Quartulli May 16, 2013, 7:43 p.m. UTC | #3
On Thu, May 16, 2013 at 08:22:25PM +0200, Linus Lüssing wrote:
> On Sun, May 12, 2013 at 01:29:29AM +0200, Antonio Quartulli wrote:
> > On Sat, May 11, 2013 at 07:23:27PM +0200, Linus Lüssing wrote:
> > > With this patch a multicast packet is not always simply flooded anymore,
> > > the bevahiour for the following cases is changed to reduce
> > > unnecessary overhead:
> > > 
> > > If all nodes within the horizon of a certain node have signalized
> > > multicast listener announcement capability
> > > (BATADV_MCAST_LISTENER_ANNOUNCEMENT) then an IPv6 multicast packet
> > > with a destination of IPv6 link-local scope coming from the upstream
> > > of this node...
> > > 
> > > * ...is dropped if there is no according multicast listener in the
> > >   translation table.
> > > * ...is forwarded via unicast if there is a single node with interested
> > >   multicast listeners.
> > > 
> > 
> > othwerwise? Does it get flooded like now if there is more than one receiver?
> 
> Yes, it will get flooded. Thought it was clear because I was
> saying "for the following cases is changed", trying to imply that
> for anything else it'll still get flooded. But ok, I can make it
> more explicit.

Yes, please

Cheers,
  

Patch

diff --git a/multicast.c b/multicast.c
index 95beac4..673d5f1 100644
--- a/multicast.c
+++ b/multicast.c
@@ -218,6 +218,48 @@  out:
 }
 
 /**
+ * batadv_mcast_flood - Checks on how to forward a multicast packet
+ * @skb: The multicast packet to check
+ * @bat_priv: the bat priv with all the soft interface information
+ *
+ * Returns 1 if the packet should be flooded, 0 if it should be forwarded
+ * via unicast or -1 if it should be drooped.
+ */
+int batadv_mcast_flood(struct sk_buff *skb, struct batadv_priv *bat_priv)
+{
+	struct ethhdr *ethhdr = (struct ethhdr *)(skb->data);
+	struct ipv6hdr *ip6hdr;
+	int count, ret = 1;
+
+	if (atomic_read(&bat_priv->mcast_group_awareness) &&
+	    !atomic_read(&bat_priv->mcast_num_non_aware) &&
+	    ntohs(ethhdr->h_proto) == ETH_P_IPV6) {
+		if (!pskb_may_pull(skb, sizeof(*ethhdr) + sizeof(*ip6hdr))) {
+			ret = -1;
+			goto out;
+		}
+
+		ip6hdr = ipv6_hdr(skb);
+
+		/* TODO: Implement Multicast Router Discovery, then add
+		 * scope >= IPV6_ADDR_SCOPE_LINKLOCAL, too */
+		if (IPV6_ADDR_MC_SCOPE(&ip6hdr->daddr) !=
+		    IPV6_ADDR_SCOPE_LINKLOCAL)
+			goto out;
+
+		count = batadv_tt_global_hash_count(bat_priv, ethhdr->h_dest);
+
+		if (!count)
+			ret = -1;
+		else if (count == 1)
+			ret = 0;
+	}
+
+out:
+	return ret;
+}
+
+/**
  * batadv_mcast_tvlv_ogm_handler_v1 - process incoming multicast tvlv container
  * @bat_priv: the bat priv with all the soft interface information
  * @orig: the orig_node of the ogm
diff --git a/multicast.h b/multicast.h
index 9955a18..0c2baad 100644
--- a/multicast.h
+++ b/multicast.h
@@ -24,6 +24,8 @@ 
 
 void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv);
 
+int batadv_mcast_flood(struct sk_buff *skb, struct batadv_priv *bat_priv);
+
 int batadv_mcast_init(struct batadv_priv *bat_priv);
 
 void batadv_mcast_free(struct batadv_priv *bat_priv);
@@ -35,6 +37,12 @@  static inline void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv)
 	return;
 }
 
+static inline int batadv_mcast_flood(struct sk_buff *skb,
+				     struct batadv_priv *bat_priv)
+{
+	return 1;
+}
+
 static inline int batadv_mcast_init(struct batadv_priv *bat_priv)
 {
 	return 0;
diff --git a/soft-interface.c b/soft-interface.c
index c1142c2..831a618 100644
--- a/soft-interface.c
+++ b/soft-interface.c
@@ -36,6 +36,7 @@ 
 #include <linux/if_vlan.h>
 #include <linux/if_ether.h>
 #include "unicast.h"
+#include "multicast.h"
 #include "bridge_loop_avoidance.h"
 #include "network-coding.h"
 
@@ -222,6 +223,15 @@  static int batadv_interface_tx(struct sk_buff *skb,
 		}
 	}
 
+	if (do_bcast && !is_broadcast_ether_addr(ethhdr->h_dest)) {
+		ret = batadv_mcast_flood(skb, bat_priv);
+		if (ret < 0)
+			goto dropped;
+
+		if (!ret)
+			do_bcast = false;
+	}
+
 	/* ethernet packet should be broadcasted */
 	if (do_bcast) {
 		primary_if = batadv_primary_if_get_selected(bat_priv);
diff --git a/translation-table.c b/translation-table.c
index 37e7d47..1d2d618 100644
--- a/translation-table.c
+++ b/translation-table.c
@@ -83,6 +83,40 @@  batadv_tt_hash_find(struct batadv_hashtable *hash, const void *data)
 	return tt_common_entry_tmp;
 }
 
+/**
+ * batadv_tt_hash_count - Counts the number of tt entries for the given data
+ * @hash: hash table containing the tt entries
+ * @data: The data to count entries for
+ */
+static int batadv_tt_hash_count(struct batadv_hashtable *hash, const void *data)
+{
+	struct hlist_head *head;
+	struct batadv_tt_common_entry *tt_common_entry;
+	uint32_t index;
+	int count = 0;
+
+	if (!hash)
+		goto out;
+
+	index = batadv_choose_orig(data, hash->size);
+	head = &hash->table[index];
+
+	rcu_read_lock();
+	hlist_for_each_entry_rcu(tt_common_entry, head, hash_entry) {
+		if (!batadv_compare_eth(tt_common_entry, data))
+			continue;
+
+		if (!atomic_read(&tt_common_entry->refcount))
+			continue;
+
+		count++;
+	}
+	rcu_read_unlock();
+
+out:
+	return count;
+}
+
 static struct batadv_tt_local_entry *
 batadv_tt_local_hash_find(struct batadv_priv *bat_priv, const void *data)
 {
@@ -111,6 +145,16 @@  batadv_tt_global_hash_find(struct batadv_priv *bat_priv, const void *data)
 	return tt_global_entry;
 }
 
+/**
+ * batadv_tt_hash_count - Counts the number of global tt entries for some data
+ * @bat_priv: the bat priv with all the soft interface information
+ * @data: The data to count entries for
+ */
+int batadv_tt_global_hash_count(struct batadv_priv *bat_priv, const void *data)
+{
+	return batadv_tt_hash_count(bat_priv->tt.global_hash, data);
+}
+
 static void
 batadv_tt_local_entry_free_ref(struct batadv_tt_local_entry *tt_local_entry)
 {
diff --git a/translation-table.h b/translation-table.h
index 015d8b9..5986c57 100644
--- a/translation-table.h
+++ b/translation-table.h
@@ -31,6 +31,7 @@  int batadv_tt_global_seq_print_text(struct seq_file *seq, void *offset);
 void batadv_tt_global_del_orig(struct batadv_priv *bat_priv,
 			       struct batadv_orig_node *orig_node,
 			       const char *message);
+int batadv_tt_global_hash_count(struct batadv_priv *bat_priv, const void *data);
 struct batadv_orig_node *batadv_transtable_search(struct batadv_priv *bat_priv,
 						  const uint8_t *src,
 						  const uint8_t *addr);