| Message ID | 1340947448.29822.41.camel@edumazet-glaptop |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 29 Jun 2012 07:24:08 +0200 > By the way, I am not sure NET_XMIT_CN is correctly used in RED. > > Or maybe my understanding of NET_XMIT_CN is wrong. > > If a the packet is dropped in enqueue(), why use NET_XMIT_CN instead of > NET_XMIT_DROP ? > > This seems to mean : I dropped _this_ packet, but dont worry too much, I > might accept other packets, so please go on... I am pretty sure the behavior in RED is intentional. It's a soft push back on TCP. We're taking this path when we are unable to sucessfully ECN mark a packet. But our intention was to do so. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2012-06-28 at 22:29 -0700, David Miller wrote: > I am pretty sure the behavior in RED is intentional. > > It's a soft push back on TCP. > tcp_enter_cwr() is called the same for DROP and CN > We're taking this path when we are unable to sucessfully ECN mark a > packet. But our intention was to do so. > Hmm, problem is the sender thinks the packet was queued for transmission. ret = macvlan_queue_xmit(skb, dev); if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) { struct macvlan_pcpu_stats *pcpu_stats; pcpu_stats = this_cpu_ptr(vlan->pcpu_stats); u64_stats_update_begin(&pcpu_stats->syncp); pcpu_stats->tx_packets++; pcpu_stats->tx_bytes += len; u64_stats_update_end(&pcpu_stats->syncp); } else { this_cpu_inc(vlan->pcpu_stats->tx_dropped); } NET_XMIT_CN has a lazy semantic it seems. I will just dont rely on it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 29 Jun 2012 07:50:08 +0200 > Hmm, problem is the sender thinks the packet was queued for > transmission. > > ret = macvlan_queue_xmit(skb, dev); > if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) { > struct macvlan_pcpu_stats *pcpu_stats; > > pcpu_stats = this_cpu_ptr(vlan->pcpu_stats); > u64_stats_update_begin(&pcpu_stats->syncp); > pcpu_stats->tx_packets++; > pcpu_stats->tx_bytes += len; > u64_stats_update_end(&pcpu_stats->syncp); > } else { > this_cpu_inc(vlan->pcpu_stats->tx_dropped); > } > > NET_XMIT_CN has a lazy semantic it seems. > > I will just dont rely on it. I think we cannot just ignore this issue. I will take a deeper look, because we should have NET_XMIT_CN be very well defined and adjust any mis-use. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 29 Jun 2012 07:50:08 +0200 > Hmm, problem is the sender thinks the packet was queued for > transmission. > > ret = macvlan_queue_xmit(skb, dev); > if (likely(ret == NET_XMIT_SUCCESS || ret == NET_XMIT_CN)) { > struct macvlan_pcpu_stats *pcpu_stats; > > pcpu_stats = this_cpu_ptr(vlan->pcpu_stats); > u64_stats_update_begin(&pcpu_stats->syncp); > pcpu_stats->tx_packets++; > pcpu_stats->tx_bytes += len; > u64_stats_update_end(&pcpu_stats->syncp); > } else { > this_cpu_inc(vlan->pcpu_stats->tx_dropped); > } Ok, that is the meaning this has taken on. Same test exists in vlan_dev.c and this test used to be present also in the ipip.h macros some time ago. Nobody really does anything special with this value, except to translate it to a zero 0 when propagating back to sockets. The only thing it guards is the selection of which statistic to increment. For all practical purposes it is treated as NET_XMIT_SUCCESS except in one location, pktgen, where it causes the errors counter to increment. Looking this over, I'd say we should just get rid of it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c index 633e32d..0fc5b6c 100644 --- a/net/sched/sch_red.c +++ b/net/sched/sch_red.c @@ -77,7 +77,7 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch) sch->qstats.overlimits++; if (!red_use_ecn(q) || !INET_ECN_set_ce(skb)) { q->stats.prob_drop++; - goto congestion_drop; + goto drop; } q->stats.prob_mark++; @@ -88,7 +88,7 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch) if (red_use_harddrop(q) || !red_use_ecn(q) || !INET_ECN_set_ce(skb)) { q->stats.forced_drop++; - goto congestion_drop; + goto drop; } q->stats.forced_mark++; @@ -104,9 +104,8 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc *sch) } return ret; -congestion_drop: - qdisc_drop(skb, sch); - return NET_XMIT_CN; +drop: + return qdisc_drop(skb, sch); } static struct sk_buff *red_dequeue(struct Qdisc *sch)