lots of useless recvfrom() calls

[Michael Tokarev]

04.05.2018 19:05, Lennert Buytenhek wrote:
> Hi!
> 
> I am using the patch below to avoid one useless recvfrom() call (that
> returns EAGAIN) for every call to dns_ioevent().  Under low load, where
> every call to dns_ioevent() processes one DNS reply, this saves half of
> the total number of recvfrom() calls.

Is it really a problem under low load? If the load is low, it doesn't
really matter how many recvfrom() system calls are made, I think.

The idea was to handle as much data as possible, so not to lose
edge-triggered events. And hopefully there will be not many calls
to udns_ioevent(), as there should be not much traffic, so I haven't
thought about system load here. Again, edge-triggering means we should
eat everything or else we're doomed. Current scheme works with either
level- or edge-trigger.

> Since I use level-triggered event notification, it's fine if
> dns_ioevent() doesn't process all packets pending for this dns_ctx,
> because if the fd is still active when dns_ioevent() returns, it
> will be called again at some point.

Note that here, under high load, there will be several extra system
calls to check if the new data is pending.

> Also, looping until EAGAIN makes your application somewhat susceptible
> to being DoSed.  (The event polling framework I use spends a lot of
> effort to make sure that all event sources are handled fairly, and that
> no one event source can starve out processing of other event sources.)

This is a good point. Maybe limiting number of packets to process to some
"reasonable" number, such as 5? :) Still, edge-triggering notification is
interesting here.

> If udns should work with edge-triggered event notification frameworks,
> then perhaps we could add a new function, say, dns_ioevent_single(),
> that only processes a single packet for this dns_ctx, and then have
> dns_ioevent() just call the _single() version in a loop until EAGAIN,
> or something like that?

A new function sounds good.

Also, if the number of system calls really is a concern, maybe using
recvmmsg() instead of recvfrom(), on linux, will be a good idea.

> diff -up ./udns_resolver.c.orig ./udns_resolver.c
> --- ./udns_resolver.c.orig	2018-05-04 15:52:58.368856384 +0300
> +++ ./udns_resolver.c	2018-05-04 16:11:48.159375836 +0300
> @@ -976,7 +976,6 @@ void dns_ioevent(struct dns_ctx *ctx, ti
>  
>    if (!now) now = time(NULL);
>  
> -again: /* receive the reply */
>  
>    slen = sizeof(sns);
>    r = recvfrom(ctx->dnsc_udpsock, (void*)pbuf, ctx->dnsc_udpbuf,
> @@ -1197,6 +1196,8 @@ again: /* receive the reply */
>    }
>    goto again;
>  
> +again: /* receive the reply */
> +  dns_request_utm(ctx, now);
>  }

So you're effectively cancelling the loop, forcing it to be a single-step.

Admittedly, error handling here in my version is also "interesting", -
it actually does not exist at all :)

How about adding a function, say, udns_handle_reply(ctx, buf, buflen),
so that all the functionality of receiving data and handling errors will
belong to the caller?

Also I think it's a good idea to use recvmmsg(), - at the time when
udns library has been written there were no such syscall, now it is
widespread already, and is easy to use. Receive, say, 5 packets at
once, and loop if we received all 5. Again, if it is edge-triggered
notification, we should absorb everything, so the loop should be
cycling until there's really nothing to proceed (think about fairness),
and in case of level-triggering this is not necessary.

Maybe I can export all 3 of them - current eat-everything function
(maybe with switching to recvmmsg() to optimize things), a new function
(or a new setting/flag of a ctx - in which case there will be no need
for a separate function) that process just one packet, and actual packet
processing routine without the receiving part.

Hmm..

/mjt