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

rec: Add a distribution-pipe-buffer-size setting #7571

Merged
merged 1 commit into from
Apr 30, 2019

Conversation

rgacogne
Copy link
Member

Short description

Allows setting the size in bytes of the internal buffer of the pipe used by the distributor to pass incoming queries to a worker thread.
Requires support for F_SETPIPE_SZ which is present in Linux since 2.6.35. The actual size might be rounded up to a multiple of a page size. 0 means that the OS default size is used.
A large buffer might allow the recursor to deal with very short-lived load spikes during which a worker thread gets overloaded, but it will be at the cost of an increased latency.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@rgacogne rgacogne added this to the rec-4.2.0 milestone Mar 13, 2019
@rgacogne rgacogne force-pushed the rec-pipe-buffer-size branch from 536fe73 to e0a7174 Compare April 18, 2019 09:49
@rgacogne
Copy link
Member Author

Rebased to fix a conflict.

@rgacogne rgacogne requested review from chbruyand and omoerbeek April 18, 2019 10:06
@rgacogne rgacogne modified the milestones: rec-4.2.0, rec-4.2.0-beta1 Apr 18, 2019
@rgacogne rgacogne force-pushed the rec-pipe-buffer-size branch from e0a7174 to ee271fc Compare April 26, 2019 14:14
@rgacogne
Copy link
Member Author

Rebased to fix a conflict.

#endif /* F_GETPIPE_SZ */
}

bool setPipeBufferSize(int fd, size_t size)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a size_t ?
argsvMap.asNum() returns an int, and the fcntl takes an int as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a size_t because a negative value makes no sense here. asNum() and fcntl() are generic interfaces, but IMHO that does not mean we can't use a more correct type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with the unsigned part of size_t, but negative numbers will be converted to very large values by the int to size_t conversion. We probably need an asSize() or similar that rejects negative values.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be awesome, yes!

@aerique aerique merged commit b0ff34d into PowerDNS:master Apr 30, 2019
@rgacogne rgacogne deleted the rec-pipe-buffer-size branch May 6, 2019 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants