param: New deadline_pipe parameter

[dridi]

Followup from the last VDD, the idea of having deadlines was discussed.

This patch series fixes a bug, improves logging slightly, and introduces a new deadline_pipe parameter. It leaves deadline_client and deadline_backend for later as this is the simplest to implement among the three.

The parameter is disabled by default to match the current behavior.

[nigoroll]

bugwash:

• parameter name is pipe_task_deadline
• should have vcl control
• document that 0s == inf()
• can go in

[dridi]

I don't think the last commit can go in without a review.

[nigoroll]

My homework submission after bugwash discussion:

In preparation of addition of a new (req|client)_task_deadline parameter (which is not part of this PR), which would probably be made accessible to VCL as req.task_deadline, we discussed whether or not the pipe deadline should be independent of this additional deadline or not.
I now think that, actually, I do not see much sense in having multiple VCL variables at all: Unless there is a use case which I am missing, (be)req.task_deadline could become the task deadline variables for client/backend tasks *1), respectively, be they pipe, synth, purge or whatever. We could introduce it now for pipe only, restrict it to calls from vcl_pipe{} and extend the scope later.
The vcl variable is one question, the parameter to define the default is another. We absolutely should have (req|client)_task_deadline and pipe_task_deadline. The latter would be the default for vcl_pipe{}.
Whether or not pipe_task_deadline could be longer than (req|client)_task_deadline, I am a bit torn: For consistency, I would prefer req.task_deadline to be an upper limit to all "subordinate" tasks *2), but for practical purposes it might be simpler to apply the default from pipe_task_deadline if req.task_deadline has not been set explicitly when entering vcl_pipe {}.

So, with respect to this PR, I think that:

• the vcl variable should just be req.task_deadline
• the deadline can have a field in struct req / struct busyobj.

*1) and then we come back to the question of whether pipe is client or backend 😵‍💫
*2) that is, bereq.deadline should be limited to the remaining time of req.deadline etc.

[dridi]

*1) and then we come back to the question of whether pipe is client or backend 😵‍💫

As of today, no question, pipe is client 😁

So to summarize the VCL control you want:

• req.task_deadline in all client subroutines
• bereq.task_deadline in all backend subroutines
• when unset, defaulting to [be]req_task_timeout parameter
• when unset inside vcl_pipe, req.task_timeout defaults to pipe_task_timeout

Correct me if I'm wrong with the above.

Since req_task_deadline would not be effective outside vcl_pipe, we start by exposing req.task_deadline only in vcl_pipe, backed by an eponymous field in struct req. Is that all right?

[dridi]

Of course the moment I submit the comment a thought occurs to me.

In vcl_pipe we have a read-only req so here is what we should probably do instead:

• req.task_deadline in all client subroutines
• bereq.task_deadline in all backend subroutines
• when unset, they default to [be]req_task_timeout parameters
• inside vcl_pipe
  • bereq.task_timeout defaults to req.task_timeout
  • when unset, it defaults to the pipe_task_timeout parameter

So in pipe mode we may override a client deadline independently. I think task deadlines should remain independent because if a backend fetch is too slow for a client request, it should be allowed to proceed in the absence of a bereq deadline since it may also service other client requests with different deadlines or none at all (waiting list hits, hits during streaming etc).

It's also simpler to not have those relationships, a win on both sides.

[nigoroll]

@dridi 👏🏽

I fully agree with your proposed design with respect to the pipe deadline.

You also convinced me to make the bereq timeout independent, but we need to consider these aspects:

• any waits of the client side need to be bound by the deadline (waitinglist, busy obj)
• the bereq should (still) be canceled "asap" if it is for a single req (pass) which timed out

But I still think that subordinate (ESI) req timeouts need to be bound hierarchically.

[nigoroll]

Apologies for the accidental close

[dridi]

But I still think that subordinate (ESI) req timeouts need to be bound hierarchically.

I agree, sub-requests are effectively sub-tasks.

[dridi]

I am confident now that the last rebase-force-push meets the bugwash "push it when it's ready" criteria. I'll wait (for CI and) until the end of my business day before merging.

[nigoroll]

LGTM from 👀

[dridi]

For reference, the following was added afterwards:

• 0959f35 (uninitialized variable)
• 1ff0c0f (missing unset support)
• v1p: Fix invalid poll() usage with a new VTIM_poll() #4050