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.

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

Add expiration of incomplete reassembly buffers to IPv6 #1384

Merged
merged 15 commits into from Jun 6, 2019

Conversation

Projects
None yet
3 participants
@alexandergall
Copy link
Contributor

commented Sep 20, 2018

No description provided.

alexandergall added some commits Jun 21, 2018

ipv6.reassembly: reduce scope of local variables in handle_fragment()
Frequent trace aborts due to "register coalescing too complex" have
been observed within handle_fragment().  This commit attempts to
mitigate this by limiting the scope of local variables, either by
explicit scoping with do/end or by placing variable declarations after
function calls that don't need them.
ipv6.reassembly: perform reassembly in separate loop
The amount of work being done in the main push() loop is reduced by
moving fragments to a separate queue.  This is beneficial if the
workload is unbiased with respect to fragmentation or if the workload
shifts from mainly fragmented to mainly unfragmented packets.

All packets needing reassembly are processed in a separate loop.

alexandergall added some commits Dec 14, 2018

lwaftr.ctable_wrapper: add cleanup function to random eviction
Add an optional callback which is invoked on an evicted entry to
perform cleanup before the entry is purged from the table.
apps.ipv6.reassemble: avoid spurious sort
Respect the precondition for sort_array().

Use bit.band instead of modulo operator to check for multple of 8 of
the fragment size.
apps.ipv6.reassemble: copy header from first fragment
The headers of all fragments are derived from the unfragmentable part
of the original packet, hence the distinction for fragment #0 is not
necessary.
@dpino

dpino approved these changes Jan 4, 2019

Copy link
Contributor

left a comment

LGTM. I think a review from @wingo would be nice as he is wrote a big chunk of this code.

local max_data_offset = ether_ipv6_header_len + frag_start + frag_size
if max_data_offset > ffi.sizeof(reassembly.packet.data) then
-- Snabb packets have a maximum size of 10240 bytes.
return self:reassembly_error(entry)

This comment has been minimized.

Copy link
@dpino

dpino Jan 4, 2019

Contributor

Maximum size of a packet is defined as a constant in core/packet.lua:max_payload.

This comment has been minimized.

Copy link
@alexandergall

alexandergall Jan 4, 2019

Author Contributor

Note that this is from the original code (I only added a do...end to limit the scope of locals). This would be a cosmetic change, I suppose?

This comment has been minimized.

Copy link
@dpino

dpino Jan 4, 2019

Contributor

Sorry I didn't realize this wasn't a new change. Rather than cosmetic, the difference is that the size is being computed every time this function is called, when it can be retrieved from a constant. I leave it up to you if you'd like to change it.

This comment has been minimized.

Copy link
@alexandergall

alexandergall Jan 4, 2019

Author Contributor

I'd have expected that this would be taken care of by the optimizer, but if it's not, that change would certainly be a good idea.

@@ -264,24 +284,27 @@ function Reassembler:handle_fragment(h)
else
reassembly.final_start = frag_start
end
elseif frag_size % 8 ~= 0 then
elseif bit.band(frag_size, 0x7) ~= 0 then

This comment has been minimized.

Copy link
@dpino

dpino Jan 4, 2019

Contributor

This changed isn't justified. I benchmarked % and "mod with logical and" and they're the same speed. What's indeed much slower is math.fmod. I think this change should be reverted.

This comment has been minimized.

Copy link
@alexandergall

alexandergall Jan 4, 2019

Author Contributor

Ok, I assume that % optimizes for this case?

This comment has been minimized.

Copy link
@dpino

dpino Jan 4, 2019

Contributor

The results I got from benchmarking % and & showed no difference. Here's a gist of the code I tried https://gist.github.com/dpino/925a79494d3da27e3681e8b3acf67b32. I don't know what LuaJIT compiles % to, though.

This comment has been minimized.

Copy link
@alexandergall

alexandergall Jan 4, 2019

Author Contributor

I just checked and the compiler does indeed specialize to bitops if the module is a power of 2.

@eugeneia

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

@alexandergall If you think this is ready I can I merge this onto max-next.

@alexandergall

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2019

Pushed the changes suggested by @dpino. Ready to merge for me.

eugeneia added a commit that referenced this pull request Jan 9, 2019

@eugeneia eugeneia added the merged label Jan 9, 2019

apps.ipv6.reassemble: fix check for valid packet length
Take padding to minimum Ethernet frame size into account.

eugeneia added a commit that referenced this pull request Jan 29, 2019

@eugeneia eugeneia merged commit 69473eb into snabbco:master Jun 6, 2019

1 of 2 checks passed

davos-eugeneia/snabb-nfv-test-vanilla Linux davos 4.4.31 x86_64 Intel(R) Xeon(R) CPU E5-2603 v2 @ 1.80GHz / eugeneia/snabb-nfv-test-vanilla
Details
SnabbDoc Documentation as single HTML file
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.