Re: [Dnsmasq-discuss] [PATCH] Optimize speed on massive server=/.../... records

2022-11-26 Thread Simon Kelley 

Patch tweaked and applied.

Given the rate of good changes coming in, I'm not going to make the 
final 2.88 release this weekend. Let's give it  a few more days to settle.



Cheers,

Simon.


On 25/11/2022 13:11, Petr Menšík wrote:
When looking what this change did, I have noticed mark_servers() cleanup 
of local_domains is using serv->next after it has freed serv. Use 
additional variable just like in cleanup_servers().


Patch attached.

On 11/21/22 23:22, Simon Kelley wrote:
Thanks for this. It was in my mind that vary large number of domains 
would be --local=/domain/ or --address=/domain/, not forwarding to 
servers.


I've applied something that looks very like your patch, but with 
cosmetic code changes.


Cheers,

Simon.


On 20/11/2022 05:50, Ye Zhou wrote:

Hi all,

I'm attaching a patch to optimize a speed issue introduced in version 
2.86.


I have two ISP upstreams and need to forward different sites to 
different ISP's DNS providers. For example:


server=/meituan.com/114.114.114.114 
... (lots of records)
server=/taobao.com/223.5.5.5 
... (lots of records)

It works well before v2.86. Since v2.86 the configuration load time 
becomes extremely long (more than 1 minutes to load all server 
records). The time consuming part is inside the rewritten 
domain-match.c. When adding a new server record, the code will 
traverse all existing records so the configuration load becomes 
quadratic time complexity. The issue still persists on v2.88rc3.
This patch will optimize the config load time by bypassing the time 
consuming code block. During the config load mark_servers() will 
never be called so does not need to waste time on the record 
traversal and re-order part.


Before:
dnsmasq --test  77.50s user 0.30s system 99% cpu 1:17.84 total
After:
dnsmasq --test  0.16s user 0.02s system 99% cpu 0.188 total

https://gist.github.com/zhouye/adfd509f51645d314f53992331449c45 




___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss



___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [PATCH] Optimize speed on massive server=/.../... records

2022-11-25 Thread Petr Menšík 
When looking what this change did, I have noticed mark_servers() cleanup 
of local_domains is using serv->next after it has freed serv. Use 
additional variable just like in cleanup_servers().


Patch attached.

On 11/21/22 23:22, Simon Kelley wrote:
Thanks for this. It was in my mind that vary large number of domains 
would be --local=/domain/ or --address=/domain/, not forwarding to 
servers.


I've applied something that looks very like your patch, but with 
cosmetic code changes.


Cheers,

Simon.


On 20/11/2022 05:50, Ye Zhou wrote:

Hi all,

I'm attaching a patch to optimize a speed issue introduced in version 
2.86.


I have two ISP upstreams and need to forward different sites to 
different ISP's DNS providers. For example:


server=/meituan.com/114.114.114.114 
... (lots of records)
server=/taobao.com/223.5.5.5 
... (lots of records)

It works well before v2.86. Since v2.86 the configuration load time 
becomes extremely long (more than 1 minutes to load all server 
records). The time consuming part is inside the rewritten 
domain-match.c. When adding a new server record, the code will 
traverse all existing records so the configuration load becomes 
quadratic time complexity. The issue still persists on v2.88rc3.
This patch will optimize the config load time by bypassing the time 
consuming code block. During the config load mark_servers() will 
never be called so does not need to waste time on the record 
traversal and re-order part.


Before:
dnsmasq --test  77.50s user 0.30s system 99% cpu 1:17.84 total
After:
dnsmasq --test  0.16s user 0.02s system 99% cpu 0.188 total

https://gist.github.com/zhouye/adfd509f51645d314f53992331449c45 




___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss


--
Petr Menšík
Software Engineer, RHEL
Red Hat, https://www.redhat.com/
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
From 11fe9dd86740865ebd6911a3a2ccdaa6b943d043 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20Men=C5=A1=C3=ADk?= 
Date: Fri, 25 Nov 2022 14:06:17 +0100
Subject: [PATCH] Avoid use-after-free when cleaning up local_domains

mark_servers() must use additional variable to hold next pointer. Just
like very similar loop in cleanup_servers() does.
---
 src/domain-match.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/domain-match.c b/src/domain-match.c
index bef460a..52a0342 100644
--- a/src/domain-match.c
+++ b/src/domain-match.c
@@ -559,7 +559,7 @@ static int maybe_free_servers = 0;
 /* Must be called before  add_update_server() to set daemon->servers_tail */
 void mark_servers(int flag)
 {
-  struct server *serv, **up;
+  struct server *serv, *next = NULL, **up;
 
   maybe_free_servers = !!flag;
   
@@ -580,8 +580,9 @@ void mark_servers(int flag)
  1) numerous and 2) not reloaded often. We just delete 
  and recreate. */
   if (flag)
-for (serv = daemon->local_domains, up = >local_domains; serv; serv = serv->next)
+for (serv = daemon->local_domains, up = >local_domains; serv; serv = next)
   {
+	next = serv->next;
 	if (serv->flags & flag)
 	  {
 	*up = serv->next;
-- 
2.38.1

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [PATCH] Optimize speed on massive server=/.../... records

2022-11-21 Thread Simon Kelley 
Thanks for this. It was in my mind that vary large number of domains 
would be --local=/domain/ or --address=/domain/, not forwarding to servers.


I've applied something that looks very like your patch, but with 
cosmetic code changes.


Cheers,

Simon.


On 20/11/2022 05:50, Ye Zhou wrote:

Hi all,

I'm attaching a patch to optimize a speed issue introduced in version 2.86.

I have two ISP upstreams and need to forward different sites to 
different ISP's DNS providers. For example:


server=/meituan.com/114.114.114.114 
... (lots of records)
server=/taobao.com/223.5.5.5 
... (lots of records)

It works well before v2.86. Since v2.86 the configuration load time 
becomes extremely long (more than 1 minutes to load all server records). 
The time consuming part is inside the rewritten domain-match.c. When 
adding a new server record, the code will traverse all existing records 
so the configuration load becomes quadratic time complexity. The issue 
still persists on v2.88rc3.
This patch will optimize the config load time by bypassing the time 
consuming code block. During the config load mark_servers() will never 
be called so does not need to waste time on the record traversal and 
re-order part.


Before:
dnsmasq --test  77.50s user 0.30s system 99% cpu 1:17.84 total
After:
dnsmasq --test  0.16s user 0.02s system 99% cpu 0.188 total

https://gist.github.com/zhouye/adfd509f51645d314f53992331449c45 




___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss


___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
https://lists.thekelleys.org.uk/cgi-bin/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [PATCH] Optimize speed on massive server=/.../... records

2022-11-20 Thread Geert Stappers via Dnsmasq-discuss 
On Sun, Nov 20, 2022 at 01:50:10PM +0800, Ye Zhou wrote:
> Hi all,
> 
> I'm attaching a patch to optimize a speed issue introduced in version 2.86.
 
Now is the patch attached 

> I have two ISP upstreams and need to forward different sites to different
> ISP's DNS providers. For example:
> 
> server=/meituan.com/114.114.114.114
> ... (lots of records)
> server=/taobao.com/223.5.5.5
> ... (lots of records)
> 
> It works well before v2.86. Since v2.86 the configuration load time becomes
> extremely long (more than 1 minutes to load all server records). The time
> consuming part is inside the rewritten domain-match.c. When adding a new
> server record, the code will traverse all existing records so the
> configuration load becomes quadratic time complexity. The issue still
> persists on v2.88rc3.
> This patch will optimize the config load time by bypassing the time
> consuming code block. During the config load mark_servers() will never be
> called so does not need to waste time on the record traversal and re-order
> part.
> 
> Before:
> dnsmasq --test  77.50s user 0.30s system 99% cpu 1:17.84 total
> After:
> dnsmasq --test  0.16s user 0.02s system 99% cpu 0.188 total
> 
> https://gist.github.com/zhouye/adfd509f51645d314f53992331449c45


stappers@alpaca:~/src/dnsmasq
$ wget -O fastreload.patch 
https://gist.github.com/zhouye/adfd509f51645d314f53992331449c45
--2022-11-20 08:23:57--  
https://gist.github.com/zhouye/adfd509f51645d314f53992331449c45
Resolving gist.github.com (gist.github.com)... 140.82.121.3
Connecting to gist.github.com (gist.github.com)|140.82.121.3|:443... connected.
HTTP request sent, awaiting response... 200 OK
Length: unspecified [text/html]
Saving to: 'fastreload.patch'

fastreload.patch  [ <=> ]  91.26K  --.-KB/s 
   in 0.07s

2022-11-20 08:23:59 (1.30 MB/s) - 'fastreload.patch' saved [93447]

stappers@alpaca:~/src/dnsmasq
$ file fastreload.patch
fastreload.patch: HTML document, UTF-8 Unicode text, with very long lines
stappers@alpaca:~/src/dnsmasq
$


curl --silent 
https://gist.githubusercontent.com/zhouye/adfd509f51645d314f53992331449c45/raw/3d948c9b4e7eac419a3c669ffba59b84341b4577/dnsmasq-server-fix.diff


diff --git a/src/dnsmasq.h b/src/dnsmasq.h
index 90dc986..5aaffcc 100644
--- a/src/dnsmasq.h
+++ b/src/dnsmasq.h
@@ -1144,6 +1144,7 @@ extern struct daemon {
   struct iname *if_names, *if_addrs, *if_except, *dhcp_except, *auth_peers, 
*tftp_interfaces;
   struct bogus_addr *bogus_addr, *ignore_addr;
   struct server *servers, *servers_tail, *local_domains, **serverarray;
+  int servers_flag;
   struct rebind_domain *no_rebind;
   int server_has_wildcard;
   int serverarraysz, serverarrayhwm;
diff --git a/src/domain-match.c b/src/domain-match.c
index 76a1109..56ae015 100644
--- a/src/domain-match.c
+++ b/src/domain-match.c
@@ -552,6 +552,7 @@ void mark_servers(int flag)
   struct server *serv, **up;
 
   daemon->servers_tail = NULL;
+  daemon->servers_flag = flag;
   
   /* mark everything with argument flag */
   for (serv = daemon->servers; serv; serv = serv->next)
@@ -602,6 +603,8 @@ void cleanup_servers(void)
  daemon->servers_tail = serv;
}
 }
+
+  daemon->servers_flag = 0;
 }
 
 int add_update_server(int flags,
@@ -663,29 +666,32 @@ int add_update_server(int flags,
 }
   else
 { 
-  /* Upstream servers. See if there is a suitable candidate, if so unmark
-and move to the end of the list, for order. The entry found may already
-be at the end. */
-  struct server **up, *tmp;
-  
-  for (serv = daemon->servers, up = >servers; serv; serv = tmp)
+  if (daemon->servers_flag)
{
- tmp = serv->next;
- if ((serv->flags & SERV_MARK) &&
- hostname_isequal(alloc_domain, serv->domain))
+ /* Upstream servers. See if there is a suitable candidate, if so 
unmark
+   and move to the end of the list, for order. The entry found may 
already
+   be at the end. */
+ struct server **up, *tmp;
+ 
+ for (serv = daemon->servers, up = >servers; serv; serv = tmp)
{
- /* Need to move down? */
- if (serv->next)
+ tmp = serv->next;
+ if ((serv->flags & SERV_MARK) &&
+ hostname_isequal(alloc_domain, serv->domain))
{
- *up = serv->next;
- daemon->servers_tail->next = serv;
- daemon->servers_tail = serv;
- serv->next = NULL;
+ /* Need to move down? */
+ if (serv->next)
+   {
+ *up = serv->next;
+ daemon->servers_tail->next = serv;
+ daemon->servers_tail = serv;
+ serv->next = NULL;
+   }
+ break;
}
- break;
+ else
+   up = >next;
}
- else
-