> main: split out nm_main_utils_ensure_rundir ()

This doesn't really seem necessary: nm-iface-helper should only be launched by NetworkManager itself, and NM will already have ensured that rundir exists, so nm-iface-helper doesn't have to.


> main: (order) call g_type_init() very early in main()
> 
> g_type_init() is independent of all NetworkManager
> functionlity. Just get it done early on.
          ^
           typo


> main: (order) move root user check after help/version option

>+		fprintf (stderr, _("You must be root to run %s!\n"), progname);

Anything that runs after nm_main_utils_early_setup() can just call g_get_prgname() rather than you needing to pass the progname name to it.


> nm-iface-helper: fix pidfile name and obtain the ifindex earlier

This looks correct, but why don't we just use the ifname in the pidfile name rather than the ifindex? nm-iface-helper is only used in static configurations, so we don't have to worry about an interface being destroyed and then a different interface with the same ifname being created.


> main: (order) check pidfile earlier for running NM

This moves the ensure_rundir call for no obvious reason


> main: (order) early start D-Bus service

Have you actually tested that this does make systemd consider NM to be started earlier? I remember some discussion before about the possibility that dbus-glib/gdbus might postpone some things until the main loop starts running, and so it would be necessary to actually move the post-dbus-setup part of the initialization out into an idle handler.

(In reply to Dan Winship from comment #1)
> > main: split out nm_main_utils_ensure_rundir ()
> 
> This doesn't really seem necessary: nm-iface-helper should only be launched
> by NetworkManager itself, and NM will already have ensured that rundir
> exists, so nm-iface-helper doesn't have to.

you mean, it doesn't seem necessary to call nm_main_utils_ensure_rundir() in nm-iface-helper?

Shortly after we try to write the pidfile to NMRUNDIR. While probably not necessary, doesn't seem to hurt... (?)



> > main: (order) call g_type_init() very early in main()
> > 
> > g_type_init() is independent of all NetworkManager
> > functionlity. Just get it done early on.
>           ^
>            typo

fixed


> > main: (order) move root user check after help/version option
> 
> >+		fprintf (stderr, _("You must be root to run %s!\n"), progname);
> 
> Anything that runs after nm_main_utils_early_setup() can just call
> g_get_prgname() rather than you needing to pass the progname name to it.

done.


> > nm-iface-helper: fix pidfile name and obtain the ifindex earlier
> 
> This looks correct, but why don't we just use the ifname in the pidfile name
> rather than the ifindex? nm-iface-helper is only used in static
> configurations, so we don't have to worry about an interface being destroyed
> and then a different interface with the same ifname being created.

good point. But I would still prefer this, because the ifindex is guaranteed not to repeat, the ifname not. While one might argue that a race of a repeating ifname is "unlikely", I would prefer a race-agnostic solution.


 
> > main: (order) check pidfile earlier for running NM
> 
> This moves the ensure_rundir call for no obvious reason

fixed.


> > main: (order) early start D-Bus service
> 
> Have you actually tested that this does make systemd consider NM to be
> started earlier? I remember some discussion before about the possibility
> that dbus-glib/gdbus might postpone some things until the main loop starts
> running, and so it would be necessary to actually move the post-dbus-setup
> part of the initialization out into an idle handler.

Yes.

$ systemd-analyze blame | grep NetworkManager.service

gives for current master (on my system) something around 450 ms.
With the change, it's around 18 ms.





Added new commit:

>> main-utils: don't leak description for command line arguments in nm_main_utils_early_setup()


repushed.

(In reply to Thomas Haller from comment #2)
> you mean, it doesn't seem necessary to call nm_main_utils_ensure_rundir() in
> nm-iface-helper?

Yes (and therefore doesn't seem necessary to split it out from main.c into main-utils.c).

> > > nm-iface-helper: fix pidfile name and obtain the ifindex earlier
> > 
> > This looks correct, but why don't we just use the ifname in the pidfile name
> > rather than the ifindex? nm-iface-helper is only used in static
> > configurations, so we don't have to worry about an interface being destroyed
> > and then a different interface with the same ifname being created.
> 
> good point. But I would still prefer this, because the ifindex is guaranteed
> not to repeat, the ifname not. While one might argue that a race of a
> repeating ifname is "unlikely", I would prefer a race-agnostic solution.

OK. (I prefer the ifname version since it makes it more immediately obvious which nm-iface-helper process it goes with.)

> Added new commit:
> 
> >> main-utils: don't leak description for command line arguments in nm_main_utils_early_setup()

ok

(In reply to Dan Winship from comment #3)
> (In reply to Thomas Haller from comment #2)
> > you mean, it doesn't seem necessary to call nm_main_utils_ensure_rundir() in
> > nm-iface-helper?
> 
> Yes (and therefore doesn't seem necessary to split it out from main.c into
> main-utils.c).

nm_main_utils_ensure_rundir() splits out from nm_main_utils_check_pidfile(). It seems wrong to create the rundir before checking for conflicting PID.

It doesn't move from main.c to main-utils.c (it was always there already).

A later commit orders ensure_rundir() after ensure_not_running_pidfile().

RE iface vs. ifindex I think I was just trying to be safe against interface renames, but I don't think that's relevant in the cases nm-iface-helper would be used.  So we might as well just use the ifname, or at least put the ifname into the pidfile name too.

The rest of it looks OK to me.

(In reply to Dan Winship from comment #1)
> > main: (order) early start D-Bus service
> 
> Have you actually tested that this does make systemd consider NM to be
> started earlier? I remember some discussion before about the possibility
> that dbus-glib/gdbus might postpone some things until the main loop starts
> running, and so it would be necessary to actually move the post-dbus-setup
> part of the initialization out into an idle handler.

I tested it too in Brno in Feb, and also found a significant reduction in the startup time of NM according to systemd-analyze --blame.  It's a real speedup, though in some ways it's just systemd printing fake metrics.

(In reply to Dan Williams from comment #5)

> I tested it too in Brno in Feb, and also found a significant reduction in
> the startup time of NM according to systemd-analyze --blame.  It's a real
> speedup, though in some ways it's just systemd printing fake metrics.

I think it's not only cosmetic or fake. "network" tarkget is ordered After NetworkManager, and many services are ordered after "network". So, this actually blocks services by 400 ms (on my system).

I might be wrong about that.

merged as:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=6ce7b7df96e5a53b4e43a687dfedd8ae8a383fa0


We can rename the PID file (though I'd vote for ifindex instead of ifname). But let's just do that in a separate patch.

backported to nm-1-0

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=3ac59b84c554dd12901d9b47f2fe970b1132dc70


I figure the speedup at start is a really nice improvement.

And the fix
  >> nm-iface-helper: fix pidfile name and obtain the ifindex earlier
seems very important to me, because otherwise, multiple nm-iface-helper would use the same pid file "nm-iface-helper--1.pid".
Did it actually work to start more then one nm-iface-helper at a time?