From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-yx0-f188.google.com ([209.85.210.188]:35433 "EHLO mail-yx0-f188.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753728AbZGHRCx convert rfc822-to-8bit (ORCPT ); Wed, 8 Jul 2009 13:02:53 -0400 Received: by yxe26 with SMTP id 26so8137767yxe.33 for ; Wed, 08 Jul 2009 10:02:52 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1247049613.4755.65.camel@johannes.local> References: <1247032403-8312-1-git-send-email-javier@cozybit.com> <1247049613.4755.65.camel@johannes.local> Date: Wed, 8 Jul 2009 10:02:50 -0700 Message-ID: <445f43ac0907081002i50c23aeelc2e52d6082f7c9af@mail.gmail.com> Subject: Re: [PATCH] Assign next hop address to pending mesh frames once the path is resolved. From: Javier Cardona To: Johannes Berg Cc: linux-wireless@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-wireless-owner@vger.kernel.org List-ID: Johannes, On Wed, Jul 8, 2009 at 3:40 AM, Johannes Berg 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