On 8 March 2016 at 00:06, Dave Taht <dave.t...@gmail.com> wrote: > Dear Michal: > > Going through this patchset... (while watching it compile) > > > + if (!local->hw.txq_cparams.target) > + local->hw.txq_cparams.target = MS2TIME(5); > > MS2TIME(20) for now and/or add something saner to this than !*backlog > > target will not be a constant in the long run. > > + if (now - custom_codel_get_enqueue_time(skb) < p->target || > + !*backlog) { > + /* went below - stay below for at least interval */ > + vars->first_above_time = 0; > + return false; > + } > > > *backlog < some_sane_value_for_an_aggregate_for_this_station > > Unlike regular codel *backlog should be a ptr to the queuesize for > this station, not the total queue. > > regular codel, by using the shared backlog for all queues, is trying > to get to a 1 packet depth for all queues, (see commit: > 865ec5523dadbedefbc5710a68969f686a28d928 ), and store the flow in the > network, not the queue... > > BUT in wifi's case you want to provide good service to all stations, > which is filling up an aggregate > for each... (and varying the "sane_value_for_the_aggregate" to suit > per sta service time requirements in a given round of all stations).
Hmm.. This actually makes me think that: skb = codel_dequeue(flow, &flow->backlog, &flow->cvars, &hw->txq_cparams, codel_get_time(), false); is kind of wrong. If you want to maintain per-sta aggregate-long queue this probably needs a sta->backlog instead of flow->backlog (because you can have multiple flows per station) in the first place. Not quite sure about cvars though. Thoughts? Michał -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html