[PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct_xmit()

[PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct_xmit()

[Song Liu]

Summary:

At the end of sch_direct_xmit(), we are in the else path of
!dev_xmit_complete(ret), which means ret == NETDEV_TX_OK. The following
condition will always fail and netif_xmit_frozen_or_stopped() is not
checked at all.

    if (ret && netif_xmit_frozen_or_stopped(txq))
         return false;

In this patch, this condition is fixed as:

    if (netif_xmit_frozen_or_stopped(txq))
         return false;

and further simplifies the code as:

    return !netif_xmit_frozen_or_stopped(txq);

Fixes: 29b86cdac00a ("net: sched: remove remaining uses for qdisc_qlen in xmit path")
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Song Liu <songliubraving@fb.com>
---
 net/sched/sch_generic.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 39c144b..8261d48 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -346,10 +346,7 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
 		return false;
 	}
 
-	if (ret && netif_xmit_frozen_or_stopped(txq))
-		return false;
-
-	return true;
+	return !netif_xmit_frozen_or_stopped(txq);
 }
 
 /*
-- 
2.9.5

Re: [PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct_xmit()

[Song Liu]

On Fri, May 25, 2018 at 11:11 AM, Song Liu <songliubraving@fb.com> wrote:
> Summary:
>
> At the end of sch_direct_xmit(), we are in the else path of
> !dev_xmit_complete(ret), which means ret == NETDEV_TX_OK. The following
> condition will always fail and netif_xmit_frozen_or_stopped() is not
> checked at all.
>
>     if (ret && netif_xmit_frozen_or_stopped(txq))
>          return false;
>
> In this patch, this condition is fixed as:
>
>     if (netif_xmit_frozen_or_stopped(txq))
>          return false;
>
> and further simplifies the code as:
>
>     return !netif_xmit_frozen_or_stopped(txq);
>
> Fixes: 29b86cdac00a ("net: sched: remove remaining uses for qdisc_qlen in xmit path")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
>  net/sched/sch_generic.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 39c144b..8261d48 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -346,10 +346,7 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>                 return false;
>         }
>
> -       if (ret && netif_xmit_frozen_or_stopped(txq))
> -               return false;
> -
> -       return true;
> +       return !netif_xmit_frozen_or_stopped(txq);
>  }
>
>  /*
> --
> 2.9.5
>

Alexei and I discussed about this offline. We would like to share our
discussion here to
clarify the motivation.

Before 29b86cdac00a, ret in condition "if (ret &&
netif_xmit_frozen_or_stopped()" is not
the value from dev_hard_start_xmit(), because ret is overwritten by
either qdisc_qlen()
or dev_requeue_skb(). Therefore, 29b86cdac00a changed the behavior of
this condition.

For ret from dev_hard_start_xmit(), I dig into the function and found
it is from return value
of ndo_start_xmit(). Per netdevice.h, ndo_start_xmit() should only
return NETDEV_TX_OK
or NETDEV_TX_BUSY. I survey many drivers, and they all follow the rule. The only
exception is vlan.

Given ret could only be NETDEV_TX_OK or NETDEV_TX_BUSY (ignore vlan for now),
if it fails condition "if (!dev_xmit_complete(ret))", ret must be
NETDEV_TX_OK == 0. So
netif_xmit_frozen_or_stopped() will always be bypassed.

It is probably OK to ignore netif_xmit_frozen_or_stopped(), and return true from
sch_direct_xmit(), as I didn't see that break any functionality. But
it is more like "correct
by accident" to me. This is the motivation of my original patch.

Alexei pointed out that, the following condition is more like original logic:

      if (qdisc_qlen(q) && netif_xmit_frozen_or_stopped(txq))
            return false;

However, I think John would like to remove qdisc_qlen() from the tx
path. I didn't see
any issue without the extra qdisc_qlen() check, so the patch is
probably good AS-IS.

Please share your comments and feedback on this.

Thanks,
Song

Re: [PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct_xmit()

[John Fastabend]

On 05/25/2018 12:46 PM, Song Liu wrote:
> On Fri, May 25, 2018 at 11:11 AM, Song Liu <songliubraving@fb.com> wrote:
>> Summary:
>>
>> At the end of sch_direct_xmit(), we are in the else path of
>> !dev_xmit_complete(ret), which means ret == NETDEV_TX_OK. The following
>> condition will always fail and netif_xmit_frozen_or_stopped() is not
>> checked at all.
>>
>>     if (ret && netif_xmit_frozen_or_stopped(txq))
>>          return false;
>>
>> In this patch, this condition is fixed as:
>>
>>     if (netif_xmit_frozen_or_stopped(txq))
>>          return false;
>>
>> and further simplifies the code as:
>>
>>     return !netif_xmit_frozen_or_stopped(txq);
>>
>> Fixes: 29b86cdac00a ("net: sched: remove remaining uses for qdisc_qlen in xmit path")
>> Cc: John Fastabend <john.fastabend@gmail.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
>> ---
>>  net/sched/sch_generic.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>> index 39c144b..8261d48 100644
>> --- a/net/sched/sch_generic.c
>> +++ b/net/sched/sch_generic.c
>> @@ -346,10 +346,7 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>>                 return false;
>>         }
>>
>> -       if (ret && netif_xmit_frozen_or_stopped(txq))
>> -               return false;
>> -
>> -       return true;
>> +       return !netif_xmit_frozen_or_stopped(txq);
>>  }
>>
>>  /*
>> --
>> 2.9.5
>>
> 
> Alexei and I discussed about this offline. We would like to share our
> discussion here to
> clarify the motivation.
> 
> Before 29b86cdac00a, ret in condition "if (ret &&
> netif_xmit_frozen_or_stopped()" is not
> the value from dev_hard_start_xmit(), because ret is overwritten by
> either qdisc_qlen()
> or dev_requeue_skb(). Therefore, 29b86cdac00a changed the behavior of
> this condition.
> 
> For ret from dev_hard_start_xmit(), I dig into the function and found
> it is from return value
> of ndo_start_xmit(). Per netdevice.h, ndo_start_xmit() should only
> return NETDEV_TX_OK
> or NETDEV_TX_BUSY. I survey many drivers, and they all follow the rule. The only
> exception is vlan.
> 
> Given ret could only be NETDEV_TX_OK or NETDEV_TX_BUSY (ignore vlan for now),
> if it fails condition "if (!dev_xmit_complete(ret))", ret must be
> NETDEV_TX_OK == 0. So
> netif_xmit_frozen_or_stopped() will always be bypassed.
> 
> It is probably OK to ignore netif_xmit_frozen_or_stopped(), and return true from
> sch_direct_xmit(), as I didn't see that break any functionality. But
> it is more like "correct
> by accident" to me. This is the motivation of my original patch.
> 
> Alexei pointed out that, the following condition is more like original logic:
> 
>       if (qdisc_qlen(q) && netif_xmit_frozen_or_stopped(txq))
>             return false;
> 
> However, I think John would like to remove qdisc_qlen() from the tx
> path. I didn't see

Yep qdisc_qlen() is not very friendly for lockless users. At
some point we will get around to writing a distributed rate
limiter qdisc and it will be nice to not have to work-around
qdisc_qlen().

> any issue without the extra qdisc_qlen() check, so the patch is
> probably good AS-IS.
> 
> Please share your comments and feedback on this.
> 

Thanks for the detailed analysis. The above patch looks OK
to me. Actually I'm debating if we should just drop the check.
But, there looks to be a case where drivers return NETDEV_TX_OK
and then stop the queue because it is nearly overrun. By putting
the check there we stop early instead of doing some extra work
before realizing the driver ring is full.

Still this overrun case should be rare so removing the check
should be OK. Plus as you note its not been running anyways. My
current recommendation is just remove the check altogether.

Thanks,
John 

> Thanks,
> Song
> 

Re: [PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct_xmit()

[Song Liu]

> On May 26, 2018, at 12:43 PM, John Fastabend <john.fastabend@gmail.com> wrote:
> 
> On 05/25/2018 12:46 PM, Song Liu wrote:
>> On Fri, May 25, 2018 at 11:11 AM, Song Liu <songliubraving@fb.com> wrote:
>>> Summary:
>>> 
>>> At the end of sch_direct_xmit(), we are in the else path of
>>> !dev_xmit_complete(ret), which means ret == NETDEV_TX_OK. The following
>>> condition will always fail and netif_xmit_frozen_or_stopped() is not
>>> checked at all.
>>> 
>>>    if (ret && netif_xmit_frozen_or_stopped(txq))
>>>         return false;
>>> 
>>> In this patch, this condition is fixed as:
>>> 
>>>    if (netif_xmit_frozen_or_stopped(txq))
>>>         return false;
>>> 
>>> and further simplifies the code as:
>>> 
>>>    return !netif_xmit_frozen_or_stopped(txq);
>>> 
>>> Fixes: 29b86cdac00a ("net: sched: remove remaining uses for qdisc_qlen in xmit path")
>>> Cc: John Fastabend <john.fastabend@gmail.com>
>>> Cc: David S. Miller <davem@davemloft.net>
>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>> ---
>>> net/sched/sch_generic.c | 5 +----
>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>> 
>>> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
>>> index 39c144b..8261d48 100644
>>> --- a/net/sched/sch_generic.c
>>> +++ b/net/sched/sch_generic.c
>>> @@ -346,10 +346,7 @@ bool sch_direct_xmit(struct sk_buff *skb, struct Qdisc *q,
>>>                return false;
>>>        }
>>> 
>>> -       if (ret && netif_xmit_frozen_or_stopped(txq))
>>> -               return false;
>>> -
>>> -       return true;
>>> +       return !netif_xmit_frozen_or_stopped(txq);
>>> }
>>> 
>>> /*
>>> --
>>> 2.9.5
>>> 
>> 
>> Alexei and I discussed about this offline. We would like to share our
>> discussion here to
>> clarify the motivation.
>> 
>> Before 29b86cdac00a, ret in condition "if (ret &&
>> netif_xmit_frozen_or_stopped()" is not
>> the value from dev_hard_start_xmit(), because ret is overwritten by
>> either qdisc_qlen()
>> or dev_requeue_skb(). Therefore, 29b86cdac00a changed the behavior of
>> this condition.
>> 
>> For ret from dev_hard_start_xmit(), I dig into the function and found
>> it is from return value
>> of ndo_start_xmit(). Per netdevice.h, ndo_start_xmit() should only
>> return NETDEV_TX_OK
>> or NETDEV_TX_BUSY. I survey many drivers, and they all follow the rule. The only
>> exception is vlan.
>> 
>> Given ret could only be NETDEV_TX_OK or NETDEV_TX_BUSY (ignore vlan for now),
>> if it fails condition "if (!dev_xmit_complete(ret))", ret must be
>> NETDEV_TX_OK == 0. So
>> netif_xmit_frozen_or_stopped() will always be bypassed.
>> 
>> It is probably OK to ignore netif_xmit_frozen_or_stopped(), and return true from
>> sch_direct_xmit(), as I didn't see that break any functionality. But
>> it is more like "correct
>> by accident" to me. This is the motivation of my original patch.
>> 
>> Alexei pointed out that, the following condition is more like original logic:
>> 
>>      if (qdisc_qlen(q) && netif_xmit_frozen_or_stopped(txq))
>>            return false;
>> 
>> However, I think John would like to remove qdisc_qlen() from the tx
>> path. I didn't see
> 
> Yep qdisc_qlen() is not very friendly for lockless users. At
> some point we will get around to writing a distributed rate
> limiter qdisc and it will be nice to not have to work-around
> qdisc_qlen().
> 
>> any issue without the extra qdisc_qlen() check, so the patch is
>> probably good AS-IS.
>> 
>> Please share your comments and feedback on this.
>> 
> 
> Thanks for the detailed analysis. The above patch looks OK
> to me. Actually I'm debating if we should just drop the check.
> But, there looks to be a case where drivers return NETDEV_TX_OK
> and then stop the queue because it is nearly overrun. By putting
> the check there we stop early instead of doing some extra work
> before realizing the driver ring is full.
> 
> Still this overrun case should be rare so removing the check
> should be OK. Plus as you note its not been running anyways. My
> current recommendation is just remove the check altogether.
> 
> Thanks,
> John 

Thanks John! I will resend a clean up patch to net-next. 

Song

Re: [PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct_xmit()

[David Miller]

From: Song Liu <songliubraving@fb.com>
Date: Fri, 25 May 2018 11:11:44 -0700

> Summary:
> 
> At the end of sch_direct_xmit(), we are in the else path of
> !dev_xmit_complete(ret), which means ret == NETDEV_TX_OK. The following
> condition will always fail and netif_xmit_frozen_or_stopped() is not
> checked at all.
> 
>     if (ret && netif_xmit_frozen_or_stopped(txq))
>          return false;
> 
> In this patch, this condition is fixed as:
> 
>     if (netif_xmit_frozen_or_stopped(txq))
>          return false;
> 
> and further simplifies the code as:
> 
>     return !netif_xmit_frozen_or_stopped(txq);
> 
> Fixes: 29b86cdac00a ("net: sched: remove remaining uses for qdisc_qlen in xmit path")
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Song Liu <songliubraving@fb.com>

I expect a new version of this patch which removes the test entirely.

Re: [PATCH net] net: sched: check netif_xmit_frozen_or_stopped() in sch_direct_xmit()

[Song Liu]

> On May 29, 2018, at 7:02 AM, David Miller <davem@davemloft.net> wrote:
> 
> From: Song Liu <songliubraving@fb.com>
> Date: Fri, 25 May 2018 11:11:44 -0700
> 
>> Summary:
>> 
>> At the end of sch_direct_xmit(), we are in the else path of
>> !dev_xmit_complete(ret), which means ret == NETDEV_TX_OK. The following
>> condition will always fail and netif_xmit_frozen_or_stopped() is not
>> checked at all.
>> 
>>    if (ret && netif_xmit_frozen_or_stopped(txq))
>>         return false;
>> 
>> In this patch, this condition is fixed as:
>> 
>>    if (netif_xmit_frozen_or_stopped(txq))
>>         return false;
>> 
>> and further simplifies the code as:
>> 
>>    return !netif_xmit_frozen_or_stopped(txq);
>> 
>> Fixes: 29b86cdac00a ("net: sched: remove remaining uses for qdisc_qlen in xmit path")
>> Cc: John Fastabend <john.fastabend@gmail.com>
>> Cc: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Song Liu <songliubraving@fb.com>
> 
> I expect a new version of this patch which removes the test entirely.

The new version of it is here: http://patchwork.ozlabs.org/patch/921708/

Thanks,
Song