Re: [Cake] Possible BUG - parent backlog incorrectly updated in case of NET_XMIT_CN

[Jonathan Morton <chromatix99@gmail.com>]

>> And there’s also the problem that we might not need to drop packets as
>> large as the incoming packet in order to fit the latter into the queue
>> - so this corrected correction may be *negative* (the queue is longer
>> than before) - but qdisc_tree_reduce_backlog() only takes an unsigned
>> parameter here.
> 
> That's a very minor detail.
> 
> If the code does :
> 
> reduce_backlog(unsigned quantity)
> {
> 	q->backlog -= quantity;
> }
> 
> Then the fact that @quantity is signed or unsigned is irrelevant.

Not so - unless you are very sure that q->backlog is the same size as quantity.  In an increasingly 64-bit world, that is by no means assured in the future.  I don’t like relying on wraparound behaviour without making that assumption explicit, which is precisely what the signed types in C are for.

>> IMHO the NET_XMIT_CN semantics are broken.  It might be better to drop
>> support for it, since it should rarely be triggered.
> 
> What exact part is broken ?
> Semantic looks good to me.

It’s broken in that a negative correction may be required in the first place.  It places additional burden on every producer of the CN signal who isn’t a tail-dropper.  I can only assume that the behaviour was designed with only tail-drop in mind - and as we both know, that is not the only option any more.

It appears to me that there are four things than enqueue() may want to tell its caller:

1: That the packet was enqueued successfully (NET_XMIT_SUCCESS).

2: That the packet was enqueued successfully, but some other relevant packet had to be dropped due to link congestion.

3: That the packet was *not* enqueued, and this was due to link congestion.  This is potentially useful for tail-dropping queues, including AQMs.

4: That the packet was *not* enqueued, for some reason other than link congestion; the “error case".

Currently NET_XMIT_CN appears to cover case 3, whereas Cake normally wants cases 1 & 2 (and will only signal case 4 if a GSO aggregate couldn’t be segmented).

For the time being, I have changed my development branch of Cake to always signal case 1 except for the error case.

 - Jonathan Morton