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: Use a separate, non-blocking pipe to distribute queries #6566

Merged
merged 3 commits into from May 16, 2018

Conversation

Projects
None yet
2 participants
@rgacogne
Copy link
Member

rgacogne commented May 3, 2018

Short description

This allows us to drop queries when a pipe goes full, thus still distributing queries to other threads instead of blocking. It also adds a new metric to keep track of queries dropped because the pipe was full.

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)
rec: Use a separate, non-blocking pipe to distribute queries
This allows us to drop queries when a pipe goes full, thus still
distributing queries to other threads instead of blocking. It also
adds a new metric to keep track of queries dropped because the pipe
was full.

@rgacogne rgacogne added this to the rec-4.2.0 milestone May 3, 2018

@rgacogne rgacogne requested a review from chbruyand May 3, 2018

@chbruyand
Copy link
Member

chbruyand left a comment

Looks good, just added a few questions.

@@ -2463,17 +2471,29 @@ void distributeAsyncFunction(const string& packet, const pipefunc_t& func)
tmsg->func = func;
tmsg->wantAnswer = false;

if(write(tps.writeToThread, &tmsg, sizeof(tmsg)) != sizeof(tmsg)) {
ssize_t written = write(tps.writeQueriesToThread, &tmsg, sizeof(tmsg));

This comment has been minimized.

@chbruyand

chbruyand May 4, 2018

Member

Is there any specific reason you did not keep the test like it was before ? I find it more readable like that

  if (write(tps.writeQueriesToThread, &tmsg, sizeof(tmsg)) != sizeof(tmsg)) {                                                                                                 
    delete tmsg;                                                                                                                                                              
    int error = errno;                                                                                                                                                        
    if (error == EAGAIN || error == EWOULDBLOCK) {                                                                                                                            
      g_stats.queryPipeFullDrops++;                                                                                                                                           
    } else {                                                                                                                                                                  
      unixDie("write to thread pipe returned wrong size or error:" + error);                                                                                                  
    }                                                                                                                                                                         
  } 

This comment has been minimized.

@rgacogne

rgacogne May 4, 2018

Author Member

The value or errno only makes sense if write() returned -1, so we need to make sure it's the case, because otherwise we might be reading a garbage value of errno because of a short write. Agreed, it should not be happening because writing a 64-bits pointer on a pipe is supposed to be atomic since 64 < PIPE_BUF, but I'd rather be on the safe side.

MAX-ACCESS read-only
STATUS current
DESCRIPTION
"Number of responses dropped because the query distribution pipe was full"

This comment has been minimized.

@chbruyand

chbruyand May 4, 2018

Member

Number of queries maybe ?

This comment has been minimized.

@rgacogne

rgacogne May 4, 2018

Author Member

Indeed, good catch!

unixDie("Creating pipe for inter-thread communications");
tps.readQueriesToThread = fd[0];
tps.writeQueriesToThread = fd[1];
setNonBlocking(tps.writeQueriesToThread);

This comment has been minimized.

@chbruyand

chbruyand May 4, 2018

Member

Would it be a problem if setNonBlocking() would fail ?

This comment has been minimized.

@rgacogne

rgacogne May 4, 2018

Author Member

It would not really be an issue since we would just be in the same situation than we were previously but I'll add a check nevertheless!

rgacogne added some commits May 7, 2018

rgacogne added a commit to rgacogne/pdns that referenced this pull request May 16, 2018

rgacogne added a commit that referenced this pull request May 16, 2018

@rgacogne rgacogne merged commit ba574de into PowerDNS:master May 16, 2018

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: C/C++ No alert changes
Details
lgtm analysis: JavaScript No alert changes
Details
lgtm analysis: Python No alert changes
Details

@rgacogne rgacogne deleted the rgacogne:rec-queries-pipe branch May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.