[v2,3/3] batman-adv: Modified forwarding behaviour for multicast packets
Commit Message
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
* ...and otherwise still gets flooded.
Signed-off-by: Linus Lüssing <linus.luessing@web.de>
---
multicast.c | 43 +++++++++++++++++++++++++++++++++
multicast.h | 8 +++++++
soft-interface.c | 10 ++++++++
translation-table.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++
translation-table.h | 1 +
5 files changed, 128 insertions(+)
Comments
Only a few style suggestions here, nothing critical and you can ignore
them if you don't like them. :)
On Fri, May 24, 2013 at 10:02:28AM +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
> * ...and otherwise still gets flooded.
>
> Signed-off-by: Linus Lüssing <linus.luessing@web.de>
> ---
> multicast.c | 43 +++++++++++++++++++++++++++++++++
> multicast.h | 8 +++++++
> soft-interface.c | 10 ++++++++
> translation-table.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++
> translation-table.h | 1 +
> 5 files changed, 128 insertions(+)
>
> diff --git a/multicast.c b/multicast.c
> index 36e4c59..bd55c8f 100644
> --- a/multicast.c
> +++ b/multicast.c
> @@ -213,6 +213,49 @@ out:
> }
>
> /**
> + * batadv_mcast_flood - check on how to forward a multicast packet
> + * @skb: The multicast packet to check
> + * @bat_priv: the bat priv with all the soft interface information
> + *
> + * Return 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) {
You can safe an indendation below if you return -1 immediately here
if the statement above is false. Also multiple statements might be better
for readability and later changes, e.g.
if (!atomic_read(&bat_priv->mcast_group_awareness))
return 1;
if (atomic_read(&bat_priv->mcast.num_non_aware))
return 1;
if (ntohs(ethhdr->h_proto) != ETH_P_IPV6)
return 1;
> + if (!pskb_may_pull(skb, sizeof(*ethhdr) + sizeof(*ip6hdr))) {
> + ret = -1;
> + goto out;
> + }
You could directly return -1 here, the out label is not needed
(as we don't unlock/free anything here).
I don't quite understand why you return -1, maybe the packet could still
be forwarded even if it could not be pulled?
> +
> + 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;
> + }
You could use a switch statement here instead for readability, e.g.:
switch (count) {
case 0:
return -1;
case 1:
return 0;
default:
return 1;
}
> +
> +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/soft-interface.c b/soft-interface.c
> index 8bdd649..83e4679 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)) {
I'd suggest to put this inside the "is_multicast_etheraddr()" above to make more clear
that this handles multicast packets. I was a little confused by the
do_bcast && !is_broadcast_ether_addr() first, but that might just be me.
Cheers,
Simon
On Tuesday, May 28, 2013 23:40:54 Simon Wunderlich wrote:
> You can safe an indendation below if you return -1 immediately here
> if the statement above is false. Also multiple statements might be better
> for readability and later changes, e.g.
>
> if (!atomic_read(&bat_priv->mcast_group_awareness))
> return 1;
>
> if (atomic_read(&bat_priv->mcast.num_non_aware))
> return 1;
>
> if (ntohs(ethhdr->h_proto) != ETH_P_IPV6)
> return 1;
A quick hint from my end: The kernel maintainer don't like code that returns
"random" values like 1/0/-1. Either use bool or meaningful defines instead of
raw numbers.
Cheers,
Marek
On Wed, May 29, 2013 at 12:39:33AM +0800, Marek Lindner wrote:
> On Tuesday, May 28, 2013 23:40:54 Simon Wunderlich wrote:
> > You can safe an indendation below if you return -1 immediately here
> > if the statement above is false. Also multiple statements might be better
> > for readability and later changes, e.g.
> >
> > if (!atomic_read(&bat_priv->mcast_group_awareness))
> > return 1;
> >
> > if (atomic_read(&bat_priv->mcast.num_non_aware))
> > return 1;
> >
> > if (ntohs(ethhdr->h_proto) != ETH_P_IPV6)
> > return 1;
>
> A quick hint from my end: The kernel maintainer don't like code that returns
> "random" values like 1/0/-1. Either use bool or meaningful defines instead of
> raw numbers.
Very good suggestion.
I think you can define an enum for this. It will make the code much easier to
understand.
Cheers,
@@ -213,6 +213,49 @@ out:
}
/**
+ * batadv_mcast_flood - check on how to forward a multicast packet
+ * @skb: The multicast packet to check
+ * @bat_priv: the bat priv with all the soft interface information
+ *
+ * Return 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
@@ -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;
@@ -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);
@@ -83,6 +83,72 @@ batadv_tt_hash_find(struct batadv_hashtable *hash, const void *data)
return tt_common_entry_tmp;
}
+/**
+ * batadv_tt_orig_entries_count - count the number of originators
+ * @head: a list of originators
+ *
+ * Return the number of originator entries in the given list.
+ *
+ * The caller needs to hold the rcu_read_lock().
+ */
+static int batadv_tt_orig_entries_count(struct hlist_head *head)
+{
+ struct batadv_tt_orig_list_entry *orig_entry;
+ int count = 0;
+
+ hlist_for_each_entry_rcu(orig_entry, head, list) {
+ if (!atomic_read(&orig_entry->refcount))
+ continue;
+
+ count++;
+ }
+
+ return count;
+}
+
+/**
+ * batadv_tt_global_hash_count - count the number of orig entries
+ * @hash: hash table containing the tt entries
+ * @data: the data to count entries for
+ *
+ * Return the number of originators advertising the given address/data
+ * (excluding ourself).
+ */
+int batadv_tt_global_hash_count(struct batadv_priv *bat_priv, const void *data)
+{
+ struct hlist_head *head, *orig_list;
+ struct batadv_tt_common_entry *tt_common_entry;
+ struct batadv_tt_global_entry *tt_global_entry;
+ uint32_t index;
+ int count = 0;
+
+ if (!bat_priv->tt.global_hash)
+ goto out;
+
+ index = batadv_choose_orig(data, bat_priv->tt.global_hash->size);
+ head = &bat_priv->tt.global_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;
+
+ tt_global_entry = container_of(tt_common_entry,
+ struct batadv_tt_global_entry,
+ common);
+ orig_list = &tt_global_entry->orig_list;
+ count = batadv_tt_orig_entries_count(orig_list);
+ break;
+ }
+ 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)
{
@@ -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);