Implement the reference counting of NMPObject as part of
NMDedupMultiObj and get rid of NMDedupMultiBox.
With this change, the NMPObject is aware in which NMDedupMultiIndex
instance it is tracked.
- this saves an additional GSlice allocation for the NMDedupMultiBox.
- it is immediately known, whether an NMPObject is tracked by a
certain NMDedupMultiIndex or not. This saves an additional hash
lookup.
- previously, when all idx-types cease to reference an NMDedupMultiObj
instance, it was removed. Now, a tracked objects stays in the
NMDedupMultiIndex until it's last reference is deleted. This possibly
extends the lifetime of the object and we may reuse it better.
- it is no longer possible to add one object to more then one
NMDedupMultiIndex instance. As we anyway want to have only one
instance to deduplicate the objects, this is fine.
- the ref-counting implementation is now part of NMDedupMultiObj.
Previously, NMDedupMultiIndex could also track objects that were
not ref-counted. Hoever, the object anyway *must* implement the
NMDedupMultiObj API, so this flexibility is unneeded and was not
used.
- a downside is, that NMPObject grows by one pointer size, even if
it isn't tracked in the NMDedupMultiIndex. But we really want to
put all objects into the index for sharing and deduplication. So
this downside should be acceptable. Still, code like
nmp_object_stackinit*() needs to handle a larger object.
NMPlatform's cache should be directly accessible to the users,
at least the NMPLookup part and the fact that the cache contains
ref-counted, immutable NMPObjects.
This allows users to inspect the cache with zero overhead. Meaning,
they can obtain an NMDedupMultiHeadEntry and iterate the objects
themself. It also means, the are free to take and keep references
of the NMPObject instances (of course, without modifying them!).
NMFakePlatform will use the very same cache. The fake platform should
only differ when modifying the objects.
Another reason why this makes sense is because NMFakePlatform is for one
a test-stup but also tests behavior of platform itself. Using a separate
internal implementation for the caching is a pointless excecise, because
only the real NMPCache's implementation really matters for production.
So, either NMFakePlatform behaves idential, or it is buggy. Reuse it.
Port fake platform's tracking of routes to NMPCache and move duplicate
code from NMLinuxPlatform to the base class.
This commit only ports IP routes, eventually also addresses and links
should be tracked via the NMPCache instance.
Maintaining an index is expensive. We can merge indexes that
are strictly distinct, because one index can just partition the
objects into distinct sets.
Rework platform object cache to use NMDedupMultiIndex.
Already previously, NMPCache used NMMultiIndex and had thus
O(1) for most operations. What is new is:
- Contrary to NMMultiIndex, NMDedupMultiIndex preserves the order of
the cached items. That is crucial to handle routes properly as kernel
will replace the first matching route based on network/plen/metric
properties. See related bug rh#1337855.
Without tracking the order of routes as they are exposed
by kernel, we cannot properly maintain the route cache.
- All NMPObject instances are now treated immutable, refcounted
and get de-duplicated via NMDedupMultiIndex. This allows
to have a global NMDedupMultiIndex that can be shared with
NMIP4Config and NMRouteManager. It also allows to share the
objects themselves.
Immutable objects are so much nicer. We can get rid of the
update pre-hook callback, which was required previously because
we would mutate the object inplace. Now, we can just update
the cache, and compare obj_old and obj_new after the fact.
- NMMultiIndex was treated as an internal of NMPCache. On the other
hand, NMDedupMultiIndex exposes NMDedupMultiHeadEntry, which is
basically an object that allows to iterate over all related
objects. That means, we can now lookup objects in the cache
and give the NMDedupMultiHeadEntry instance to the caller,
which then can iterate the list on it's own -- without need
for copying anything.
Currently, at various places we still create copies of lookup
results. That can be improved later.
The ability to share NMPObject instances should enable us to
significantly improve performance and scale with large number
of routes.
Of course there is a memory overhead of having an index for each list
entry. Each NMPObject may also require an NMDedupMultiEntry,
NMDedupMultiHeadEntry, and NMDedupMultiBox item, which are tracked
in a GHashTable. Optimally, one NMDedupMultiHeadEntry is the head
for multiple objects, and NMDedupMultiBox is able to deduplicate several
NMPObjects, so that there is a net saving.
Also, each object type has several indexes of type NMPCacheIdType.
So, worst case an NMPlatformIP4Route in the platform cache is tracked
by 8 NMPCacheIdType indexes, for each we require a NMDedupMultiEntry,
plus the shared NMDedupMultiHeadEntry. The NMDedupMultiBox instance
is shared between the 8 indexes (and possibly other).
Platform has it's own, simple implementation of object types:
NMPObject. Extract a base type and move it to "shared/nm-utils/nm-obj.h"
so it can be reused.
The base type is trival, but it allows us to implement other objects
which are compatible with NMPObjects. Currently there is no API for generic
NMObjBaseInst type, so compatible in this case only means, that they
can be used in the same context (see example below).
The only thing that you can do with a NMObjBaseInst is check it's
NMObjBaseClass.
Incidentally, NMObjBaseInst is also made compatible to GTypeInstance.
It means, an NMObjBaseInst is not necessarily a valid GTypeInstance (like NMPObject
is not), but it could be implemented as such.
For example, you could do:
if (NMP_CLASS_IS_VALID ((NMPClass *) obj->klass)) {
/* is an NMPObject */
} else if (G_TYPE_CHECK_INSTANCE_TYPE (obj, NM_TYPE_SOMETHING)) {
/* it a NMSometing GType */
} else {
/* something else? */
}
The reason why NMPObject is not implemented as proper GTypeInstance is
because it would require us to register a GType (like
g_type_register_fundamental). However, then the NMPClass struct can
no longer be const and immutable memory. But we could.
NMObjBaseInst may or may not be a GTypeInstance. In a sense, it's
a base type of GTypeInstance and all our objects should be based
on it (optionally, they we may make them valid GTypes too).
We end up calling nmp_cache_id_init_*() a lot to initialize stack-allocated
cache-ids to lookup the NMMultiIndex. There is no need to memset() it to
zero, because all relevant fields are supposed to be set explicitly.
Consider:
unshare -n
ip link add d0 type dummy
ip link add d1 type dummy
ip link set d0 up
ip link set d1 up
ip addr add 192.168.100.5/24 dev d0
ip addr add 192.168.101.5/24 dev d1
ip route add 192.168.200.0/24 via 192.168.100.1
ip monitor &
ip route change 192.168.200.0/24 via 192.168.101.1
#prints 192.168.200.0/24 via 192.168.101.1 dev d1
ip route show
#192.168.100.0/24 dev d0 proto kernel scope link src 192.168.100.5
#192.168.101.0/24 dev d1 proto kernel scope link src 192.168.101.5
#192.168.200.0/24 via 192.168.101.1 dev d1
Note that `ip route change` replaced the exising route. "Replaced" in this
case means: the previous route on device "d0" got removed and a new route
on "d1" was added. However, kernel only sent one RTM_NEWROUTE event, no
RTM_DELROUTE that notifies about this change.
We need to workaround that by re-synching the routes when we receive a
RTM_NEWROUTE notification.
NMPCacheId is a union with fields for all known NMPCacheIdTypes.
Up to now, we always cloned the entire union, computed the hash
over all (possibly unset) fields and used memcmp() unanimously.
That was ok, because NMPCacheId was 16 bytes in total and cache-id
types that consumed less bytes didn't have a large overhead.
Next, we will add a new cache id type which increases the size of
NMPCacheId to 24 bytes. So, while possibly only a fraction of the
instances is that large, they would all have to pay that price.
Change that to consider and clone only those parts of the id
that are actually used.
As we get more NMPCacheIdType values, it's better to have for
each type a pre-declared list of supported types, instead of
iterating over all types and letting _nmp_object_init_cache_id()
figure out that the cache-id-type is unsupported on that object.
Inside container, where we don't use udev we don't receive
any events from udev client. Thus the client only returns
devices when iterating it initially, but no events for newly
added devices that appear later.
Thus, inside containers we don't want to create a udev client
at all.
- "gsystem-local-alloc.h" and <gio/gio.h> are already included via
"nm-default.h". No need to include them separately.
- include "nm-macros-internal.h" via "nm-default.h" and drop all
explict includes.
- in the modified files, ensure that we always include "config.h"
and "nm-default.h" first. As second, include the header file
for the current source file (if applicable). Then follow external
includes and finally internal nm includes.
- include nm headers inside source code files with quotes
- internal header files don't need to include default headers.
They can savely assume that "nm-default.h" is already included
and with it glib, nm-glib.h, nm-macros-internal.h, etc.
Downsides:
- Add some additional overhead to manage the index
- The NMPCacheId struct grows to 16 bytes, affecting
hashing performance for all object types.
Still do it, based on the assumption that it doesn't matter
for a low number of interfaces. But the O(1) access time matters
when having lots of interfaces.
We potentially emit a lot of signals. Don't look up the
signal by name because that adds quite some additional
overhead, like peeking for a GQuark.
Instead pass the numeric signal-id directly.
Previously, we could only set the ingress-qos-mappings/egress-qos-mappings.
Now also cache the mappings and expose them from the platform cache.
Also, support changing the vlan flags not only when creating the vlan
interface.
Instead of using libnl-route-3 library to serialize netlink messages,
construct the netlink messages ourselves.
This has several advantages:
- Creating the netlink message ourself is actually more straight
forward then having an intermediate layer between NM and the kernel.
Now it is immediately clear, how a platform request translates to
a netlink/kernel request.
You can look at the kernel sources how a certain netlink attribute
behaves, and then it's immediately clear how to set that (and vice
versa).
- Older libnl versions might have bugs or missing features for which
we needed to workaround (often by offering a reduced/broken/untested
functionality). Now we can get rid or workaround like _nl_has_capability(),
check_support_libnl_extended_ifa_flags(), HAVE_LIBNL_INET6_TOKEN.
Another example is a libnl bug when setting vlan ingress map which
isn't even yet fixed in libnl upstream.
- We no longer need libnl-route-3 at all and can drop that runtime
requirement, saving some 400k.
Constructing the messages ourselves also gives better performance
because we don't have to create the intermediate libnl object.
- In the future we will add more link-type support which is easier
to support by basing directly on the plain kernel/netlink API,
instead of requiring also libnl3 to expose this functionality.
E.g. adding macvtap support: we already parsed macvtap properties
ourselves because of missing libnl support. To *add* macvtap
support, we also would have to do it ourself (or extend libnl).
Constructing the libnl3 object only to parse the message
is wasteful. It involves several memory allocations, thread
synchronization and parsing fields that we don't care about.
But moreover, older libnl version might not support all the
fields we are interested in, hence we have workarounds like
_nl_link_parse_info_data(). Certain features might not fully
work unless libnl supports it too (although kernel would).
As we already parse the data ourselves sometimes, just go
all they way and don't construct the intermediate libnl object.
This will allow us to drop the _nl_link_parse_info_data() workarounds
in next commits. That is important, because _nl_link_parse_info_data()
sidesteps our platform cache and is not in sync with the cache (not to
mention the extra work to explicitly refetch the data on every lookup).
Also, it gets us 60% on the way to no longer needing 'libnl-route-3.so'
at all and eventually drop requiring the library.
For link objects, nmp_object_cmp() did not consider the private fields and
thus behaved different then nmp_object_equal(). This is wrong.
Implement nmp_object_equal() based on compare.
Arguably, it is more convenient to use the static buffer as
it saves typing.
But having such a low-level function use a static buffer also
limits the way how to use it. As it was, you could not avoid
using the static buffer.
E.g. you cannot do:
char buf[100];
_LOGD ("nmp-object: %s; platform-link: %s",
nmp_object_to_string (nmpobj, buf, sizeof(buf)),
nm_platform_link_to_string (link));
This will fail for non-obvious reasons because both
to-string functions end up using the same static buffer.
Also change the to-string implementations to accept NULL
as valid and return it as "(null)".
https://bugzilla.gnome.org/show_bug.cgi?id=756427
Kernel allows to add the same IPv4 address that only differs by
peer-address (IFL_ADDRESS):
$ ip link add dummy type dummy
$ ip address add 1.1.1.1 peer 1.1.1.3/24 dev dummy
$ ip address add 1.1.1.1 peer 1.1.1.4/24 dev dummy
RTNETLINK answers: File exists
$ ip address add 1.1.1.1 peer 1.1.2.3/24 dev dummy
$ ip address show dev dummy
2: dummy@NONE: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN group default
link/ether 52:58:a7:1e:e8:93 brd ff:ff:ff:ff:ff:ff
inet 1.1.1.1 peer 1.1.1.3/24 scope global dummy
valid_lft forever preferred_lft forever
inet 1.1.1.1 peer 1.1.2.3/24 scope global dummy
valid_lft forever preferred_lft forever
We must also consider peer-address, otherwise platform will treat
two different addresses as one and the same.
https://bugzilla.gnome.org/show_bug.cgi?id=756356
