On Fri, Nov 13, 2020 at 05:53:53PM +0100, Florian Obser wrote:
> The recent fix for handling large (about 16k) answers in unwind has
> the downside that we are now always copying at least 16k per answer
> between the resolver and frontend process.
> That seems wasteful.
>
> This re-arranges things to only copy what its needed.
>
> Tests, OKs?
>
Anyone? So far I had one (1) test report.
diff --git frontend.c frontend.c
index 8adbb07219b..74715087c5f 100644
--- frontend.c
+++ frontend.c
@@ -86,7 +86,8 @@ struct pending_query {
int fd;
int bogus;
int rcode_override;
- ssize_t answer_len;
+ int answer_len;
+ int received;
uint8_t *answer;
};
@@ -424,10 +425,7 @@ frontend_dispatch_resolver(int fd, short event, void *bula)
struct imsgev *iev = bula;
struct imsgbuf *ibuf = &iev->ibuf;
struct imsg imsg;
- struct query_imsg *query_imsg;
- struct answer_imsg *answer_imsg;
int n, shut = 0, chg;
- uint8_t *p;
if (event & EV_READ) {
if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
@@ -449,45 +447,30 @@ frontend_dispatch_resolver(int fd, short event, void
*bula)
break;
switch (imsg.hdr.type) {
- case IMSG_ANSWER_HEADER:
- if (IMSG_DATA_SIZE(imsg) != sizeof(*query_imsg))
- fatalx("%s: IMSG_ANSWER_HEADER wrong length: "
- "%lu", __func__, IMSG_DATA_SIZE(imsg));
- query_imsg = (struct query_imsg *)imsg.data;
- if ((pq = find_pending_query(query_imsg->id)) ==
- NULL) {
- log_warnx("cannot find pending query %llu",
- query_imsg->id);
- break;
- }
- if (query_imsg->err) {
- send_answer(pq);
- pq = NULL;
- break;
- }
- pq->bogus = query_imsg->bogus;
- break;
- case IMSG_ANSWER:
- if (IMSG_DATA_SIZE(imsg) != sizeof(*answer_imsg))
+ case IMSG_ANSWER: {
+ struct answer_header *answer_header;
+ int data_len;
+ uint8_t *data;
+
+ if (IMSG_DATA_SIZE(imsg) < sizeof(*answer_header))
fatalx("%s: IMSG_ANSWER wrong length: "
"%lu", __func__, IMSG_DATA_SIZE(imsg));
- answer_imsg = (struct answer_imsg *)imsg.data;
- if ((pq = find_pending_query(answer_imsg->id)) ==
+ answer_header = (struct answer_header *)imsg.data;
+ data = (uint8_t *)imsg.data + sizeof(*answer_header);
+ if (answer_header->answer_len > 65535)
+ fatalx("%s: IMSG_ANSWER answer too big: %d",
+ __func__, answer_header->answer_len);
+ data_len = IMSG_DATA_SIZE(imsg) -
+ sizeof(*answer_header);
+
+ if ((pq = find_pending_query(answer_header->id)) ==
NULL) {
- log_warnx("cannot find pending query %llu",
- answer_imsg->id);
+ log_warnx("%s: cannot find pending query %llu",
+ __func__, answer_header->id);
break;
}
- p = realloc(pq->answer, pq->answer_len +
- answer_imsg->len);
-
- if (p != NULL) {
- pq->answer = p;
- memcpy(pq->answer + pq->answer_len,
- answer_imsg->answer, answer_imsg->len);
- pq->answer_len += answer_imsg->len;
- } else {
+ if (answer_header->srvfail) {
free(pq->answer);
pq->answer_len = 0;
pq->answer = NULL;
@@ -495,9 +478,32 @@ frontend_dispatch_resolver(int fd, short event, void *bula)
send_answer(pq);
break;
}
- if (!answer_imsg->truncated)
+
+ if (pq->answer == NULL) {
+ pq->answer = malloc(answer_header->answer_len);
+ if (pq->answer == NULL) {
+ pq->answer_len = 0;
+ pq->rcode_override =
+ LDNS_RCODE_SERVFAIL;
+ send_answer(pq);
+ break;
+ }
+ pq->answer_len = answer_header->answer_len;
+ pq->received = 0;
+ pq->bogus = answer_header->bogus;
+ }
+
+ if (pq->received + data_len > pq->answer_len)
+ fatalx("%s: IMSG_ANSWER answer too big: %d",
+ __func__, data_len);
+
+ memcpy(pq->answer + pq->received, data, data_len);
+ pq->received += data_len;
+
+ if (pq->received == pq->answer_len)
send_answer(pq);
break;
+ }
case IMSG_CTL_RESOLVER_INFO:
case IMSG_CTL_AUTOCONF_RESOLVER_INFO:
case IMSG_CTL_MEM_INFO:
diff --git resolver.c resolver.c
index fb34dee87d6..c3faba08faf 100644
--- resolver.c
+++ resolver.c
@@ -884,7 +884,7 @@ resolve_done(struct uw_resolver *res, void *arg, int rcode,
sldns_buffer *buf = NULL;
struct regional *region = NULL;
struct query_imsg *query_imsg;
- struct answer_imsg answer_imsg;
+ struct answer_header *answer_header;
struct running_query *rq;
struct timespec tp, elapsed;
int64_t ms;
@@ -894,12 +894,19 @@ resolve_done(struct uw_resolver *res, void *arg, int
rcode,
char rcode_buf[16];
char qclass_buf[16];
char qtype_buf[16];
- uint8_t *p;
+ uint8_t *p, *data;
+ uint8_t answer_imsg[MAX_IMSGSIZE - IMSG_HEADER_SIZE];
clock_gettime(CLOCK_MONOTONIC, &tp);
query_imsg = (struct query_imsg *)arg;
+ answer_header = (struct answer_header *)answer_imsg;
+ data = answer_imsg + sizeof(*answer_header);
+ answer_header->id = query_imsg->id;
+ answer_header->srvfail = 0;
+ answer_header->answer_len = answer_len;
+
timespecsub(&tp, &query_imsg->tp, &elapsed);
ms = elapsed.tv_sec * 1000 + elapsed.tv_nsec / 1000000;
@@ -1004,43 +1011,33 @@ resolve_done(struct uw_resolver *res, void *arg, int
rcode,
if (result->rcode == LDNS_RCODE_SERVFAIL)
goto servfail;
- query_imsg->err = 0;
-
if (sec == SECURE)
res->state = VALIDATING;
if (res->state == VALIDATING && sec == BOGUS) {
- query_imsg->bogus = !force_acceptbogus;
- if (query_imsg->bogus && why_bogus != NULL)
+ answer_header->bogus = !force_acceptbogus;
+ if (answer_header->bogus && why_bogus != NULL)
log_warnx("%s", why_bogus);
} else
- query_imsg->bogus = 0;
+ answer_header->bogus = 0;
- if (resolver_imsg_compose_frontend(IMSG_ANSWER_HEADER, 0, query_imsg,
- sizeof(*query_imsg)) == -1)
- fatalx("IMSG_ANSWER_HEADER failed for \"%s %s %s\"",
- query_imsg->qname, qclass_buf, qtype_buf);
-
- answer_imsg.id = query_imsg->id;
p = answer_packet;
- while ((size_t)answer_len > MAX_ANSWER_SIZE) {
- answer_imsg.truncated = 1;
- answer_imsg.len = MAX_ANSWER_SIZE;
- memcpy(&answer_imsg.answer, p, MAX_ANSWER_SIZE);
- if (resolver_imsg_compose_frontend(IMSG_ANSWER, 0, &answer_imsg,
- sizeof(answer_imsg)) == -1)
+ do {
+ int len;
+
+ if ((size_t)answer_len > sizeof(answer_imsg) -
+ sizeof(*answer_header))
+ len = sizeof(answer_imsg) - sizeof(*answer_header);
+ else
+ len = answer_len;
+ memcpy(data, p, len);
+ if (resolver_imsg_compose_frontend(IMSG_ANSWER, 0,
+ &answer_imsg, sizeof(*answer_header) + len) == -1)
fatalx("IMSG_ANSWER failed for \"%s %s %s\"",
query_imsg->qname, qclass_buf, qtype_buf);
- p += MAX_ANSWER_SIZE;
- answer_len -= MAX_ANSWER_SIZE;
- }
- answer_imsg.truncated = 0;
- answer_imsg.len = answer_len;
- memcpy(&answer_imsg.answer, p, answer_len);
- if (resolver_imsg_compose_frontend(IMSG_ANSWER, 0, &answer_imsg,
- sizeof(answer_imsg)) == -1)
- fatalx("IMSG_ANSWER failed for \"%s %s %s\"",
- query_imsg->qname, qclass_buf, qtype_buf);
+ answer_len -= len;
+ p += len;
+ } while (answer_len > 0);
TAILQ_REMOVE(&running_queries, rq, entry);
evtimer_del(&rq->timer_ev);
@@ -1052,9 +1049,9 @@ resolve_done(struct uw_resolver *res, void *arg, int
rcode,
/* try_next_resolver() might free rq */
if (try_next_resolver(rq) != 0 && running_res == 0) {
/* we are the last one, send SERVFAIL */
- query_imsg->err = -4; /* UB_SERVFAIL */
- resolver_imsg_compose_frontend(IMSG_ANSWER_HEADER, 0,
- query_imsg, sizeof(*query_imsg));
+ answer_header->srvfail = 1;
+ resolver_imsg_compose_frontend(IMSG_ANSWER, 0,
+ answer_imsg, sizeof(*answer_header));
}
out:
free(query_imsg);
diff --git unwind.h unwind.h
index 9ebd8f47f87..161619cd34c 100644
--- unwind.h
+++ unwind.h
@@ -116,7 +116,6 @@ enum imsg_type {
IMSG_SOCKET_IPC_FRONTEND,
IMSG_SOCKET_IPC_RESOLVER,
IMSG_QUERY,
- IMSG_ANSWER_HEADER,
IMSG_ANSWER,
IMSG_CTL_RESOLVER_INFO,
IMSG_CTL_AUTOCONF_RESOLVER_INFO,
@@ -170,18 +169,14 @@ struct query_imsg {
char qname[NI_MAXHOST];
int t;
int c;
- int err;
- int bogus;
struct timespec tp;
};
-struct answer_imsg {
-#define MAX_ANSWER_SIZE MAX_IMSGSIZE - IMSG_HEADER_SIZE -
sizeof(uint64_t) - \
- 2 * sizeof(int)
- uint64_t id;
- int truncated;
- int len;
- uint8_t answer[MAX_ANSWER_SIZE];
+struct answer_header {
+ uint64_t id;
+ int srvfail;
+ int bogus;
+ int answer_len;
};
extern uint32_t cmd_opts;
--
I'm not entirely sure you are real.
