[PATCH net v2] net: sched: do not requeue a NULL skb

[PATCH net v2] net: sched: do not requeue a NULL skb

[Lars Persson]

A failure in validate_xmit_skb_list() triggered an unconditional call
to dev_requeue_skb with skb=NULL. This slowly grows the queue
discipline's qlen count until all traffic through the queue stops.

By introducing a NULL check in dev_requeue_skb it was also necessary
to make the __netif_schedule call conditional to avoid scheduling an
empty queue.

Fixes: 55a93b3ea780 ("qdisc: validate skb without holding lock")
Signed-off-by: Lars Persson <larper@axis.com>
---
 net/sched/sch_generic.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index f18c350..4e6a79c 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -47,10 +47,13 @@ EXPORT_SYMBOL(default_qdisc_ops);
 
 static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 {
-	q->gso_skb = skb;
-	q->qstats.requeues++;
-	q->q.qlen++;	/* it's still part of the queue */
-	__netif_schedule(q);
+	if (skb) {
+		q->gso_skb = skb;
+		q->qstats.requeues++;
+		q->q.qlen++;	/* it's still part of the queue */
+	}
+	if (qdisc_qlen(q))
+		__netif_schedule(q);
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Eric Dumazet]

On Mon, 2016-04-11 at 08:24 +0200, Lars Persson wrote:
> A failure in validate_xmit_skb_list() triggered an unconditional call
> to dev_requeue_skb with skb=NULL. This slowly grows the queue
> discipline's qlen count until all traffic through the queue stops.
> 
> By introducing a NULL check in dev_requeue_skb it was also necessary
> to make the __netif_schedule call conditional to avoid scheduling an
> empty queue.
> 
> Fixes: 55a93b3ea780 ("qdisc: validate skb without holding lock")
> Signed-off-by: Lars Persson <larper@axis.com>
> ---
>  net/sched/sch_generic.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index f18c350..4e6a79c 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -47,10 +47,13 @@ EXPORT_SYMBOL(default_qdisc_ops);
>  
>  static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
>  {
> -	q->gso_skb = skb;
> -	q->qstats.requeues++;
> -	q->q.qlen++;	/* it's still part of the queue */
> -	__netif_schedule(q);
> +	if (skb) {
> +		q->gso_skb = skb;
> +		q->qstats.requeues++;
> +		q->q.qlen++;	/* it's still part of the queue */
> +	}
> +	if (qdisc_qlen(q))
> +		__netif_schedule(q);
>  
>  	return 0;
>  }


Please always CC patch author when fixing a bug.

Why adding the if (qdisc_qlen(q)) extra test ?

This seems unrelated to the bug fix, and probably should be part of a
second patch targeting net-next tree.

Also please add a likely() clause

if (likely(skb)) {
        q->gso_skb = skb;
        q->qstats.requeues++;
        q->q.qlen++;    /* it's still part of the queue */
        __netif_schedule(q);
}

Thanks !

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Lars Persson]

On 04/11/2016 03:23 PM, Eric Dumazet wrote:
> On Mon, 2016-04-11 at 08:24 +0200, Lars Persson wrote:
>> A failure in validate_xmit_skb_list() triggered an unconditional call
>> to dev_requeue_skb with skb=NULL. This slowly grows the queue
>> discipline's qlen count until all traffic through the queue stops.
>>
>> By introducing a NULL check in dev_requeue_skb it was also necessary
>> to make the __netif_schedule call conditional to avoid scheduling an
>> empty queue.
>>
>> Fixes: 55a93b3ea780 ("qdisc: validate skb without holding lock")
>> Signed-off-by: Lars Persson <larper@axis.com>
>> ---
>>   net/sched/sch_generic.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index f18c350..4e6a79c 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -47,10 +47,13 @@ EXPORT_SYMBOL(default_qdisc_ops);
>>
>>   static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
>>   {
>> -	q->gso_skb = skb;
>> -	q->qstats.requeues++;
>> -	q->q.qlen++;	/* it's still part of the queue */
>> -	__netif_schedule(q);
>> +	if (skb) {
>> +		q->gso_skb = skb;
>> +		q->qstats.requeues++;
>> +		q->q.qlen++;	/* it's still part of the queue */
>> +	}
>> +	if (qdisc_qlen(q))
>> +		__netif_schedule(q);
>>
>>   	return 0;
>>   }
>
>
> Please always CC patch author when fixing a bug.
>
> Why adding the if (qdisc_qlen(q)) extra test ?
>
> This seems unrelated to the bug fix, and probably should be part of a
> second patch targeting net-next tree.

I though it would be prudent because the queue can be non-empty even for 
the case of skb=NULL. So should it be there in this patch, another patch 
or not at all ?

>
> Also please add a likely() clause
>
> if (likely(skb)) {
>          q->gso_skb = skb;
>          q->qstats.requeues++;
>          q->q.qlen++;    /* it's still part of the queue */
>          __netif_schedule(q);
> }

Will fix.

> Thanks !
>
>
>
>
>

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Eric Dumazet]

On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:

> I though it would be prudent because the queue can be non-empty even for 
> the case of skb=NULL. So should it be there in this patch, another patch 
> or not at all ?

Then maybe change return code ?

It seems strange that a validate_xmit_skb_list() failure stops the
__qdisc_run() loop but schedules another round.

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Lars Persson]

On 04/11/2016 04:22 PM, Eric Dumazet wrote:
> On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
>
>> I though it would be prudent because the queue can be non-empty even for
>> the case of skb=NULL. So should it be there in this patch, another patch
>> or not at all ?
>
> Then maybe change return code ?
>
> It seems strange that a validate_xmit_skb_list() failure stops the
> __qdisc_run() loop but schedules another round.
>
>

It was suggested by Cong Wang to return 0 in order to stop the loop. Do 
you guys agree that the loop should be stopped for such failures ? Then 
I will put the schedule call inside the if as you proposed earlier.

- Lars

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Eric Dumazet]

On Mon, 2016-04-11 at 17:17 +0200, Lars Persson wrote:
> 
> On 04/11/2016 04:22 PM, Eric Dumazet wrote:
> > On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
> >
> >> I though it would be prudent because the queue can be non-empty even for
> >> the case of skb=NULL. So should it be there in this patch, another patch
> >> or not at all ?
> >
> > Then maybe change return code ?
> >
> > It seems strange that a validate_xmit_skb_list() failure stops the
> > __qdisc_run() loop but schedules another round.
> >
> >
> 
> It was suggested by Cong Wang to return 0 in order to stop the loop. Do 
> you guys agree that the loop should be stopped for such failures ? Then 
> I will put the schedule call inside the if as you proposed earlier.

What are the causes of validate_xmit_skb_list() failures ?

If gso segmentations fail because of memory pressure, better free more
skbs right now.

In any case, having a single test " if (skb)  " sounds better to me,
to have a fast path.

So your first patch was probably a better idea.

v2 has two tests instead of one.

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Cong Wang]

On Mon, Apr 11, 2016 at 8:17 AM, Lars Persson <lars.persson@axis.com> wrote:
>
>
> On 04/11/2016 04:22 PM, Eric Dumazet wrote:
>>
>> On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
>>
>>> I though it would be prudent because the queue can be non-empty even for
>>> the case of skb=NULL. So should it be there in this patch, another patch
>>> or not at all ?
>>
>>
>> Then maybe change return code ?
>>
>> It seems strange that a validate_xmit_skb_list() failure stops the
>> __qdisc_run() loop but schedules another round.
>>
>>
>
> It was suggested by Cong Wang to return 0 in order to stop the loop. Do you
> guys agree that the loop should be stopped for such failures ? Then I will
> put the schedule call inside the if as you proposed earlier.

I don't see any reason why we should continue on failure. And, I didn't
suggest you to return reschedule it either. I was suggesting to just return
0 for skb == NULL case.

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Cong Wang]

On Mon, Apr 11, 2016 at 8:52 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2016-04-11 at 17:17 +0200, Lars Persson wrote:
>>
>> On 04/11/2016 04:22 PM, Eric Dumazet wrote:
>> > On Mon, 2016-04-11 at 15:38 +0200, Lars Persson wrote:
>> >
>> >> I though it would be prudent because the queue can be non-empty even for
>> >> the case of skb=NULL. So should it be there in this patch, another patch
>> >> or not at all ?
>> >
>> > Then maybe change return code ?
>> >
>> > It seems strange that a validate_xmit_skb_list() failure stops the
>> > __qdisc_run() loop but schedules another round.
>> >
>> >
>>
>> It was suggested by Cong Wang to return 0 in order to stop the loop. Do
>> you guys agree that the loop should be stopped for such failures ? Then
>> I will put the schedule call inside the if as you proposed earlier.
>
> What are the causes of validate_xmit_skb_list() failures ?
>
> If gso segmentations fail because of memory pressure, better free more
> skbs right now.
>
> In any case, having a single test " if (skb)  " sounds better to me,
> to have a fast path.
>
> So your first patch was probably a better idea.
>
> v2 has two tests instead of one.

I am fine with either way as long as the loop stops on failure.
Folding the test "if (skb)" into one also requires to retake the spinlock.

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Eric Dumazet]

On Mon, 2016-04-11 at 11:02 -0700, Cong Wang wrote:

> I am fine with either way as long as the loop stops on failure.
> Folding the test "if (skb)" into one also requires to retake the spinlock.

Adding the likely() in this path would probably help as well.

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index f18c35024207..07202d9ac4f6 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -159,12 +159,14 @@ int sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
        if (validate)
                skb = validate_xmit_skb_list(skb, dev);
 
-       if (skb) {
+       if (likely(skb)) {
                HARD_TX_LOCK(dev, txq, smp_processor_id());
                if (!netif_xmit_frozen_or_stopped(txq))
                        skb = dev_hard_start_xmit(skb, dev, txq, &ret);
 
                HARD_TX_UNLOCK(dev, txq);
+       } else {
+               ... does all and return...
        }
        spin_lock(root_lock);

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Eric Dumazet]

On Mon, 2016-04-11 at 11:26 -0700, Eric Dumazet wrote:
> On Mon, 2016-04-11 at 11:02 -0700, Cong Wang wrote:
> 
> > I am fine with either way as long as the loop stops on failure.


Note that skb that could not be validated is already freed.

So I do not see any value from stopping the loop, since
we need to schedule the queue to avoid tx hang.

Just process following skb if there is one, fact that skb is sent or
dropped does not matter.

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Cong Wang]

On Mon, Apr 11, 2016 at 11:30 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2016-04-11 at 11:26 -0700, Eric Dumazet wrote:
>> On Mon, 2016-04-11 at 11:02 -0700, Cong Wang wrote:
>>
>> > I am fine with either way as long as the loop stops on failure.
>
>
> Note that skb that could not be validated is already freed.
>
> So I do not see any value from stopping the loop, since
> we need to schedule the queue to avoid tx hang.
>
> Just process following skb if there is one, fact that skb is sent or
> dropped does not matter.

My point is, for example, in OOM case, we don't know processing
more SKB would make it better or worse. Maybe we really need to
check the error code to decide to continue to exit?

Re: [PATCH net v2] net: sched: do not requeue a NULL skb

[Eric Dumazet]

On Mon, 2016-04-11 at 16:19 -0700, Cong Wang wrote:

> My point is, for example, in OOM case, we don't know processin
> more SKB would make it better or worse. Maybe we really need to
> check the error code to decide to continue to exit?

Really, given this bug has been there for a long time (v3.18 ???), I
doubt it matters.

Nothing can tell that following packets in the qdisc need any
transformation, and memory allocations.

So I would just fix the bug in the simplest way.

__qdisc_run() has all the checks needed to yield when needed 
(if (quota <= 0 || need_resched())) , no need to add more complexity
there.