This set of patches is the first one for a more efficient, group aware multicast forwarding infrastructure in batman-adv.
This initial set mainly consists of the integration of announcing the location of multicast listeners via the translation table mechanism to be able to find out which batman-adv nodes are actually interested in certain multicast traffic. As well as the signalizing of this announcement capability via a new multicast TVLV.
Finally some basic multicast forwarding opitimizations are introduced: If all nodes signalized the MLA capability then link-local IPv6 traffic will be dropped if there is no interested listener or gets forwarded via a batman-adv unicast packet if there is just one node interested in the data.
For now, these optimizations only apply if all nodes in the mesh have no bridge interface on top their bat0 interface. However making it possible with bridges, too, and other features are on the roadmap. See the according wiki page for details [1].
[1]: http://www.open-mesh.org/projects/batman-adv/wiki/Multicast-ideas-updated#Ro...
With this patch a node which has no bridge interface on top of its soft interface announces its local multicast listeners via the translation table.
Signed-off-by: Linus Lüssing linus.luessing@web.de --- Makefile | 2 + Makefile.kbuild | 1 + compat.c | 21 +++++ compat.h | 18 ++++ gen-compat-autoconf.sh | 1 + main.c | 6 ++ main.h | 9 ++ multicast.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++++ multicast.h | 43 ++++++++++ soft-interface.c | 3 + sysfs.c | 6 ++ translation-table.c | 17 +++- types.h | 12 +++ 13 files changed, 358 insertions(+), 3 deletions(-) create mode 100644 multicast.c create mode 100644 multicast.h
diff --git a/Makefile b/Makefile index 407cdc4..d7c6fa6 100644 --- a/Makefile +++ b/Makefile @@ -27,6 +27,8 @@ export CONFIG_BATMAN_ADV_BLA=y export CONFIG_BATMAN_ADV_DAT=y # B.A.T.M.A.N network coding (catwoman): export CONFIG_BATMAN_ADV_NC=n +# B.A.T.M.A.N. multicast optimizations: +export CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS=y
PWD:=$(shell pwd) KERNELPATH ?= /lib/modules/$(shell uname -r)/build diff --git a/Makefile.kbuild b/Makefile.kbuild index 8ddbfe6..d3efe3a 100644 --- a/Makefile.kbuild +++ b/Makefile.kbuild @@ -38,3 +38,4 @@ batman-adv-y += soft-interface.o batman-adv-y += sysfs.o batman-adv-y += translation-table.o batman-adv-y += unicast.o +batman-adv-$(CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS) += multicast.o diff --git a/compat.c b/compat.c index 1f3a39d..27df197 100644 --- a/compat.c +++ b/compat.c @@ -25,6 +25,27 @@ #include <linux/version.h> #include "main.h"
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 38) + +void batadv_for_each_pmc_rcu_init(struct in_device *in_dev, + struct ip_mc_list **pmc) +{ + read_lock(&in_dev->mc_list_lock); + *pmc = in_dev->mc_list; +} + +bool batadv_for_each_pmc_rcu_check(struct in_device *in_dev, + struct ip_mc_list *pmc) +{ + if (pmc == NULL) + read_unlock(&in_dev->mc_list_lock); + + return (pmc != NULL); +} + +#endif /* < KERNEL_VERSION(2, 6, 38) */ + + #if LINUX_VERSION_CODE < KERNEL_VERSION(3, 0, 0)
void batadv_free_rcu_gw_node(struct rcu_head *rcu) diff --git a/compat.h b/compat.h index 8bd4f62..de1556a 100644 --- a/compat.h +++ b/compat.h @@ -152,6 +152,24 @@ static inline int batadv_param_set_copystring(const char *val,
#endif /* < KERNEL_VERSION(2, 6, 37) */
+ +#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 38) + +void batadv_for_each_pmc_rcu_init(struct in_device *in_dev, + struct ip_mc_list **pmc); +bool batadv_for_each_pmc_rcu_check(struct in_device *in_dev, + struct ip_mc_list *pmc); + +#undef for_each_pmc_rcu +#define for_each_pmc_rcu(in_dev, pmc) \ + for (batadv_for_each_pmc_rcu_init(in_dev, &pmc); \ + batadv_for_each_pmc_rcu_check(in_dev, pmc); \ + pmc = NULL) \ + for (; pmc != NULL; pmc = pmc->next) + +#endif /* < KERNEL_VERSION(2, 6, 38) */ + + #if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 39)
#define kstrtoul strict_strtoul diff --git a/gen-compat-autoconf.sh b/gen-compat-autoconf.sh index 78573e4..b56ac29 100755 --- a/gen-compat-autoconf.sh +++ b/gen-compat-autoconf.sh @@ -39,6 +39,7 @@ gen_config() { gen_config 'CONFIG_BATMAN_ADV_DEBUG' ${CONFIG_BATMAN_ADV_DEBUG:="n"} >> "${TMP}" gen_config 'CONFIG_BATMAN_ADV_BLA' ${CONFIG_BATMAN_ADV_BLA:="y"} >> "${TMP}" gen_config 'CONFIG_BATMAN_ADV_DAT' ${CONFIG_BATMAN_ADV_DAT:="y"} >> "${TMP}" +gen_config 'CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS' ${CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS:="y"} >> "${TMP}" gen_config 'CONFIG_BATMAN_ADV_NC' ${CONFIG_BATMAN_ADV_NC:="n"} >> "${TMP}"
# only regenerate compat-autoconf.h when config was changed diff --git a/main.c b/main.c index af89c09..1c82c18 100644 --- a/main.c +++ b/main.c @@ -33,6 +33,7 @@ #include "bridge_loop_avoidance.h" #include "distributed-arp-table.h" #include "unicast.h" +#include "multicast.h" #include "gateway_common.h" #include "hash.h" #include "bat_algo.h" @@ -115,6 +116,9 @@ int batadv_mesh_init(struct net_device *soft_iface) INIT_LIST_HEAD(&bat_priv->tt.changes_list); INIT_LIST_HEAD(&bat_priv->tt.req_list); INIT_LIST_HEAD(&bat_priv->tt.roam_list); +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + INIT_LIST_HEAD(&bat_priv->mcast.mla_list); +#endif INIT_HLIST_HEAD(&bat_priv->tvlv.container_list); INIT_HLIST_HEAD(&bat_priv->tvlv.handler_list);
@@ -168,6 +172,8 @@ void batadv_mesh_free(struct net_device *soft_iface) batadv_dat_free(bat_priv); batadv_bla_free(bat_priv);
+ batadv_mcast_free(bat_priv); + /* Free the TT and the originator tables only after having terminated * all the other depending components which may use these structures for * their purposes. diff --git a/main.h b/main.h index 795345f..39c683d 100644 --- a/main.h +++ b/main.h @@ -141,6 +141,14 @@ enum batadv_uev_type { /* Append 'batman-adv: ' before kernel messages */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define UINT8_MAX 255 + +/* identical to the one used in net/ipv4/igmp.c */ +#define for_each_pmc_rcu(in_dev, pmc) \ + for (pmc = rcu_dereference(in_dev->mc_list); \ + pmc != NULL; \ + pmc = rcu_dereference(pmc->next_rcu)) + /* Kernel headers */
#include <linux/mutex.h> /* mutex */ @@ -155,6 +163,7 @@ enum batadv_uev_type { #include <linux/percpu.h> #include <linux/slab.h> #include <net/sock.h> /* struct sock */ +#include <net/addrconf.h> /* ipv6 address stuff */ #include <net/rtnetlink.h> #include <linux/jiffies.h> #include <linux/seq_file.h> diff --git a/multicast.c b/multicast.c new file mode 100644 index 0000000..88bde61 --- /dev/null +++ b/multicast.c @@ -0,0 +1,222 @@ +/* Copyright (C) 2013 B.A.T.M.A.N. contributors: + * + * Linus Lüssing + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of version 2 of the GNU General Public + * License as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA + */ + +#include "main.h" +#include "originator.h" +#include "hard-interface.h" +#include "translation-table.h" + +/* should match batadv_ogm_packet's mcast_num_mla */ +#define BATADV_MLA_MAX UINT8_MAX + +/** + * batadv_mcast_mla_local_collect - Collects local multicast listeners + * @dev: The device to collect multicast addresses from + * @mcast_list: A list to put found addresses into + * @num_mla_max: The maximum number of items to add + * + * Collects up to num_mla_max multicast addresses of the local multicast + * listeners on the given interface, dev, in the given mcast_list. + * + * Returns -ENOMEM on memory allocation error or the number of + * items added to the mcast_list otherwise. + */ +static int batadv_mcast_mla_local_collect(struct net_device *dev, + struct list_head *mcast_list, + int num_mla_max) { + struct netdev_hw_addr *mc_list_entry, *new; + int num_mla = 0, ret = 0; + + netif_addr_lock_bh(dev); + netdev_for_each_mc_addr(mc_list_entry, dev) { + if (num_mla >= num_mla_max) { + pr_warn("Too many local multicast listener announcements here, just adding %i\n", + num_mla_max); + break; + } + + new = kmalloc(sizeof(struct netdev_hw_addr), GFP_ATOMIC); + if (!new) { + ret = -ENOMEM; + break; + } + + memcpy(&new->addr, &mc_list_entry->addr, ETH_ALEN); + list_add(&new->list, mcast_list); + num_mla++; + } + netif_addr_unlock_bh(dev); + + return ret < 0 ? ret : num_mla; +} + +/** + * batadv_mcast_mla_is_duplicate - Checks whether an address is in a list + * @mcast_addr: The multicast address to check + * @mcast_list: The list with multicast addresses to search in + * + * Returns true if the given address is already in the given list. + * Otherwise returns false. + */ +static bool batadv_mcast_mla_is_duplicate(uint8_t *mcast_addr, + struct list_head *mcast_list) +{ + struct netdev_hw_addr *mcast_entry; + + list_for_each_entry(mcast_entry, mcast_list, list) + if (!memcmp(mcast_entry->addr, mcast_addr, ETH_ALEN)) + return true; + + return false; +} + +/** + * batadv_mcast_mla_collect_free - Frees a list of multicast addresses + * @mcast_list: The list to free + * + * Removes and frees all items in the given mcast_list. + */ +void batadv_mcast_mla_collect_free(struct list_head *mcast_list) +{ + struct netdev_hw_addr *mcast_entry, *tmp; + + list_for_each_entry_safe(mcast_entry, tmp, mcast_list, list) { + list_del(&mcast_entry->list); + kfree(mcast_entry); + } +} + +/** + * batadv_mcast_mla_tt_clean - Cleans up multicast listener announcements + * @bat_priv: the bat priv with all the soft interface information + * @mcast_list: A list of addresses which should _not_ be removed + * + * This method retracts the announcement of any multicast listener from the + * translation table except the ones listed in the given mcast_list. + */ +static void batadv_mcast_mla_tt_clean(struct batadv_priv *bat_priv, + struct list_head *mcast_list) +{ + struct netdev_hw_addr *mcast_entry, *tmp; + + list_for_each_entry_safe(mcast_entry, tmp, &bat_priv->mcast.mla_list, + list) { + if (batadv_mcast_mla_is_duplicate(mcast_entry->addr, + mcast_list)) + continue; + + batadv_tt_local_remove(bat_priv, mcast_entry->addr, + "mcast TT outdated", 0); + + list_del(&mcast_entry->list); + kfree(mcast_entry); + } +} + +/** + * batadv_mcast_mla_tt_add - Adds multicast listener announcements + * @soft_iface: the soft interface where MLAs get added to + * @mcast_list: A list of addresses which are going to get added + * + * Adds multicast listener announcements from the given mcast_list to the + * translation table if they have not been added yet. + */ +static void batadv_mcast_mla_tt_add(struct net_device *soft_iface, + struct list_head *mcast_list) +{ + struct netdev_hw_addr *mcast_entry, *tmp; + struct batadv_priv *bat_priv = netdev_priv(soft_iface); + + list_for_each_entry_safe(mcast_entry, tmp, mcast_list, list) { + if (batadv_mcast_mla_is_duplicate(mcast_entry->addr, + &bat_priv->mcast.mla_list)) + continue; + + batadv_tt_local_add(soft_iface, mcast_entry->addr, + BATADV_NULL_IFINDEX); + list_move_tail(&mcast_entry->list, &bat_priv->mcast.mla_list); + } +} + +/** + * batadv_mcast_mla_tt_update - Updates the own MLAs + * @bat_priv: the bat priv with all the soft interface information + * + * Updates the own multicast listener announcements in the translation + * table. Also takes care of registering or unregistering the multicast + * tvlv depending on whether the user activated or deactivated + * multicast optimizations. + */ +void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv) +{ + struct batadv_hard_iface *primary_if; + struct net_device *soft_iface; + struct list_head mcast_list; + int ret; + static bool enabled; + + INIT_LIST_HEAD(&mcast_list); + + primary_if = batadv_primary_if_get_selected(bat_priv); + if (!primary_if) + goto out; + + soft_iface = primary_if->soft_iface; + + /* Avoid attaching MLAs, if multicast optimization is disabled + * or there is a bridge on top of our soft interface (TODO) */ + if (!atomic_read(&bat_priv->mcast_group_awareness) || + bat_priv->soft_iface->priv_flags & IFF_BRIDGE_PORT) { + if (enabled) + enabled = false; + + goto update; + } + + if (!enabled) + enabled = true; + + ret = batadv_mcast_mla_local_collect(soft_iface, &mcast_list, + BATADV_MLA_MAX); + if (ret < 0) + goto out; + +update: + batadv_mcast_mla_tt_clean(bat_priv, &mcast_list); + batadv_mcast_mla_tt_add(soft_iface, &mcast_list); + +out: + if (primary_if) + batadv_hardif_free_ref(primary_if); + + batadv_mcast_mla_collect_free(&mcast_list); +} + +/** + * batadv_mcast_free - Frees the multicast optimizaitons structures + * @bat_priv: the bat priv with all the soft interface information + */ +void batadv_mcast_free(struct batadv_priv *bat_priv) +{ + struct list_head mcast_list; + + INIT_LIST_HEAD(&mcast_list); + + batadv_mcast_mla_tt_clean(bat_priv, &mcast_list); +} diff --git a/multicast.h b/multicast.h new file mode 100644 index 0000000..3b68e3b --- /dev/null +++ b/multicast.h @@ -0,0 +1,43 @@ +/* Copyright (C) 2013 B.A.T.M.A.N. contributors: + * + * Linus Lüssing + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of version 2 of the GNU General Public + * License as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301, USA + */ + +#ifndef _NET_BATMAN_ADV_MULTICAST_H_ +#define _NET_BATMAN_ADV_MULTICAST_H_ + +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + +void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv); + +void batadv_mcast_free(struct batadv_priv *bat_priv); + +#else + +static inline void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv) +{ + return; +} + +static inline void batadv_mcast_free(struct batadv_priv *bat_priv) +{ + return; +} + +#endif /* CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS */ + +#endif /* _NET_BATMAN_ADV_MULTICAST_H_ */ diff --git a/soft-interface.c b/soft-interface.c index a6ab9e4..b97dfa2 100644 --- a/soft-interface.c +++ b/soft-interface.c @@ -457,6 +457,9 @@ static int batadv_softif_init_late(struct net_device *dev) #ifdef CONFIG_BATMAN_ADV_DAT atomic_set(&bat_priv->distributed_arp_table, 1); #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + atomic_set(&bat_priv->mcast_group_awareness, 1); +#endif atomic_set(&bat_priv->ap_isolation, 0); atomic_set(&bat_priv->gw_mode, BATADV_GW_MODE_OFF); atomic_set(&bat_priv->gw_sel_class, 20); diff --git a/sysfs.c b/sysfs.c index cbbb77b..76390d0 100644 --- a/sysfs.c +++ b/sysfs.c @@ -369,6 +369,9 @@ BATADV_ATTR_SIF_UINT(gw_sel_class, S_IRUGO | S_IWUSR, 1, BATADV_TQ_MAX_VALUE, batadv_post_gw_deselect); static BATADV_ATTR(gw_bandwidth, S_IRUGO | S_IWUSR, batadv_show_gw_bwidth, batadv_store_gw_bwidth); +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS +BATADV_ATTR_SIF_BOOL(mcast_group_awareness, S_IRUGO | S_IWUSR, NULL); +#endif #ifdef CONFIG_BATMAN_ADV_DEBUG BATADV_ATTR_SIF_UINT(log_level, S_IRUGO | S_IWUSR, 0, BATADV_DBG_ALL, NULL); #endif @@ -386,6 +389,9 @@ static struct batadv_attribute *batadv_mesh_attrs[] = { #ifdef CONFIG_BATMAN_ADV_DAT &batadv_attr_distributed_arp_table, #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + &batadv_attr_mcast_group_awareness, +#endif &batadv_attr_fragmentation, &batadv_attr_ap_isolation, &batadv_attr_routing_algo, diff --git a/translation-table.c b/translation-table.c index df6b5cd..37e7d47 100644 --- a/translation-table.c +++ b/translation-table.c @@ -26,6 +26,7 @@ #include "originator.h" #include "routing.h" #include "bridge_loop_avoidance.h" +#include "multicast.h"
#include <linux/crc32c.h>
@@ -281,7 +282,8 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr, bool roamed_back = false;
tt_local = batadv_tt_local_hash_find(bat_priv, addr); - tt_global = batadv_tt_global_hash_find(bat_priv, addr); + tt_global = is_multicast_ether_addr(addr) ? NULL : + batadv_tt_global_hash_find(bat_priv, addr);
if (tt_local) { tt_local->last_seen = jiffies; @@ -332,8 +334,10 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr, tt_local->last_seen = jiffies; tt_local->common.added_at = tt_local->last_seen;
- /* the batman interface mac address should never be purged */ - if (batadv_compare_eth(addr, soft_iface->dev_addr)) + /* the batman interface mac and multicast addresses should never be + * purged */ + if (batadv_compare_eth(addr, soft_iface->dev_addr) || + is_multicast_ether_addr(addr)) tt_local->common.flags |= BATADV_TT_CLIENT_NOPURGE;
hash_added = batadv_hash_add(bat_priv->tt.local_hash, batadv_compare_tt, @@ -919,6 +923,10 @@ add_orig_entry: ret = true;
out_remove: + /* Do not remove multicast addresses from the local hash on + * global additions */ + if (is_multicast_ether_addr(tt_addr)) + goto out;
/* remove address from local hash if present */ local_flags = batadv_tt_local_remove(bat_priv, tt_addr, @@ -2346,6 +2354,9 @@ void batadv_tt_local_commit_changes(struct batadv_priv *bat_priv) { uint16_t changed_num = 0;
+ /* Update multicast addresses in local translation table */ + batadv_mcast_mla_tt_update(bat_priv); + if (atomic_read(&bat_priv->tt.local_changes) < 1) { if (!batadv_atomic_dec_not_zero(&bat_priv->tt.ogm_append_cnt)) batadv_tt_tvlv_container_update(bat_priv); diff --git a/types.h b/types.h index c84f5cc..5d73a75 100644 --- a/types.h +++ b/types.h @@ -473,6 +473,12 @@ struct batadv_priv_dat { }; #endif
+#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS +struct batadv_priv_mcast { + struct list_head mla_list; +}; +#endif + /** * struct batadv_priv_nc - per mesh interface network coding private data * @work: work queue callback item for cleanup @@ -561,6 +567,9 @@ struct batadv_priv { #ifdef CONFIG_BATMAN_ADV_DAT atomic_t distributed_arp_table; #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + atomic_t mcast_group_awareness; +#endif atomic_t gw_mode; atomic_t gw_sel_class; atomic_t orig_interval; @@ -595,6 +604,9 @@ struct batadv_priv { #ifdef CONFIG_BATMAN_ADV_DAT struct batadv_priv_dat dat; #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + struct batadv_priv_mcast mcast; +#endif #ifdef CONFIG_BATMAN_ADV_NC atomic_t network_coding; struct batadv_priv_nc nc;
Hey Linus,
here you have some comments inline
On Sat, May 11, 2013 at 07:23:25PM +0200, Linus Lüssing wrote:
diff --git a/main.h b/main.h index 795345f..39c683d 100644 --- a/main.h +++ b/main.h @@ -141,6 +141,14 @@ enum batadv_uev_type { /* Append 'batman-adv: ' before kernel messages */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define UINT8_MAX 255
I think you should define it as 255U, otherwise it would get the int type and may generate warnings somewhere (or also unexpected result sometime..)
+/* identical to the one used in net/ipv4/igmp.c */ +#define for_each_pmc_rcu(in_dev, pmc) \
- for (pmc = rcu_dereference(in_dev->mc_list); \
pmc != NULL; \pmc = rcu_dereference(pmc->next_rcu))
Since it is exactly the same code reported in net/ipv4/igmp.c and since it is a define, don't you thin kit is worth trying to send a patch to netdev to move the define in a proper header file so that we/everybody can re-use it?
+/**
- batadv_mcast_mla_local_collect - Collects local multicast listeners
please start the description with a small letter (like we do everywhere..just keep the same style)
- @dev: The device to collect multicast addresses from
don't put tabs between the name of the arg and the description. don't use the capital letter. Just:
@dev: the device..
would be nice :)
+static int batadv_mcast_mla_local_collect(struct net_device *dev,
struct list_head *mcast_list,int num_mla_max) {
The opening parenthesis of a function must go at the beginning of the next line.
- struct netdev_hw_addr *mc_list_entry, *new;
- int num_mla = 0, ret = 0;
- netif_addr_lock_bh(dev);
- netdev_for_each_mc_addr(mc_list_entry, dev) {
if (num_mla >= num_mla_max) {pr_warn("Too many local multicast listener announcements here, just adding %i\n",num_mla_max);
why pr_warn and not just a debug message? Is this a symptom of a possible malfunctioning?
break;}new = kmalloc(sizeof(struct netdev_hw_addr), GFP_ATOMIC);
please use sizeof(*new) here. we agreed on this some time ago. This helps because if we decide to change the type of new in the future then we do not need to change all the kmalloc invocations.
+static bool batadv_mcast_mla_is_duplicate(uint8_t *mcast_addr,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry;
- list_for_each_entry(mcast_entry, mcast_list, list)
if (!memcmp(mcast_entry->addr, mcast_addr, ETH_ALEN))
in main.h we have batadv_compare_eth(), you can re-use it here
+static void batadv_mcast_mla_tt_clean(struct batadv_priv *bat_priv,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry, *tmp;
- list_for_each_entry_safe(mcast_entry, tmp, &bat_priv->mcast.mla_list,
list) {if (batadv_mcast_mla_is_duplicate(mcast_entry->addr,mcast_list))continue;batadv_tt_local_remove(bat_priv, mcast_entry->addr,"mcast TT outdated", 0);
^^^
parameter is bool, please use "false" instead of 0
+static void batadv_mcast_mla_tt_add(struct net_device *soft_iface,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry, *tmp;
- struct batadv_priv *bat_priv = netdev_priv(soft_iface);
why not simply pass bat_priv as first argument (as you do in all the other functions) and then use bat_priv->soft_iface when needed ? (this soft_iface member has been added recently to save code for lazy people :))
+/**
- batadv_mcast_mla_tt_update - Updates the own MLAs
- @bat_priv: the bat priv with all the soft interface information
- Updates the own multicast listener announcements in the translation
For some reason we agreed on not using the third person while describing the function. For consistency I'd suggest you to do the same your kernel doc.
- table. Also takes care of registering or unregistering the multicast
- tvlv depending on whether the user activated or deactivated
- multicast optimizations.
- */
+void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv) +{
- struct batadv_hard_iface *primary_if;
- struct net_device *soft_iface;
- struct list_head mcast_list;
- int ret;
- static bool enabled;
- INIT_LIST_HEAD(&mcast_list);
- primary_if = batadv_primary_if_get_selected(bat_priv);
- if (!primary_if)
goto out;- soft_iface = primary_if->soft_iface;
- /* Avoid attaching MLAs, if multicast optimization is disabled
* or there is a bridge on top of our soft interface (TODO) */
/* comments should * be closed * like this */
/* not like * this */
(this is something checkpatch should also tell you (but I guess only when your code is in the net/ folder inside the kernel tree...yes it is a netdev rule only :))
+/**
- batadv_mcast_free - Frees the multicast optimizaitons structures
- @bat_priv: the bat priv with all the soft interface information
- */
+void batadv_mcast_free(struct batadv_priv *bat_priv) +{
- struct list_head mcast_list;
- INIT_LIST_HEAD(&mcast_list);
- batadv_mcast_mla_tt_clean(bat_priv, &mcast_list);
can't you make batadv_mcast_mla_tt_clean() take a NULL second argument and in that case skip the check in the loop? would be cleaner I guess? what do you think?
diff --git a/translation-table.c b/translation-table.c index df6b5cd..37e7d47 100644 --- a/translation-table.c +++ b/translation-table.c @@ -26,6 +26,7 @@ #include "originator.h" #include "routing.h" #include "bridge_loop_avoidance.h" +#include "multicast.h"
#include <linux/crc32c.h>
@@ -281,7 +282,8 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr, bool roamed_back = false;
tt_local = batadv_tt_local_hash_find(bat_priv, addr);
- tt_global = batadv_tt_global_hash_find(bat_priv, addr);
- tt_global = is_multicast_ether_addr(addr) ? NULL :
batadv_tt_global_hash_find(bat_priv, addr);
what about:
tt_global = NULL; (you can put this directly in the declaration) if (!is_multicast_ether_addr(addr)) tt_global = batadv_tt_global_hash_find(bat_priv, addr);
it is a bit easier to read, no?
if (tt_local) { tt_local->last_seen = jiffies; @@ -332,8 +334,10 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr, tt_local->last_seen = jiffies; tt_local->common.added_at = tt_local->last_seen;
- /* the batman interface mac address should never be purged */
- if (batadv_compare_eth(addr, soft_iface->dev_addr))
- /* the batman interface mac and multicast addresses should never be
* purged */
comment style like above
if (batadv_compare_eth(addr, soft_iface->dev_addr) ||
is_multicast_ether_addr(addr))tt_local->common.flags |= BATADV_TT_CLIENT_NOPURGE;
hash_added = batadv_hash_add(bat_priv->tt.local_hash, batadv_compare_tt,
@@ -919,6 +923,10 @@ add_orig_entry: ret = true;
out_remove:
/* Do not remove multicast addresses from the local hash on
* global additions */if (is_multicast_ether_addr(tt_addr))
goto out;/* remove address from local hash if present */ local_flags = batadv_tt_local_remove(bat_priv, tt_addr,
@@ -2346,6 +2354,9 @@ void batadv_tt_local_commit_changes(struct batadv_priv *bat_priv) { uint16_t changed_num = 0;
- /* Update multicast addresses in local translation table */
- batadv_mcast_mla_tt_update(bat_priv);
- if (atomic_read(&bat_priv->tt.local_changes) < 1) { if (!batadv_atomic_dec_not_zero(&bat_priv->tt.ogm_append_cnt)) batadv_tt_tvlv_container_update(bat_priv);
diff --git a/types.h b/types.h index c84f5cc..5d73a75 100644 --- a/types.h +++ b/types.h @@ -473,6 +473,12 @@ struct batadv_priv_dat { }; #endif
+#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS +struct batadv_priv_mcast {
- struct list_head mla_list;
+}; +#endif
/**
- struct batadv_priv_nc - per mesh interface network coding private data
- @work: work queue callback item for cleanup
@@ -561,6 +567,9 @@ struct batadv_priv { #ifdef CONFIG_BATMAN_ADV_DAT atomic_t distributed_arp_table; #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- atomic_t mcast_group_awareness;
+#endif atomic_t gw_mode; atomic_t gw_sel_class; atomic_t orig_interval; @@ -595,6 +604,9 @@ struct batadv_priv { #ifdef CONFIG_BATMAN_ADV_DAT struct batadv_priv_dat dat; #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- struct batadv_priv_mcast mcast;
+#endif #ifdef CONFIG_BATMAN_ADV_NC atomic_t network_coding; struct batadv_priv_nc nc; -- 1.7.10.4
Cheers,
Hi Antonio,
On Sun, May 12, 2013 at 12:55:25AM +0200, Antonio Quartulli wrote:
Hey Linus,
here you have some comments inline
On Sat, May 11, 2013 at 07:23:25PM +0200, Linus Lüssing wrote:
diff --git a/main.h b/main.h index 795345f..39c683d 100644 --- a/main.h +++ b/main.h @@ -141,6 +141,14 @@ enum batadv_uev_type { /* Append 'batman-adv: ' before kernel messages */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#define UINT8_MAX 255
I think you should define it as 255U, otherwise it would get the int type and may generate warnings somewhere (or also unexpected result sometime..)
ok
+/* identical to the one used in net/ipv4/igmp.c */ +#define for_each_pmc_rcu(in_dev, pmc) \
- for (pmc = rcu_dereference(in_dev->mc_list); \
pmc != NULL; \pmc = rcu_dereference(pmc->next_rcu))Since it is exactly the same code reported in net/ipv4/igmp.c and since it is a define, don't you thin kit is worth trying to send a patch to netdev to move the define in a proper header file so that we/everybody can re-use it?
Yes. Hm, I think I'd prefer adding the potential user in netdev first and then moving it to a header file. I'll add an explicit 'TODO' in the comment.
+/**
- batadv_mcast_mla_local_collect - Collects local multicast listeners
please start the description with a small letter (like we do everywhere..just keep the same style)
- @dev: The device to collect multicast addresses from
don't put tabs between the name of the arg and the description. don't use the capital letter. Just:
@dev: the device..
would be nice :)
ok
+static int batadv_mcast_mla_local_collect(struct net_device *dev,
struct list_head *mcast_list,int num_mla_max) {The opening parenthesis of a function must go at the beginning of the next line.
- struct netdev_hw_addr *mc_list_entry, *new;
- int num_mla = 0, ret = 0;
- netif_addr_lock_bh(dev);
- netdev_for_each_mc_addr(mc_list_entry, dev) {
if (num_mla >= num_mla_max) {pr_warn("Too many local multicast listener announcements here, just adding %i\n",num_mla_max);why pr_warn and not just a debug message? Is this a symptom of a possible malfunctioning?
It was intended as a warning for a malfunction. But actually - I think I can remove it and the limitation of number of entries, it's more a relic of the old announcing approach. It should be up to the TT mechanism to issue such warnings now instead.
break;}new = kmalloc(sizeof(struct netdev_hw_addr), GFP_ATOMIC);please use sizeof(*new) here. we agreed on this some time ago. This helps because if we decide to change the type of new in the future then we do not need to change all the kmalloc invocations.
ok
+static bool batadv_mcast_mla_is_duplicate(uint8_t *mcast_addr,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry;
- list_for_each_entry(mcast_entry, mcast_list, list)
if (!memcmp(mcast_entry->addr, mcast_addr, ETH_ALEN))in main.h we have batadv_compare_eth(), you can re-use it here
ok
+static void batadv_mcast_mla_tt_clean(struct batadv_priv *bat_priv,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry, *tmp;
- list_for_each_entry_safe(mcast_entry, tmp, &bat_priv->mcast.mla_list,
list) {if (batadv_mcast_mla_is_duplicate(mcast_entry->addr,mcast_list))continue;batadv_tt_local_remove(bat_priv, mcast_entry->addr,"mcast TT outdated", 0);^^^parameter is bool, please use "false" instead of 0
ok
+static void batadv_mcast_mla_tt_add(struct net_device *soft_iface,
struct list_head *mcast_list)+{
- struct netdev_hw_addr *mcast_entry, *tmp;
- struct batadv_priv *bat_priv = netdev_priv(soft_iface);
why not simply pass bat_priv as first argument (as you do in all the other functions) and then use bat_priv->soft_iface when needed ? (this soft_iface member has been added recently to save code for lazy people :))
ok
+/**
- batadv_mcast_mla_tt_update - Updates the own MLAs
- @bat_priv: the bat priv with all the soft interface information
- Updates the own multicast listener announcements in the translation
For some reason we agreed on not using the third person while describing the function. For consistency I'd suggest you to do the same your kernel doc.
I don't quite understand what you mean, talking in the first or second person seems strange, like "I update the multicast..." or "You update the multicast...". Do you have an example?
- table. Also takes care of registering or unregistering the multicast
- tvlv depending on whether the user activated or deactivated
- multicast optimizations.
- */
+void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv) +{
- struct batadv_hard_iface *primary_if;
- struct net_device *soft_iface;
- struct list_head mcast_list;
- int ret;
- static bool enabled;
- INIT_LIST_HEAD(&mcast_list);
- primary_if = batadv_primary_if_get_selected(bat_priv);
- if (!primary_if)
goto out;- soft_iface = primary_if->soft_iface;
- /* Avoid attaching MLAs, if multicast optimization is disabled
* or there is a bridge on top of our soft interface (TODO) *//* comments should
- be closed
- like this
*/
/* not like
- this */
(this is something checkpatch should also tell you (but I guess only when your code is in the net/ folder inside the kernel tree...yes it is a netdev rule only :))
Ah! Didn't know about that, thanks. Yes, I was only checking raw git-format-patch'ed patch files from the batman repository.
+/**
- batadv_mcast_free - Frees the multicast optimizaitons structures
- @bat_priv: the bat priv with all the soft interface information
- */
+void batadv_mcast_free(struct batadv_priv *bat_priv) +{
- struct list_head mcast_list;
- INIT_LIST_HEAD(&mcast_list);
- batadv_mcast_mla_tt_clean(bat_priv, &mcast_list);
can't you make batadv_mcast_mla_tt_clean() take a NULL second argument and in that case skip the check in the loop? would be cleaner I guess? what do you think?
Sounds good, will change that!
diff --git a/translation-table.c b/translation-table.c index df6b5cd..37e7d47 100644 --- a/translation-table.c +++ b/translation-table.c @@ -26,6 +26,7 @@ #include "originator.h" #include "routing.h" #include "bridge_loop_avoidance.h" +#include "multicast.h"
#include <linux/crc32c.h>
@@ -281,7 +282,8 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr, bool roamed_back = false;
tt_local = batadv_tt_local_hash_find(bat_priv, addr);
- tt_global = batadv_tt_global_hash_find(bat_priv, addr);
- tt_global = is_multicast_ether_addr(addr) ? NULL :
batadv_tt_global_hash_find(bat_priv, addr);what about:
tt_global = NULL; (you can put this directly in the declaration) if (!is_multicast_ether_addr(addr)) tt_global = batadv_tt_global_hash_find(bat_priv, addr);
it is a bit easier to read, no?
Yes.
if (tt_local) { tt_local->last_seen = jiffies; @@ -332,8 +334,10 @@ void batadv_tt_local_add(struct net_device *soft_iface, const uint8_t *addr, tt_local->last_seen = jiffies; tt_local->common.added_at = tt_local->last_seen;
- /* the batman interface mac address should never be purged */
- if (batadv_compare_eth(addr, soft_iface->dev_addr))
- /* the batman interface mac and multicast addresses should never be
* purged */comment style like above
if (batadv_compare_eth(addr, soft_iface->dev_addr) ||
is_multicast_ether_addr(addr))tt_local->common.flags |= BATADV_TT_CLIENT_NOPURGE;
hash_added = batadv_hash_add(bat_priv->tt.local_hash, batadv_compare_tt,
@@ -919,6 +923,10 @@ add_orig_entry: ret = true;
out_remove:
/* Do not remove multicast addresses from the local hash on
* global additions */if (is_multicast_ether_addr(tt_addr))
goto out;/* remove address from local hash if present */ local_flags = batadv_tt_local_remove(bat_priv, tt_addr,
@@ -2346,6 +2354,9 @@ void batadv_tt_local_commit_changes(struct batadv_priv *bat_priv) { uint16_t changed_num = 0;
- /* Update multicast addresses in local translation table */
- batadv_mcast_mla_tt_update(bat_priv);
- if (atomic_read(&bat_priv->tt.local_changes) < 1) { if (!batadv_atomic_dec_not_zero(&bat_priv->tt.ogm_append_cnt)) batadv_tt_tvlv_container_update(bat_priv);
diff --git a/types.h b/types.h index c84f5cc..5d73a75 100644 --- a/types.h +++ b/types.h @@ -473,6 +473,12 @@ struct batadv_priv_dat { }; #endif
+#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS +struct batadv_priv_mcast {
- struct list_head mla_list;
+}; +#endif
/**
- struct batadv_priv_nc - per mesh interface network coding private data
- @work: work queue callback item for cleanup
@@ -561,6 +567,9 @@ struct batadv_priv { #ifdef CONFIG_BATMAN_ADV_DAT atomic_t distributed_arp_table; #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- atomic_t mcast_group_awareness;
+#endif atomic_t gw_mode; atomic_t gw_sel_class; atomic_t orig_interval; @@ -595,6 +604,9 @@ struct batadv_priv { #ifdef CONFIG_BATMAN_ADV_DAT struct batadv_priv_dat dat; #endif +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- struct batadv_priv_mcast mcast;
+#endif #ifdef CONFIG_BATMAN_ADV_NC atomic_t network_coding; struct batadv_priv_nc nc; -- 1.7.10.4
Cheers,
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
Cheers, Linus
On Thu, May 16, 2013 at 08:16:40PM +0200, Linus Lüssing wrote:
+/**
- batadv_mcast_mla_tt_update - Updates the own MLAs
- @bat_priv: the bat priv with all the soft interface information
- Updates the own multicast listener announcements in the translation
For some reason we agreed on not using the third person while describing the function. For consistency I'd suggest you to do the same your kernel doc.
I don't quite understand what you mean, talking in the first or second person seems strange, like "I update the multicast..." or "You update the multicast...". Do you have an example?
It should look like: "Update the own multicast listener announcements in the translation"
Cheers,
If the soft interface of a node is not part of a bridge then a node announces a new multicast TVLV: The according flag (BATADV_MCAST_LISTENER_ANNOUNCEMENT) signalizes that this node is announcing all of its multicast listeners via the translation table infrastructure. More precisely, all multicast listeners of scope greater than link-local for IPv4 and of scope greater or equal to link-local for IPv6.
Signed-off-by: Linus Lüssing linus.luessing@web.de --- main.c | 4 ++++ multicast.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- multicast.h | 7 ++++++ originator.c | 3 +++ packet.h | 7 ++++++ soft-interface.c | 1 + types.h | 4 ++++ 7 files changed, 86 insertions(+), 2 deletions(-)
diff --git a/main.c b/main.c index 1c82c18..6ab5b2d 100644 --- a/main.c +++ b/main.c @@ -145,6 +145,10 @@ int batadv_mesh_init(struct net_device *soft_iface) if (ret < 0) goto err;
+ ret = batadv_mcast_init(bat_priv); + if (ret < 0) + goto err; + ret = batadv_gw_init(bat_priv); if (ret < 0) goto err; diff --git a/multicast.c b/multicast.c index 88bde61..95beac4 100644 --- a/multicast.c +++ b/multicast.c @@ -170,6 +170,7 @@ void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv) struct list_head mcast_list; int ret; static bool enabled; + uint8_t mcast_flags;
INIT_LIST_HEAD(&mcast_list);
@@ -183,14 +184,22 @@ void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv) * or there is a bridge on top of our soft interface (TODO) */ if (!atomic_read(&bat_priv->mcast_group_awareness) || bat_priv->soft_iface->priv_flags & IFF_BRIDGE_PORT) { - if (enabled) + if (enabled) { + batadv_tvlv_container_unregister(bat_priv, + BATADV_TVLV_MCAST, 1); enabled = false; + }
goto update; }
- if (!enabled) + if (!enabled) { + mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT; + batadv_tvlv_container_register(bat_priv, BATADV_TVLV_MCAST, 1, + &mcast_flags, + sizeof(mcast_flags)); enabled = true; + }
ret = batadv_mcast_mla_local_collect(soft_iface, &mcast_list, BATADV_MLA_MAX); @@ -209,6 +218,52 @@ out: }
/** + * 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 + * @flags: flags indicating the tvlv state (see batadv_tvlv_handler_flags) + * @tvlv_value: tvlv buffer containing the multicast data + * @tvlv_value_len: tvlv buffer length + */ +static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv, + struct batadv_orig_node *orig, + uint8_t flags, + void *tvlv_value, + uint16_t tvlv_value_len) +{ + uint8_t mcast_flags = BATADV_NO_FLAGS; + + /* only fetch the tvlv value if the handler wasn't called via the + * CIFNOTFND flag and if there is data to fetch + */ + if (!(flags & BATADV_TVLV_HANDLER_OGM_CIFNOTFND) && + (tvlv_value) && (tvlv_value_len == 1)) + mcast_flags = *(unsigned char *)tvlv_value; + + if (!(mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) && + orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) { + atomic_inc(&bat_priv->mcast_num_non_aware); + } else if (mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT && + !(orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT)) { + atomic_dec(&bat_priv->mcast_num_non_aware); + } + + orig->mcast_flags = mcast_flags; +} + +/** + * batadv_mcast_init - Initializes the multicast optimizations structures + * @bat_priv: the bat priv with all the soft interface information + */ +int batadv_mcast_init(struct batadv_priv *bat_priv) +{ + batadv_tvlv_handler_register(bat_priv, batadv_mcast_tvlv_ogm_handler_v1, + NULL, BATADV_TVLV_MCAST, 1, + BATADV_TVLV_HANDLER_OGM_CIFNOTFND); + return 0; +} + +/** * batadv_mcast_free - Frees the multicast optimizaitons structures * @bat_priv: the bat priv with all the soft interface information */ @@ -218,5 +273,8 @@ void batadv_mcast_free(struct batadv_priv *bat_priv)
INIT_LIST_HEAD(&mcast_list);
+ batadv_tvlv_container_unregister(bat_priv, BATADV_TVLV_MCAST, 1); + batadv_tvlv_handler_unregister(bat_priv, BATADV_TVLV_MCAST, 1); + batadv_mcast_mla_tt_clean(bat_priv, &mcast_list); } diff --git a/multicast.h b/multicast.h index 3b68e3b..9955a18 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_init(struct batadv_priv *bat_priv); + void batadv_mcast_free(struct batadv_priv *bat_priv);
#else @@ -33,6 +35,11 @@ static inline void batadv_mcast_mla_tt_update(struct batadv_priv *bat_priv) return; }
+static inline int batadv_mcast_init(struct batadv_priv *bat_priv) +{ + return 0; +} + static inline void batadv_mcast_free(struct batadv_priv *bat_priv) { return; diff --git a/originator.c b/originator.c index 5d53d2f..acc0c2d 100644 --- a/originator.c +++ b/originator.c @@ -257,6 +257,9 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv, 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; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + orig_node->mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT; +#endif
atomic_set(&orig_node->bond_candidates, 0);
diff --git a/packet.h b/packet.h index 7d45890..c89df30 100644 --- a/packet.h +++ b/packet.h @@ -97,6 +97,11 @@ enum batadv_unicast_frag_flags { BATADV_UNI_FRAG_LARGETAIL = BIT(1), };
+/* multicast capabilities */ +enum batadv_mcast_flags { + BATADV_MCAST_LISTENER_ANNOUNCEMENT = BIT(0), +}; + /* tt data subtypes */ #define BATADV_TT_DATA_TYPE_MASK 0x0F
@@ -143,6 +148,7 @@ enum batadv_bla_claimframe { * @BATADV_TVLV_NC: network coding tvlv * @BATADV_TVLV_TT: translation table tvlv * @BATADV_TVLV_ROAM: roaming advertisement tvlv + * @BATADV_TVLV_MCAST: multicast capability tvlv */ enum batadv_tvlv_type { BATADV_TVLV_GW = 0x01, @@ -150,6 +156,7 @@ enum batadv_tvlv_type { BATADV_TVLV_NC = 0x03, BATADV_TVLV_TT = 0x04, BATADV_TVLV_ROAM = 0x05, + BATADV_TVLV_MCAST = 0x06, };
/* the destination hardware field in the ARP frame is used to diff --git a/soft-interface.c b/soft-interface.c index b97dfa2..c1142c2 100644 --- a/soft-interface.c +++ b/soft-interface.c @@ -459,6 +459,7 @@ static int batadv_softif_init_late(struct net_device *dev) #endif #ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS atomic_set(&bat_priv->mcast_group_awareness, 1); + atomic_set(&bat_priv->mcast_num_non_aware, 0); #endif atomic_set(&bat_priv->ap_isolation, 0); atomic_set(&bat_priv->gw_mode, BATADV_GW_MODE_OFF); diff --git a/types.h b/types.h index 5d73a75..4ef5fb9 100644 --- a/types.h +++ b/types.h @@ -146,6 +146,9 @@ struct batadv_orig_node { unsigned long last_seen; unsigned long bcast_seqno_reset; unsigned long batman_seqno_reset; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + uint8_t mcast_flags; +#endif uint8_t capabilities; atomic_t last_ttvn; uint32_t tt_crc; @@ -569,6 +572,7 @@ struct batadv_priv { #endif #ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS atomic_t mcast_group_awareness; + atomic_t mcast_num_non_aware; #endif atomic_t gw_mode; atomic_t gw_sel_class;
On Sat, May 11, 2013 at 07:23:26PM +0200, Linus Lüssing wrote:
/**
- 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
- @flags: flags indicating the tvlv state (see batadv_tvlv_handler_flags)
- @tvlv_value: tvlv buffer containing the multicast data
- @tvlv_value_len: tvlv buffer length
- */
+static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
struct batadv_orig_node *orig,uint8_t flags,void *tvlv_value,uint16_t tvlv_value_len)+{
- uint8_t mcast_flags = BATADV_NO_FLAGS;
- /* only fetch the tvlv value if the handler wasn't called via the
* CIFNOTFND flag and if there is data to fetch*/- if (!(flags & BATADV_TVLV_HANDLER_OGM_CIFNOTFND) &&
(tvlv_value) && (tvlv_value_len == 1))
just for style reason I'd suggest to use sizeof(mcast_flags) instead of 1. and there is no need for parentheses around tvlv_value.
mcast_flags = *(unsigned char *)tvlv_value;
mcast_flags is uint8_t, therefore (even if it may practically be the same) you should use the same type for the cast.
- if (!(mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) &&
orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) {atomic_inc(&bat_priv->mcast_num_non_aware);- } else if (mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT &&
!(orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT)) {atomic_dec(&bat_priv->mcast_num_non_aware);- }
- orig->mcast_flags = mcast_flags;
+}
diff --git a/originator.c b/originator.c index 5d53d2f..acc0c2d 100644 --- a/originator.c +++ b/originator.c @@ -257,6 +257,9 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv, 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; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- orig_node->mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT;
+#endif
why do you start assuming that an originator has the optimisation enabled? would it be better to wait for the first mcast tvlv from it to claim this?
+/* multicast capabilities */ +enum batadv_mcast_flags {
- BATADV_MCAST_LISTENER_ANNOUNCEMENT = BIT(0),
+};
diff --git a/types.h b/types.h index 5d73a75..4ef5fb9 100644 --- a/types.h +++ b/types.h @@ -146,6 +146,9 @@ struct batadv_orig_node { unsigned long last_seen; unsigned long bcast_seqno_reset; unsigned long batman_seqno_reset; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- uint8_t mcast_flags;
+#endif uint8_t capabilities; atomic_t last_ttvn; uint32_t tt_crc; @@ -569,6 +572,7 @@ struct batadv_priv { #endif #ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS atomic_t mcast_group_awareness;
- atomic_t mcast_num_non_aware;
why isn't this variable in the mcast_priv struct? It is not a user knob (as far as I can see)
Cheers,
On Sun, May 12, 2013 at 01:11:13AM +0200, Antonio Quartulli wrote:
On Sat, May 11, 2013 at 07:23:26PM +0200, Linus Lüssing wrote:
/**
- 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
- @flags: flags indicating the tvlv state (see batadv_tvlv_handler_flags)
- @tvlv_value: tvlv buffer containing the multicast data
- @tvlv_value_len: tvlv buffer length
- */
+static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
struct batadv_orig_node *orig,uint8_t flags,void *tvlv_value,uint16_t tvlv_value_len)+{
- uint8_t mcast_flags = BATADV_NO_FLAGS;
- /* only fetch the tvlv value if the handler wasn't called via the
* CIFNOTFND flag and if there is data to fetch*/- if (!(flags & BATADV_TVLV_HANDLER_OGM_CIFNOTFND) &&
(tvlv_value) && (tvlv_value_len == 1))just for style reason I'd suggest to use sizeof(mcast_flags) instead of 1. and there is no need for parentheses around tvlv_value.
mcast_flags = *(unsigned char *)tvlv_value;mcast_flags is uint8_t, therefore (even if it may practically be the same) you should use the same type for the cast.
ok
- if (!(mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) &&
orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) {atomic_inc(&bat_priv->mcast_num_non_aware);- } else if (mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT &&
!(orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT)) {atomic_dec(&bat_priv->mcast_num_non_aware);- }
- orig->mcast_flags = mcast_flags;
+}
diff --git a/originator.c b/originator.c index 5d53d2f..acc0c2d 100644 --- a/originator.c +++ b/originator.c @@ -257,6 +257,9 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv, 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; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- orig_node->mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT;
+#endif
why do you start assuming that an originator has the optimisation enabled? would it be better to wait for the first mcast tvlv from it to claim this?
Because it is easier code-wise. I had it the other way round first and issuing an atomic_inc(&bat_priv->mcast_num_non_ware) in batadv_get_orig_node(), but then I ended up counting up too many times because I was increasing that counter for secondary interface originators, too while not decreasing it again because no TVLV handler will be called for these. And within batadv_get_orig_node() I don't see an easy way to determine whether I was called for a primary or secondary interface originator.
+/* multicast capabilities */ +enum batadv_mcast_flags {
- BATADV_MCAST_LISTENER_ANNOUNCEMENT = BIT(0),
+};
diff --git a/types.h b/types.h index 5d73a75..4ef5fb9 100644 --- a/types.h +++ b/types.h @@ -146,6 +146,9 @@ struct batadv_orig_node { unsigned long last_seen; unsigned long bcast_seqno_reset; unsigned long batman_seqno_reset; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- uint8_t mcast_flags;
+#endif uint8_t capabilities; atomic_t last_ttvn; uint32_t tt_crc; @@ -569,6 +572,7 @@ struct batadv_priv { #endif #ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS atomic_t mcast_group_awareness;
- atomic_t mcast_num_non_aware;
why isn't this variable in the mcast_priv struct? It is not a user knob (as far as I can see)
You're right, will move it.
Cheers,
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
Cheers, Linus
On Thu, May 16, 2013 at 08:19:45PM +0200, Linus Lüssing wrote:
- if (!(mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) &&
orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) {atomic_inc(&bat_priv->mcast_num_non_aware);- } else if (mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT &&
!(orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT)) {atomic_dec(&bat_priv->mcast_num_non_aware);- }
- orig->mcast_flags = mcast_flags;
+}
diff --git a/originator.c b/originator.c index 5d53d2f..acc0c2d 100644 --- a/originator.c +++ b/originator.c @@ -257,6 +257,9 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv, 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; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- orig_node->mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT;
+#endif
why do you start assuming that an originator has the optimisation enabled? would it be better to wait for the first mcast tvlv from it to claim this?
Because it is easier code-wise. I had it the other way round first and issuing an atomic_inc(&bat_priv->mcast_num_non_ware) in batadv_get_orig_node(), but then I ended up counting up too many times because I was increasing that counter for secondary interface originators, too while not decreasing it again because no TVLV handler will be called for these. And within batadv_get_orig_node() I don't see an easy way to determine whether I was called for a primary or secondary interface originator.
mh..I don't really understand this (maybe because I don't have a deep knowledge of this code). My idea was to start with:
orig_node->mcast_flags = BATADV_NO_FLAGS;
and to set
orig_node->mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT;
only in the received TVLV parsing function (if any TVLV has been received). Is this inconsistent with what you have in the code?
Cheers,
Hi Antonio,
On Thu, May 16, 2013 at 09:41:20PM +0200, Antonio Quartulli wrote:
On Thu, May 16, 2013 at 08:19:45PM +0200, Linus Lüssing wrote:
- if (!(mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) &&
orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT) {atomic_inc(&bat_priv->mcast_num_non_aware);- } else if (mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT &&
!(orig->mcast_flags & BATADV_MCAST_LISTENER_ANNOUNCEMENT)) {atomic_dec(&bat_priv->mcast_num_non_aware);- }
- orig->mcast_flags = mcast_flags;
+}
diff --git a/originator.c b/originator.c index 5d53d2f..acc0c2d 100644 --- a/originator.c +++ b/originator.c @@ -257,6 +257,9 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv, 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; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS
- orig_node->mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT;
+#endif
why do you start assuming that an originator has the optimisation enabled? would it be better to wait for the first mcast tvlv from it to claim this?
Because it is easier code-wise. I had it the other way round first and issuing an atomic_inc(&bat_priv->mcast_num_non_ware) in batadv_get_orig_node(), but then I ended up counting up too many times because I was increasing that counter for secondary interface originators, too while not decreasing it again because no TVLV handler will be called for these. And within batadv_get_orig_node() I don't see an easy way to determine whether I was called for a primary or secondary interface originator.
mh..I don't really understand this (maybe because I don't have a deep knowledge of this code). My idea was to start with:
orig_node->mcast_flags = BATADV_NO_FLAGS;
and to set
orig_node->mcast_flags = BATADV_MCAST_LISTENER_ANNOUNCEMENT;
only in the received TVLV parsing function (if any TVLV has been received). Is this inconsistent with what you have in the code?
Yes, that was what I initially had and it unfortunately didn't work that easily.
The thing is, I want to be able to easily determine whether all nodes have this flag, this capability to be able to know whether I can safely make use of the new optimization.
I could for any multicast packet I want to send loop through all originators and check for this flag. But I thought that might be too performance hungry in that code path. Therefore I decided to keep track of that via the "mcast_num_non_aware" variable. If it is zero than I can optimize, otherwise not.
If I were initializing flags to BATADV_NO_FLAGS in batadv_get_orig_node(), then I'd have to increase "mcast_num_non_aware" in that same function because the handler wouldn't be able to do so (unless I'd use some magic value instead of BATADV_NO_FLAGS, like BATADV_FLAGS_NOT_INIT). So I had something like:
----- diff --git a/originator.c b/originator.c index 5d53d2f..40a3611 100644 --- a/originator.c +++ b/originator.c @@ -257,6 +257,9 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv, 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; +#ifdef CONFIG_BATMAN_ADV_MCAST_OPTIMIZATIONS + orig_node->mcast_flags = BATADV_NO_FLAGS; +#endif
atomic_set(&orig_node->bond_candidates, 0);
@@ -281,6 +284,8 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv, if (hash_added != 0) goto free_bcast_own_sum;
+ atomic_inc(&bat_priv->mcast_num_non_aware); + return orig_node; free_bcast_own_sum: kfree(orig_node->bcast_own_sum); ---
But that doesn't work because if I am seeing an originator via its secondary interface then I'll increase that counter twice. And even if that originator actually has the new MCAST flag set, then I'll only decrease it once in the handler because the handler is not called for secondary interface originators.
So ideally I'd do something like: ----- diff --git a/originator.c b/originator.c index 5d53d2f..40a3611 100644 --- a/originator.c +++ b/originator.c @@ -281,6 +284,8 @@ struct batadv_orig_node *batadv_get_orig_node(struct batadv_priv *bat_priv, if (hash_added != 0) goto free_bcast_own_sum;
+ if (IS_PRIMARY_ORIG_ADDR(addr)) + atomic_inc(&bat_priv->mcast_num_non_aware); + return orig_node; free_bcast_own_sum: kfree(orig_node->bcast_own_sum); -----
But unfortunately that doesn't exist as far as I know.
Cheers,
-- Antonio Quartulli
..each of us alone is worth nothing.. Ernesto "Che" Guevara
Cheers, Linus
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(+)
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);
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,
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
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,
Hey Linus,
thanks for working and integrating this patchset!
On Sat, May 11, 2013 at 07:23:24PM +0200, Linus Lüssing wrote:
This set of patches is the first one for a more efficient, group aware multicast forwarding infrastructure in batman-adv.
This initial set mainly consists of the integration of announcing the location of multicast listeners via the translation table mechanism to be able to find out which batman-adv nodes are actually interested in certain multicast traffic. As well as the signalizing of this announcement capability via a new multicast TVLV.
Finally some basic multicast forwarding opitimizations are introduced: If all nodes signalized the MLA capability then link-local IPv6 traffic will be dropped if there is no interested listener or gets forwarded via a batman-adv unicast packet if there is just one node interested in the data.
I've seen that you've only implemented that for IPv6, would it be possible to use it for IPv4 too in the current state? (I use some media streaming thingy at home where I could test ;])
For now, these optimizations only apply if all nodes in the mesh have no bridge interface on top their bat0 interface. However making it possible with bridges, too, and other features are on the roadmap. See the according wiki page for details [1].
So from what I understand this means:
* multicast support is working for non-bridged interfaces for now, because we lack proper multicast router discovery support (MRD) in the bridge code --> you told me someone is working on that, or you'll pick up to get this done?
* the current optimization is to handle multicast like: * no listener - drop * one listener - unicast * more listener - broadcast --> I think that is a good and simple optimization. :) The "tracker" packets can come later (as shown in your roadmap). This might require to announce that tracker packets are supported in the TVLV?
Would you consider Antonios comments and update your patchset? I would like to test it in the next days ...
Thanks, Simon
Hi Simon,
On Thu, May 16, 2013 at 01:51:29PM +0200, Simon Wunderlich wrote:
Hey Linus,
thanks for working and integrating this patchset!
On Sat, May 11, 2013 at 07:23:24PM +0200, Linus Lüssing wrote:
This set of patches is the first one for a more efficient, group aware multicast forwarding infrastructure in batman-adv.
This initial set mainly consists of the integration of announcing the location of multicast listeners via the translation table mechanism to be able to find out which batman-adv nodes are actually interested in certain multicast traffic. As well as the signalizing of this announcement capability via a new multicast TVLV.
Finally some basic multicast forwarding opitimizations are introduced: If all nodes signalized the MLA capability then link-local IPv6 traffic will be dropped if there is no interested listener or gets forwarded via a batman-adv unicast packet if there is just one node interested in the data.
I've seen that you've only implemented that for IPv6, would it be possible to use it for IPv4 too in the current state? (I use some media streaming thingy at home where I could test ;])
Hmm, I need to think about it. The thing is it looks like IPv4 does not seem to require IGMP. And according to RFC4541, an informational one, link-local IPv4 multicast shoud be excluded as there might be some applications not issuing IGMP reports.
For non-link-local addresses we'd need MRD first, as we'd otherwise miss to forward things to multicast routers.
However when using no bridge, like it is the case now, RFC4541 does not apply and we can in fact reliably get and distribute link-local IPv4 multicast addresses as we do not rely on IGMP here...
One option would be to introduce another flag. But that might become confusing for users, it might not be easy to explain to users why their 224.0.0.123 would be working on raw bat0 but not when they add a bridge and why this would never work. Instead we could wait for the MRD implementation and tell people that 224.0.0.0/24 won't work.
For now, these optimizations only apply if all nodes in the mesh have no bridge interface on top their bat0 interface. However making it possible with bridges, too, and other features are on the roadmap. See the according wiki page for details [1].
So from what I understand this means:
- multicast support is working for non-bridged interfaces for now, because we lack proper multicast router discovery support (MRD) in the bridge code
--> you told me someone is working on that, or you'll pick up to get this done?
Yes, I had been exchanging some emails with a group of people a few months ago who wanted to implement MRD and a proper IGMP/MLD querier protocol in the bridge code but didn't hear from them again. So yes, I guess I'll pick up on that.
For MRD I was thinking about taking a short-cut for now as implementing would need a few more lines of code. Instead I think I go for detecting whether a querier is on the link both in batman-adv and the bridge code for IPv4 or IPv6 and if not disabling optimizations accordingly and issuing a warning. That way non-link-local IPv6 multicast traffic could be optimized already for instance if running an mrd6 instance in userspace which already performs both MLD querying and MRD.
- the current optimization is to handle multicast like:
- no listener - drop
- one listener - unicast
- more listener - broadcast
--> I think that is a good and simple optimization. :) The "tracker" packets can come later (as shown in your roadmap). This might require to announce that tracker packets are supported in the TVLV?
Yes, we'll probably need to add another capability flag to the multicast TVLV.
Would you consider Antonios comments and update your patchset? I would like to test it in the next days ...
Yes, I will. I had written some comments for the comments on IRC, but I guess it'll be better to write them here on the list again.
Thanks, Simon
Another thing I was thinking about conceptually yesterday was whether we should use more refined flags instead of just MULTICAST_LISTENER_ANNOUNCEMENTS for everything (non-link-local IPv4, all IPv6 except the all-nodes address).
That way we could for instance already add some cases for when to use the multicast optimizations when having a bridge, for instance:
When batman-adv detects that there is an MLD querier and if all nodes have a MULTICAST_LISTENER_ANNOUNCEMENTS or MLA_IPV6_TRANSIENT_LINK_LOCAL flag it could already optimize link-local, transient IPv6 multicast traffic without needing to modify anything in the bridge code except the addition of the export which was already posted as an RFC on the bridge mailing list.
Hm, again will need to think about that. Whether the extra conceptual complexity is okay because of being able to add some more use-cases with less code in small chunks already.
Cheers, Linus
On Thu, May 16, 2013 at 07:42:00PM +0200, Linus Lüssing wrote:
Hi Simon,
On Thu, May 16, 2013 at 01:51:29PM +0200, Simon Wunderlich wrote:
Hey Linus,
thanks for working and integrating this patchset!
On Sat, May 11, 2013 at 07:23:24PM +0200, Linus Lüssing wrote:
This set of patches is the first one for a more efficient, group aware multicast forwarding infrastructure in batman-adv.
This initial set mainly consists of the integration of announcing the location of multicast listeners via the translation table mechanism to be able to find out which batman-adv nodes are actually interested in certain multicast traffic. As well as the signalizing of this announcement capability via a new multicast TVLV.
Finally some basic multicast forwarding opitimizations are introduced: If all nodes signalized the MLA capability then link-local IPv6 traffic will be dropped if there is no interested listener or gets forwarded via a batman-adv unicast packet if there is just one node interested in the data.
I've seen that you've only implemented that for IPv6, would it be possible to use it for IPv4 too in the current state? (I use some media streaming thingy at home where I could test ;])
Hmm, I need to think about it. The thing is it looks like IPv4 does not seem to require IGMP. And according to RFC4541, an informational one, link-local IPv4 multicast shoud be excluded as there might be some applications not issuing IGMP reports.
For non-link-local addresses we'd need MRD first, as we'd otherwise miss to forward things to multicast routers.
However when using no bridge, like it is the case now, RFC4541 does not apply and we can in fact reliably get and distribute link-local IPv4 multicast addresses as we do not rely on IGMP here...
One option would be to introduce another flag. But that might become confusing for users, it might not be easy to explain to users why their 224.0.0.123 would be working on raw bat0 but not when they add a bridge and why this would never work. Instead we could wait for the MRD implementation and tell people that 224.0.0.0/24 won't work.
Hmm, as far as I read the RFC 4541) (2.1.2, point 2) we should just treat 224.0.0.0/24 (which would translate to 01:00:5e:00:00:xx) as broadcast - we should do that in any state of the multicast implementation IMHO, and are then on the safe side. For all other IPv4 multicast addresses we can assume that IGMP is used.
For the non-link-local IPv4 multicast addresses, could we use the current mechanism (drop/unicast/broadcast) by getting the assigned multicast addresses? (of course, assuming we have no bridge)
For now, these optimizations only apply if all nodes in the mesh have no bridge interface on top their bat0 interface. However making it possible with bridges, too, and other features are on the roadmap. See the according wiki page for details [1].
So from what I understand this means:
- multicast support is working for non-bridged interfaces for now, because we lack proper multicast router discovery support (MRD) in the bridge code
--> you told me someone is working on that, or you'll pick up to get this done?
Yes, I had been exchanging some emails with a group of people a few months ago who wanted to implement MRD and a proper IGMP/MLD querier protocol in the bridge code but didn't hear from them again. So yes, I guess I'll pick up on that.
OK, cool.
For MRD I was thinking about taking a short-cut for now as implementing would need a few more lines of code. Instead I think I go for detecting whether a querier is on the link both in batman-adv and the bridge code for IPv4 or IPv6 and if not disabling optimizations accordingly and issuing a warning. That way non-link-local IPv6 multicast traffic could be optimized already for instance if running an mrd6 instance in userspace which already performs both MLD querying and MRD.
Hmm, sorry I still get a little confused in that MRD/MLD/IGMP terminology. So as I understand there might be a userspace component which does the MLD/MRD instead of the kernel? Also, is it required to get implement MRD/MLD specifically in batman-adv or bridges, or would it be possible to use a "general" approach which could be used for any interface to emit/exchange these kind of messages?
Sorry if I ask stupid questions. :)
Maybe some example or architecture overview (could be put on the wiki page) would help?
- the current optimization is to handle multicast like:
- no listener - drop
- one listener - unicast
- more listener - broadcast
--> I think that is a good and simple optimization. :) The "tracker" packets can come later (as shown in your roadmap). This might require to announce that tracker packets are supported in the TVLV?
Yes, we'll probably need to add another capability flag to the multicast TVLV.
OK. We could also increase the version number of the TVLV and interpret both versions in newer multicast implementations, but if we already know that we can define this information in the current implementation.
Would you consider Antonios comments and update your patchset? I would like to test it in the next days ...
Yes, I will. I had written some comments for the comments on IRC, but I guess it'll be better to write them here on the list again.
Mailing list would be better, yes. Thanks. :)
Thanks, Simon
Another thing I was thinking about conceptually yesterday was whether we should use more refined flags instead of just MULTICAST_LISTENER_ANNOUNCEMENTS for everything (non-link-local IPv4, all IPv6 except the all-nodes address).
That way we could for instance already add some cases for when to use the multicast optimizations when having a bridge, for instance:
When batman-adv detects that there is an MLD querier and if all nodes have a MULTICAST_LISTENER_ANNOUNCEMENTS or MLA_IPV6_TRANSIENT_LINK_LOCAL flag it could already optimize link-local, transient IPv6 multicast traffic without needing to modify anything in the bridge code except the addition of the export which was already posted as an RFC on the bridge mailing list.
Hm, again will need to think about that. Whether the extra conceptual complexity is okay because of being able to add some more use-cases with less code in small chunks already.
Hmm, again I'm not completely sure to follow, but the idea here is to enable functionality when having a userspace MLD instead of the (planned) bridge MLD stuff?
If we need the bridge MLD stuff anyway (to have the full feature set etc), I'd rather not do more intermediate steps which might be obsolete later. However, if the userspace MLD thing is equivalent feature-wise than this might be interesting to do.
Cheers, Simon
Hi,
On Thu, May 16, 2013 at 08:31:32PM +0200, Simon Wunderlich wrote:
On Thu, May 16, 2013 at 07:42:00PM +0200, Linus Lüssing wrote:
Hi Simon,
On Thu, May 16, 2013 at 01:51:29PM +0200, Simon Wunderlich wrote:
Hey Linus,
thanks for working and integrating this patchset!
On Sat, May 11, 2013 at 07:23:24PM +0200, Linus Lüssing wrote:
This set of patches is the first one for a more efficient, group aware multicast forwarding infrastructure in batman-adv.
This initial set mainly consists of the integration of announcing the location of multicast listeners via the translation table mechanism to be able to find out which batman-adv nodes are actually interested in certain multicast traffic. As well as the signalizing of this announcement capability via a new multicast TVLV.
Finally some basic multicast forwarding opitimizations are introduced: If all nodes signalized the MLA capability then link-local IPv6 traffic will be dropped if there is no interested listener or gets forwarded via a batman-adv unicast packet if there is just one node interested in the data.
I've seen that you've only implemented that for IPv6, would it be possible to use it for IPv4 too in the current state? (I use some media streaming thingy at home where I could test ;])
Hmm, I need to think about it. The thing is it looks like IPv4 does not seem to require IGMP. And according to RFC4541, an informational one, link-local IPv4 multicast shoud be excluded as there might be some applications not issuing IGMP reports.
For non-link-local addresses we'd need MRD first, as we'd otherwise miss to forward things to multicast routers.
However when using no bridge, like it is the case now, RFC4541 does not apply and we can in fact reliably get and distribute link-local IPv4 multicast addresses as we do not rely on IGMP here...
One option would be to introduce another flag. But that might become confusing for users, it might not be easy to explain to users why their 224.0.0.123 would be working on raw bat0 but not when they add a bridge and why this would never work. Instead we could wait for the MRD implementation and tell people that 224.0.0.0/24 won't work.
Hmm, as far as I read the RFC 4541) (2.1.2, point 2) we should just treat 224.0.0.0/24 (which would translate to 01:00:5e:00:00:xx) as broadcast - we should do that in any state of the multicast implementation IMHO, and are then on the safe side. For all other IPv4 multicast addresses we can assume that IGMP is used.
Ok, yes, that's what I thought, too (although like I said before technically there is the possibility to safely optimize 224.0.0.0/24, too if there are no bridges involved - but ok, let's forget that thought).
For the non-link-local IPv4 multicast addresses, could we use the current mechanism (drop/unicast/broadcast) by getting the assigned multicast addresses? (of course, assuming we have no bridge)
We could - after implementing MRD. The thing is we might have one or more IPv4 multicast routers in our network. If we are having two multicast listeners on our link, then everything will be fine, we will broadcast, multicast routers receive the multicast packets, too and the multicast data gets routed further. However with no MRD and if there is just one or no multicast listener then we'd do the unicast/drop and the multicast router on the link and any listener more than one hop away would not receive that data anymore.
For now, these optimizations only apply if all nodes in the mesh have no bridge interface on top their bat0 interface. However making it possible with bridges, too, and other features are on the roadmap. See the according wiki page for details [1].
So from what I understand this means:
- multicast support is working for non-bridged interfaces for now, because we lack proper multicast router discovery support (MRD) in the bridge code
--> you told me someone is working on that, or you'll pick up to get this done?
Yes, I had been exchanging some emails with a group of people a few months ago who wanted to implement MRD and a proper IGMP/MLD querier protocol in the bridge code but didn't hear from them again. So yes, I guess I'll pick up on that.
OK, cool.
For MRD I was thinking about taking a short-cut for now as implementing would need a few more lines of code. Instead I think I go for detecting whether a querier is on the link both in batman-adv and the bridge code for IPv4 or IPv6 and if not disabling optimizations accordingly and issuing a warning. That way non-link-local IPv6 multicast traffic could be optimized already for instance if running an mrd6 instance in userspace which already performs both MLD querying and MRD.
Sorry, I ment: "For MLD querying I was...". mrd6 does perform MRD, so sending things like multicast router advertisements. But bridges or batman-adv would still need to parse these advertisements and should send multicast router solicitations (e.g. when an interface comes up) to be able to quickly determine which bridge ports or nodes have multicast routers and want the non-link-local multicast traffic.
Hmm, sorry I still get a little confused in that MRD/MLD/IGMP terminology. So as I understand there might be a userspace component which does the MLD/MRD instead of the kernel?
Yes, the layer 3 multicast routing table is in the kernel, but it is configured from userspace from daemons like mrd6. And mrd6 also does some non-performance critical protocol stuff like the MLD Querier protocol and the MRD multicast router advertisements like I said above.
Also, is it required to get implement MRD/MLD specifically in batman-adv or bridges, or would it be possible to use a "general" approach which could be used for any interface to emit/exchange these kind of messages?
The MLD querier is something which only makes sense to me in case of multicast snooping bridges and I don't see any benefit in having it anywhere else. If there are just the plain interfaces with no multicast router then you don't need an MLD querier (and in fact no MLD reports from the listeners either). If there's just batman-adv with no bridges, then you don't need it either. Although...
There is one use-case where a more general approach might be of interest: Wireless interfaces - it might be of interest for the mac80211 to know whether there is a listener (or router) behind the link provided by your wlan0 device. The wifi driver could then decide to refrain from broadcasting a multicast packet or to use the (minimum) bitrate of just the(se) listener(s) (/router(s)). Or maybe a drop/unicast/flood on an eth0 itself might be nice, too, especially if there is a large switch/hub connected to it.
Although that'd probably be awesome to have, I think it's easier to just have the MLD querier in the bridge code for now (especilly as there already is some MLD querier code in the bridge - though it is so incomplete that it got disabled in April 2012 because of causing issues).
Hm, for the MRD RA parsing and MRD RS I thought about implementing that both in the bridge and batman-adv code (it's about 300 lines of C++ code in mrd6). Not sure how easy it'd be to implement a more general approach on top of a Linux netdev for instance. Would need to check that.
Sorry if I ask stupid questions. :)
Maybe some example or architecture overview (could be put on the wiki page) would help?
I guess you mean some visualizaton with things like simple devices, bridge, batman-adv, maybe various kinds of multicast types (e.g. scope == link-local vs. scope > link-local, IPv6 transient vs. IPv6 well-known) and the according RFCs? Hm, not quite sure how that could look like would need to think about it - or maybe you have some idea about how you think that could/should look like?
- the current optimization is to handle multicast like:
- no listener - drop
- one listener - unicast
- more listener - broadcast
--> I think that is a good and simple optimization. :) The "tracker" packets can come later (as shown in your roadmap). This might require to announce that tracker packets are supported in the TVLV?
Yes, we'll probably need to add another capability flag to the multicast TVLV.
OK. We could also increase the version number of the TVLV and interpret both versions in newer multicast implementations, but if we already know that we can define this information in the current implementation.
But a newer TVLV version would break compatibility for older batman-adv versions, they wouldn't recognize the newer TVLV version. Or a node would need to add TVLVs for both versions to its OGM. But that'd be some more overhead compared to just adding another flag.
Would you consider Antonios comments and update your patchset? I would like to test it in the next days ...
Yes, I will. I had written some comments for the comments on IRC, but I guess it'll be better to write them here on the list again.
Mailing list would be better, yes. Thanks. :)
Done :).
Thanks, Simon
Another thing I was thinking about conceptually yesterday was whether we should use more refined flags instead of just MULTICAST_LISTENER_ANNOUNCEMENTS for everything (non-link-local IPv4, all IPv6 except the all-nodes address).
That way we could for instance already add some cases for when to use the multicast optimizations when having a bridge, for instance:
When batman-adv detects that there is an MLD querier and if all nodes have a MULTICAST_LISTENER_ANNOUNCEMENTS or MLA_IPV6_TRANSIENT_LINK_LOCAL flag it could already optimize link-local, transient IPv6 multicast traffic without needing to modify anything in the bridge code except the addition of the export which was already posted as an RFC on the bridge mailing list.
Hm, again will need to think about that. Whether the extra conceptual complexity is okay because of being able to add some more use-cases with less code in small chunks already.
Hmm, again I'm not completely sure to follow, but the idea here is to enable functionality when having a userspace MLD instead of the (planned) bridge MLD stuff?
Yes, that would be the idea.
If we need the bridge MLD stuff anyway (to have the full feature set etc), I'd rather not do more intermediate steps which might be obsolete later. However, if the userspace MLD thing is equivalent feature-wise than this might be interesting to do.
Hm, don't know. I was thinking that most setups I know of involve bridges on top of bat0 and to be able to make use of any multicast optimizations in batman-adv in case of such bridging we need an MLD querier on the link. Implementing a proper MLD querier might need several iterations over the bridge / netdev mailing lists and might therefore need quite some time. During that time those setups won't be able to make any use of these optimizations.
Unless taking the suggested short-cut which shouldn't be that difficult to implement, I think, then these bat0+bridge setups could already enjoy some multicast optimizations and such mesh networks might already be able to play with multicast streams as I think updating and merging the tracker packet patches will be done some time before getting MLD querier bridge code upstream. (but maybe I'm estimating all this wrong, dunno)
Cheers, Simon
Cheers, Linus
Hey Linus,
On Fri, May 17, 2013 at 03:38:56AM +0200, Linus Lüssing wrote:
For the non-link-local IPv4 multicast addresses, could we use the current mechanism (drop/unicast/broadcast) by getting the assigned multicast addresses? (of course, assuming we have no bridge)
We could - after implementing MRD. The thing is we might have one or more IPv4 multicast routers in our network. If we are having two multicast listeners on our link, then everything will be fine, we will broadcast, multicast routers receive the multicast packets, too and the multicast data gets routed further. However with no MRD and if there is just one or no multicast listener then we'd do the unicast/drop and the multicast router on the link and any listener more than one hop away would not receive that data anymore.
OK, I see. Well, then we should save that for later ...
For MRD I was thinking about taking a short-cut for now as implementing would need a few more lines of code. Instead I think I go for detecting whether a querier is on the link both in batman-adv and the bridge code for IPv4 or IPv6 and if not disabling optimizations accordingly and issuing a warning. That way non-link-local IPv6 multicast traffic could be optimized already for instance if running an mrd6 instance in userspace which already performs both MLD querying and MRD.
Sorry, I ment: "For MLD querying I was...". mrd6 does perform MRD, so sending things like multicast router advertisements. But bridges or batman-adv would still need to parse these advertisements and should send multicast router solicitations (e.g. when an interface comes up) to be able to quickly determine which bridge ports or nodes have multicast routers and want the non-link-local multicast traffic.
OK
Hmm, sorry I still get a little confused in that MRD/MLD/IGMP terminology. So as I understand there might be a userspace component which does the MLD/MRD instead of the kernel?
Yes, the layer 3 multicast routing table is in the kernel, but it is configured from userspace from daemons like mrd6. And mrd6 also does some non-performance critical protocol stuff like the MLD Querier protocol and the MRD multicast router advertisements like I said above.
OK
Also, is it required to get implement MRD/MLD specifically in batman-adv or bridges, or would it be possible to use a "general" approach which could be used for any interface to emit/exchange these kind of messages?
The MLD querier is something which only makes sense to me in case of multicast snooping bridges and I don't see any benefit in having it anywhere else. If there are just the plain interfaces with no multicast router then you don't need an MLD querier (and in fact no MLD reports from the listeners either). If there's just batman-adv with no bridges, then you don't need it either. Although...
There is one use-case where a more general approach might be of interest: Wireless interfaces - it might be of interest for the mac80211 to know whether there is a listener (or router) behind the link provided by your wlan0 device. The wifi driver could then decide to refrain from broadcasting a multicast packet or to use the (minimum) bitrate of just the(se) listener(s) (/router(s)).
Actually that might be interesting for both batman-adv and "regular" WiFi setups. I think some commercial vendors already do exactly that to optimize Multicast. My setup at home is a good example how this could benefit: * I have 6 APs having mesh and ap bridged * Right now, when I multicast stream from my LAN, first batman-adv floods all the broadcast through the mesh, then every AP sends the broadcast again - all on the lowest rate (at least on the AP). --> so if we listen to music using multicast only in the kitchen, the WiFi becomes really slow. :D
With the multicast optimization in batman-adv, we could solve the broadcast storm in the batman-adv network (only send to the APs where listener registered). Then still the AP would broadcast these packets on the lowest rate (1 MBit/s). Not all APs at least, but still the one where the client is connected. If mac80211 could detect that and send it via unicast, we could use even HT rates (e.g. 300 Mbit/s) here. I could enjoy wireless multicast HD video streams - yay. ;)
Probably not the most typical example/use case, but still we can see how this scenario would benefit from the optimization.
Or maybe a drop/unicast/flood on an eth0 itself might be nice, too, especially if there is a large switch/hub connected to it.
Although that'd probably be awesome to have, I think it's easier to just have the MLD querier in the bridge code for now (especilly as there already is some MLD querier code in the bridge - though it is so incomplete that it got disabled in April 2012 because of causing issues).
Actually I'm not sure about the technical implementation of such a general approach. Maybe it would be possible to query the bridge from the various components as well.
Hm, for the MRD RA parsing and MRD RS I thought about implementing that both in the bridge and batman-adv code (it's about 300 lines of C++ code in mrd6). Not sure how easy it'd be to implement a more general approach on top of a Linux netdev for instance. Would need to check that.
Sorry if I ask stupid questions. :)
Maybe some example or architecture overview (could be put on the wiki page) would help?
I guess you mean some visualizaton with things like simple devices, bridge, batman-adv, maybe various kinds of multicast types (e.g. scope == link-local vs. scope > link-local, IPv6 transient vs. IPv6 well-known) and the according RFCs? Hm, not quite sure how that could look like would need to think about it - or maybe you have some idea about how you think that could/should look like?
Hmm, not sure either. Maybe we choose the typical examples (bat0 only, bat0 + ap + bridge), and show which component sends what. Like having "blocks" for batman, bridge, userspace mrd, etc, and show which component sends/querys who for which information.
I know that's pretty vague, but maybe there is a way to bring light in that for others which are not so experienced in that (me included).
Multicast types (IPv4, IPv6) and their mac-address and RFC recommendations should probably better go in an extra section. We could make a table of what types exist, how we handle them (optimize, just broadcast) and why (RFC, design decisions).
OK. We could also increase the version number of the TVLV and interpret both versions in newer multicast implementations, but if we already know that we can define this information in the current implementation.
But a newer TVLV version would break compatibility for older batman-adv versions, they wouldn't recognize the newer TVLV version. Or a node would need to add TVLVs for both versions to its OGM. But that'd be some more overhead compared to just adding another flag.
Yeah you are right. Maybe just add the flag. :D
Would you consider Antonios comments and update your patchset? I would like to test it in the next days ...
Yes, I will. I had written some comments for the comments on IRC, but I guess it'll be better to write them here on the list again.
Mailing list would be better, yes. Thanks. :)
Done :).
Thanks!
Thanks, Simon
Another thing I was thinking about conceptually yesterday was whether we should use more refined flags instead of just MULTICAST_LISTENER_ANNOUNCEMENTS for everything (non-link-local IPv4, all IPv6 except the all-nodes address).
That way we could for instance already add some cases for when to use the multicast optimizations when having a bridge, for instance:
When batman-adv detects that there is an MLD querier and if all nodes have a MULTICAST_LISTENER_ANNOUNCEMENTS or MLA_IPV6_TRANSIENT_LINK_LOCAL flag it could already optimize link-local, transient IPv6 multicast traffic without needing to modify anything in the bridge code except the addition of the export which was already posted as an RFC on the bridge mailing list.
Hm, again will need to think about that. Whether the extra conceptual complexity is okay because of being able to add some more use-cases with less code in small chunks already.
Hmm, again I'm not completely sure to follow, but the idea here is to enable functionality when having a userspace MLD instead of the (planned) bridge MLD stuff?
Yes, that would be the idea.
If we need the bridge MLD stuff anyway (to have the full feature set etc), I'd rather not do more intermediate steps which might be obsolete later. However, if the userspace MLD thing is equivalent feature-wise than this might be interesting to do.
Hm, don't know. I was thinking that most setups I know of involve bridges on top of bat0 and to be able to make use of any multicast optimizations in batman-adv in case of such bridging we need an MLD querier on the link. Implementing a proper MLD querier might need several iterations over the bridge / netdev mailing lists and might therefore need quite some time. During that time those setups won't be able to make any use of these optimizations.
Unless taking the suggested short-cut which shouldn't be that difficult to implement, I think, then these bat0+bridge setups could already enjoy some multicast optimizations and such mesh networks might already be able to play with multicast streams as I think updating and merging the tracker packet patches will be done some time before getting MLD querier bridge code upstream. (but maybe I'm estimating all this wrong, dunno)
Actually I did not hear much screaming to have multicast support so far, so I don't think we "need" an intermediate solution. If you want to it anyway or need it for something, go ahead, but I'd prefer aiming for the final implementation from the start without taking detours. :)
Thanks, Simon
b.a.t.m.a.n@lists.open-mesh.org
-
Antonio Quartulli -
Linus Lüssing -
Simon Wunderlich