Modify

Opened 21 months ago

Last modified 21 months ago

#22349 new defect

OpenWRT wrong adjustment of fq_codel defaults

Reported by: brouer@… Owned by: developers
Priority: highest Milestone: Bugs Paradise
Component: kernel Version: Trunk
Keywords: Cc:

Description

This is an important fix for OpenWRT, please read!

OpenWRT changed the default fq_codel sch->limit from 10240 to 1024, without also adjusting q->flows_cnt (number of buckets). Eric Dumazet suggest to have at least 8 packets per bucket, to let Codel have a chance to trigger. Thus you must also adjust the buckets (q->flows_cnt) for this not to break. (With 1024 / 8 adjust it to 128)

Problematic OpenWRT commit in question:

http://git.openwrt.org/?p=openwrt.git;a=patch;h=12cd6578084e
12cd6578084e ("kernel: revert fq_codel quantum override to prevent it from causing too much cpu load with higher speed (#21326)")

I also highly recommend you cherry-pick this very recent commit:

net-next: 9d18562a2278 ("fq_codel: add batch ability to fq_codel_drop()")
https://git.kernel.org/davem/net-next/c/9d18562a227
https://patchwork.ozlabs.org/patch/617307/

This should fix very high CPU usage in-case fq_codel goes into drop mode.

The problem is that drop mode was considered rare, and implementation wise it was chosen to be more expensive (to save cycles on normal mode). Unfortunately is it easy to trigger with an UDP flood. Drop mode is especially expensive for smaller devices, as it scans a 4K big array, thus 64 cache misses for small devices!

The fix is to allow drop-mode to bulk-drop more packets when entering drop-mode (default 64 bulk drop). That way we don't suddenly experience a significantly higher processing cost per packet, but instead can amortize this.

This is basically the conclusion of a very long email thread avail via: http://thread.gmane.org/gmane.network.routing.codel/873

Attachments (0)

Change History (3)

comment:1 follow-up: Changed 21 months ago by hauke

Thank you for the hint, I will look into this, but currently I do not have a device at hand. wouldn't it be better to calculate q->flows_cnt based on sch->limit? This way this would not happen.

We are talking about r47811.

comment:2 in reply to: ↑ 1 Changed 21 months ago by brouer@…

Replying to hauke:

[...] wouldn't it be better to calculate q->flows_cnt based on sch->limit? This way this would not happen.

It would not change/improve much in this place in the code (fq_codel_init) as the patch just change the default "init" settings.

The problem of calculating q->flows_cnt based on sch->limit, is that we allow userspace to redefine these two settings. If you want to, you can propose an upstream change that in function fq_codel_change(), calculate q->flows_cnt iif (if-and-only-if) it is not set from userspace/tc and that limit is configured from userspace. That would bring value, but do it upstream first please (you can Cc me).

comment:3 Changed 21 months ago by RubenKelevra

Nice one, thanks for the report, please fix!

Add Comment

Modify Ticket

Action
as new .
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.