From: Tonghao Zhang <redacted>
This patch fix issue:
* If we install tc filters with act_skbedit in clsact hook.
It doesn't work, because *netdev_core_pick_tx will overwrite
queue_mapping.
$ tc filter add dev $NETDEV egress .. action skbedit queue_mapping 1
And this patch is useful:
* In containter networking environment, one kind of pod/containter/
net-namespace (e.g. P1, P2) which outbound traffic limited, can
use one specific tx queue which used HTB/TBF Qdisc. But other kind
of pods (e.g. Pn) can use other specific tx queue too, which used fifio
Qdisc. Then the lock contention of HTB/TBF Qdisc will not affect Pn.
+----+ +----+ +----+
| P1 | | P2 | | Pn |
+----+ +----+ +----+
| | |
+-----------+-----------+
|
| clsact/skbedit
| MQ
v
+-----------+-----------+
| q0 | q1 | qn
v v v
HTB HTB ... FIFO
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <redacted>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jonathan Lemon <redacted>
Cc: Eric Dumazet <redacted>
Cc: Alexander Lobakin <redacted>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Talal Ahmad <redacted>
Cc: Kevin Hao <redacted>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Kees Cook <redacted>
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: Antoine Tenart <atenart@kernel.org>
Cc: Wei Wang <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Tonghao Zhang <redacted>
---
include/linux/skbuff.h | 1 +
net/core/dev.c | 12 +++++++++---
net/sched/act_skbedit.c | 4 +++-
3 files changed, 13 insertions(+), 4 deletions(-)
From: Tonghao Zhang <redacted>
This patch allows users to select queue_mapping, range
from A to B. And users can use skb-hash or cgroup classid
to select queues. Then the packets can load balance from A to
B queue.
$ tc filter ... action skbedit queue_mapping hash-type normal 0 4
"skbedit queue_mapping QUEUE_MAPPING"[0] is enhanced with two flags:
SKBEDIT_F_QUEUE_MAPPING_HASH, SKBEDIT_F_QUEUE_MAPPING_CLASSID.
The range is an unsigned 8bit value in decimal format.
[0]: https://man7.org/linux/man-pages/man8/tc-skbedit.8.html
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Cong Wang <redacted>
Cc: Jiri Pirko <jiri@resnulli.us>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Jonathan Lemon <redacted>
Cc: Eric Dumazet <redacted>
Cc: Alexander Lobakin <redacted>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Talal Ahmad <redacted>
Cc: Kevin Hao <redacted>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Kees Cook <redacted>
Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Cc: Antoine Tenart <atenart@kernel.org>
Cc: Wei Wang <redacted>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Tonghao Zhang <redacted>
---
include/net/tc_act/tc_skbedit.h | 1 +
include/uapi/linux/tc_act/tc_skbedit.h | 5 +++
net/sched/act_skbedit.c | 48 +++++++++++++++++++++++---
3 files changed, 50 insertions(+), 4 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-12-06 20:40:07
On Mon, 6 Dec 2021 16:05:11 +0800 xiangxia.m.yue@gmail.com wrote:
+----+ +----+ +----+
| P1 | | P2 | | Pn |
+----+ +----+ +----+
| | |
+-----------+-----------+
|
| clsact/skbedit
| MQ
v
+-----------+-----------+
| q0 | q1 | qn
v v v
HTB HTB ... FIFO
The usual suggestion these days is to try to use FQ + EDT to
implement efficient policies. You don't need dedicated qdiscs,
just modulate transmission time appropriately on egress of the
container.
In general recording the decision in the skb seems a little heavy
handed. We just need to carry the information from the egress hook
to the queue selection a few lines below. Or in fact maybe egress
hook shouldn't be used for this in the first place, and we need
a more appropriate root qdisc than simple mq?
Not sure. What I am sure of is that you need to fix these warnings:
include/linux/skbuff.h:937: warning: Function parameter or member 'tc_skip_txqueue' not described in 'sk_buff'
ERROR: spaces required around that '=' (ctx:VxW)
#103: FILE: net/sched/act_skbedit.c:42:
+ queue_mapping= (queue_mapping & 0xff) + hash % mapping_mod;
^
;)
On Tue, Dec 7, 2021 at 4:40 AM Jakub Kicinski [off-list ref] wrote:
On Mon, 6 Dec 2021 16:05:11 +0800 xiangxia.m.yue@gmail.com wrote:
quoted
+----+ +----+ +----+
| P1 | | P2 | | Pn |
+----+ +----+ +----+
| | |
+-----------+-----------+
|
| clsact/skbedit
| MQ
v
+-----------+-----------+
| q0 | q1 | qn
v v v
HTB HTB ... FIFO
Hi Jakub, thanks for your comments
The usual suggestion these days is to try to use FQ + EDT to
implement efficient policies. You don't need dedicated qdiscs,
just modulate transmission time appropriately on egress of the
container.
FQ+EDT is good solution. But this patch should be used on another scenario.
1. the containers which outbound traffic is not limited, want to use
the fifo qdisc.
If this traffic share the FQ/HTB Qdisc, the qdisc lock will affect the
performance and latency.
2. we can support user to select tx queue, range from A to B. skb hash
or cgroup classid is good to do load balance.
patch 2/2: https://patchwork.kernel.org/project/netdevbpf/patch/20211206080512.36610-3-xiangxia.m.yue@gmail.com/
In general recording the decision in the skb seems a little heavy
handed. We just need to carry the information from the egress hook
to the queue selection a few lines below. Or in fact maybe egress
Yes, we can refactor netdev_core_pick_tx to
1. select queue_index and invoke skb_set_queue_mapping, but don't
return the txq.
2. after egress hook, use skb_get_queue_mapping/netdev_get_tx_queue to get txq.
hook shouldn't be used for this in the first place, and we need
a more appropriate root qdisc than simple mq?
I have no idea about mq, I think clsact may make the things more flexible.
and act_bpf can also support to change sk queue_mapping. queue_mapping
was included in __sk_buff.
Not sure. What I am sure of is that you need to fix these warnings:
Ok
include/linux/skbuff.h:937: warning: Function parameter or member 'tc_skip_txqueue' not described in 'sk_buff'
ERROR: spaces required around that '=' (ctx:VxW)
#103: FILE: net/sched/act_skbedit.c:42:
+ queue_mapping= (queue_mapping & 0xff) + hash % mapping_mod;
^
;)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-12-07 02:33:07
On Tue, 7 Dec 2021 10:10:22 +0800 Tonghao Zhang wrote:
quoted
In general recording the decision in the skb seems a little heavy
handed. We just need to carry the information from the egress hook
to the queue selection a few lines below. Or in fact maybe egress
Yes, we can refactor netdev_core_pick_tx to
1. select queue_index and invoke skb_set_queue_mapping, but don't
return the txq.
2. after egress hook, use skb_get_queue_mapping/netdev_get_tx_queue to get txq.
I'm not sure that's what I meant, I meant the information you need to
store does not need to be stored in the skb, you can pass a pointer to
a stack variable to both egress handling and pick_tx.
quoted
hook shouldn't be used for this in the first place, and we need
a more appropriate root qdisc than simple mq?
I have no idea about mq, I think clsact may make the things more flexible.
and act_bpf can also support to change sk queue_mapping. queue_mapping
was included in __sk_buff.
Qdiscs can run a classifier to select a sub-queue. The advantage of
the classifier run by the Qdisc is that it runs after pick_tx.
On Tue, Dec 7, 2021 at 10:33 AM Jakub Kicinski [off-list ref] wrote:
On Tue, 7 Dec 2021 10:10:22 +0800 Tonghao Zhang wrote:
quoted
quoted
In general recording the decision in the skb seems a little heavy
handed. We just need to carry the information from the egress hook
to the queue selection a few lines below. Or in fact maybe egress
Yes, we can refactor netdev_core_pick_tx to
1. select queue_index and invoke skb_set_queue_mapping, but don't
return the txq.
2. after egress hook, use skb_get_queue_mapping/netdev_get_tx_queue to get txq.
I'm not sure that's what I meant, I meant the information you need to
store does not need to be stored in the skb, you can pass a pointer to
a stack variable to both egress handling and pick_tx.
Thanks, I got it. I think we store the txq index in skb->queue_mapping
better. because in egress hook,
act_skbedit/act_bpf can change the skb queue_mapping. Then we can
pick_tx depending on queue_mapping.
quoted
quoted
hook shouldn't be used for this in the first place, and we need
a more appropriate root qdisc than simple mq?
I have no idea about mq, I think clsact may make the things more flexible.
and act_bpf can also support to change sk queue_mapping. queue_mapping
was included in __sk_buff.
Qdiscs can run a classifier to select a sub-queue. The advantage of
the classifier run by the Qdisc is that it runs after pick_tx.
Yes, we should consider the qdisc lock too. Qdisc lock may affect
performance and latency when running a classifier in Qdisc
and clsact is outside of qdisc.
--
Best regards, Tonghao
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-12-07 15:45:04
On Tue, 7 Dec 2021 11:22:28 +0800 Tonghao Zhang wrote:
On Tue, Dec 7, 2021 at 10:33 AM Jakub Kicinski [off-list ref] wrote:
quoted
On Tue, 7 Dec 2021 10:10:22 +0800 Tonghao Zhang wrote:
quoted
Yes, we can refactor netdev_core_pick_tx to
1. select queue_index and invoke skb_set_queue_mapping, but don't
return the txq.
2. after egress hook, use skb_get_queue_mapping/netdev_get_tx_queue to get txq.
I'm not sure that's what I meant, I meant the information you need to
store does not need to be stored in the skb, you can pass a pointer to
a stack variable to both egress handling and pick_tx.
Thanks, I got it. I think we store the txq index in skb->queue_mapping
better. because in egress hook,
act_skbedit/act_bpf can change the skb queue_mapping. Then we can
pick_tx depending on queue_mapping.
Actually Eric pointed out in another thread that xmit_more() is now
done via a per-CPU variable, you can try that instead of plumbing a
variable all the way into actions and back out to pick_tx().
Please make sure to include the analysis of the performance impact
when the feature is _not_ used in the next version.
quoted
quoted
I have no idea about mq, I think clsact may make the things more flexible.
and act_bpf can also support to change sk queue_mapping. queue_mapping
was included in __sk_buff.
Qdiscs can run a classifier to select a sub-queue. The advantage of
the classifier run by the Qdisc is that it runs after pick_tx.
Yes, we should consider the qdisc lock too. Qdisc lock may affect
performance and latency when running a classifier in Qdisc
and clsact is outside of qdisc.
On Tue, Dec 7, 2021 at 11:45 PM Jakub Kicinski [off-list ref] wrote:
On Tue, 7 Dec 2021 11:22:28 +0800 Tonghao Zhang wrote:
quoted
On Tue, Dec 7, 2021 at 10:33 AM Jakub Kicinski [off-list ref] wrote:
quoted
On Tue, 7 Dec 2021 10:10:22 +0800 Tonghao Zhang wrote:
quoted
Yes, we can refactor netdev_core_pick_tx to
1. select queue_index and invoke skb_set_queue_mapping, but don't
return the txq.
2. after egress hook, use skb_get_queue_mapping/netdev_get_tx_queue to get txq.
I'm not sure that's what I meant, I meant the information you need to
store does not need to be stored in the skb, you can pass a pointer to
a stack variable to both egress handling and pick_tx.
Thanks, I got it. I think we store the txq index in skb->queue_mapping
better. because in egress hook,
act_skbedit/act_bpf can change the skb queue_mapping. Then we can
pick_tx depending on queue_mapping.
Actually Eric pointed out in another thread that xmit_more() is now
done via a per-CPU variable, you can try that instead of plumbing a
variable all the way into actions and back out to pick_tx().
Please make sure to include the analysis of the performance impact
when the feature is _not_ used in the next version.
Ok, I updated the commit message. Thanks!
quoted
quoted
quoted
I have no idea about mq, I think clsact may make the things more flexible.
and act_bpf can also support to change sk queue_mapping. queue_mapping
was included in __sk_buff.
Qdiscs can run a classifier to select a sub-queue. The advantage of
the classifier run by the Qdisc is that it runs after pick_tx.
Yes, we should consider the qdisc lock too. Qdisc lock may affect
performance and latency when running a classifier in Qdisc
and clsact is outside of qdisc.