On Sat, Mar 28, 2009 at 10:41 AM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> On Sat, 2009-03-28 at 00:55 -0400, Luis R. Rodriguez wrote:
>
>
>> > This changes how mac80211 behaves towards drivers that support
>> > aggregation but have no hardware queues -- those drivers will now
>> > not be handed packets while the aggregation session is being
>> > established, but only after it has been fully established.
>>
>> That's fine from a logical point of view as compromise here as our
>> ampdu start should be relatively quick. Now while I can say this
>> from a logical point of view this patch obviously needed more
>> testing but lets see how it holds up and I'm glad you're the one
>> who wrote it. I'm confident there won't be any issues but that
>> is something I cannot gaurantee just based on the review. We now
>> need to go out and tests this. All other patches previous to this
>> make 100% perfect sense.
>
> :)
> I think it also makes more _sense_ to not ask the driver to handle these
> frames, because we won't set the AMPDU flag correctly if the session
> start is declined.
Hm, wouldn't we send them as normal frames if its declined here eventually?
>> > @@ -602,22 +637,19 @@ void ieee80211_stop_tx_ba_cb(struct ieee
>> > WLAN_BACK_INITIATOR, WLAN_REASON_QSTA_NOT_USE);
>> >
>> > spin_lock_bh(&sta->lock);
>> > + spin_lock(&local->ampdu_lock);
>> >
>> > - if (*state & HT_AGG_STATE_INITIATOR_MSK &&
>> > - hw->ampdu_queues) {
>> > - /*
>> > - * Wake up this queue, we stopped it earlier,
>> > - * this will in turn wake the entire AC.
>> > - */
>> > - ieee80211_wake_queue_by_reason(hw,
>> > - hw->queues + sta->tid_to_tx_q[tid],
>> > - IEEE80211_QUEUE_STOP_REASON_AGGREGATION);
>> > - }
>> > + ieee80211_agg_splice_packets(local, sta, tid);
>>
>> Is it possible for ieee80211_stop_tx_ba_cb() to be called while
>> state of the tid is not yet operational? from what I can tell
>> that's the only case when we would have added skbs to the STA's tid
>> pending queue.
>
> This happens when the session is denied by the peer.
Ah, got it, thanks.
>> > *state = HT_AGG_STATE_IDLE;
>> > + /* from now on packets are no longer put onto sta->pending */
>>
>> I think it would help to refer to the pendign queue consitantly instead
>> as something like "STA's TID pending queue" as that is what it is. We also
>> now have the local->pending queue. STA's pending queue seems to imply
>> its also used for non-aggregation sessions.
>>
>> See -- if the state is not operation nothing would have gone
>> been put into the STA's tid pending queue. Might also be worth
>> mentioning that in the comment.
>
> Hmm, yeah, might rewrite the comments a bit. Mind if I do a separate
> patch later?
Nope.
>> > @@ -1013,20 +1013,53 @@ __ieee80211_tx_prepare(struct ieee80211_
>> > */
>> > }
>> >
>> > + /*
>> > + * If this flag is set to true anywhere, and we get here,
>> > + * we are doing the needed processing, so remove the flag
>> > + * now.
>> > + */
>> > + info->flags &= ~IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> > +
>> > hdr = (struct ieee80211_hdr *) skb->data;
>> >
>> > tx->sta = sta_info_get(local, hdr->addr1);
>> >
>> > - if (tx->sta && ieee80211_is_data_qos(hdr->frame_control)) {
>> > + if (tx->sta && ieee80211_is_data_qos(hdr->frame_control) &&
>> > + (local->hw.flags & IEEE80211_HW_AMPDU_AGGREGATION)) {
>>
>> Hm, why were we not crashing here before? I figure we would have
>> with something like this:
>>
>> state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
>>
>> That it itself should be separate patch, but too late now. Anyway
>> the new added check for IEEE80211_HW_AMPDU_AGGREGATION should go
>> to stable.
>
> Nah, state_tx always exists (and will be IDLE) but this was just an
> added check to make us not need the spinlock for non-ampdu hw.
Ah yes, its the ampdu_mlme.tid_tx that gets kmalloc'd only on aggr
session start.
>> > unsigned long flags;
>> > + struct tid_ampdu_tx *tid_tx;
>> > +
>> > qc = ieee80211_get_qos_ctl(hdr);
>> > tid = *qc & IEEE80211_QOS_CTL_TID_MASK;
>> >
>> > spin_lock_irqsave(&tx->sta->lock, flags);
>> > + /*
>> > + * XXX: This spinlock could be fairly expensive, but see the
>> > + * comment in agg-tx.c:ieee80211_agg_tx_operational().
>> > + * One way to solve this would be to do something RCU-like
>> > + * for managing the tid_tx struct and using atomic bitops
>> > + * for the actual state -- by introducing an actual
>> > + * 'operational' bit that would be possible. It would
>> > + * require changing ieee80211_agg_tx_operational() to
>> > + * set that bit, and changing the way tid_tx is managed
>> > + * everywhere, including races between that bit and
>> > + * tid_tx going away (tid_tx being added can be easily
>> > + * committed to memory before the 'operational' bit).
>> > + */
>> > + tid_tx = tx->sta->ampdu_mlme.tid_tx[tid];
>> > state = &tx->sta->ampdu_mlme.tid_state_tx[tid];
>> > - if (*state == HT_AGG_STATE_OPERATIONAL)
>> > + if (*state == HT_AGG_STATE_OPERATIONAL) {
>> > info->flags |= IEEE80211_TX_CTL_AMPDU;
>>
>> See, when its operational we don't add stuff to the STA's TID pending
>> queue.
>>
>> This piece:
>>
>> > + } else if (*state != HT_AGG_STATE_IDLE) {
>> > + /* in progress */
>> > + queued = true;
>> > + info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
>> > + __skb_queue_tail(&tid_tx->pending, skb);
>> > + }
>>
>> Can probably be ported to stable too, to just TX_DROP.
>
> No, why? We're handing the frames to the driver. It wants to handle
> those frames before agg session is started correctly. That's what I was
> referring to before with this making more sense now.
OK I see that but I am not sure of what's done to the skb in the old
case if the session is note complete yet, nor now if the session gets
declined. I can check later, right now I'm feeling lazy.
>> > @@ -1251,7 +1295,12 @@ static int ieee80211_tx(struct net_devic
>> > do {
>> > next = skb->next;
>> > skb->next = NULL;
>> > - skb_queue_tail(&local->pending[queue], skb);
>> > + if (unlikely(txpending))
>> > + skb_queue_head(&local->pending[queue],
>> > + skb);
>> > + else
>> > + skb_queue_tail(&local->pending[queue],
>> > + skb);
>>
>> For someone who hasn't read all the pending queue code stuff the above would be
>> a little brain twister. It might be good to leave a comment explaining why
>> txpendign would be set and why we add it to the head (i.e. because the "skb" sent
>> at a previous time was put into a pending queue). Maybe even rename txpending to
>> something like skb_was_pending.
>
> Ok that might make some sense, though as a parameter you can easily see
> where it's coming from, I think :) I agree that the entire code is a
> little brain twister...
Yeah.. I had to re-read aggregation code again to get the hang of it.
Anything we can do to make
it easier for people to pick would be nice. I think I'll go patch up
the aggr-tx kdoc too now, unless we
plan on nuking some stuff soon again. I suspect the next big change
will be to move software based
aggregation queue handling and mapping to ACs within mac80211 for that
type of hardware which
we'll need for ar9170, ralink stuff, broadcom (if that ever happens)
and our next generation 11n USB
driver.
>> > @@ -1360,7 +1407,7 @@ int ieee80211_master_start_xmit(struct s
>> > "originating device\n", dev->name);
>> > #endif
>> > dev_kfree_skb(skb);
>> > - return 0;
>> > + return NETDEV_TX_OK;
>>
>> All these NETDEV_TX_OK could've gone into a separate patch.
>
> I guess, they're also not strictly necessary since 0 == TX_OK :)
Sure, what I'm trying to say is these patches are pretty hard to
review because of the topic,
it just helps to trim them down as much as possible -- but I realize
even that is painful.
Luis
--
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
