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
