[Cake] NET_XMIT_CN question

[Cake] NET_XMIT_CN question

[Dave Taht]

We decreased the size of the queue, regardless of what flow we dropped
from... so why do we not need to decrease the queue size when using
NET_XMIT_CN? Where is that done by the caller? I have grepped and
googled...

As best as I recall the semantics of NET_XMIT_CN changed over time, it used to
only work on enqueue on a tail drop queue, where it would push back on
the stack to not, actually, drop the packet, but to halve the window
and requeue.

Staring at the code now, I don't see how it could be right... here OR
in the existing fq_codel code in the mainline.

        /* Return Congestion Notification only if we dropped a packet
         * from this flow.
         */
        if (fq_codel_drop(sch) == idx)
                return NET_XMIT_CN;

        /* As we dropped a packet, better let upper stack know this */
        qdisc_tree_decrease_qlen(sch, 1);
        return NET_XMIT_SUCCESS;


(I am not sure NET_XMIT_CN works right when there is a hash collision,
for example, although the updated doc does seem to indicate that,

... still, then, where does (an equivalent for)
qdisc_tree_decrease_qlen kick in?)

So I ripped it out. Otherwise... the code does look correct, now that
I've reviewed it further.
But does putting it back in have any measurable effect?

...

Also I note there was a bunch of commits to sch_fq_codel mainline since july,
notably this one.

c0afd9ce4d6a646fb6433536f95a418bb348fab1


Dave Täht
Let's go make home routers and wifi faster! With better software!
https://www.gofundme.com/savewifi


On Fri, Nov 27, 2015 at 3:33 PM, Kevin Darbyshire-Bryant
<kevin@darbyshire-bryant.me.uk> wrote:
> I've been pondering this commit and wonder if we're throwing out baby
> with bathwater:
>
> https://github.com/dtaht/sch_cake/commit/90e792115f10fa2a31327208302e66c0ab3f2251
>
> The original code, to paraphrase in something resembling English went:
>
> 1. I've just enqueud a packet & Oh sh*t we're over our q_disc buffer
> limit, we'd better do something about it, hmmm
> 2. Whilst we're over this limit, go look through our flows (cake_drop)
> and kill something, if that something was in the same flow as the thing
> we just enqueued then set a flag 'cos these guys are part of the
> problem!  Keep track of how many things we killed.
> 3 Now we're below the limit and finished killing packets, return back to
> our caller - if we killed stuff in the same flow the caller is sending,
> then return 'NET_XMIT_CN' (we're congested), else just say 'happy as Larry'
>
>  *//* NET_XMIT_CN is special. It does not guarantee that this packet is
> lost. It/*
> */* indicates that the device will soon be dropping packets, or already
> drops/*
> */* some packets of the same priority; prompting us to send less
> aggressively. *//*
>
> What I do agree looked odd in the original code was the 'dropped -
> same_flow':  We haven't dropped any more or fewer packets just 'cos we
> found one or more in 'the same flow'.
>
> -        qdisc_tree_decrease_qlen(sch, dropped - same_flow);  <--????????
> +        qdisc_tree_decrease_qlen(sch, dropped);
>
> Incidentally, a search of 'NET_XMIT_CN' turned up
> http://www.bufferbloat.net/issues/398.pdf
>
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake
>

Re: [Cake] NET_XMIT_CN question

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]



On 27/11/15 15:16, Dave Taht wrote:
> We decreased the size of the queue, regardless of what flow we dropped
> from... so why do we not need to decrease the queue size when using
> NET_XMIT_CN? Where is that done by the caller? I have grepped and
> googled...
>
> As best as I recall the semantics of NET_XMIT_CN changed over time, it used to
> only work on enqueue on a tail drop queue, where it would push back on
> the stack to not, actually, drop the packet, but to halve the window
> and requeue.
>
> Staring at the code now, I don't see how it could be right... here OR
> in the existing fq_codel code in the mainline.
>
>         /* Return Congestion Notification only if we dropped a packet
>          * from this flow.
>          */
>         if (fq_codel_drop(sch) == idx)
>                 return NET_XMIT_CN;
>
>         /* As we dropped a packet, better let upper stack know this */
>         qdisc_tree_decrease_qlen(sch, 1);
>         return NET_XMIT_SUCCESS;
>
>
> (I am not sure NET_XMIT_CN works right when there is a hash collision,
> for example, although the updated doc does seem to indicate that,
>
> ... still, then, where does (an equivalent for)
> qdisc_tree_decrease_qlen kick in?)
>
> So I ripped it out. Otherwise... the code does look correct, now that
> I've reviewed it further.
> But does putting it back in have any measurable effect?
I've no realistic way of testing & finding out.  I'd say that if that
bit of code ever gets executed then things are in an unfortunate place
already, however if there's a way of telling a 'very active talker' (and
they must be 'cos they've just queued a packet *and* cake_drop() has
chosen them as having the biggest b->backlogs[i] - and that may help
answer one of your todo items as well) to back off, then we should take it.

Spaghetti monster knows what *actually* happens further up the stack :-)

>
> ...
>
> Also I note there was a bunch of commits to sch_fq_codel mainline since july,
> notably this one.
>
> c0afd9ce4d6a646fb6433536f95a418bb348fab1
>
>
> Dave Täht
> Let's go make home routers and wifi faster! With better software!
> https://www.gofundme.com/savewifi
>



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4816 bytes --]

Re: [Cake] NET_XMIT_CN question

[Kevin Darbyshire-Bryant]

[-- Attachment #1: Type: text/plain, Size: 3580 bytes --]

Eric Dumazet very kindly answered a few of my questions regarding XMIT_CN.

I think the cake 'NET_XMIT_CN' optimisation has been prematurely ripped out.

Eric's helpful comments:
"
    If a packet scheduler returns NET_XMIT_CN , caller know a packet was
    dropped.
   
    If a packet scheduler returns NET_XMIT_SUCCESS , caller does not know
    a(nother) packet was dropped.
   
    Look for example in htb this part :
   
        } else if ((ret = qdisc_enqueue(skb, cl->un.leaf.q)) !=
    NET_XMIT_SUCCESS) {
             ....
            return ret;
       } else {
           ....
      }
   
    sch->q.qlen++;  // HERE : this part wont be run if NET_XMIT_CN was
    returned.
   
    A packet scheduler returns NET_XMIT_CN when it wants to tell the upper
    stack (like TCP) that it is worthless trying to push more packets for
    this flow, as it is (self) inflicting losses.
   
    Basically it is a (best effort) signal/hint to backoff.
   
    IP fragmentation layer for example aborts its attempt to send
    following fragments.
"

Kevin



On 27/11/15 15:44, Kevin Darbyshire-Bryant wrote:
>
> On 27/11/15 15:16, Dave Taht wrote:
>> We decreased the size of the queue, regardless of what flow we dropped
>> from... so why do we not need to decrease the queue size when using
>> NET_XMIT_CN? Where is that done by the caller? I have grepped and
>> googled...
>>
>> As best as I recall the semantics of NET_XMIT_CN changed over time, it used to
>> only work on enqueue on a tail drop queue, where it would push back on
>> the stack to not, actually, drop the packet, but to halve the window
>> and requeue.
>>
>> Staring at the code now, I don't see how it could be right... here OR
>> in the existing fq_codel code in the mainline.
>>
>>         /* Return Congestion Notification only if we dropped a packet
>>          * from this flow.
>>          */
>>         if (fq_codel_drop(sch) == idx)
>>                 return NET_XMIT_CN;
>>
>>         /* As we dropped a packet, better let upper stack know this */
>>         qdisc_tree_decrease_qlen(sch, 1);
>>         return NET_XMIT_SUCCESS;
>>
>>
>> (I am not sure NET_XMIT_CN works right when there is a hash collision,
>> for example, although the updated doc does seem to indicate that,
>>
>> ... still, then, where does (an equivalent for)
>> qdisc_tree_decrease_qlen kick in?)
>>
>> So I ripped it out. Otherwise... the code does look correct, now that
>> I've reviewed it further.
>> But does putting it back in have any measurable effect?
> I've no realistic way of testing & finding out.  I'd say that if that
> bit of code ever gets executed then things are in an unfortunate place
> already, however if there's a way of telling a 'very active talker' (and
> they must be 'cos they've just queued a packet *and* cake_drop() has
> chosen them as having the biggest b->backlogs[i] - and that may help
> answer one of your todo items as well) to back off, then we should take it.
>
> Spaghetti monster knows what *actually* happens further up the stack :-)
>
>> ...
>>
>> Also I note there was a bunch of commits to sch_fq_codel mainline since july,
>> notably this one.
>>
>> c0afd9ce4d6a646fb6433536f95a418bb348fab1
>>
>>
>> Dave Täht
>> Let's go make home routers and wifi faster! With better software!
>> https://www.gofundme.com/savewifi
>>
>
>
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4816 bytes --]