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

Refactor: Move V1L private pointer from wrk to the vdp entry #4219

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nigoroll
Copy link
Member

The http1 line layer V1L is a bit special in that it is first used to send1 http headers and then, optionally, used as a delivery processor (VDP) to send the body.

Historically, V1L was contained directly in struct worker under the name WRW and when it got separated, it retained its private pointer directly in struct worker.

From today's perspective and with the advent of more protocol support, having this special case seems like an anachronism, because we have everything in place to not require it.

This PR consists of two patches. The first prepares the ground by simple refactoring of V1L_Open() to return the struct v1l * rather than storing it in the struct worker itself. It contains some transient code to update struct worker outside V1L_Open(), this is only done to enable the split into two patches.

The actual change is in the second patch, which implements the goal of removing the v1l member from struct worker. This necessarily (but trivially) changes the V1L related function signatures, so the textual diff might look like a mouthful. But most of it is simple, really:

  • Change V1L_Close() to take a struct v1l **
  • Change the VDP_Push() to pass the struct v1l * as the private pointer
  • Adjust all other V1L related code to take struct v1l * instead of struct worker *

One detail which remains special has to do with the duality of V1L remaining in place: Because V1L_Close() remains responsible for closing the v1l, the VDP layer must not touch it. For this interaction to work, V1L_Close() needs to clear the private pointer in the VDP entry. The existing check in VDP_Close() for the private pointer to be cleared implies a check for V1L to be closed before VDP, so we can be certain that when the VDP closes, V1L is already closed.

Related note: Before this second iteration, I had also completed a patch series where V1L would be entirely moved under control of the VDP layer (that is, the VDP would need to be pushed whenever V1L was used, so also for the "no body" case), but I found this option too intrusive and implying too much overhead. So keeping V1L a bit special seemed like the better option.

Footnotes

  1. "send" is a simplification, actually the whole point about using V1L for both header and body is to not write the headers to the HTTP1 connection individually, but rather batch both together into a single writev() call, if possible.

@nigoroll nigoroll force-pushed the v1l_refactor_keep_v1l_open branch 5 times, most recently from 0ee53e0 to d474d8a Compare October 31, 2024 14:04
V1L is used to send HTTP headers and the body, so it is used directly from
delivery/fetch first and then as a VDP.

From the times before VDPs, the V1L VDP still had its private pointer in struct
wrk.

This commit is to move the private pointer to the VDP entry. The caller remains
responsible for calling V1L_Close() to keep V1L independent of VDP.
Found with less Flexelint silencing
…ements

Found with less Flexelint silencing

Change of data type of struct v1l members was guided by their main use:

{s,n,c}iov are all related to the iovcnt argument of writev(), which is
of type int.

{l,cl}iov are lengths (to be) written, which can not be negative.

cnt is ultimately going to be returned as a uint64_t, so it makes sense
to have that in the first place.

The other changes are consequences of these, fixing all sign and type
related Flexelint complaints.
@nigoroll
Copy link
Member Author

nigoroll commented Nov 1, 2024

I added two Flexelint commits with polishing found with less silencing. These are independent of the actual topic of this patch, but I wrote them on top of the other changed and would prefer to have them reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant