* [PATCH net] fq_codel: fix NET_XMIT_CN behavior @ 2016-06-04 17:43 Eric Dumazet 2016-06-04 19:03 ` [net] " Florian Westphal 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2016-06-04 17:43 UTC (permalink / raw) To: David Miller; +Cc: netdev, Stas Nichiporovich, WANG Cong, Jamal Hadi Salim From: Eric Dumazet <edumazet@google.com> My prior attempt to fix the backlogs of parents failed. If we return NET_XMIT_CN, our parents wont increase their backlog, so our qdisc_tree_reduce_backlog() should take this into account. Fixes: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()") Fixes: 2ccccf5fb43f ("net_sched: update hierarchical backlog too") Reported-by: Stas Nichiporovich <stasn77@gmail.com> Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: WANG Cong <xiyou.wangcong@gmail.com> Cc: Jamal Hadi Salim <jhs@mojatatu.com> --- net/sched/sch_fq_codel.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c index 6883a8971562..c57ec480a2da 100644 --- a/net/sched/sch_fq_codel.c +++ b/net/sched/sch_fq_codel.c @@ -240,11 +240,19 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch) q->drop_overlimit += prev_qlen - sch->q.qlen; if (memory_limited) q->drop_overmemory += prev_qlen - sch->q.qlen; - /* As we dropped packet(s), better let upper stack know this */ - qdisc_tree_reduce_backlog(sch, prev_qlen - sch->q.qlen, - prev_backlog - sch->qstats.backlog); - - return ret == idx ? NET_XMIT_CN : NET_XMIT_SUCCESS; + /* As we dropped packet(s), better let upper stack know this. + * If we dropped a packet for this flow, return NET_XMIT_CN, + * but in this case, our parents wont increase their backlogs. + */ + prev_qlen -= sch->q.qlen; + prev_backlog -= sch->qstats.backlog; + if (ret == idx) { + qdisc_tree_reduce_backlog(sch, prev_qlen - 1, + prev_backlog - qdisc_pkt_len(skb)); + return NET_XMIT_CN; + } + qdisc_tree_reduce_backlog(sch, prev_qlen, prev_backlog); + return NET_XMIT_SUCCESS; } /* This is the specific function called from codel_dequeue() ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [net] fq_codel: fix NET_XMIT_CN behavior 2016-06-04 17:43 [PATCH net] fq_codel: fix NET_XMIT_CN behavior Eric Dumazet @ 2016-06-04 19:03 ` Florian Westphal 2016-06-04 19:40 ` Eric Dumazet 2016-06-04 19:55 ` [PATCH v2 net] " Eric Dumazet 0 siblings, 2 replies; 16+ messages in thread From: Florian Westphal @ 2016-06-04 19:03 UTC (permalink / raw) To: Eric Dumazet Cc: David Miller, netdev, Stas Nichiporovich, WANG Cong, Jamal Hadi Salim Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > My prior attempt to fix the backlogs of parents failed. > > If we return NET_XMIT_CN, our parents wont increase their backlog, > so our qdisc_tree_reduce_backlog() should take this into account. [..] > diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c > index 6883a8971562..c57ec480a2da 100644 > --- a/net/sched/sch_fq_codel.c > +++ b/net/sched/sch_fq_codel.c > @@ -240,11 +240,19 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch) > q->drop_overlimit += prev_qlen - sch->q.qlen; > if (memory_limited) > q->drop_overmemory += prev_qlen - sch->q.qlen; > - /* As we dropped packet(s), better let upper stack know this */ > - qdisc_tree_reduce_backlog(sch, prev_qlen - sch->q.qlen, > - prev_backlog - sch->qstats.backlog); > - > - return ret == idx ? NET_XMIT_CN : NET_XMIT_SUCCESS; > + /* As we dropped packet(s), better let upper stack know this. > + * If we dropped a packet for this flow, return NET_XMIT_CN, > + * but in this case, our parents wont increase their backlogs. > + */ > + prev_qlen -= sch->q.qlen; > + prev_backlog -= sch->qstats.backlog; > + if (ret == idx) { > + qdisc_tree_reduce_backlog(sch, prev_qlen - 1, > + prev_backlog - qdisc_pkt_len(skb)); > + return NET_XMIT_CN; Is skb still valid here? AFAICS its possible that fq_codel_drop() drops it. Other than that this looks good, thanks Eric! ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [net] fq_codel: fix NET_XMIT_CN behavior 2016-06-04 19:03 ` [net] " Florian Westphal @ 2016-06-04 19:40 ` Eric Dumazet 2016-06-04 19:55 ` [PATCH v2 net] " Eric Dumazet 1 sibling, 0 replies; 16+ messages in thread From: Eric Dumazet @ 2016-06-04 19:40 UTC (permalink / raw) To: Florian Westphal Cc: David Miller, netdev, Stas Nichiporovich, WANG Cong, Jamal Hadi Salim On Sat, 2016-06-04 at 21:03 +0200, Florian Westphal wrote: > Is skb still valid here? AFAICS its possible that fq_codel_drop() drops it. > > Other than that this looks good, thanks Eric! You are are right. skb is guaranteed to be valid only when we return NET_XMIT_SUCCESS. I'll send a V2, thanks ! ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-04 19:03 ` [net] " Florian Westphal 2016-06-04 19:40 ` Eric Dumazet @ 2016-06-04 19:55 ` Eric Dumazet 2016-06-05 13:03 ` Jamal Hadi Salim ` (2 more replies) 1 sibling, 3 replies; 16+ messages in thread From: Eric Dumazet @ 2016-06-04 19:55 UTC (permalink / raw) To: Florian Westphal Cc: David Miller, netdev, Stas Nichiporovich, WANG Cong, Jamal Hadi Salim From: Eric Dumazet <edumazet@google.com> My prior attempt to fix the backlogs of parents failed. If we return NET_XMIT_CN, our parents wont increase their backlog, so our qdisc_tree_reduce_backlog() should take this into account. v2: Florian Westphal pointed out that we could drop the packet, so we need to save qdisc_pkt_len(skb) in a temp variable before calling fq_codel_drop() Fixes: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()") Fixes: 2ccccf5fb43f ("net_sched: update hierarchical backlog too") Reported-by: Stas Nichiporovich <stasn77@gmail.com> Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: WANG Cong <xiyou.wangcong@gmail.com> Cc: Jamal Hadi Salim <jhs@mojatatu.com> --- net/sched/sch_fq_codel.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c index 6883a8971562..fff7867f4a4f 100644 --- a/net/sched/sch_fq_codel.c +++ b/net/sched/sch_fq_codel.c @@ -199,6 +199,7 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch) unsigned int idx, prev_backlog, prev_qlen; struct fq_codel_flow *flow; int uninitialized_var(ret); + unsigned int pkt_len; bool memory_limited; idx = fq_codel_classify(skb, sch, &ret); @@ -230,6 +231,8 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch) prev_backlog = sch->qstats.backlog; prev_qlen = sch->q.qlen; + /* save this packet length as it might be dropped by fq_codel_drop() */ + pkt_len = qdisc_pkt_len(skb); /* fq_codel_drop() is quite expensive, as it performs a linear search * in q->backlogs[] to find a fat flow. * So instead of dropping a single packet, drop half of its backlog @@ -237,14 +240,23 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch) */ ret = fq_codel_drop(sch, q->drop_batch_size); - q->drop_overlimit += prev_qlen - sch->q.qlen; + prev_qlen -= sch->q.qlen; + prev_backlog -= sch->qstats.backlog; + q->drop_overlimit += prev_qlen; if (memory_limited) - q->drop_overmemory += prev_qlen - sch->q.qlen; - /* As we dropped packet(s), better let upper stack know this */ - qdisc_tree_reduce_backlog(sch, prev_qlen - sch->q.qlen, - prev_backlog - sch->qstats.backlog); + q->drop_overmemory += prev_qlen; - return ret == idx ? NET_XMIT_CN : NET_XMIT_SUCCESS; + /* As we dropped packet(s), better let upper stack know this. + * If we dropped a packet for this flow, return NET_XMIT_CN, + * but in this case, our parents wont increase their backlogs. + */ + if (ret == idx) { + qdisc_tree_reduce_backlog(sch, prev_qlen - 1, + prev_backlog - pkt_len); + return NET_XMIT_CN; + } + qdisc_tree_reduce_backlog(sch, prev_qlen, prev_backlog); + return NET_XMIT_SUCCESS; } /* This is the specific function called from codel_dequeue() ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-04 19:55 ` [PATCH v2 net] " Eric Dumazet @ 2016-06-05 13:03 ` Jamal Hadi Salim 2016-06-05 20:30 ` Cong Wang 2016-06-07 22:39 ` David Miller 2 siblings, 0 replies; 16+ messages in thread From: Jamal Hadi Salim @ 2016-06-05 13:03 UTC (permalink / raw) To: Eric Dumazet, Florian Westphal Cc: David Miller, netdev, Stas Nichiporovich, WANG Cong On 16-06-04 03:55 PM, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > My prior attempt to fix the backlogs of parents failed. > > If we return NET_XMIT_CN, our parents wont increase their backlog, > so our qdisc_tree_reduce_backlog() should take this into account. > > v2: Florian Westphal pointed out that we could drop the packet, > so we need to save qdisc_pkt_len(skb) in a temp variable before > calling fq_codel_drop() > > Fixes: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()") > Fixes: 2ccccf5fb43f ("net_sched: update hierarchical backlog too") > Reported-by: Stas Nichiporovich <stasn77@gmail.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: WANG Cong <xiyou.wangcong@gmail.com> > Cc: Jamal Hadi Salim <jhs@mojatatu.com> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com> cheers, jamal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-04 19:55 ` [PATCH v2 net] " Eric Dumazet 2016-06-05 13:03 ` Jamal Hadi Salim @ 2016-06-05 20:30 ` Cong Wang 2016-06-05 20:54 ` Eric Dumazet 2016-06-07 22:39 ` David Miller 2 siblings, 1 reply; 16+ messages in thread From: Cong Wang @ 2016-06-05 20:30 UTC (permalink / raw) To: Eric Dumazet Cc: Florian Westphal, David Miller, netdev, Stas Nichiporovich, Jamal Hadi Salim On Sat, Jun 4, 2016 at 12:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > My prior attempt to fix the backlogs of parents failed. > > If we return NET_XMIT_CN, our parents wont increase their backlog, > so our qdisc_tree_reduce_backlog() should take this into account. > Acked-by: Cong Wang <xiyou.wangcong@gmail.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-05 20:30 ` Cong Wang @ 2016-06-05 20:54 ` Eric Dumazet 2016-06-05 20:55 ` Eric Dumazet 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2016-06-05 20:54 UTC (permalink / raw) To: Cong Wang Cc: Florian Westphal, David Miller, netdev, Stas Nichiporovich, Jamal Hadi Salim On Sun, 2016-06-05 at 13:30 -0700, Cong Wang wrote: > On Sat, Jun 4, 2016 at 12:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > > > > My prior attempt to fix the backlogs of parents failed. > > > > If we return NET_XMIT_CN, our parents wont increase their backlog, > > so our qdisc_tree_reduce_backlog() should take this into account. > > > > Acked-by: Cong Wang <xiyou.wangcong@gmail.com> BTW, we might need a similar fix in sch_fq.c ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-05 20:54 ` Eric Dumazet @ 2016-06-05 20:55 ` Eric Dumazet 2016-06-05 23:30 ` Cong Wang 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2016-06-05 20:55 UTC (permalink / raw) To: Cong Wang Cc: Florian Westphal, David Miller, netdev, Stas Nichiporovich, Jamal Hadi Salim On Sun, 2016-06-05 at 13:54 -0700, Eric Dumazet wrote: > BTW, we might need a similar fix in sch_fq.c I meant sch_sfq.c ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-05 20:55 ` Eric Dumazet @ 2016-06-05 23:30 ` Cong Wang 2016-06-06 10:49 ` Jamal Hadi Salim 0 siblings, 1 reply; 16+ messages in thread From: Cong Wang @ 2016-06-05 23:30 UTC (permalink / raw) To: Eric Dumazet Cc: Florian Westphal, David Miller, netdev, Stas Nichiporovich, Jamal Hadi Salim On Sun, Jun 5, 2016 at 1:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Sun, 2016-06-05 at 13:54 -0700, Eric Dumazet wrote: > >> BTW, we might need a similar fix in sch_fq.c > > I meant sch_sfq.c > Potentially all of the following: net/sched/sch_choke.c: return NET_XMIT_CN; net/sched/sch_fifo.c: return NET_XMIT_CN; net/sched/sch_generic.c: return NET_XMIT_CN; net/sched/sch_gred.c: return NET_XMIT_CN; net/sched/sch_hhf.c: return NET_XMIT_CN; net/sched/sch_red.c: return NET_XMIT_CN; net/sched/sch_sfb.c: return NET_XMIT_CN; net/sched/sch_sfq.c: return NET_XMIT_CN; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-05 23:30 ` Cong Wang @ 2016-06-06 10:49 ` Jamal Hadi Salim 2016-06-06 11:42 ` Florian Westphal 2016-06-06 14:29 ` Eric Dumazet 0 siblings, 2 replies; 16+ messages in thread From: Jamal Hadi Salim @ 2016-06-06 10:49 UTC (permalink / raw) To: Cong Wang, Eric Dumazet Cc: Florian Westphal, David Miller, netdev, Stas Nichiporovich On 16-06-05 07:30 PM, Cong Wang wrote: > Potentially all of the following: > > net/sched/sch_choke.c: return NET_XMIT_CN; > net/sched/sch_fifo.c: return NET_XMIT_CN; > net/sched/sch_generic.c: return NET_XMIT_CN; > net/sched/sch_gred.c: return NET_XMIT_CN; > net/sched/sch_hhf.c: return NET_XMIT_CN; > net/sched/sch_red.c: return NET_XMIT_CN; > net/sched/sch_sfb.c: return NET_XMIT_CN; > net/sched/sch_sfq.c: return NET_XMIT_CN; > As long as we are not loosing the stat that the packet is dropped. Some qdiscs have a counter which indicates congestion drops(look at RED variants of early drops); maybe codel needs one. The parent also must account for childs drops. BTW, returning NET_XMIT_CN could be confusing to tcp; it does not mean that the packet that we are getting return code for was dropped; it could mean _another_ packet in the queue was dropped. cheers, jamal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-06 10:49 ` Jamal Hadi Salim @ 2016-06-06 11:42 ` Florian Westphal 2016-06-06 14:37 ` Eric Dumazet 2016-06-06 14:29 ` Eric Dumazet 1 sibling, 1 reply; 16+ messages in thread From: Florian Westphal @ 2016-06-06 11:42 UTC (permalink / raw) To: Jamal Hadi Salim Cc: Cong Wang, Eric Dumazet, Florian Westphal, David Miller, netdev, Stas Nichiporovich Jamal Hadi Salim <jhs@mojatatu.com> wrote: > BTW, returning NET_XMIT_CN could be confusing to tcp; > it does not mean that the packet that we are getting return > code for was dropped; it could mean _another_ packet in > the queue was dropped. Yes, but we currently conceal NET_XMIT_CN from upper layer (tcp) via the net_xmit_* macros: #define net_xmit_eval(e) ((e) == NET_XMIT_CN ? 0 : (e)) #define net_xmit_errno(e) ((e) != NET_XMIT_CN ? -ENOBUFS : 0) Might be worth changing this so tcp reduces cwnd in _CN case too. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-06 11:42 ` Florian Westphal @ 2016-06-06 14:37 ` Eric Dumazet 2016-06-06 16:18 ` Florian Westphal 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2016-06-06 14:37 UTC (permalink / raw) To: Florian Westphal Cc: Jamal Hadi Salim, Cong Wang, David Miller, netdev, Stas Nichiporovich On Mon, 2016-06-06 at 13:42 +0200, Florian Westphal wrote: > Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > BTW, returning NET_XMIT_CN could be confusing to tcp; > > it does not mean that the packet that we are getting return > > code for was dropped; it could mean _another_ packet in > > the queue was dropped. > > Yes, but we currently conceal NET_XMIT_CN from upper layer (tcp) > via the net_xmit_* macros: > > #define net_xmit_eval(e) ((e) == NET_XMIT_CN ? 0 : (e)) > #define net_xmit_errno(e) ((e) != NET_XMIT_CN ? -ENOBUFS : 0) > > Might be worth changing this so tcp reduces cwnd in _CN case too. It always had been the case. tcp_transmit_skb() calls tcp_enter_cwr(), unless someone broke this. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-06 14:37 ` Eric Dumazet @ 2016-06-06 16:18 ` Florian Westphal 2016-06-10 12:02 ` Jamal Hadi Salim 0 siblings, 1 reply; 16+ messages in thread From: Florian Westphal @ 2016-06-06 16:18 UTC (permalink / raw) To: Eric Dumazet Cc: Florian Westphal, Jamal Hadi Salim, Cong Wang, David Miller, netdev, Stas Nichiporovich Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Mon, 2016-06-06 at 13:42 +0200, Florian Westphal wrote: > > Jamal Hadi Salim <jhs@mojatatu.com> wrote: > > > BTW, returning NET_XMIT_CN could be confusing to tcp; > > > it does not mean that the packet that we are getting return > > > code for was dropped; it could mean _another_ packet in > > > the queue was dropped. > > > > Yes, but we currently conceal NET_XMIT_CN from upper layer (tcp) > > via the net_xmit_* macros: > > > > #define net_xmit_eval(e) ((e) == NET_XMIT_CN ? 0 : (e)) > > #define net_xmit_errno(e) ((e) != NET_XMIT_CN ? -ENOBUFS : 0) > > > > Might be worth changing this so tcp reduces cwnd in _CN case too. > > It always had been the case. > > tcp_transmit_skb() calls tcp_enter_cwr(), unless someone broke this. No, you're right. We hide error in ip_send_skb but that function is not used in tcp case. So all is good here. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-06 16:18 ` Florian Westphal @ 2016-06-10 12:02 ` Jamal Hadi Salim 0 siblings, 0 replies; 16+ messages in thread From: Jamal Hadi Salim @ 2016-06-10 12:02 UTC (permalink / raw) To: Florian Westphal, Eric Dumazet Cc: Cong Wang, David Miller, netdev, Stas Nichiporovich On 16-06-06 12:18 PM, Florian Westphal wrote: > Eric Dumazet <eric.dumazet@gmail.com> wrote: >> On Mon, 2016-06-06 at 13:42 +0200, Florian Westphal wrote: >>> Jamal Hadi Salim <jhs@mojatatu.com> wrote: >>>> BTW, returning NET_XMIT_CN could be confusing to tcp; >>>> it does not mean that the packet that we are getting return >>>> code for was dropped; it could mean _another_ packet in >>>> the queue was dropped. >>> >>> Yes, but we currently conceal NET_XMIT_CN from upper layer (tcp) >>> via the net_xmit_* macros: >>> >>> #define net_xmit_eval(e) ((e) == NET_XMIT_CN ? 0 : (e)) >>> #define net_xmit_errno(e) ((e) != NET_XMIT_CN ? -ENOBUFS : 0) >>> >>> Might be worth changing this so tcp reduces cwnd in _CN case too. >> >> It always had been the case. >> >> tcp_transmit_skb() calls tcp_enter_cwr(), unless someone broke this. > > No, you're right. We hide error in ip_send_skb but that function is > not used in tcp case. > > So all is good here. > Just making sure because that little gem is not obvious;-> Eric, on the NET_XMIT_CN digression: depends on the qdisc scheduler. NET_XMIT_CN could mean one of two things: "There is congestion. The packet you sent was dropped locally" and "There is congestion. The packet you sent was not dropped locally" Yes, they both react with tcp_enter_cwr() and from a macroscopic view it probably doesnt matter as much because tcp state machine will kick in at some point. I think better use could be made of such knowledge. cheers, jamal ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-06 10:49 ` Jamal Hadi Salim 2016-06-06 11:42 ` Florian Westphal @ 2016-06-06 14:29 ` Eric Dumazet 1 sibling, 0 replies; 16+ messages in thread From: Eric Dumazet @ 2016-06-06 14:29 UTC (permalink / raw) To: Jamal Hadi Salim Cc: Cong Wang, Florian Westphal, David Miller, netdev, Stas Nichiporovich On Mon, 2016-06-06 at 06:49 -0400, Jamal Hadi Salim wrote: > On 16-06-05 07:30 PM, Cong Wang wrote: > > > Potentially all of the following: > > > > net/sched/sch_choke.c: return NET_XMIT_CN; > > net/sched/sch_fifo.c: return NET_XMIT_CN; > > net/sched/sch_generic.c: return NET_XMIT_CN; > > net/sched/sch_gred.c: return NET_XMIT_CN; > > net/sched/sch_hhf.c: return NET_XMIT_CN; > > net/sched/sch_red.c: return NET_XMIT_CN; > > net/sched/sch_sfb.c: return NET_XMIT_CN; > > net/sched/sch_sfq.c: return NET_XMIT_CN; > > > > As long as we are not loosing the stat that the packet > is dropped. Some qdiscs have a counter which indicates > congestion drops(look at RED variants of early drops); > maybe codel needs one. The parent also must account for > childs drops. > BTW, returning NET_XMIT_CN could be confusing to tcp; > it does not mean that the packet that we are getting return > code for was dropped; it could mean _another_ packet in > the queue was dropped. NET_XMIT_CN does not confuse TCP at all. Look, the issue here is the accuracy of byte backlog in upper qdiscs, after Cong patch. When at enqueue(sch, P2) we drop P1, but enqueue P2, we need to take care of reporting to upper qdisc qdisc_pkt_len(P2) - qdisc_pkt_len(P1) We used to not call qdisc_tree_reduce_backlog() in this case, since it was qdisc_tree_decrease_qlen(sch, delta_packets) before Cong patch. We now need to call qdisc_tree_reduce_backlog(sch, 0, delta_bytes) ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 net] fq_codel: fix NET_XMIT_CN behavior 2016-06-04 19:55 ` [PATCH v2 net] " Eric Dumazet 2016-06-05 13:03 ` Jamal Hadi Salim 2016-06-05 20:30 ` Cong Wang @ 2016-06-07 22:39 ` David Miller 2 siblings, 0 replies; 16+ messages in thread From: David Miller @ 2016-06-07 22:39 UTC (permalink / raw) To: eric.dumazet; +Cc: fw, netdev, stasn77, xiyou.wangcong, jhs From: Eric Dumazet <eric.dumazet@gmail.com> Date: Sat, 04 Jun 2016 12:55:13 -0700 > From: Eric Dumazet <edumazet@google.com> > > My prior attempt to fix the backlogs of parents failed. > > If we return NET_XMIT_CN, our parents wont increase their backlog, > so our qdisc_tree_reduce_backlog() should take this into account. > > v2: Florian Westphal pointed out that we could drop the packet, > so we need to save qdisc_pkt_len(skb) in a temp variable before > calling fq_codel_drop() > > Fixes: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()") > Fixes: 2ccccf5fb43f ("net_sched: update hierarchical backlog too") > Reported-by: Stas Nichiporovich <stasn77@gmail.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> Applied. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-06-10 12:03 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-04 17:43 [PATCH net] fq_codel: fix NET_XMIT_CN behavior Eric Dumazet 2016-06-04 19:03 ` [net] " Florian Westphal 2016-06-04 19:40 ` Eric Dumazet 2016-06-04 19:55 ` [PATCH v2 net] " Eric Dumazet 2016-06-05 13:03 ` Jamal Hadi Salim 2016-06-05 20:30 ` Cong Wang 2016-06-05 20:54 ` Eric Dumazet 2016-06-05 20:55 ` Eric Dumazet 2016-06-05 23:30 ` Cong Wang 2016-06-06 10:49 ` Jamal Hadi Salim 2016-06-06 11:42 ` Florian Westphal 2016-06-06 14:37 ` Eric Dumazet 2016-06-06 16:18 ` Florian Westphal 2016-06-10 12:02 ` Jamal Hadi Salim 2016-06-06 14:29 ` Eric Dumazet 2016-06-07 22:39 ` David Miller
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.