From 973db2d41b957c4ee9d4ee9863f4b35c6890ac30 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 1 May 2019 09:19:59 +0200 Subject: [PATCH] platform: fix handling of default value for TCA_FQ_CODEL_CE_THRESHOLD iproute2 uses the special value ~0u to indicate not to set TCA_FQ_CODEL_CE_THRESHOLD in RTM_NEWQDISC. When not explicitly setting the value, kernel treats the threshold as disabled. However note that 0xFFFFFFFFu is not an invalid threshold (as far as kernel is concerned). Thus, we should not use that as value to indicate that the value is unset. Note that iproute2 uses the special value ~0u only internally thereby making it impossible to set the threshold to 0xFFFFFFFFu). But kernel does not have this limitation. Maybe the cleanest way would be to add another field to NMPlatformQDisc: guint32 ce_threshold; bool ce_threshold_set:1; that indicates whether the threshold is enable or not. But note that kernel does: static void codel_params_init(struct codel_params *params) { ... params->ce_threshold = CODEL_DISABLED_THRESHOLD; static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt, struct netlink_ext_ack *extack) { ... if (tb[TCA_FQ_CODEL_CE_THRESHOLD]) { u64 val = nla_get_u32(tb[TCA_FQ_CODEL_CE_THRESHOLD]); q->cparams.ce_threshold = (val * NSEC_PER_USEC) >> CODEL_SHIFT; } static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb) { ... if (q->cparams.ce_threshold != CODEL_DISABLED_THRESHOLD && nla_put_u32(skb, TCA_FQ_CODEL_CE_THRESHOLD, codel_time_to_us(q->cparams.ce_threshold))) goto nla_put_failure; This means, kernel internally uses the special value 0x83126E97u to indicate that the threshold is disabled (WTF). That is because (((guint64) 0x83126E97u) * NSEC_PER_USEC) >> CODEL_SHIFT == CODEL_DISABLED_THRESHOLD So in kernel API this value is reserved (and has a special meaning to indicate that the threshold is disabled). So, instead of adding a ce_threshold_set flag, use the same value that kernel anyway uses. --- libnm-core/nm-utils.c | 3 +++ src/devices/nm-device.c | 2 +- src/platform/nm-linux-platform.c | 6 ++++-- src/platform/nm-platform.c | 2 +- src/platform/nm-platform.h | 12 +++++++++++- 5 files changed, 20 insertions(+), 5 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 833cc30e5..cff2131ca 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -2332,6 +2332,9 @@ static const NMVariantAttributeSpec *const tc_qdisc_fq_codel_spec[] = { NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("target", G_VARIANT_TYPE_UINT32, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("interval", G_VARIANT_TYPE_UINT32, ), NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("quantum", G_VARIANT_TYPE_UINT32, ), + + /* 0x83126E97u is not a valid value (it means "disabled"). We should reject that + * value. Or alternatively, reject all values >= MAX_INT(32). */ NM_VARIANT_ATTRIBUTE_SPEC_DEFINE ("ce_threshold", G_VARIANT_TYPE_UINT32, ), /* kernel clamps the value at 2^31. Possibly such values should be rejected from configuration diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 7e1a6e711..cca26ad7b 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -6542,7 +6542,7 @@ tc_commit (NMDevice *self) GET_ATTR("target", qdisc->fq_codel.target, UINT32, uint32, 0); GET_ATTR("interval", qdisc->fq_codel.interval, UINT32, uint32, 0); GET_ATTR("quantum", qdisc->fq_codel.quantum, UINT32, uint32, 0); - GET_ATTR("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, -1); + GET_ATTR("ce_threshold", qdisc->fq_codel.ce_threshold, UINT32, uint32, NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED); GET_ATTR("memory", qdisc->fq_codel.memory, UINT32, uint32, NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET); GET_ATTR("ecn", qdisc->fq_codel.ecn, BOOLEAN, boolean, FALSE); } diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 2c7ba4015..f89052c01 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -3515,8 +3515,10 @@ _new_from_nl_qdisc (struct nlmsghdr *nlh, gboolean id_only) obj->qdisc.parent = tcm->tcm_parent; obj->qdisc.info = tcm->tcm_info; - if (nm_streq0 (obj->qdisc.kind, "fq_codel")) + if (nm_streq0 (obj->qdisc.kind, "fq_codel")) { obj->qdisc.fq_codel.memory = NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET; + obj->qdisc.fq_codel.ce_threshold = NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED; + } if (tb[TCA_OPTIONS]) { struct nlattr *options_attr; @@ -4242,7 +4244,7 @@ _nl_msg_new_qdisc (int nlmsg_type, NLA_PUT_U32 (msg, TCA_FQ_CODEL_INTERVAL, qdisc->fq_codel.interval); if (qdisc->fq_codel.quantum) NLA_PUT_U32 (msg, TCA_FQ_CODEL_QUANTUM, qdisc->fq_codel.quantum); - if (qdisc->fq_codel.ce_threshold != -1) + if (qdisc->fq_codel.ce_threshold != NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED) NLA_PUT_U32 (msg, TCA_FQ_CODEL_CE_THRESHOLD, qdisc->fq_codel.ce_threshold); if (qdisc->fq_codel.memory != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET) NLA_PUT_U32 (msg, TCA_FQ_CODEL_MEMORY_LIMIT, qdisc->fq_codel.memory); diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index fc49db19a..cbf9eed5c 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -6453,7 +6453,7 @@ nm_platform_qdisc_to_string (const NMPlatformQdisc *qdisc, char *buf, gsize len) nm_utils_strbuf_append (&buf, &len, " interval %u", qdisc->fq_codel.interval); if (qdisc->fq_codel.quantum) nm_utils_strbuf_append (&buf, &len, " quantum %u", qdisc->fq_codel.quantum); - if (qdisc->fq_codel.ce_threshold != -1) + if (qdisc->fq_codel.ce_threshold != NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED) nm_utils_strbuf_append (&buf, &len, " ce_threshold %u", qdisc->fq_codel.ce_threshold); if (qdisc->fq_codel.memory != NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET) nm_utils_strbuf_append (&buf, &len, " memory %u", qdisc->fq_codel.memory); diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 9f9fcf5c3..d7c388b1b 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -598,13 +598,23 @@ typedef struct { #define NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET (~((guint32) 0)) +#define NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED ((guint32) 0x83126E97u) + +G_STATIC_ASSERT (((((guint64) NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED) * 1000u) >> 10) == (guint64) INT_MAX); + typedef struct { guint32 limit; guint32 flows; guint32 target; guint32 interval; guint32 quantum; - guint32 ce_threshold; + guint32 ce_threshold; /* TCA_FQ_CODEL_CE_THRESHOLD: kernel internally stores this value as + * ((val64 * NSEC_PER_USEC) >> CODEL_SHIFT). The default value (in + * the domain with this coersion) is CODEL_DISABLED_THRESHOLD (INT_MAX). + * That means, "disabled" is expressed on RTM_NEWQDISC netlink API by absence of the + * netlink attribute but also as the special value 0x83126E97u + * (NM_PLATFORM_FQ_CODEL_CE_THRESHOLD_DISABLED). + * Beware: zero is not the default you must always explicitly set this value. */ guint32 memory; /* TCA_FQ_CODEL_MEMORY_LIMIT: note that only values <= 2^31 are accepted by kernel * and kernel defaults to 32MB. * Note that we use the special value NM_PLATFORM_FQ_CODEL_MEMORY_LIMIT_UNSET