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)