Fix cobalt_should_drop() and reduce memory reallocations

[tohojo]

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]

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]

Ilya notifications@github.com writes:

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).

Right, that's a useful data point! And reasonable change in that case. Please put this rationale into the commit message as well...

[inste]

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]

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]

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

[inste]

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

[tohojo]

Ilya notifications@github.com writes:

@tohojo sorry for delay, I've realized that 'bus factor' is not only joke...

Yeah, please don't get hit by a bus! ;)

Will rework and test today.

Great!

[inste]

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

[tohojo]

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]

@tohojo opened https://github.com/dtaht/sch_cake/pull/137 for proper history.