Re: [B.A.T.M.A.N.] [PATCH 07/19] batman-adv: Prepare and send own multicast tracker packets

[Simon Wunderlich <simon.wunderlich@s2003.tu-chemnitz.de>]

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

Hey Linus,

please see some comments inline.

On Sat, Jan 22, 2011 at 02:21:30AM +0100, Linus Lüssing wrote:
> [...]
>  
> +static inline int find_mca_match(struct orig_node *orig_node,
> +		int mca_pos, uint8_t *mc_addr_list, int num_mcast_entries)
> +{
> +	int pos;
> +
> +	for (pos = 0; pos < num_mcast_entries; pos++)
> +		if (!memcmp(&mc_addr_list[pos*ETH_ALEN],
> +			    &orig_node->mca_buff[ETH_ALEN*mca_pos], ETH_ALEN))
> +			return pos;
> +	return -1;
> +}

A comment explaining this function find_mca_match() would be nice.

> +
> +/**
> + * Prepares a multicast tracker packet on a multicast member with all its
> + * groups and their members attached. Note, that the proactive tracking
> + * mode does not differentiate between multicast senders and receivers,
> + * resulting in tracker packets between each node.
> + *
> + * Returns NULL if this node is not a member of any group or if there are
> + * no other members in its groups.
> + *
> + * @bat_priv:		bat_priv for the mesh we are preparing this packet
> + */
> +static struct mcast_tracker_packet *mcast_proact_tracker_prepare(
> +			struct bat_priv *bat_priv, int *tracker_packet_len)
> +{
> +	struct net_device *soft_iface = bat_priv->primary_if->soft_iface;
> +	uint8_t *mc_addr_list;
> +	MC_LIST *mc_entry;
> +	struct element_t *bucket;
> +	struct orig_node *orig_node;
> +	struct hashtable_t *hash = bat_priv->orig_hash;
> +	struct hlist_node *walk;
> +	struct hlist_head *head;
> +	int i;
> +
> +	/* one dest_entries_list per multicast group,
> +	 * they'll collect dest_entries[x] */
> +	int num_mcast_entries, used_mcast_entries = 0;
> +	struct list_head *dest_entries_list;
> +	struct dest_entries_list dest_entries[UINT8_MAX], *dest, *tmp;

This will reserve 256 * 18 = 4608 byte on the stack, which 
is too much for 4k stacks. Please allocate this somewhere else.


> +	int num_dest_entries, dest_entries_total = 0;
> +
> +	uint8_t *dest_entry;
> +	int pos, mca_pos;
> +	struct mcast_tracker_packet *tracker_packet = NULL;
> +	struct mcast_entry *mcast_entry;
> +
> +	if (!hash)
> +		goto out;
> +
> +	/* Make a copy so we don't have to rush because of locking */
> +	netif_addr_lock_bh(soft_iface);
> +	num_mcast_entries = netdev_mc_count(soft_iface);
> +	mc_addr_list = kmalloc(ETH_ALEN * num_mcast_entries, GFP_ATOMIC);
> +	if (!mc_addr_list) {
> +		netif_addr_unlock_bh(soft_iface);
> +		goto out;
> +	}
> +	pos = 0;
> +	netdev_for_each_mc_addr(mc_entry, soft_iface) {
> +		memcpy(&mc_addr_list[pos * ETH_ALEN], mc_entry->MC_LIST_ADDR,
> +		       ETH_ALEN);
> +		pos++;
> +	}
> +	netif_addr_unlock_bh(soft_iface);
> +
> +	if (num_mcast_entries > UINT8_MAX)
> +		num_mcast_entries = UINT8_MAX;
> +	dest_entries_list = kmalloc(num_mcast_entries *
> +					sizeof(struct list_head), GFP_ATOMIC);
> +	if (!dest_entries_list)
> +		goto free;
> +
> +	for (pos = 0; pos < num_mcast_entries; pos++)
> +		INIT_LIST_HEAD(&dest_entries_list[pos]);
> +

dest_entries[...].list should be initialized here too, shouldn't they?
BTW, the names a re a little bit confusing (dest_entries_list vs. dest_entries),
other names and an explanation would be helpful.

> +	/* fill the lists and buffers */
> +	for (i = 0; i < hash->size; i++) {
> +		head = &hash->table[i];
> +
> +		rcu_read_lock();
> +		hlist_for_each_entry_rcu(bucket, walk, head, hlist) {
> +			orig_node = bucket->data;
> +			if (!orig_node->num_mca)
> +				continue;
> +
> +			num_dest_entries = 0;
> +			for (mca_pos = 0; mca_pos < orig_node->num_mca &&
> +			     dest_entries_total != UINT8_MAX; mca_pos++) {
> +				pos = find_mca_match(orig_node, mca_pos,
> +					mc_addr_list, num_mcast_entries);
> +				if (pos > UINT8_MAX || pos < 0)

Shouldn't this rather be 
	if (pos >= num_mcast_entries || pos < 0)

(it is used for dest_entries_list after all).

> +					continue;
> +				memcpy(dest_entries[dest_entries_total].dest,
> +				       orig_node->orig, ETH_ALEN);
> +				list_add(
> +					&dest_entries[dest_entries_total].list,
> +					&dest_entries_list[pos]);
> +
> +				num_dest_entries++;
num_dest_entries is obsolete IMHO, it is only increase but never used.

> +				dest_entries_total++;
> +			}
> +		}
> +		rcu_read_unlock();
> +	}
> +
> +	/* Any list left empty? */
> +	for (pos = 0; pos < num_mcast_entries; pos++)
> +		if (!list_empty(&dest_entries_list[pos]))
> +			used_mcast_entries++;
> +
> +	if (!used_mcast_entries)
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]