[PATCH] call aug_load() at most once per second
by Laine Stump
Previously, netcf would call aug_load() at the start of each public
API call, and rely on augeas quickly determining if the files needed
to be reread based on checking the mtime of all files. With a large
number of files (i.e. several hundred ifcfg files) just checking the
mtime of all files ends up taking quite a long time; enough to turn a
simple "virsh iface-list" of 300 bridges + 300 vlans into a 22 second
ordeal.
With this patch applied, netcf will only call aug_load() at most once
every second, resulting in runtime for virsh iface-list going down to
< 1 second.
The trade-off is that the results of a netcf API call could be up to 1
second out of date (but only due to changes in the config external to
netcf). Since ifcfg files change very infrequently, this is likely
acceptable.
---
src/dutil_linux.c | 8 +++++++-
src/dutil_linux.h | 1 +
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/dutil_linux.c b/src/dutil_linux.c
index 0850593..24f4d95 100644
--- a/src/dutil_linux.c
+++ b/src/dutil_linux.c
@@ -32,6 +32,7 @@
#include <unistd.h>
#include <ctype.h>
#include <errno.h>
+#include <time.h>
#include <dirent.h>
#include <sys/wait.h>
@@ -151,6 +152,7 @@ int remove_augeas_xfm_table(struct netcf *ncf,
*/
augeas *get_augeas(struct netcf *ncf) {
int r;
+ time_t current_time;
if (ncf->driver->augeas == NULL) {
augeas *aug;
@@ -186,9 +188,12 @@ augeas *get_augeas(struct netcf *ncf) {
}
ncf->driver->copy_augeas_xfm = 0;
ncf->driver->load_augeas = 1;
+ ncf->driver->load_augeas_time = 0;
}
- if (ncf->driver->load_augeas) {
+ current_time = time(NULL);
+ if (ncf->driver->load_augeas &&
+ ncf->driver->load_augeas_time != current_time) {
augeas *aug = ncf->driver->augeas;
r = aug_load(aug);
@@ -207,6 +212,7 @@ augeas *get_augeas(struct netcf *ncf) {
}
ERR_THROW(r > 0, ncf, EOTHER, "errors in loading some config files");
ncf->driver->load_augeas = 0;
+ ncf->driver->load_augeas_time = current_time;
}
return ncf->driver->augeas;
error:
diff --git a/src/dutil_linux.h b/src/dutil_linux.h
index a06a15c..75ac631 100644
--- a/src/dutil_linux.h
+++ b/src/dutil_linux.h
@@ -41,6 +41,7 @@ struct driver {
struct nl_sock *nl_sock;
struct nl_cache *link_cache;
struct nl_cache *addr_cache;
+ time_t load_augeas_time;
unsigned int load_augeas : 1;
unsigned int copy_augeas_xfm : 1;
unsigned int augeas_xfm_num_tables;
--
2.4.3
8 years, 6 months
Re: [netcf-devel] [libvirt] [PATCH 0/4] improve virConnectListAllInterfaces()
by Daniel P. Berrange
On Fri, Sep 25, 2015 at 12:41:23PM -0700, David Lutterkort wrote:
> On Fri, Sep 25, 2015 at 11:35 AM, Laine Stump <laine(a)laine.org> wrote:
>
> > On 09/25/2015 01:27 PM, Daniel P. Berrange wrote:
> >
> >>
> >> So, I instrumented the netcf and augeas code to checking timings.
> >>
> >> The aug_get calls time less than a millisecond, as do the various
> >> other calls. I found the bulk of the time is actually coming from
> >> the netcf function "get_augeas", which in turns call "aug_load"
> >> for every single damn netcf function call. So when we have 500
> >> interfaces, we're telling augeas to load all the config files
> >> 1000 times. That's where the slowness is coming from....
> >>
> >> Either we need to stop loading config files on every fnuction
> >> call in netcf,
> >>
> >
> > Right you are! netcf has a variable "load_augeas" that is set each time an
> > API call is started. I removed the line that sets it, and the time for
> > virsh iface-list --all for 309 toplevel interfaces (300 vlans connected to
> > 300 bridges + whatever is really on the host) went from 22.2 sec. to 1.47
> > sec!
> >
>
> Strange .. that variable should just control whether we call aug_load or
> not, it shouldn't affect whether we actual create a new augeas instance
> (which indeed can be very expensive)
>
> I see that we pass AUG_NO_MODL_AUTOLOAD to aug_init - it should not be
> necessary, but you might want to try quickly if passing
> AUG_NO_MODL_AUTOLOAD|AUG_NO_LOAD changes things (it shouldn't)
>
>
> > But of course we can't blindly re-use the initial data forever. Looking at
> > aug_load() itself, I see a lot of comments about everything it does to
> > avoid unnecessary re-loading of files. This was added in 2010, and is
> > probably what I remember David talking about:
> >
> > commit 5ee8163051be8214507c13c86171ac90ca7cb91f
> > Author: David Lutterkort <lutter(a)redhat.com>
> > Date: Tue Jun 29 15:32:44 2010 -0700
> >
> > Avoid unnecessary file parsing when reloading the tree
> >
> > We used to reparse every file we knew about upon aug_load. Now, we only
> > reparse files if the file has changed on disk.
> >
> > We test a few scenarios to make sure aug_load retains its behavior of
> > obliterating the tree and filling it with the latest from disk. This
> > includes throwing away unsaved changes or trees that have been deleted.
> >
> > So now the question is whether something can be done to improve aug_load()
> > (maybe it used to perform better and there has been a regression?), or if
> > netcf should derive a list of files from the list sent to augeas, and do
> > its own checking of the timestamps.
> >
>
> What should happen when you call aug_load is that augeas goes and stats all
> the files it knows about and if their mtime hasn't changed since we last
> loaded them, that should be the end of it. I have a hard time believing
> that stating 309 files takes ~ 1.5s though - seems there's something else
> going on. Maybe there are still some cases in which a new augeas instance
> gets created ?
Oh, you are mis-reading the numbers that Laine gives here. He has 310
interfaces, which corresponds to 620 API calls into netcf, which is
620 cals to aug_load() in 22 seconds. So each aug_load() is 35 milliseconds.
I've just tested on my machine with SSD and 500 interfaces. I get 25 ms
for each call to aug_load().
strace'ing it, I confirm that is really is only doing stat() on a whole
bunch of files.
So really the problem we have here is that we are doing O(n^2) calls to
aug_load() where n == number of interfaces.
So we really must change what we do so that we only do 1 call to aug_load
in order to list interfaces. I'm thinking the simplest way is to just
define a new API in augeas to bulk query the interfaces + their macs
in a single call.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8 years, 7 months
Re: [netcf-devel] [libvirt] [PATCH 0/4] improve virConnectListAllInterfaces()
by Daniel P. Berrange
On Fri, Sep 25, 2015 at 12:41:23PM -0700, David Lutterkort wrote:
> On Fri, Sep 25, 2015 at 11:35 AM, Laine Stump <laine(a)laine.org> wrote:
>
> > On 09/25/2015 01:27 PM, Daniel P. Berrange wrote:
> >
> >>
> >> So, I instrumented the netcf and augeas code to checking timings.
> >>
> >> The aug_get calls time less than a millisecond, as do the various
> >> other calls. I found the bulk of the time is actually coming from
> >> the netcf function "get_augeas", which in turns call "aug_load"
> >> for every single damn netcf function call. So when we have 500
> >> interfaces, we're telling augeas to load all the config files
> >> 1000 times. That's where the slowness is coming from....
> >>
> >> Either we need to stop loading config files on every fnuction
> >> call in netcf,
> >>
> >
> > Right you are! netcf has a variable "load_augeas" that is set each time an
> > API call is started. I removed the line that sets it, and the time for
> > virsh iface-list --all for 309 toplevel interfaces (300 vlans connected to
> > 300 bridges + whatever is really on the host) went from 22.2 sec. to 1.47
> > sec!
> >
>
> Strange .. that variable should just control whether we call aug_load or
> not, it shouldn't affect whether we actual create a new augeas instance
> (which indeed can be very expensive)
>
> I see that we pass AUG_NO_MODL_AUTOLOAD to aug_init - it should not be
> necessary, but you might want to try quickly if passing
> AUG_NO_MODL_AUTOLOAD|AUG_NO_LOAD changes things (it shouldn't)
>
>
> > But of course we can't blindly re-use the initial data forever. Looking at
> > aug_load() itself, I see a lot of comments about everything it does to
> > avoid unnecessary re-loading of files. This was added in 2010, and is
> > probably what I remember David talking about:
> >
> > commit 5ee8163051be8214507c13c86171ac90ca7cb91f
> > Author: David Lutterkort <lutter(a)redhat.com>
> > Date: Tue Jun 29 15:32:44 2010 -0700
> >
> > Avoid unnecessary file parsing when reloading the tree
> >
> > We used to reparse every file we knew about upon aug_load. Now, we only
> > reparse files if the file has changed on disk.
> >
> > We test a few scenarios to make sure aug_load retains its behavior of
> > obliterating the tree and filling it with the latest from disk. This
> > includes throwing away unsaved changes or trees that have been deleted.
> >
> > So now the question is whether something can be done to improve aug_load()
> > (maybe it used to perform better and there has been a regression?), or if
> > netcf should derive a list of files from the list sent to augeas, and do
> > its own checking of the timestamps.
> >
>
> What should happen when you call aug_load is that augeas goes and stats all
> the files it knows about and if their mtime hasn't changed since we last
> loaded them, that should be the end of it. I have a hard time believing
> that stating 309 files takes ~ 1.5s though - seems there's something else
> going on. Maybe there are still some cases in which a new augeas instance
> gets created ?
>
> If it does indeed turn out that augeas is still doing something stupid
> there, I'd much rather fix it in augeas than have netcf try and work around
> it ...
I'll look at augeas in a bit more detail and try to find out where it is
sinking time in aug_load.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
8 years, 7 months
Re: [netcf-devel] [libvirt] [PATCH 0/4] improve virConnectListAllInterfaces()
by Laine Stump
On 09/25/2015 01:27 PM, Daniel P. Berrange wrote:
>
> So, I instrumented the netcf and augeas code to checking timings.
>
> The aug_get calls time less than a millisecond, as do the various
> other calls. I found the bulk of the time is actually coming from
> the netcf function "get_augeas", which in turns call "aug_load"
> for every single damn netcf function call. So when we have 500
> interfaces, we're telling augeas to load all the config files
> 1000 times. That's where the slowness is coming from....
>
> Either we need to stop loading config files on every fnuction
> call in netcf,
Right you are! netcf has a variable "load_augeas" that is set each time
an API call is started. I removed the line that sets it, and the time
for virsh iface-list --all for 309 toplevel interfaces (300 vlans
connected to 300 bridges + whatever is really on the host) went from
22.2 sec. to 1.47 sec!
But of course we can't blindly re-use the initial data forever. Looking
at aug_load() itself, I see a lot of comments about everything it does
to avoid unnecessary re-loading of files. This was added in 2010, and is
probably what I remember David talking about:
commit 5ee8163051be8214507c13c86171ac90ca7cb91f
Author: David Lutterkort <lutter(a)redhat.com>
Date: Tue Jun 29 15:32:44 2010 -0700
Avoid unnecessary file parsing when reloading the tree
We used to reparse every file we knew about upon aug_load. Now, we only
reparse files if the file has changed on disk.
We test a few scenarios to make sure aug_load retains its behavior of
obliterating the tree and filling it with the latest from disk. This
includes throwing away unsaved changes or trees that have been deleted.
So now the question is whether something can be done to improve
aug_load() (maybe it used to perform better and there has been a
regression?), or if netcf should derive a list of files from the list
sent to augeas, and do its own checking of the timestamps.
(After this message I think we can remove libvir-list from the Cc, since
it's clear that everything is between netcf and augeas).
8 years, 7 months