On Thu, 2016-03-31 at 12:28 +0200, Michal Kazior wrote:
> +++ b/net/mac80211/codel.h
> +++ b/net/mac80211/codel_i.h
Do we really need all this code in .h files? It seems very odd to me to
have all the algorithm implementation there rather than a C file, you
should (can?) only include codel.h into a single C file anyway.
> struct txq_info {
> - struct sk_buff_head queue;
> + struct txq_flow flow;
> + struct list_head new_flows;
> + struct list_head old_flows;
This is confusing, can you please document that? Why are there two
lists of flows, *and* an embedded flow? Is the embedded flow on any of
the lists?
> + u32 backlog_bytes;
> + u32 backlog_packets;
> + u32 drop_codel;
Would it make some sense to at least conceptually layer this a bit?
I.e. rather than calling this "drop_codel" call it "drop_congestion" or
something like that?
> @@ -977,12 +978,9 @@ static void ieee80211_do_stop(struct
> ieee80211_sub_if_data *sdata,
> if (sdata->vif.txq) {
> struct txq_info *txqi = to_txq_info(sdata->vif.txq);
>
> - spin_lock_bh(&txqi->queue.lock);
> - ieee80211_purge_tx_queue(&local->hw, &txqi->queue);
> - txqi->byte_cnt = 0;
> - spin_unlock_bh(&txqi->queue.lock);
> -
> - atomic_set(&sdata->txqs_len[txqi->txq.ac], 0);
> + spin_lock_bh(&fq->lock);
> + ieee80211_purge_txq(local, txqi);
> + spin_unlock_bh(&fq->lock);
This isn't very nice - you're going from locking a single txqi to
having a global hardware lock.
It's probably fine in this particular case, but I'll need to look for
other places :)
> +/**
> + * struct txq_flow - per traffic flow queue
> + *
> + * This structure is used to distinguish and queue different traffic flows
> + * separately for fair queueing/AQM purposes.
> + *
> + * @txqi: txq_info structure it is associated at given time
Do we actually have to keep that? It's on a list per txqi, no?
> + * @flowchain: can be linked to other flows for RR purposes
RR?
> +void ieee80211_teardown_flows(struct ieee80211_local *local)
> +{
> + struct ieee80211_fq *fq = &local->fq;
> + struct ieee80211_sub_if_data *sdata;
> + struct sta_info *sta;
> + int i;
> +
> + if (!local->ops->wake_tx_queue)
> + return;
> +
> + list_for_each_entry_rcu(sta, &local->sta_list, list)
> + for (i = 0; i < IEEE80211_NUM_TIDS; i++)
> + ieee80211_purge_txq(local,
> + to_txq_info(sta->sta.txq[i]));
> +
> + list_for_each_entry_rcu(sdata, &local->interfaces, list)
> + ieee80211_purge_txq(local, to_txq_info(sdata->vif.txq));
Using RCU iteration here seems rather strange, since it's a teardown
flow? That doesn't seem necessary, since it's control path and must be
holding appropriate locks anyway to make sure nothing is added to the
lists.
> + skb = codel_dequeue(flow,
> + &flow->backlog,
> + 0,
> + &flow->cvars,
> + &fq->cparams,
> + codel_get_time(),
> + false);
What happened here? :)
> + if (!skb) {
> + if ((head == &txqi->new_flows) &&
> + !list_empty(&txqi->old_flows)) {
> + list_move_tail(&flow->flowchain, &txqi->old_flows);
> + } else {
> + list_del_init(&flow->flowchain);
> + flow->txqi = NULL;
> + }
> + goto begin;
> + }
Ouch. Any way you can make that easier to follow?
johannes
_______________________________________________
Codel mailing list
[email protected]
https://lists.bufferbloat.net/listinfo/codel
