| Message ID | 152905845136.88583.6197221349040585517.stgit@buzz |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net_sched: blackhole: tell upper qdisc about dropped packets | expand |
On 06/15/2018 03:27 AM, Konstantin Khlebnikov wrote: > When blackhole is used on top of classful qdisc like hfsc it breaks > qlen and backlog counters because packets are disappear without notice. > > In HFSC non-zero qlen while all classes are inactive triggers warning: > WARNING: ... at net/sched/sch_hfsc.c:1393 hfsc_dequeue+0xba4/0xe90 [sch_hfsc] > and schedules watchdog work endlessly. > > This patch return __NET_XMIT_BYPASS in addition to NET_XMIT_SUCCESS, > this flag tells upper layer: this packet is gone and isn't queued. > > Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> > --- > net/sched/sch_blackhole.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/sched/sch_blackhole.c b/net/sched/sch_blackhole.c > index c98a61e980ba..9c4c2bb547d7 100644 > --- a/net/sched/sch_blackhole.c > +++ b/net/sched/sch_blackhole.c > @@ -21,7 +21,7 @@ static int blackhole_enqueue(struct sk_buff *skb, struct Qdisc *sch, > struct sk_buff **to_free) > { > qdisc_drop(skb, sch, to_free); > - return NET_XMIT_SUCCESS; > + return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; Why do not we use instead : return qdisc_drop(skb, sch, to_free); Although noop_enqueue() seems to use : return NET_XMIT_CN; Oh well.
On 15.06.2018 16:13, Eric Dumazet wrote: > > > On 06/15/2018 03:27 AM, Konstantin Khlebnikov wrote: >> When blackhole is used on top of classful qdisc like hfsc it breaks >> qlen and backlog counters because packets are disappear without notice. >> >> In HFSC non-zero qlen while all classes are inactive triggers warning: >> WARNING: ... at net/sched/sch_hfsc.c:1393 hfsc_dequeue+0xba4/0xe90 [sch_hfsc] >> and schedules watchdog work endlessly. >> >> This patch return __NET_XMIT_BYPASS in addition to NET_XMIT_SUCCESS, >> this flag tells upper layer: this packet is gone and isn't queued. >> >> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> >> --- >> net/sched/sch_blackhole.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/net/sched/sch_blackhole.c b/net/sched/sch_blackhole.c >> index c98a61e980ba..9c4c2bb547d7 100644 >> --- a/net/sched/sch_blackhole.c >> +++ b/net/sched/sch_blackhole.c >> @@ -21,7 +21,7 @@ static int blackhole_enqueue(struct sk_buff *skb, struct Qdisc *sch, >> struct sk_buff **to_free) >> { >> qdisc_drop(skb, sch, to_free); >> - return NET_XMIT_SUCCESS; >> + return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; > > Why do not we use instead : > > return qdisc_drop(skb, sch, to_free); > > Although noop_enqueue() seems to use : > > return NET_XMIT_CN; > > Oh well. > > I suppose "blackhole" should work like "successful" xmit, but counted as drop.
On Fri, Jun 15, 2018 at 6:21 AM, Konstantin Khlebnikov <khlebnikov@yandex-team.ru> wrote: > On 15.06.2018 16:13, Eric Dumazet wrote: >> >> >> >> On 06/15/2018 03:27 AM, Konstantin Khlebnikov wrote: >>> >>> When blackhole is used on top of classful qdisc like hfsc it breaks >>> qlen and backlog counters because packets are disappear without notice. >>> >>> In HFSC non-zero qlen while all classes are inactive triggers warning: >>> WARNING: ... at net/sched/sch_hfsc.c:1393 hfsc_dequeue+0xba4/0xe90 >>> [sch_hfsc] >>> and schedules watchdog work endlessly. >>> >>> This patch return __NET_XMIT_BYPASS in addition to NET_XMIT_SUCCESS, >>> this flag tells upper layer: this packet is gone and isn't queued. >>> >>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> >>> --- >>> net/sched/sch_blackhole.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/net/sched/sch_blackhole.c b/net/sched/sch_blackhole.c >>> index c98a61e980ba..9c4c2bb547d7 100644 >>> --- a/net/sched/sch_blackhole.c >>> +++ b/net/sched/sch_blackhole.c >>> @@ -21,7 +21,7 @@ static int blackhole_enqueue(struct sk_buff *skb, >>> struct Qdisc *sch, >>> struct sk_buff **to_free) >>> { >>> qdisc_drop(skb, sch, to_free); >>> - return NET_XMIT_SUCCESS; >>> + return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; >> >> >> Why do not we use instead : >> >> return qdisc_drop(skb, sch, to_free); >> >> Although noop_enqueue() seems to use : >> >> return NET_XMIT_CN; >> >> Oh well. >> >> > > I suppose "blackhole" should work like "successful" xmit, but counted as > drop. But anything !NET_XMIT_SUCCESS is basically same for upper layer: err = qdisc_enqueue(skb, cl->qdisc, to_free); if (unlikely(err != NET_XMIT_SUCCESS)) { if (net_xmit_drop_count(err)) { cl->qstats.drops++; qdisc_qstats_drop(sch); } return err; } So using NET_XMIT_DROP is same in this case?
On Fri, Jun 15, 2018 at 9:00 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Fri, Jun 15, 2018 at 6:21 AM, Konstantin Khlebnikov > <khlebnikov@yandex-team.ru> wrote: >> On 15.06.2018 16:13, Eric Dumazet wrote: >>> >>> >>> >>> On 06/15/2018 03:27 AM, Konstantin Khlebnikov wrote: >>>> >>>> When blackhole is used on top of classful qdisc like hfsc it breaks >>>> qlen and backlog counters because packets are disappear without notice. >>>> >>>> In HFSC non-zero qlen while all classes are inactive triggers warning: >>>> WARNING: ... at net/sched/sch_hfsc.c:1393 hfsc_dequeue+0xba4/0xe90 >>>> [sch_hfsc] >>>> and schedules watchdog work endlessly. >>>> >>>> This patch return __NET_XMIT_BYPASS in addition to NET_XMIT_SUCCESS, >>>> this flag tells upper layer: this packet is gone and isn't queued. >>>> >>>> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> >>>> --- >>>> net/sched/sch_blackhole.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/net/sched/sch_blackhole.c b/net/sched/sch_blackhole.c >>>> index c98a61e980ba..9c4c2bb547d7 100644 >>>> --- a/net/sched/sch_blackhole.c >>>> +++ b/net/sched/sch_blackhole.c >>>> @@ -21,7 +21,7 @@ static int blackhole_enqueue(struct sk_buff *skb, >>>> struct Qdisc *sch, >>>> struct sk_buff **to_free) >>>> { >>>> qdisc_drop(skb, sch, to_free); >>>> - return NET_XMIT_SUCCESS; >>>> + return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; >>> >>> >>> Why do not we use instead : >>> >>> return qdisc_drop(skb, sch, to_free); >>> >>> Although noop_enqueue() seems to use : >>> >>> return NET_XMIT_CN; >>> >>> Oh well. >>> >>> >> >> I suppose "blackhole" should work like "successful" xmit, but counted as >> drop. > > But anything !NET_XMIT_SUCCESS is basically same for upper > layer: > > err = qdisc_enqueue(skb, cl->qdisc, to_free); > if (unlikely(err != NET_XMIT_SUCCESS)) { > if (net_xmit_drop_count(err)) { > cl->qstats.drops++; > qdisc_qstats_drop(sch); > } > return err; > } > > So using NET_XMIT_DROP is same in this case? Not same for __dev_xmit_skb()... Yeah, we should keep the NET_XMIT_SUCCESS bit.
From: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> Date: Fri, 15 Jun 2018 13:27:31 +0300 > When blackhole is used on top of classful qdisc like hfsc it breaks > qlen and backlog counters because packets are disappear without notice. > > In HFSC non-zero qlen while all classes are inactive triggers warning: > WARNING: ... at net/sched/sch_hfsc.c:1393 hfsc_dequeue+0xba4/0xe90 [sch_hfsc] > and schedules watchdog work endlessly. > > This patch return __NET_XMIT_BYPASS in addition to NET_XMIT_SUCCESS, > this flag tells upper layer: this packet is gone and isn't queued. > > Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> Applied, thanks.
diff --git a/net/sched/sch_blackhole.c b/net/sched/sch_blackhole.c index c98a61e980ba..9c4c2bb547d7 100644 --- a/net/sched/sch_blackhole.c +++ b/net/sched/sch_blackhole.c @@ -21,7 +21,7 @@ static int blackhole_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free) { qdisc_drop(skb, sch, to_free); - return NET_XMIT_SUCCESS; + return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; } static struct sk_buff *blackhole_dequeue(struct Qdisc *sch)
When blackhole is used on top of classful qdisc like hfsc it breaks qlen and backlog counters because packets are disappear without notice. In HFSC non-zero qlen while all classes are inactive triggers warning: WARNING: ... at net/sched/sch_hfsc.c:1393 hfsc_dequeue+0xba4/0xe90 [sch_hfsc] and schedules watchdog work endlessly. This patch return __NET_XMIT_BYPASS in addition to NET_XMIT_SUCCESS, this flag tells upper layer: this packet is gone and isn't queued. Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru> --- net/sched/sch_blackhole.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)