Add expiration of incomplete reassembly buffers to IPv6 #1384
Conversation
…into ipv6-reass-expire
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.
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.
Add an optional callback which is invoked on an evicted entry to perform cleanup before the entry is purged from the table.
Respect the precondition for sort_array(). Use bit.band instead of modulo operator to check for multple of 8 of the fragment size.
The headers of all fragments are derived from the unfragmentable part of the original packet, hence the distinction for fragment #0 is not necessary.
| 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) |
There was a problem hiding this comment.
Maximum size of a packet is defined as a constant in core/packet.lua:max_payload.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/apps/ipv6/reassemble.lua
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, I assume that % optimizes for this case?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I just checked and the compiler does indeed specialize to bitops if the module is a power of 2.
|
@alexandergall If you think this is ready I can I merge this onto max-next. |
The operator specializes to bitops automatically.
|
Pushed the changes suggested by @dpino. Ready to merge for me. |
Take padding to minimum Ethernet frame size into account.
No description provided.