From ce9211500eae769ad255d6d8309f24964e85c0f6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 16 Apr 2021 10:13:59 +0200 Subject: [PATCH] platform/tests: work around rounding errors for bridge values in unit tests For certain options, kernel stores the numeric values in jiffies scale, while the user space value is in USER_HZ (1/100th of a second) scale. Jiffies scale depends on HZ setting (CONFIG_HZ), and depending on kernel configuration its 100, 250, 300, or 1000. That means, the round trip of clock_t_to_jiffies()/jiffies_to_clock_t() has different rounding errors, depending on CONFIG_HZ and it maybe be +/- 1 of the requested value. Since the rounding error depends on CONFIG_HZ, we cannot find "good" values for testing, that always behave the same. So we need to workaround that. Normalize the bridge values, if they look as if the value was mangled due to rounding. Related: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/665 --- src/core/platform/tests/test-common.c | 46 ++++++++++++++++++++++++--- src/core/platform/tests/test-common.h | 23 ++++++++++++++ src/core/platform/tests/test-link.c | 42 +++++++++++++++++------- 3 files changed, 95 insertions(+), 16 deletions(-) diff --git a/src/core/platform/tests/test-common.c b/src/core/platform/tests/test-common.c index 1b977145e5..d6fffe4cdb 100644 --- a/src/core/platform/tests/test-common.c +++ b/src/core/platform/tests/test-common.c @@ -1413,6 +1413,44 @@ nmtstp_ip6_address_del(NMPlatform * platform, } \ G_STMT_END +/* Due to rounding errors with clock_t_to_jiffies()/jiffies_to_clock_t(), kernel cannot + * store all requested values. That means, when we try to configure a bridge with + * the @requested values, the actually configured settings are slightly off, as + * @kernel. + * + * This function takes @requested and returns it as @dst output. All fields + * that might be mangled by kernel (according to @kernel) are adjusted. The + * result is almost identical to @requested, but some fields might be adjusted + * to their @kernel value. */ +const NMPlatformLnkBridge * +nmtstp_link_bridge_normalize_jiffies_time(const NMPlatformLnkBridge *requested, + const NMPlatformLnkBridge *kernel, + NMPlatformLnkBridge * dst) +{ + if (dst != requested) + *dst = *requested; + +#define _normalize_field(dst, kernel, field) \ + G_STMT_START \ + { \ + (dst)->field = nmtstp_normalize_jiffies_time((dst)->field, (kernel)->field); \ + } \ + G_STMT_END + + _normalize_field(dst, kernel, forward_delay); + _normalize_field(dst, kernel, hello_time); + _normalize_field(dst, kernel, max_age); + _normalize_field(dst, kernel, ageing_time); + _normalize_field(dst, kernel, mcast_last_member_interval); + _normalize_field(dst, kernel, mcast_membership_interval); + _normalize_field(dst, kernel, mcast_querier_interval); + _normalize_field(dst, kernel, mcast_query_interval); + _normalize_field(dst, kernel, mcast_query_response_interval); + _normalize_field(dst, kernel, mcast_startup_query_interval); + + return dst; +} + const NMPlatformLink * nmtstp_link_bridge_add(NMPlatform * platform, gboolean external_command, @@ -1421,7 +1459,8 @@ nmtstp_link_bridge_add(NMPlatform * platform, { const NMPlatformLink * pllink = NULL; const NMPlatformLnkBridge *ll = NULL; - int r = 0; + NMPlatformLnkBridge lnk_normalized; + int r = 0; g_assert(nm_utils_ifname_valid_kernel(name, NULL)); @@ -1542,10 +1581,9 @@ nmtstp_link_bridge_add(NMPlatform * platform, ll = NMP_OBJECT_CAST_LNK_BRIDGE(NMP_OBJECT_UP_CAST(pllink)->_link.netlink.lnk); - /* account for roundtrip rounding error with clock_t_to_jiffies()/jiffies_to_clock_t(). */ - g_assert_cmpint(lnk->forward_delay, >=, ll->forward_delay - 1); - g_assert_cmpint(lnk->forward_delay, <=, ll->forward_delay); + lnk = nmtstp_link_bridge_normalize_jiffies_time(lnk, ll, &lnk_normalized); + g_assert_cmpint(lnk->forward_delay, ==, ll->forward_delay); g_assert_cmpint(lnk->hello_time, ==, ll->hello_time); g_assert_cmpint(lnk->max_age, ==, ll->max_age); g_assert_cmpint(lnk->ageing_time, ==, ll->ageing_time); diff --git a/src/core/platform/tests/test-common.h b/src/core/platform/tests/test-common.h index ff116fa97f..d0e3716c8f 100644 --- a/src/core/platform/tests/test-common.h +++ b/src/core/platform/tests/test-common.h @@ -22,6 +22,24 @@ /*****************************************************************************/ +#define nmtstp_normalize_jiffies_time(requested_value, kernel_value) \ + ({ \ + typeof(kernel_value) _kernel_value = (kernel_value); \ + typeof(_kernel_value) _requested_value = (requested_value); \ + \ + /* kernel stores some values (like bridge's forward_delay) in jiffies. When converting + * back and forth (clock_t_to_jiffies()/jiffies_to_clock_t()), the value reported back + * to user space may have rounding errors (of +/- 1), depending on CONFIG_HZ setting. + * + * Normalize the requested_value to the kernel_value, if it look as if a rounding + * error happens. If the difference is larger than +/- 1, no normalization happens! */ \ + \ + ((_requested_value >= (NM_MAX(_kernel_value, 1) - 1)) \ + && (_requested_value <= (NM_MIN(_kernel_value, ~((typeof(_kernel_value)) 0) - 1) + 1))) \ + ? _kernel_value \ + : _requested_value; \ + }) + #define _NMLOG_PREFIX_NAME "platform-test" #define _NMLOG_DOMAIN LOGD_PLATFORM #define _NMLOG(level, ...) _LOG(level, _NMLOG_DOMAIN, __VA_ARGS__) @@ -437,6 +455,11 @@ gboolean nmtstp_kernel_support_get(NMPlatformKernelSupportType type); void nmtstp_link_set_updown(NMPlatform *platform, gboolean external_command, int ifindex, gboolean up); +const NMPlatformLnkBridge * +nmtstp_link_bridge_normalize_jiffies_time(const NMPlatformLnkBridge *requested, + const NMPlatformLnkBridge *kernel, + NMPlatformLnkBridge * dst); + const NMPlatformLink *nmtstp_link_bridge_add(NMPlatform * platform, gboolean external_command, const char * name, diff --git a/src/core/platform/tests/test-link.c b/src/core/platform/tests/test-link.c index 1613703251..61650af012 100644 --- a/src/core/platform/tests/test-link.c +++ b/src/core/platform/tests/test-link.c @@ -1679,17 +1679,25 @@ test_software_detect(gconstpointer user_data) case NM_LINK_TYPE_BRIDGE: { const NMPlatformLnkBridge *plnk = &lnk->lnk_bridge; + NMPlatformLnkBridge lnk_bridge_norm_stack; + const NMPlatformLnkBridge *lnk_bridge_norm; g_assert(plnk == nm_platform_link_get_lnk_bridge(NM_PLATFORM_GET, ifindex, NULL)); - g_assert_cmpint(nm_platform_lnk_bridge_cmp(&lnk_bridge, plnk), ==, 0); - g_assert_cmpint(plnk->forward_delay, ==, 1560); - g_assert_cmpint(plnk->hello_time, ==, 150); - g_assert_cmpint(plnk->max_age, ==, 2100); - g_assert_cmpint(plnk->ageing_time, ==, 2200); + + lnk_bridge_norm = nmtstp_link_bridge_normalize_jiffies_time(&lnk_bridge, + plnk, + &lnk_bridge_norm_stack); + + g_assert_cmpint(nm_platform_lnk_bridge_cmp(lnk_bridge_norm, plnk), ==, 0); + + g_assert_cmpint(plnk->forward_delay, ==, lnk_bridge_norm->forward_delay); + g_assert_cmpint(plnk->hello_time, ==, lnk_bridge_norm->hello_time); + g_assert_cmpint(plnk->max_age, ==, lnk_bridge_norm->max_age); + g_assert_cmpint(plnk->ageing_time, ==, lnk_bridge_norm->ageing_time); g_assert_cmpint(plnk->stp_state, ==, TRUE); g_assert_cmpint(plnk->priority, ==, 22); g_assert_cmpint(plnk->vlan_protocol, ==, 0x8100); - g_assert_cmpint(plnk->vlan_stats_enabled, ==, lnk_bridge.vlan_stats_enabled); + g_assert_cmpint(plnk->vlan_stats_enabled, ==, lnk_bridge_norm->vlan_stats_enabled); g_assert_cmpint(plnk->group_fwd_mask, ==, 8); g_assert_cmpint(plnk->mcast_snooping, ==, TRUE); g_assert_cmpint(plnk->mcast_router, ==, 1); @@ -1698,12 +1706,22 @@ test_software_detect(gconstpointer user_data) g_assert_cmpint(plnk->mcast_hash_max, ==, 1024); g_assert_cmpint(plnk->mcast_last_member_count, ==, 2); g_assert_cmpint(plnk->mcast_startup_query_count, ==, 3); - g_assert_cmpint(plnk->mcast_last_member_interval, ==, 5000); - g_assert_cmpint(plnk->mcast_membership_interval, ==, 25000); - g_assert_cmpint(plnk->mcast_querier_interval, ==, 26000); - g_assert_cmpint(plnk->mcast_query_interval, ==, 12000); - g_assert_cmpint(plnk->mcast_query_response_interval, ==, 5200); - g_assert_cmpint(plnk->mcast_startup_query_interval, ==, 3000); + g_assert_cmpint(plnk->mcast_last_member_interval, + ==, + lnk_bridge_norm->mcast_last_member_interval); + g_assert_cmpint(plnk->mcast_membership_interval, + ==, + lnk_bridge_norm->mcast_membership_interval); + g_assert_cmpint(plnk->mcast_querier_interval, + ==, + lnk_bridge_norm->mcast_querier_interval); + g_assert_cmpint(plnk->mcast_query_interval, ==, lnk_bridge_norm->mcast_query_interval); + g_assert_cmpint(plnk->mcast_query_response_interval, + ==, + lnk_bridge_norm->mcast_query_response_interval); + g_assert_cmpint(plnk->mcast_startup_query_interval, + ==, + lnk_bridge_norm->mcast_startup_query_interval); g_assert_cmpint(nm_platform_lnk_bridge_cmp(&lnk_bridge, plnk), ==, 0); break; }