[Dnsmasq-discuss] [PATCH]

Petr Menšík pemensik at redhat.com
Fri Sep 10 14:28:18 UTC 2021


On 9/10/21 10:37 AM, Dominik DL6ER wrote:
> Hey Petr and Simon,
>
> On Thu, 2021-09-09 at 23:35 +0100, Simon Kelley wrote:
>> Much tidier is to simplify things and add an extra parameter to
>> log_query which is the type, if type is non-zero, log_query calls
>> querystr(), so
>>
>> log_query(flags, name, addr, querystr("description", type));
>>
>> becomes
>>
>> log_query(flags, name, addr, "description", type);
>>
> I agree this is a good solution, patch attached. I hope to have caught
> all places (I don't have the dependencies to build all possible
> combinations myself).
Builds fine with DNSSEC, LIBIDN2 and DBUS. I failed to notice querystr()
were called always and unconditionally, I thought I were optimizing just
--log-queries scenario.
>
> On Thu, 2021-09-09 at 23:35 +0100, Simon Kelley wrote:
>> querystr becomes a local function to cache.c or just gets rolled into
>> log_query().
> I did the former. Merging it into log_query() would make an already
> complex routine even longer.
>
>
> Concerning performance: The loop can at most iterate over 89 entries
> before it says: "Didn't find". However, for the vast vast majority of
> cases, the match will be in the first 30ish (AAAA and SRV) as query
> types behind those are not very likely to be seen. The loop breaks as
> soon as the match is found. Looking at the gcc11 x86_64 -O2 assembly,
> there are no surprises, only very few assembler instructions per
> iteration are needed. The next type integers in the table can always be
> found 16 bytes after the former. Hence, we just need
>
> .FOR_LOOP
>         add     rax, 1
>         cmp     rax, 89
>         je      .NOT_FOUND
>         mov     rdx, rax
>         movsx   rcx, eax
>         sal     rdx, 4
>         cmp     DWORD PTR typestr[rdx], r1d
>         jne     .FOR_LOOP
> 	[... stuff if found ...]
>
> Given we call library functions like strlen() and sprintf(), our loop
> here is surely not any kind of bottleneck. Even if it'd be even larger.
>
> Best,
> Dominik

I admit moving querystr into log_query is much more important
optimization than my table lookup change. But it can still be applied
with minor tweak and would save some loops. In patch #1. If HTTPS record
is logged, it would have to pass 64 rows before finding correct one.
With runtime dynamic data, we have not much better choices. But for
built-in static data, we can switch linear lookup to constant. I admit
64 iterations is not too much to save, but at least something. Often it
means lookup both for query and response.

I think dest = arg; should be called after querystr(), just like it was
before. It works with common addresses, but there might be some corner
cases broken. Change in patch #2.

-- 
Petr Menšík
Software Engineer
Red Hat, http://www.redhat.com/
email: pemensik at redhat.com
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Assign-dest-after-arg-is-set.patch
Type: text/x-patch
Size: 966 bytes
Desc: not available
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20210910/13c5e410/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Include-all-DNS-types-and-speed-up-lookups.patch
Type: text/x-patch
Size: 11018 bytes
Desc: not available
URL: <http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/attachments/20210910/13c5e410/attachment-0001.bin>


More information about the Dnsmasq-discuss mailing list