Johannes,
On Wed, Jul 8, 2009 at 3:40 AM, Johannes Berg<johannes@xxxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
