Re: [PATCH] Assign next hop address to pending mesh frames once the path is resolved.

[Javier Cardona <javier@cozybit.com>]

Johannes,

On Wed, Jul 8, 2009 at 3:40 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Tue, 2009-07-07 at 22:53 -0700, Javier Cardona wrote:
>> Regression.  Frames transmitted when a mesh path was wating to be resolved were
>> being transmitted with an invalid Receiver Address.
>
>> -     rcu_assign_pointer(mpath->next_hop, sta);
>> +     struct sk_buff *skb, *skb_first = NULL;
>> +     struct ieee80211_hdr *hdr;
>> +
>> +     rcu_read_lock();
>> +     mpath->next_hop = sta;
>> +
>> +     while ((skb = skb_dequeue(&mpath->frame_queue)) != skb_first) {
>> +             if (!skb_first)
>> +                     skb_first = skb;
>> +             hdr = (struct ieee80211_hdr *) skb->data;
>> +             memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
>> +             skb_queue_tail(&mpath->frame_queue, skb);
>> +     }
>> +     if (skb_first)
>> +             skb_queue_tail(&mpath->frame_queue, skb_first);
>> +
>> +     rcu_read_unlock();
>
> Since skb queues have a locks, why use rcu too?

The some mpath members are rcu protected.  I thought I had to extend
the rcu section to cover both mpath->next_hop and mpath->frame_queue.
But now I see that the latter does not need protection, so I'll revert
that to just rcu_assign_pointer(mpath->next_hop, sta);

> Also I think you should probably use a different pattern -- this looks
> prone to breakage, maybe something like
>
>        sk_buff_head tmpq;
>        unsigned long flags;
>
>        __skb_queue_head_init(&tmpq);
>
>        spin_lock_irqsave(&frame_queue->lock);
>
>        while (skb = __skb_dequeue(&frame_queue)) {
>                hdr = (struct ieee80211_hdr *) skb->data;
>                memcpy(hdr->addr1, sta->sta.addr, ETH_ALEN);
>                __skb_queue_tail(&tmpq, skb);
>        }
>
>        skb_queue_splice(&tmpq, frame_queue);
>        spin_unlock_irqrestore(&frame_queue->lock);

Oh, nice, cleaner and less locking. v2 will follow shortly.

Thanks!

Javier

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