We need to pass more alias-types. Instead of having numbered
versions, use variadic number of macro arguments.
Also, fix build failure with old compiler:
In file included from src/nm-ip6-config.c:24:
./src/nm-ip6-config.h:44:29: error: controlling expression type 'typeof (ipconf_iter->current->obj)' (aka 'const void *const') not compatible with any generic association type
*out_address = has_next ? NMP_OBJECT_CAST_IP6_ADDRESS (ipconf_iter->current->obj) : NULL;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fixes: b1810d7a68
(cherry picked from commit b339a2742a)
We often want to cascade hashing, meaning, to combine the
outcome of various hash functions in a larger hash.
Instead of having each hash function return a guint hash value,
accept a hash state argument. This saves the overhead of initializing
and completing the intermediate hash states.
It also avoids loosing entropy when we reduce the larger hash state
into the intermediate guint hash value.
CC src/devices/src_libNetworkManager_la-nm-device.lo
In file included from src/devices/nm-device.c:45:0:
src/devices/nm-device.c: In function ‘_v4_has_shadowed_routes_detect’:
./src/platform/nmp-object.h:400:54: error: ‘o’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
_obj ? &_NM_CONSTCAST (NMPObject, _obj)->ip4_route : NULL; \
^
src/devices/nm-device.c:2774:19: note: ‘o’ was declared here
const NMPObject *o;
^
nmp_lookup_init_route_visible() was originally named this way, to only return routes
that are nmp_object_is_visible(). However, all routes are visible (as long as they are
nmp_object_is_alive()). Hence, this is a historic misnomer.
Also, passing @only_default FALSE is identical to the
nmp_lookup_init_addrroute() lookup.
So, rename the function to indicate it is a lookup for default routes
only. Also, get rid of the unsupported ifindex argument for which there
is no index.
Until now, NetworkManager's platform cache for routes used the quadruple
network/plen,metric,ifindex for equaliy. That is not kernel's
understanding of how routes behave. For example, with `ip route append`
you can add two IPv4 routes that only differ by their gateway. To
the previous form of platform cache, these two routes would wrongly
look identical, as the cache could not contain both routes. This also
easily leads to cache-inconsistencies.
Now that we have NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID, fix the route's
compare operator to match kernel's.
Well, not entirely. Kernel understands more properties for routes then
NetworkManager. Some of these properties may also be part of the ID according
to kernel. To NetworkManager such routes would still look identical as
they only differ in a property that is not understood. This can still
cause cache-inconsistencies. The only fix here is to add support for
all these properties in NetworkManager as well. However, it's less serious,
because with this commit we support several of the more important properties.
See also the related bug rh#1337855 for kernel.
Another difficulty is that `ip route replace` and `ip route change`
changes an existing route. The replaced route has the same
NM_PLATFORM_IP_ROUTE_CMP_TYPE_WEAK_ID, but differ in the actual
NM_PLATFORM_IP_ROUTE_CMP_TYPE_ID:
# ip -d -4 route show dev v
# ip monitor route &
# ip route add 192.168.5.0/24 dev v
192.168.5.0/24 dev v scope link
# ip route change 192.168.5.0/24 dev v scope 10
192.168.5.0/24 dev v scope 10
# ip -d -4 route show dev v
unicast 192.168.5.0/24 proto boot scope 10
Note that we only got one RTM_NEWROUTE message, although from NMPCache's
point of view, a new route (with a particular ID) was added and another
route (with a different ID) was deleted. The cumbersome workaround is,
to keep an ordered list of the routes, and figure out which route was
replaced in response to an RTM_NEWROUTE. In absence of bugs, this should
work fine. However, as we only rely on events, we might wrongly
introduce a cache-inconsistancy as well. See the related bug rh#1337860.
Also drop nm_platform_ip4_route_get() and the like. The ID of routes
is complex, so it makes little sense to look up a route directly.
NMPCache can preserve the order of the objects. Until now, the order
was however arbitrary. Soon we will require to preserve the order of
routes.
During a dump, force appending new objects at the end. That ensures,
correct ordering during the dump.
Note that we track objects in several distrinct indexes. Those partition the
set of all objects. Outside a dump when receiving events about new objects (e.g.
RTM_NEWROUTE), it is very unclear at which place the new object should be sorted.
It is especially unclear, as an object might move from one partition (of
an index) to another.
In general, a deterministic order will only be useful in one particular
instance: the NMP_CACHE_ID_TYPE_ROUTES_BY_DESTINATION index for routes.
In this case, we will ensure a particular order of the routes.
We already had a nmp_object_id_equal() function. Generally, an equal() function
is more useful then a cmp() function.
However, implementing a cmp() function is about the same effort then implementing
an equal() function. Also, an equal function can be trivially implemented based on
a compare function, but not the other way around.
That means, it is little extra effort to have both an equal() function
and a cmp() function. Add nmp_object_id_cmp(). If only to be
consistent with other code, which also provides both.
Reasons:
- it adds an O(1) lookup index for accessing NMIPxConfig's addresses.
Hence, operations like merge/intersect have now runtime O(n) instead
of O(n^2).
Arguably, we expect low numbers of addresses in general. For low
numbers, the O(n^2) doesn't matter and quite likely in those cases
the previous implementation was just fine -- maybe even faster.
But the simple case works fine either way. It's important to scale
well in the exceptional case.
- the tracked objects can be shared between the various NMPI4Config,
NMIP6Config instances with NMPlatform and everybody else.
- the NMPObject can be treated generically, meaning it enables code to
handle both IPv4 and IPv6, or addresses and routes. See for example
_nm_ip_config_add_obj().
- I want core to evolve to somewhere where we don't keep copies of
NMPlatformIP4Address, et al. instances. Instead they shall all be
shared. I hope this will reduce memory consumption (although tracking a
reference consumes some memory too). Also, it shortcuts nmp_object_equal()
when comparing the same object. Calling nmp_object_equal() on the
identical objects would be a common case after the hash function
pre-evaluates equality.
and refactor NMFakePlatform to also track links via NMPCache.
For one, now NMFakePlatform also tests NMPCache, increasing the
coverage of what we care about.
Also, all our NMPlatform implementations now use NMPObject and NMPCache.
That means, we can expose those as part of the public API. Which is
great, because callers can keep a reference to the NMPObject object
and make use of generic functions like nmp_object_to_string().
And move some code from NMLinuxPlatform to NMPlatform, where it belongs.
The advantage is that we reuse (and test!) the NMPCache implementation for
tracking addresses.
Also, we now always expose proper NMPObjects from both linux and fake
platform.
For example,
obj = NMP_OBJECT_UP_CAST (nm_platform_ip4_address_get (...));
will work as expected. Also, the caller is now by NMPlatform API
allowed to take and keep a reference to the returned objects.
Routes and addresses don't implement cmd_obj_is_visible(),
hence they are always visible, and NMP_CACHE_ID_TYPE_OBJECT_TYPE_VISIBLE_ONLY
is identical to NMP_CACHE_ID_TYPE_OBJECT_TYPE.
Only link objects can be alive but invisible. Still, drop the index
for looking up visible links entirely. Let callers do the filtering,
if they care.
Maintaining an index is expensive.Not so much in term of runtime, but
in term of memory.
Drop some indexes, and require the caller to use a more broad index (and
filter out unwanted elements).
Dropped:
- can no longer lookup visible default-routes by ifindex.
If you care about default-routes, lookup all and search for the
desired ifindex. The overall number of default-routes is expected
to be small.
We drop NMP_CACHE_ID_TYPE_ROUTES_VISIBLE_BY_IFINDEX_WITH_DEFAULT
entirely.
- no longer have a separate index for non-default routes. We
expect that the most routes are non-default routes. So, don't
have an index without default-routes, instead let the caller
just lookup all routes, and reject default-routes themself.
We keep NMP_CACHE_ID_TYPE_ROUTES_VISIBLE_BY_DEFAULT, but it
now no longer tracks non-default routes.
This drops 1 out of 6 route indexes, and modifes another one, so
that we expect that there are almost no entires tracked by it.
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.
