Re: [B.A.T.M.A.N.] [PATCH] batman-adv: Add multicast optimization support for bridged setups

[Marek Lindner <mareklindner@neomailbox.ch>]

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

On Thursday 12 June 2014 02:00:27 Linus Lüssing wrote:
> @@ -30,17 +53,21 @@
>   * Collect multicast addresses of the local multicast listeners
>   * on the given soft interface, dev, in the given mcast_list.
>   *
> + * If there is a bridge interface on top of dev, collect from that one
> + * instead.
> + *
>   * Returns -ENOMEM on memory allocation error or the number of
>   * items added to the mcast_list otherwise.
>   */
>  static int batadv_mcast_mla_softif_get(struct net_device *dev,
>  				       struct hlist_head *mcast_list)
>  {
> +	struct net_device *bridge = batadv_mcast_get_bridge(dev);
>  	struct netdev_hw_addr *mc_list_entry;
>  	struct batadv_hw_addr *new;
>  	int ret = 0;
> 
> -	netif_addr_lock_bh(dev);
> +	netif_addr_lock_bh(bridge ? bridge : dev);
>  	netdev_for_each_mc_addr(mc_list_entry, dev) {
>  		new = kmalloc(sizeof(*new), GFP_ATOMIC);
>  		if (!new) {

Maybe I am missing something but shouldn't we fetch the entries from the 
bridge interface instead of dev ?


>  /**
> + * batadv_mcast_mla_br_addr_cpy - copy a bridge multicast address
> + * @dst: destination to write to - a multicast MAC address
> + * @src: source to read from - a multicast IP address
> + *
> + * Converts a given multicast IPv4/IPv6 address from a bridge
> + * to its matching multicast MAC address and copies it into the given
> + * destination buffer.
> + *
> + * Caller needs to make sure the destination buffer can hold
> + * at least ETH_ALEN bytes.
> + */
> +static void batadv_mcast_mla_br_addr_cpy(char *dst, const struct br_ip
> *src) +{
> +	if (src->proto == htons(ETH_P_IP)) {
> +		/* RFC 1112 */
> +		memcpy(dst, "\x01\x00\x5e", 3);
> +		memcpy(dst + 3, ((char *)&src->u.ip4) + 1, ETH_ALEN - 3);
> +		dst[3] &= 0x7F;
> +	}
> +#if IS_ENABLED(CONFIG_IPV6)
> +	else if (src->proto == htons(ETH_P_IPV6)) {
> +		/* RFC 2464 */
> +		memcpy(dst, "\x33\x33", 2);
> +		memcpy(dst + 2, &src->u.ip6.s6_addr32[3],
> +		       sizeof(src->u.ip6.s6_addr32[3]));
> +	}
> +#endif
> +	else
> +		memset(dst, 0, ETH_ALEN);
> +}

Is there no cleaner way to do this ? "\x01\x00\x5e" and "\x33\x33" looks 
pretty hackish. Are there no defines for mcast prefixes somewhere ?


> +/**
> + * batadv_mcast_mla_bridge_get - get bridged-in multicast listeners
> + * @dev: a bridge slave whose bridge to collect multicast addresses from
> + * @mcast_list: a list to put found addresses into
> + *
> + * Collects multicast addresses of the bridged-in multicast listeners
> + * from the bridge on top of the given soft 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_bridge_get(struct net_device *dev,
> +				       struct hlist_head *mcast_list)
> +{

That is probably where my confusion is coming from. Why do we query the bridge 
interface again ?


> @@ -195,19 +306,18 @@ static bool batadv_mcast_mla_tvlv_update(struct
> batadv_priv *bat_priv) mcast_data.flags = BATADV_NO_FLAGS;
>  	memset(mcast_data.reserved, 0, sizeof(mcast_data.reserved));
> 
> -	/* Avoid attaching MLAs, if there is a bridge on top of our soft
> -	 * interface, we don't support that yet (TODO)
> -	 */
> -	if (batadv_mcast_has_bridge(bat_priv)) {
> -		if (bat_priv->mcast.enabled) {
> -			batadv_tvlv_container_unregister(bat_priv,
> -							 BATADV_TVLV_MCAST, 1);
> -			bat_priv->mcast.enabled = false;
> -		}
> +	if (!batadv_mcast_has_bridge(bat_priv))
> +		goto skip;
> 
> -		return false;
> -	}
> +	mcast_data.flags |= BATADV_MCAST_WANT_ALL_UNSNOOPABLES;
> +
> +	if (br_multicast_has_querier_adjacent(bat_priv->soft_iface, ETH_P_IP))
> +		mcast_data.flags |= BATADV_MCAST_WANT_ALL_IPV4;
> 
> +	if (br_multicast_has_querier_adjacent(bat_priv->soft_iface, ETH_P_IPV6))
> +		mcast_data.flags |= BATADV_MCAST_WANT_ALL_IPV6;
> +
> +skip:
>  	if (!bat_priv->mcast.enabled ||
>  	    mcast_data.flags != bat_priv->mcast.flags) {
>  		batadv_tvlv_container_register(bat_priv, BATADV_TVLV_MCAST, 1,

Please find a more intuitive goto label ('skip' is quite generic).


> @@ -233,13 +344,17 @@ void batadv_mcast_mla_update(struct batadv_priv
> *bat_priv) int ret;
> 
>  	if (!batadv_mcast_mla_tvlv_update(bat_priv))
> -		goto update;
> +		goto skip;
> 
>  	ret = batadv_mcast_mla_softif_get(soft_iface, &mcast_list);
>  	if (ret < 0)
>  		goto out;
> 
> -update:
> +	ret = batadv_mcast_mla_bridge_get(soft_iface, &mcast_list);
> +	if (ret < 0)
> +		goto out;
> +
> +skip:
>  	batadv_mcast_mla_tt_retract(bat_priv, &mcast_list);
>  	batadv_mcast_mla_tt_add(bat_priv, &mcast_list);

Again, 'skip' isn't very self-explanatory.

Cheers,
Marek


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