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

not respecting implicit parameter dependencies triggers panics #3101

Open
nigoroll opened this issue Oct 21, 2019 · 6 comments
Open

not respecting implicit parameter dependencies triggers panics #3101

nigoroll opened this issue Oct 21, 2019 · 6 comments
Assignees

Comments

@nigoroll
Copy link
Member

This issue is to track https://varnish-cache.org/lists/pipermail/varnish-dev/2018-January/009208.html

Workspace-related parameters need to be adjusted such that included structures can be accommodated, otherwise panics can happen. One such case is vsl_buffer, if set close to *_workspace, varnish will panic.

@nigoroll nigoroll self-assigned this Oct 21, 2019
@bsdphk
Copy link
Contributor

bsdphk commented Oct 21, 2019

We do not want the parameters to be "4kb + overhead" because that would be suboptimal for VM reasons.

We should probably set a "dynamic minimum" on these params to cover the overhead of fixed allocations, but I'm not sure how ugly that would get ?

@nigoroll
Copy link
Member Author

nigoroll commented Oct 21, 2019

Yes, the dynamic minimum sounds like a suitable concept, and the result looks pretty to me.
Quick draft based on the example on the mailing list:

  • vsl_buffer stays as-is
  • http_resp_size stays as-is
  • workspace_backend becomes a dynamic minimum available to VCL and its default gets reduced by the currently deduced overhead for the other values.

The actual workspace size then becomes something like (please do not pick on details, those will are to be worked out and I do not want to spend the time before we agree in general):

PRNDUP(3 * HTTP_estimate(http_max_hdr) + vsl_buffer + http_resp_size)

@nigoroll
Copy link
Member Author

nigoroll commented Feb 7, 2020

discussed with @dridi and @bsdphk :

  • Unfortunately, the dynamic minimum is not a practical concept because it would lead to too low a setting when a dependent value is changed
  • We want to change the semantics of the workspace_* with a release to mean "usable space"
  • To help tune the parameters properly, we want to introduce a workspace high watermark which gets logged to vsl
  • WS_Release* will gain a parameter obliging the caller to report their high watermark
  • This change will happen with a major release

@nigoroll
Copy link
Member Author

FTR previous PR #3285 needs to be redone

other bugwash notes on what to do once we have a watermark:

  • Debug option for VSL reporting per task
  • VSC reporting with simple histogram/bucketing (probably log-scale-ish like 50% 90% 95%)

@nigoroll
Copy link
Member Author

#3524 is another incarnation of this.
from bugwash

  • need to look at dynamic parameters again, (it was only after bugwash that I noted the above Unfortunately, the dynamic minimum is not a practical concept...)
  • we should snapshot all task-related values (like workspaces) in the worker

@nigoroll
Copy link
Member Author

notes from bugwash:

(15:39:32) slink: I am back to thinking that just having headroom_xxx and auto-calculating the workspace_xxx would be the simplest and cleanest option
(15:39:59) slink: workspace_xxx would then become r/o
(15:41:42) slink: the reporting through the r/o parameters would also make any page size rounding obvious
(15:41:59) phk: brew a mockup and lets look at it.

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

No branches or pull requests

2 participants