diff mbox series

[net-next] fq_codel: generalise ce_threshold marking for subset of traffic

Message ID 20211019171534.66628-1-toke@redhat.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] fq_codel: generalise ce_threshold marking for subset of traffic | expand

Checks

Context Check Description
netdev/cover_letter success Single patches do not need cover letters
netdev/fixes_present success Fixes tag not required for -next series
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 5 maintainers not CCed: linux-wireless@vger.kernel.org xiyou.wangcong@gmail.com jhs@mojatatu.com johannes@sipsolutions.net jiri@resnulli.us
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 4806 this patch: 4806
netdev/kdoc success Errors and warnings before: 2 this patch: 2
netdev/verify_fixes success Fixes tag looks correct
netdev/checkpatch warning WARNING: line length of 102 exceeds 80 columns WARNING: line length of 106 exceeds 80 columns WARNING: line length of 94 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 4872 this patch: 4872
netdev/header_inline success No static functions without inline keyword in header files

Commit Message

Toke Høiland-Jørgensen Oct. 19, 2021, 5:15 p.m. UTC
The commit in the Fixes tag expanded the ce_threshold feature of FQ-CoDel
so it can be applied to a subset of the traffic, using the ECT(1) bit of
the ECN field as the classifier. However, hard-coding ECT(1) as the only
classifier for this feature seems limiting, so let's expand it to be more
general.

To this end, change the parameter from a ce_threshold_ect1 boolean, to a
one-byte selector/mask pair (ce_threshold_{selector,mask}) which is applied
to the whole diffserv/ECN field in the IP header. This makes it possible to
classify packets by any value in either the ECN field or the diffserv
field. In particular, setting a selector of INET_ECN_ECT_1 and a mask of
INET_ECN_MASK corresponds to the functionality before this patch, and a
mask of ~INET_ECN_MASK allows using the selector as a straight-forward
match against a diffserv code point.

Regardless of the selector chosen, the normal rules for ECN-marking of
packets still apply, i.e., the flow must still declare itself ECN-capable
by setting one of the bits in the ECN field to get marked at all.

Fixes: e72aeb9ee0e3 ("fq_codel: implement L4S style ce_threshold_ect1 marking")
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/net/codel.h            |  7 +++++--
 include/net/codel_impl.h       | 14 +++++++-------
 include/uapi/linux/pkt_sched.h |  3 ++-
 net/mac80211/sta_info.c        |  3 ++-
 net/sched/sch_fq_codel.c       | 13 +++++++++----
 5 files changed, 25 insertions(+), 15 deletions(-)

Comments

Eric Dumazet Oct. 19, 2021, 5:25 p.m. UTC | #1
On Tue, Oct 19, 2021 at 10:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> The commit in the Fixes tag expanded the ce_threshold feature of FQ-CoDel
> so it can be applied to a subset of the traffic, using the ECT(1) bit of
> the ECN field as the classifier. However, hard-coding ECT(1) as the only
> classifier for this feature seems limiting, so let's expand it to be more
> general.
>
> To this end, change the parameter from a ce_threshold_ect1 boolean, to a
> one-byte selector/mask pair (ce_threshold_{selector,mask}) which is applied
> to the whole diffserv/ECN field in the IP header. This makes it possible to
> classify packets by any value in either the ECN field or the diffserv
> field. In particular, setting a selector of INET_ECN_ECT_1 and a mask of
> INET_ECN_MASK corresponds to the functionality before this patch, and a
> mask of ~INET_ECN_MASK allows using the selector as a straight-forward
> match against a diffserv code point.

Please include what command line should be used once we get iproute2 support.

Thanks !

>
> Regardless of the selector chosen, the normal rules for ECN-marking of
> packets still apply, i.e., the flow must still declare itself ECN-capable
> by setting one of the bits in the ECN field to get marked at all.
>
> Fixes: e72aeb9ee0e3 ("fq_codel: implement L4S style ce_threshold_ect1 marking")
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
Toke Høiland-Jørgensen Oct. 19, 2021, 5:35 p.m. UTC | #2
Eric Dumazet <edumazet@google.com> writes:

> On Tue, Oct 19, 2021 at 10:17 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> The commit in the Fixes tag expanded the ce_threshold feature of FQ-CoDel
>> so it can be applied to a subset of the traffic, using the ECT(1) bit of
>> the ECN field as the classifier. However, hard-coding ECT(1) as the only
>> classifier for this feature seems limiting, so let's expand it to be more
>> general.
>>
>> To this end, change the parameter from a ce_threshold_ect1 boolean, to a
>> one-byte selector/mask pair (ce_threshold_{selector,mask}) which is applied
>> to the whole diffserv/ECN field in the IP header. This makes it possible to
>> classify packets by any value in either the ECN field or the diffserv
>> field. In particular, setting a selector of INET_ECN_ECT_1 and a mask of
>> INET_ECN_MASK corresponds to the functionality before this patch, and a
>> mask of ~INET_ECN_MASK allows using the selector as a straight-forward
>> match against a diffserv code point.
>
> Please include what command line should be used once we get iproute2
> support.

Sure, will add that and send a v2

-Toke
diff mbox series

Patch

diff --git a/include/net/codel.h b/include/net/codel.h
index 5e8b181b76b8..a6c9e34e62b8 100644
--- a/include/net/codel.h
+++ b/include/net/codel.h
@@ -102,7 +102,9 @@  static inline u32 codel_time_to_us(codel_time_t val)
  * @interval:	width of moving time window
  * @mtu:	device mtu, or minimal queue backlog in bytes.
  * @ecn:	is Explicit Congestion Notification enabled
- * @ce_threshold_ect1: if ce_threshold only marks ECT(1) packets
+ * @ce_threshold_selector: apply ce_threshold to packets matching this value
+ *                         in the diffserv/ECN byte of the IP header
+ * @ce_threshold_mask: mask to apply to ce_threshold_selector comparison
  */
 struct codel_params {
 	codel_time_t	target;
@@ -110,7 +112,8 @@  struct codel_params {
 	codel_time_t	interval;
 	u32		mtu;
 	bool		ecn;
-	bool		ce_threshold_ect1;
+	u8		ce_threshold_selector;
+	u8		ce_threshold_mask;
 };
 
 /**
diff --git a/include/net/codel_impl.h b/include/net/codel_impl.h
index 7af2c3eb3c43..137d40d8cbeb 100644
--- a/include/net/codel_impl.h
+++ b/include/net/codel_impl.h
@@ -54,7 +54,8 @@  static void codel_params_init(struct codel_params *params)
 	params->interval = MS2TIME(100);
 	params->target = MS2TIME(5);
 	params->ce_threshold = CODEL_DISABLED_THRESHOLD;
-	params->ce_threshold_ect1 = false;
+	params->ce_threshold_mask = 0;
+	params->ce_threshold_selector = 0;
 	params->ecn = false;
 }
 
@@ -250,13 +251,12 @@  static struct sk_buff *codel_dequeue(void *ctx,
 	if (skb && codel_time_after(vars->ldelay, params->ce_threshold)) {
 		bool set_ce = true;
 
-		if (params->ce_threshold_ect1) {
-			/* Note: if skb_get_dsfield() returns -1, following
-			 * gives INET_ECN_MASK, which is != INET_ECN_ECT_1.
-			 */
-			u8 ecn = skb_get_dsfield(skb) & INET_ECN_MASK;
+		if (params->ce_threshold_mask) {
+			int dsfield = skb_get_dsfield(skb);
 
-			set_ce = (ecn == INET_ECN_ECT_1);
+			set_ce = (dsfield >= 0 &&
+				  (((u8)dsfield & params->ce_threshold_mask) ==
+				   params->ce_threshold_selector));
 		}
 		if (set_ce && INET_ECN_set_ce(skb))
 			stats->ce_mark++;
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h
index 6be9a84cccfa..f292b467b27f 100644
--- a/include/uapi/linux/pkt_sched.h
+++ b/include/uapi/linux/pkt_sched.h
@@ -840,7 +840,8 @@  enum {
 	TCA_FQ_CODEL_CE_THRESHOLD,
 	TCA_FQ_CODEL_DROP_BATCH_SIZE,
 	TCA_FQ_CODEL_MEMORY_LIMIT,
-	TCA_FQ_CODEL_CE_THRESHOLD_ECT1,
+	TCA_FQ_CODEL_CE_THRESHOLD_SELECTOR,
+	TCA_FQ_CODEL_CE_THRESHOLD_MASK,
 	__TCA_FQ_CODEL_MAX
 };
 
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index a39830418434..bd52ac3bee90 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -513,7 +513,8 @@  struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 	sta->cparams.target = MS2TIME(20);
 	sta->cparams.interval = MS2TIME(100);
 	sta->cparams.ecn = true;
-	sta->cparams.ce_threshold_ect1 = false;
+	sta->cparams.ce_threshold_selector = 0;
+	sta->cparams.ce_threshold_mask = 0;
 
 	sta_dbg(sdata, "Allocated STA %pM\n", sta->sta.addr);
 
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 033d65d06eb1..839e1235db05 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -362,7 +362,8 @@  static const struct nla_policy fq_codel_policy[TCA_FQ_CODEL_MAX + 1] = {
 	[TCA_FQ_CODEL_CE_THRESHOLD] = { .type = NLA_U32 },
 	[TCA_FQ_CODEL_DROP_BATCH_SIZE] = { .type = NLA_U32 },
 	[TCA_FQ_CODEL_MEMORY_LIMIT] = { .type = NLA_U32 },
-	[TCA_FQ_CODEL_CE_THRESHOLD_ECT1] = { .type = NLA_U8 },
+	[TCA_FQ_CODEL_CE_THRESHOLD_SELECTOR] = { .type = NLA_U8 },
+	[TCA_FQ_CODEL_CE_THRESHOLD_MASK] = { .type = NLA_U8 },
 };
 
 static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
@@ -409,8 +410,10 @@  static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt,
 		q->cparams.ce_threshold = (val * NSEC_PER_USEC) >> CODEL_SHIFT;
 	}
 
-	if (tb[TCA_FQ_CODEL_CE_THRESHOLD_ECT1])
-		q->cparams.ce_threshold_ect1 = !!nla_get_u8(tb[TCA_FQ_CODEL_CE_THRESHOLD_ECT1]);
+	if (tb[TCA_FQ_CODEL_CE_THRESHOLD_SELECTOR])
+		q->cparams.ce_threshold_selector = nla_get_u8(tb[TCA_FQ_CODEL_CE_THRESHOLD_SELECTOR]);
+	if (tb[TCA_FQ_CODEL_CE_THRESHOLD_MASK])
+		q->cparams.ce_threshold_mask = nla_get_u8(tb[TCA_FQ_CODEL_CE_THRESHOLD_MASK]);
 
 	if (tb[TCA_FQ_CODEL_INTERVAL]) {
 		u64 interval = nla_get_u32(tb[TCA_FQ_CODEL_INTERVAL]);
@@ -552,7 +555,9 @@  static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
 		if (nla_put_u32(skb, TCA_FQ_CODEL_CE_THRESHOLD,
 				codel_time_to_us(q->cparams.ce_threshold)))
 			goto nla_put_failure;
-		if (nla_put_u8(skb, TCA_FQ_CODEL_CE_THRESHOLD_ECT1, q->cparams.ce_threshold_ect1))
+		if (nla_put_u8(skb, TCA_FQ_CODEL_CE_THRESHOLD_SELECTOR, q->cparams.ce_threshold_selector))
+			goto nla_put_failure;
+		if (nla_put_u8(skb, TCA_FQ_CODEL_CE_THRESHOLD_MASK, q->cparams.ce_threshold_mask))
 			goto nla_put_failure;
 	}