Looks quite good and seems to make sense to avoid the double-locking. Tested it here and together with patch 1/3 fixes the initial issue, too.

Just one minor thing: ---- [58518.225154] batman_adv: bat0: The MTU of interface eth1 is too small (1500) to handle the transport of batman-adv packets. Packets going over this interface will be fragmented on layer2 which could impact the performance. Setting the MTU to 1527 would solve the problem. [58518.278111] RTNL: assertion failed at /home/mesh-node/incoming/batman-adv-git-builddir/hard-interface.c (136) [58518.293167] Pid: 6366, comm: bash Not tainted 2.6.38+ #4 [58518.299142] Call Trace: [58518.300953] [<f8cb91f3>] primary_if_select+0x2f/0x9f [batman_adv] [58518.447844] [<f8cb8e9c>] ? rcu_read_unlock+0x1c/0x1e [batman_adv] [58518.452572] [<f8cb9146>] ? primary_if_get_selected+0x6a/0x71 [batman_adv] [58518.455759] [<f8cb92c6>] hardif_activate_interface+0x63/0x91 [batman_adv] [58518.459168] [<f8cb98e4>] hardif_enable_interface+0x1cc/0x21e [batman_adv] [58518.462481] [<f8cb7bca>] store_mesh_iface+0xed/0x113 [batman_adv] [58518.465564] [<f8cb7add>] ? store_aggregated_ogms+0x28/0x28 [batman_adv] [58518.468714] [<c11e1121>] kobj_attr_store+0x1a/0x22 [58518.472277] [<c11481dc>] sysfs_write_file+0xbc/0xe7 [58518.613128] [<c1148120>] ? sysfs_open_file+0x205/0x205 [58518.616400] [<c10ff7a7>] vfs_write+0x84/0xde [58518.660437] [<c10ff89a>] sys_write+0x3b/0x60 [58518.667248] [<c151fd9d>] syscall_call+0x7/0xb [58518.672117] batman_adv: bat0: Interface activated: eth1 ----

So maybe it makes sense to add the assertion in primary_if_select() in patch 3/3 instead?

Hmm, and I'm wondering if it might make sense to also document it with a little comment above the struct list_head hardif_list;, like: /* List additions and deletions have to be rtnl_lock()'ed, * list traversals just rcu-locked */ (or is that overdoing it?)

On Sat, Apr 30, 2011 at 10:54:14PM +0200, Sven Eckelmann wrote:

hardif_list_lock is unneccessary because we already ensure that no multiple admin operations can take place through rtnl_lock. hardif_list_lock only adds additional overhead and complexity.

Critical functions now check whether they are called with rtnl_lock using ASSERT_RTNL.

Signed-off-by: Sven Eckelmann sven@narfation.org

Rebased patch on top of "batman-adv: Make bat_priv->primary_if an rcu protected pointer"

hard-interface.c | 21 ++++++--------------- 1 files changed, 6 insertions(+), 15 deletions(-)

diff --git a/hard-interface.c b/hard-interface.c index 6636d2f..7e2f772 100644 --- a/hard-interface.c +++ b/hard-interface.c @@ -31,9 +31,6 @@

#include <linux/if_arp.h>

-/* protect update critical side of hardif_list - but not the content */ -static DEFINE_SPINLOCK(hardif_list_lock);

static int batman_skb_recv(struct sk_buff *skb, struct net_device *dev, @@ -136,7 +133,7 @@ static void primary_if_select(struct bat_priv *bat_priv, struct hard_iface *curr_hard_iface; struct batman_packet *batman_packet;

• spin_lock_bh(&hardif_list_lock);

• ASSERT_RTNL();

  if (new_hard_iface && !atomic_inc_not_zero(&new_hard_iface->refcount)) new_hard_iface = NULL;

@@ -148,7 +145,7 @@ static void primary_if_select(struct bat_priv *bat_priv, hardif_free_ref(curr_hard_iface);

if (!new_hard_iface)

• goto out;
  

• return;
  
  

  batman_packet = (struct batman_packet *)(new_hard_iface->packet_buff); batman_packet->flags = PRIMARIES_FIRST_HOP;

@@ -161,9 +158,6 @@ static void primary_if_select(struct bat_priv *bat_priv, * our new primary interface */ atomic_set(&bat_priv->hna_local_changed, 1);

-out:

• spin_unlock_bh(&hardif_list_lock);

}

static bool hardif_is_iface_up(struct hard_iface *hard_iface) @@ -456,6 +450,8 @@ static struct hard_iface *hardif_add_interface(struct net_device *net_dev) struct hard_iface *hard_iface; int ret;

• ASSERT_RTNL();
• ret = is_valid_iface(net_dev); if (ret != 1) goto out;

@@ -482,10 +478,7 @@ static struct hard_iface *hardif_add_interface(struct net_device *net_dev) atomic_set(&hard_iface->refcount, 2);

check_known_mac_addr(hard_iface->net_dev);

• spin_lock(&hardif_list_lock); list_add_tail_rcu(&hard_iface->list, &hardif_list);

• spin_unlock(&hardif_list_lock);

  return hard_iface;

@@ -499,6 +492,8 @@ out:

static void hardif_remove_interface(struct hard_iface *hard_iface) {

• ASSERT_RTNL();
• /* first deactivate interface */ if (hard_iface->if_status != IF_NOT_IN_USE) hardif_disable_interface(hard_iface);

@@ -516,13 +511,11 @@ void hardif_remove_interfaces(void) struct hard_iface *hard_iface, *hard_iface_tmp;

rtnl_lock();

• spin_lock(&hardif_list_lock); list_for_each_entry_safe(hard_iface, hard_iface_tmp, &hardif_list, list) { list_del_rcu(&hard_iface->list); hardif_remove_interface(hard_iface); }
• spin_unlock(&hardif_list_lock); rtnl_unlock();

}

@@ -549,9 +542,7 @@ static int hard_if_event(struct notifier_block *this, hardif_deactivate_interface(hard_iface); break; case NETDEV_UNREGISTER:

• spin_lock(&hardif_list_lock);
  

  list_del_rcu(&hard_iface->list);

• spin_unlock(&hardif_list_lock);
  
  

  hardif_remove_interface(hard_iface); break;

-- 1.7.4.4

Ok, could reproduce and verify it now (latest master branch _without_ patches 1/3 and 2/3, with that version I can still reproduce that issue reliably). Putting only patch 3/3 on top the master branch then fixes that locking dependancy. Thanks, Sven!

Cheers, Linus

On Mon, May 02, 2011 at 07:27:28PM +0200, Linus Lüssing wrote:

Looks good, seems to make sense and tried it. Works so far with no call traces or other uggly stuff. However I'm still trying to reproduce the initial issue of ticket #145 to confirm that this patch fixes it (used a different kernel and batman-adv version back then, of course... so somehow the 'always' stated there does not seem to be valid anymore :) ).

PS: Ok, just sometimes I'm still getting a locking dependancy call trace, but that one involves two different locks ((&(&bat_priv->hna_ghash_lock)->rlock){+.-...}, at: [<f83a3dd6>] hna_global_del_orig+0x22/0x90 [batman_adv] (&(&hash->list_locks[i])->rlock){+.-...}, at: [<f839fe4b>] _purge_orig+0x45/0x1be [batman_adv]) and therefore seems to be unrelated - I'll open a new ticket for that.

On Sat, Apr 30, 2011 at 06:57:00PM +0200, Sven Eckelmann wrote:

...

The hard_if_event is called by the notifier with rtnl_lock and tries to remove sysfs entries when a NETDEV_UNREGISTER event is received. This will automatically take the s_active lock.

The s_active lock is also used when a new interface is added to a meshif through sysfs. In that situation we cannot wait for the rntl_lock before creating the actual batman-adv interface to prevent a deadlock. It is still possible to try to get the rtnl_lock and immediately abort the current operation when the trylock call failed.

Signed-off-by: Sven Eckelmann sven@narfation.org

Remove '{' '}' after 'if (hard_iface->if_status != IF_NOT_IN_USE)'

bat_sysfs.c | 16 +++++++++------- soft-interface.c | 2 +- 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/bat_sysfs.c b/bat_sysfs.c index e449bf6..497a070 100644 --- a/bat_sysfs.c +++ b/bat_sysfs.c @@ -488,22 +488,24 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr, (strncmp(hard_iface->soft_iface->name, buff, IFNAMSIZ) == 0)) goto out;

• if (!rtnl_trylock()) {

• ret = -ERESTARTSYS;
  

• goto out;
  

• }
• if (status_tmp == IF_NOT_IN_USE) {

• rtnl_lock();
  

  hardif_disable_interface(hard_iface);

• rtnl_unlock();
  

• goto out;
  

• goto unlock;
  

  }

  /* if the interface already is in use */

• if (hard_iface->if_status != IF_NOT_IN_USE) {

• rtnl_lock();
  

• if (hard_iface->if_status != IF_NOT_IN_USE) hardif_disable_interface(hard_iface);

• rtnl_unlock();
  

• }

  ret = hardif_enable_interface(hard_iface, buff);

+unlock:

• rtnl_unlock();

out: hardif_free_ref(hard_iface); return ret; diff --git a/soft-interface.c b/soft-interface.c index f4d80ad..bf53aa3 100644 --- a/soft-interface.c +++ b/soft-interface.c @@ -613,7 +613,7 @@ struct net_device *softif_create(char *name) goto out; }

• ret = register_netdev(soft_iface);

• ret = register_netdevice(soft_iface); if (ret < 0) { pr_err("Unable to register the batman interface '%s': %i\n", name, ret);

-- 1.7.4.4