On 10/11/2018 06:20 AM, Daniel P. Berrangé wrote: > On Mon, Sep 24, 2018 at 10:41:37AM +0300, Nikolay Shirokovskiy wrote: > >> What speed up is possible on conservative approach? First we can remove for >> test purpuses firewall ruleLock, gentech dirver updateMutex and filter object >> mutex which do not serve function in restart scenario. This gives 36s restart >> time. The speed up is archived because heavy fork/preexec steps are now run >> concurrently. > To me the most obvious speedup is not to run any commands at all. > > 99% of the time when we restart libvirtd and re-build network filters > we are achieving nothing useful. We tear down the rules and replace > them by exactly the same rules. > > The idea behind rebuilding at startup is that someone might have changed > the config on diks while libvirtd was not running, and we want to ensure > that the VMs have the correct live config after this. > > Alternatively libvirtd has been upgraded to a newer version, and we want > to rebuild with the new code in case old code had a bug causing it to > create incorrect rules. > > I think we should look at how to optimize this to be more intelligent. > > We could somehow record a hash of what rule contents was used > originally. Only rebuild the rules if we see the hash of nwfilter > rules has changed, or if libvirtd binary has had code changes. Since we pre-emptively delete any existing copy of a rule just prior to adding that rule, another way we could reduce startup time would be to spend less time deleting. one way of doing that would (I think) be if we could add all of our rules in a set of chains that we create ourselves. Then when libvirtd restarted, we could just issue a few commands to flush those chains. (come to think of it, nwfilter *already* puts most/all of its rules in self-created chains. Hmm.... > >> Next we can try to reduce fork/preexec time. To estimate its contibution alone >> let's bring back the above locks. It turns out the most time takes fork itself >> and closing 8k (on my system) file descriptors in preexec. Using vfork gives >> 2x boost and so does dropping mass close. (I check this mass close contribution >> because I not quite understand the purpose of this step - libvirt typically set >> close-on-exec flag on it's descriptors). So this two optimizations alone can >> result in restart time of 30s. > I don't like the idea of using vfork(). It is already hard to do stuff between > fork() and execve() safely as you're restricted to async signal safe functions. > vfork() places an even greater number of restrictions on code that can be > run. It is especially dangerous in threaded programs as only the calling thread > is suspended. > > Unfortunately we cannot rely on close-on-exec. Depending on OS and/or version, > clsoe-on-exec setting may or may not be atomic. eg > > open(...., O_CLOEXEC) > > vs > > open(...) > fcntl(O_CLOSEXEC) > > the later has a race we'd hit. > > In addition libvirt links to a huge number of 3rd party libraries and I have > essentially zero confidence in them setting O_CLOEXEC correctly all the time. > > IOW, the mass close is inescapable IMHO. > > The only thing I could see us doing is to spawn some minimalist helper process > which then spawns iptables as needed. We'd only need the mass-close once for > the helper process, which can then just spawn iptables without mass-close, > or simply set a tiny max files ulimit for this helper process so that it > only 'mass close' 20 FDs. > > We'd have to write the set of desired iptables rules into some data format, > send it to the helper to execute and wait for results. Probably not too > difficult to achieve. The best way to reduce fork/exec time is of course to never fork/exec at all :-). So, a chain of current/future events that could lead to 0 fork/execs: 1) upstream firewalld now has an nftables backend available (hang with me, this really is related...) 2) some distros have already switched to it (someone running debian on #virt a couple weeks ago had a problem that turned out to be due to using the nftables backend, and Fedora 29 *tried* to switch to using the nftables backend, but had to revert/postpone the change due to the networking problems it caused with libvirt virtual networks.) 3) due to (1) and (2) we're looking into making libvirt work properly with a firewalld using nftables (it's going to take new code in both libvirt *and* firewalld) 4) an upcoming release of firewalld will be to adding/deleting nftables rules using calls to a library rather than a fork/exec of the nft command. Once libvirt is working with an nftables backend to firewalld, and firewalld is using an API to access nftables, there will be 0 fork/execs! Yay! (The bad news is that libvirt's nwfilter rules require more new functionality that doesn't yet exist in firewalld (e.g. outbound rules), so while the firewall rules that we add for libvirt networks could be switched over fairly soo, the nwfilter part is going to take awhile :-/ > >> Unfortunately combining the above two approaches does not give boost multiple >> of them along. The reason is due to concurrency and high number of VMs (100) >> preexec boost does not have significant role and using vfork dininishes >> concurrency as it freezes all parent threads before execve. So dropping locks >> and closes gives 33s restart time and adding vfork to this gives 25s restart >> time. >> >> Another approach is to use --atomic-file option for ebtables >> (iptables/ip6tables unfortunately does not have one). The idea is to save table >> to file/edit file/commit table to kernel. I hoped this could give performance >> boost because we don't need to load/store kernel network table for a single >> rule update. In order to isolate approaches I also dropped all ip/ip6 updates >> which can not be done this way. In this approach we can not drop ruleLock in >> firewall because no other VM threads should change tables between save/commit. >> This approach gives restart time 25s. But this approach is broken anyway as we >> can not be sure another application doesn't change newtork table between >> save/commit in which case these changes will be lost. >> >> After all I think we need to move in a different direction. We can add API to >> all binaries and firewalld to execute many commands in one run. We can pass >> commands as arguments or wrote them into file which is then given to binary. > This sounds like 'iptables-restore' command which accepts a batch of commands > in a file. There's also ip6tables-restore and ebtables-restore. > > There's no way to execute these via firewalld though. ...even though firewalld itself internally uses iptables-restore in certain cases (according to what I was told by one of the firewalld developers). (also, for those naughty people who have firewalld disabled, libvirt could use iptables-restore) > > It is also tricky when we need to check the output of certan commands > before making a decision on which other commands to run. It would > certainly speed things up though to load batches of rules at once. > > > IMHO the most immediately useful thing would be to work on optimization > to skip re-creating nwfilter rules on startup, unless we detect there > are likely changes. Agreed. That is something that could be done now, with no new features required from any other package. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
