Refactor http2_grpc to leverage tail calls

[rafaelroquetto]

The latest kernel versions have changed the way the verifier deals with scalar spills, reducing its tolerance towards complex programs - for a comprehensive discussion, please refer to

https://lore.kernel.org/bpf/[email protected]/T/

Our http2 code therefore no longer passes the verifier, owing to mostly a complex for loop. This PR splits the code into different bpf programs that are now tail called, and changes the _for_ loop to be a hybrid of a recursive bpf_tail_call and a loop with less iterations. This approach was chosen because neither do we want to bpf_tail_call/recurse for each iteration, nor do we wish to use bpf_loop as constrains our program to kernel versions greater than 5.17.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.97%. Comparing base (85b6cdd) to head (380fbc1).
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1447      +/-   ##
==========================================
+ Coverage   76.86%   80.97%   +4.11%     
==========================================
  Files         149      149              
  Lines       15252    15267      +15     
==========================================
+ Hits        11723    12363     +640     
+ Misses       2911     2298     -613     
+ Partials      618      606      -12     

Flag	Coverage Δ
integration-test	59.82% <100.00%> (+0.03%)	⬆️
k8s-integration-test	59.60% <100.00%> (?)
oats-test	33.96% <100.00%> (+0.04%)	⬆️
unittests	51.92% <0.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rafaelroquetto]

This FIXME will be addressed separately - there are too many other places to check and tweak.

[grcevski]

I don't think we'd ever need 64bit value to keep the position, if that's the case we are doing something wrong. The position is the index inside the buffer we are reading, we'd never read more than 1024. It likely can be reduced to smaller int.

[rafaelroquetto]

(copying this here as I commented on the wrong box - sorry about the noise)

With regards to the int usage, my biggest concern is that we have a signed int here - in most APIs (except from a few relics like libc functions such as read(), which will rely on negative values for errors, and anything else for "byte count"), we will encounter size_t, which tends to be the same as uintptr_t because it needs to be able to address the memory space. We do invariably end up mixing these ints up with pointers and alike, which are unsigned by default, so IMHO it's just best to eventually refactor them to become unsigned (and simplify underflow/overflow cases).

Using a smaller unsigned int here is potentially fine, but we have to be careful to not make it appear as an lvalue in an operation involving pointers as rvalues, as it can cause an overflow (because pointers will be 64-bit). It becomes a trade-off between saving a few bytes (or a lot depending on how many of those we have) versus less cognitive load of treating everything as size_t.

[mariomac]

LGTM as long as test are fixed!

Also, could you add some extra commenting to the new C functions 🙏 ?

[grcevski]

Great refactor, I left a few comments and nits. I think the only one that must be done is removing the prints in the loop and I think it's important to address my question on returning frame_header_t.

[grcevski]

I don't think we'd ever need 64bit value to keep the position, if that's the case we are doing something wrong. The position is the index inside the buffer we are reading, we'd never read more than 1024. It likely can be reduced to smaller int.

[grcevski]

The test failure in integration tests looks related to the change. I can help debug if you need help :)

[rafaelroquetto]

Tests now fixed - caused by a typo (missing ! in if (!frame.length ...) now reworded as if (frame.length == 0) to make it more obvious and by the introduction of FrameUknown, which caused the loop to bail earlier than it should (and not parse incoming frames), which I have now removed, making it consistent with the old code.

Apart from that, I've incorporated most review feedbacks.

[grcevski]

LGTM! Good stuff!