Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix cobalt_should_drop() and reduce memory reallocations #136

Closed
wants to merge 5 commits into from
Closed

Fix cobalt_should_drop() and reduce memory reallocations #136

wants to merge 5 commits into from

Conversation

inste
Copy link
Contributor

@inste inste commented Jun 10, 2020

No description provided.

@tohojo
Copy link
Collaborator

tohojo commented Jun 10, 2020

For the first commit, I think the proper fix would be to fix the setter in the upstream kernel (and in this repo, wrap it in compat).

Why is the second change needed?

@inste
Copy link
Contributor Author

inste commented Jun 10, 2020 •
edited

Why is the second change needed?

I've made some measurements of ipv4 forward+napt on mips router with 580 MHz single-core CPU, so every instruction is important there. It appears that on kernel 4.9 skb_try_make_writable() reallocates skb, if skb was allocated in ethernet driver via so-called 'build skb' method from page cache (it was discovered by strange increase of kmalloc-2048 slab at first). This behaviour greatly increase CPU load and decrease throughput (at least by one-third).

@tohojo
Copy link
Collaborator

tohojo commented Jun 10, 2020 via email

@inste
Copy link
Contributor Author

inste commented Jun 11, 2020

For the first commit, I think the proper fix would be to fix the setter in the upstream kernel (and in this repo, wrap it in compat).

Yes, upstream can be fixed also, but there are some corner cases:

  • upstream function can't work with fragmented skb, so we must linearize it at first. It it the second place in cake where skb must be linearized (after dscp bleaching in cake_handle_diffserv()).
  • there is no cake_skb_proto() functional in uptsream, so it should be merged first.

As for me, cobalt_set_ce() is specific enough to cake to leave it here.

@tohojo
Copy link
Collaborator

tohojo commented Jun 11, 2020

For the first commit, I think the proper fix would be to fix the setter in the upstream kernel (and in this repo, wrap it in compat).

Yes, upstream can be fixed also, but there are some corner cases:

* upstream function can't work with fragmented skb, so we must linearize it at first. It it the second place in cake where skb must be linearized (after dscp bleaching in cake_handle_diffserv()).

* there is no cake_skb_proto() functional in uptsream, so it should be merged first.

As for me, cobalt_set_ce() is specific enough to cake to leave it here.

Hmm, okay, looking at it again that's a fair point actually :)

tohojo
tohojo requested changes Jun 11, 2020
View reviewed changes
Copy link
Collaborator

@tohojo tohojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments on the diffserv extraction. Also, please add the rationale to the commit message and properly rebase your PR so there are no merge commits.

sch_cake.c Outdated Show resolved Hide resolved
sch_cake.c Outdated Show resolved Hide resolved
sch_cake.c Outdated Show resolved Hide resolved
sch_cake.c Outdated Show resolved Hide resolved
sch_cake.c Outdated Show resolved Hide resolved
tohojo referenced this pull request Jun 23, 2020
@inste
sch_cake: properly obtain skb protocol in case of VLAN/QinQ
be8e2e4 
cake_handle_diffserv() expects only ETH_P_IP or ETH_P_IPV6, which means that all tagged headers should be stripped down.
Fix this issue by implementing cake_skb_proto() helper instead of using tc_skb_proto().

Fixes: 9fba602 ("sch_cake: Use tc_skb_protocol for getting packet protocol")
Signed-off-by: Ilya Ponetayev <i.ponetaev@ndmsystems.com>
@tohojo
Copy link
Collaborator

tohojo commented Jun 23, 2020

Ping @inste - did you have a chance to look at updating this? :)

@inste
Copy link
Contributor Author

inste commented Jun 23, 2020

@tohojo sorry for delay, I've realized that 'bus factor' is not only joke... Will rework and test today.

@tohojo
Copy link
Collaborator

tohojo commented Jun 23, 2020 via email

tohojo and others added 5 commits June 23, 2020 16:05
@tohojo @inste
cobalt_compat: Fix kernel version for qdisc_tree_reduce_backlog in 4.1
9622d9a 
Seems there was an off-by-one error in which version of the 4.1 series
backported the qdisc_tree_reduce_backlog helper. Fixes dtaht#132.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
@tohojo @inste
cobalt_compat: Handle addition of __tcp_hdrlen in 4.1 series
5a56722 
Seems some Asus routers are still running 4.1 kernels, where the
__tcp_hdrlen helper was backported in 4.1.50. Handle this in the compat
version checks.

Fixes dtaht#134.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
@inste
sch_cake: properly obtain skb protocol in case of VLAN/QinQ
ac5a188 
cake_handle_diffserv() expects only ETH_P_IP or ETH_P_IPV6, which means that all tagged headers should be stripped down.
Fix this issue by implementing cake_skb_proto() helper instead of using tc_skb_proto().

Fixes: 9fba602 ("sch_cake: Use tc_skb_protocol for getting packet protocol")
Signed-off-by: Ilya Ponetayev <i.ponetaev@ndmsystems.com>
@inste
sch_cake: properly handle skb protocol on CE
204add1 
cobalt_should_drop() tries to set CE via INET_ECN_set_ce() call, but this function falsely relies on skb->protocol to determine network header.
Fix this by implementing cobalt_set_ce() helper and employ cake_skb_proto() for proper detection.

Fixes: 3fb9914 ("Retire usage of cobalt.c and merge cobalt functionality elsewhere")
Fixes: 6cc4172 ("Add COBALT core algorithm - combination of simplfied Codel and BLUE.")
Signed-off-by: Ilya Ponetayev <i.ponetaev@ndmsystems.com>
@inste
sch_cake: don't try to reallocate or unshare skb unconditionally
c7ceda4 
cake_handle_diffserv() tries to linearize mac and network header parts of skb and to make it writable unconditionally.
In some cases it leads to full skb reallocation, which reduces throughput and increases CPU load, especially on embedded devices.
Obtain DSCP value via read-only skb_header_pointer() call, and leave linearization only for DSCP bleaching or ECN CE setting.

Fixes: 6ff4561 ("sch_cake: Make sure we can write the IP header before changing DSCP bits")
Signed-off-by: Ilya Ponetayev <i.ponetaev@ndmsystems.com>
@inste
Copy link
Contributor Author

inste commented Jun 23, 2020 •
edited

@tohojo done, but got confused with endless rebasing. Is everything's ok now?

@tohojo
Copy link
Collaborator

tohojo commented Jun 23, 2020

Nah, the history is even worse now ;)
Ideally this PR should only show the two new commits. I can fix that when merging, though...

However, I would like it if you could put a bit more of your performance rationale into the second commit message. Specifically, above you said this:

I've made some measurements of ipv4 forward+napt on mips router with
580 MHz single-core CPU, so every instruction is important there. It
appears that on kernel 4.9 skb_try_make_writable() reallocates skb, if
skb was allocated in ethernet driver via so-called 'build skb' method
from page cache (it was discovered by strange increase of kmalloc-2048
slab at first). This behaviour greatly increase CPU load and decrease
throughput (at least by one-third).

Please put that into the commit message as well; if you have actual before/after numbers that would be even better!

@inste inste mentioned this pull request Jun 23, 2020
Closed
@inste
Copy link
Contributor Author

inste commented Jun 23, 2020

@tohojo opened #137 for proper history.

@tohojo tohojo closed this Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tohojo tohojo tohojo requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@inste @tohojo