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.

Sign up for GitHub

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

Jump to bottom

dnsdist: Ring buffers sharding #6191

Merged
merged 13 commits into from
Mar 29, 2018
Merged

dnsdist: Ring buffers sharding #6191

merged 13 commits into from
Mar 29, 2018

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Jan 16, 2018 •
edited
Loading

Short description

I did test this code, but I had to rebase it to be able to open a pull request against the current master and the rebase was a bit painful, so please don't merge this before I can find the time to test it more.
Combining #6190 and this PR allowed me to reach a bit more than 1M qps with a single dnsdist process at almost 100% cache hit rate and in a very controlled lab environment, so YMMV.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and 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 dnsdist-1.3.0 milestone Jan 16, 2018
@rgacogne rgacogne force-pushed the dnsdist-rings-sharding branch 2 times, most recently from e9847be to defad9d Compare March 6, 2018 16:20
@rgacogne rgacogne changed the title [WIP] dnsdist: Ring buffers sharding dnsdist: Ring buffers sharding Mar 19, 2018
@rgacogne
Copy link
Member Author

Cleaned up the code, could still use a review!

@rgacogne
Copy link
Member Author

Rebased to fix a conflict. Added unit tests and some optional locking statistics.

@rgacogne
Copy link
Member Author

Rebased to fix a conflict with #6327.

@rgacogne
Copy link
Member Author

Rebased so the rec tests pass, and be a little more permissive in the new unit tests because Travis is slow.

@rgacogne
Copy link
Member Author

Rebased to fix conflicts.

@rgacogne
Copy link
Member Author

Rebased, moved to C++11-style loops for walking the ring buffers as suggested by @chbruyand (thanks!).

@rgacogne rgacogne force-pushed the dnsdist-rings-sharding branch 2 times, most recently from e2daf47 to 58d85f5 Compare March 28, 2018 13:48
@rgacogne
Copy link
Member Author

Rebased to fix conflicts.

@rgacogne
Copy link
Member Author

Tests failure is unrelated (CDB issue in the auth's tests).

@rgacogne rgacogne requested a review from chbruyand March 28, 2018 21:28
@rgacogne rgacogne mentioned this pull request Mar 29, 2018
Merged
6 tasks
chbruyand
chbruyand reviewed Mar 29, 2018
View reviewed changes
pdns/dnsdist-rings.cc Outdated
ReadLock rl(&queryLock);
for(const auto& q : queryRing)
s.insert(q.requestor);
for (auto& shard : d_shards) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add const to the shard reference in all of your loops against d_shards (not in dnsdist-rings.hh) ?

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 would except I can't because I'm locking the mutex. I could make it mutable but that doesn't seem right here.

Copy link
Member

Choose a reason for hiding this comment

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

But you are doing so at other places : https://github.com/PowerDNS/pdns/pull/6191/files#diff-e0b6a2a362016b3cf6c829a519bad721R265

pdns/dnsdist-rings.hh
if (numberOfShards < d_numberOfShards) {
throw std::runtime_error("Decreasing the number of shards in the query and response rings is not supported");
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe make sure the newCapacity / numberOfShards ratio is coherent ?

Copy link
Member

Choose a reason for hiding this comment

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

This may reduce the size of the indivual rings and have some strange behaviour regarding d_nbQueryEntries

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, that's why it's only allowed at configuration time and not at run-time. I could write a few words about that here as well though.

pdns/dnsdist-lua-inspection.cc Outdated
}
else {
unsigned int lab = *labels;
for(auto a : shard->respRing) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe auto& a ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't because we modify the name member by calling DNSName::trimToLabels() on it below. We could consider only copying the DNSName instead of the whole entry, but I'm not sure it would make a noticeable difference. It might worth benchmarking it, or simply doing it.

@rgacogne
Copy link
Member Author

I pushed a few commits addressing @chbruyand's comments (thanks!).

@rgacogne rgacogne merged commit f05f1a3 into PowerDNS:master Mar 29, 2018
@rgacogne rgacogne deleted the dnsdist-rings-sharding branch March 29, 2018 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chbruyand chbruyand chbruyand left review comments

Assignees
No one assigned
Labels
dnsdist enhancement needs review
Projects
None yet
Milestone
dnsdist-1.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@rgacogne @chbruyand