Re: mesh RCU issues

[Javier Cardona <javier@cozybit.com>]

On Fri, May 13, 2011 at 12:13 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2011-05-12 at 15:26 -0700, Javier Cardona wrote:
>
>> > And indeed I don't see a del_timer_sync() when the mesh path is freed.
>>
>> Isn't the call to del_timer_sync() you are looking for in
>> mesh_path_node_reclaim() ?
>
> Hmm, indeed, but it looks like mesh_path_node_free() also frees a node,
> no? I'd only found the latter function freeing it and got worried.

Ah, I see.  Yes, looks like the timer should be deleted there as well.
 Patch will follow.

>> > But this is _clearly_ totally bogus. Somebody please fix ASAP.
>>
>> I'll run sparse and see if I see other rcu warnings that I can fix.
>
> Thanks. I think most of the use is probably OK or "just" missing
> rcu_dereference() wrappers. The global mesh_paths and mpp_paths
> variables should also be __rcu I think, but that caused so many warnings
> that I gave up for now, and I didn't quite understand what was going on.
>
> You'll want to apply
> http://johannes.sipsolutions.net/patches/kernel/all/LATEST/NNN-mac80211-rcu-annotations.patch to get rid of all the other spurious RCU warnings with CONFIG_SPARSE_RCU.

(You probably meant CONFIG_SPARSE_RCU_POINTER)

I must be doing something wrong because I don't see the RCU warnings
after making {mesh,mpp}_paths __rcu.  I only see two "different
address spaces" errors that are fixed in the patch below.
I do see a bunch of 'warning' errors:

/home/javier/dev/wireless-testing/arch/x86/include/asm/uaccess_32.h:199:2:
error: attribute 'warning': unknown attribute



diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index 83ce48e..1db8bba 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -36,8 +36,8 @@ struct mpath_node {
        struct mesh_path *mpath;
 };

-static struct mesh_table *mesh_paths;
-static struct mesh_table *mpp_paths; /* Store paths for MPP&MAP */
+static struct mesh_table __rcu *mesh_paths;
+static struct mesh_table __rcu *mpp_paths; /* Store paths for MPP&MAP */

 int mesh_paths_generation;

@@ -513,7 +513,7 @@ void mesh_plink_broken(struct sta_info *sta)
        for_each_mesh_entry(mesh_paths, p, node, i) {
                mpath = node->mpath;
                spin_lock_bh(&mpath->state_lock);
-               if (mpath->next_hop == sta &&
+               if (rcu_dereference(mpath->next_hop) == sta &&
                    mpath->flags & MESH_PATH_ACTIVE &&
                    !(mpath->flags & MESH_PATH_FIXED)) {
                        mpath->flags &= ~MESH_PATH_ACTIVE;
@@ -549,7 +549,7 @@ void mesh_path_flush_by_nexthop(struct sta_info *sta)

        for_each_mesh_entry(mesh_paths, p, node, i) {
                mpath = node->mpath;
-               if (mpath->next_hop == sta)
+               if (rcu_dereference(mpath->next_hop) == sta)
                        mesh_path_del(mpath->dst, mpath->sdata);
        }
 }

Thoughts?

j

-- 
Javier Cardona
cozybit Inc.
http://www.cozybit.com