* [net-next v5 0/2] net: sched: allow user to select txqueue @ 2021-12-20 12:38 xiangxia.m.yue 2021-12-20 12:38 ` [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue xiangxia.m.yue 2021-12-20 12:38 ` [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting " xiangxia.m.yue 0 siblings, 2 replies; 18+ messages in thread From: xiangxia.m.yue @ 2021-12-20 12:38 UTC (permalink / raw) To: netdev Cc: Tonghao Zhang, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann From: Tonghao Zhang <xiangxia.m.yue@gmail.com> Patch 1 allow user to select txqueue in clsact hook. Patch 2 support skbhash, classid, cpuid to select txqueue. Tonghao Zhang (2): net: sched: use queue_mapping to pick tx queue net: sched: support hash/classid/cpuid selecting tx queue include/linux/netdevice.h | 19 ++++++ include/net/tc_act/tc_skbedit.h | 1 + include/uapi/linux/tc_act/tc_skbedit.h | 8 +++ net/core/dev.c | 7 ++- net/sched/act_skbedit.c | 83 ++++++++++++++++++++++++-- 5 files changed, 112 insertions(+), 6 deletions(-) -- v5: * 1/2 merge netdev_xmit_reset_txqueue(void), netdev_xmit_skip_txqueue(void), to netdev_xmit_skip_txqueue(bool skip). v4: * 1/2 introduce netdev_xmit_reset_txqueue() and invoked in __dev_queue_xmit(), so ximt.skip_txqueue will not affect selecting tx queue in next netdev, or next packets. more details, see commit log. * 2/2 fix the coding style, rename: SKBEDIT_F_QUEUE_MAPPING_HASH -> SKBEDIT_F_TXQ_SKBHASH SKBEDIT_F_QUEUE_MAPPING_CLASSID -> SKBEDIT_F_TXQ_CLASSID SKBEDIT_F_QUEUE_MAPPING_CPUID -> SKBEDIT_F_TXQ_CPUID * 2/2 refactor tcf_skbedit_hash, if type of hash is not specified, use the queue_mapping, because hash % mapping_mod == 0 in "case 0:" * 2/2 merge the check and add extack v3: * 2/2 fix the warning, add cpuid hash type. v2: * 1/2 change skb->tc_skip_txqueue to per-cpu var, add more commit message. * 2/2 optmize the codes. Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Jiri Pirko <jiri@resnulli.us> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Jonathan Lemon <jonathan.lemon@gmail.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Alexander Lobakin <alobakin@pm.me> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Talal Ahmad <talalahmad@google.com> Cc: Kevin Hao <haokexin@gmail.com> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> Cc: Kees Cook <keescook@chromium.org> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> Cc: Antoine Tenart <atenart@kernel.org> Cc: Wei Wang <weiwan@google.com> Cc: Arnd Bergmann <arnd@arndb.de> -- 2.27.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-20 12:38 [net-next v5 0/2] net: sched: allow user to select txqueue xiangxia.m.yue @ 2021-12-20 12:38 ` xiangxia.m.yue 2021-12-20 13:57 ` Eric Dumazet ` (2 more replies) 2021-12-20 12:38 ` [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting " xiangxia.m.yue 1 sibling, 3 replies; 18+ messages in thread From: xiangxia.m.yue @ 2021-12-20 12:38 UTC (permalink / raw) To: netdev Cc: Tonghao Zhang, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann From: Tonghao Zhang <xiangxia.m.yue@gmail.com> This patch fixes issue: * If we install tc filters with act_skbedit in clsact hook. It doesn't work, because netdev_core_pick_tx() overwrites queue_mapping. $ tc filter ... action skbedit queue_mapping 1 And this patch is useful: * We can use FQ + EDT to implement efficient policies. Tx queues are picked by xps, ndo_select_queue of netdev driver, or skb hash in netdev_core_pick_tx(). In fact, the netdev driver, and skb hash are _not_ under control. xps uses the CPUs map to select Tx queues, but we can't figure out which task_struct of pod/containter running on this cpu in most case. We can use clsact filters to classify one pod/container traffic to one Tx queue. Why ? In containter networking environment, there are two kinds of pod/ containter/net-namespace. One kind (e.g. P1, P2), the high throughput is key in these applications. But avoid running out of network resource, the outbound traffic of these pods is limited, using or sharing one dedicated Tx queues assigned HTB/TBF/FQ Qdisc. Other kind of pods (e.g. Pn), the low latency of data access is key. And the traffic is not limited. Pods use or share other dedicated Tx queues assigned FIFO Qdisc. This choice provides two benefits. First, contention on the HTB/FQ Qdisc lock is significantly reduced since fewer CPUs contend for the same queue. More importantly, Qdisc contention can be eliminated completely if each CPU has its own FIFO Qdisc for the second kind of pods. There must be a mechanism in place to support classifying traffic based on pods/container to different Tx queues. Note that clsact is outside of Qdisc while Qdisc can run a classifier to select a sub-queue under the lock. In general recording the decision in the skb seems a little heavy handed. This patch introduces a per-CPU variable, suggested by Eric. The xmit.skip_txqueue flag is firstly cleared in __dev_queue_xmit(). - Tx Qdisc may install that skbedit actions, then xmit.skip_txqueue flag is set in qdisc->enqueue() though tx queue has been selected in netdev_tx_queue_mapping() or netdev_core_pick_tx(). That flag is cleared firstly in __dev_queue_xmit(), is useful: - Avoid picking Tx queue with netdev_tx_queue_mapping() in next netdev in such case: eth0 macvlan - eth0.3 vlan - eth0 ixgbe-phy: For example, eth0, macvlan in pod, which root Qdisc install skbedit queue_mapping, send packets to eth0.3, vlan in host. In __dev_queue_xmit() of eth0.3, clear the flag, does not select tx queue according to skb->queue_mapping because there is no filters in clsact or tx Qdisc of this netdev. Same action taked in eth0, ixgbe in Host. - Avoid picking Tx queue for next packet. If we set xmit.skip_txqueue in tx Qdisc (qdisc->enqueue()), the proper way to clear it is clearing it in __dev_queue_xmit when processing next packets. +----+ +----+ +----+ | P1 | | P2 | | Pn | +----+ +----+ +----+ | | | +-----------+-----------+ | | clsact/skbedit | MQ v +-----------+-----------+ | q0 | q1 | qn v v v HTB/FQ HTB/FQ ... FIFO Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Jiri Pirko <jiri@resnulli.us> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Jonathan Lemon <jonathan.lemon@gmail.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Alexander Lobakin <alobakin@pm.me> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Talal Ahmad <talalahmad@google.com> Cc: Kevin Hao <haokexin@gmail.com> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> Cc: Kees Cook <keescook@chromium.org> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> Cc: Antoine Tenart <atenart@kernel.org> Cc: Wei Wang <weiwan@google.com> Cc: Arnd Bergmann <arnd@arndb.de> Suggested-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> --- include/linux/netdevice.h | 19 +++++++++++++++++++ net/core/dev.c | 7 ++++++- net/sched/act_skbedit.c | 4 +++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 8b0bdeb4734e..8d02dafb32ba 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3009,6 +3009,7 @@ struct softnet_data { /* written and read only by owning cpu: */ struct { u16 recursion; + u8 skip_txqueue; u8 more; } xmit; #ifdef CONFIG_RPS @@ -4696,6 +4697,24 @@ static inline netdev_tx_t netdev_start_xmit(struct sk_buff *skb, struct net_devi return rc; } +static inline void netdev_xmit_skip_txqueue(bool skip) +{ + __this_cpu_write(softnet_data.xmit.skip_txqueue, skip); +} + +static inline bool netdev_xmit_txqueue_skipped(void) +{ + return __this_cpu_read(softnet_data.xmit.skip_txqueue); +} + +static inline struct netdev_queue * +netdev_tx_queue_mapping(struct net_device *dev, struct sk_buff *skb) +{ + int qm = skb_get_queue_mapping(skb); + + return netdev_get_tx_queue(dev, netdev_cap_txqueue(dev, qm)); +} + int netdev_class_create_file_ns(const struct class_attribute *class_attr, const void *ns); void netdev_class_remove_file_ns(const struct class_attribute *class_attr, diff --git a/net/core/dev.c b/net/core/dev.c index a855e41bbe39..e3f548c54dda 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -4048,6 +4048,7 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) skb_update_prio(skb); qdisc_pkt_len_init(skb); + netdev_xmit_skip_txqueue(false); #ifdef CONFIG_NET_CLS_ACT skb->tc_at_ingress = 0; #endif @@ -4073,7 +4074,11 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) else skb_dst_force(skb); - txq = netdev_core_pick_tx(dev, skb, sb_dev); + if (netdev_xmit_txqueue_skipped()) + txq = netdev_tx_queue_mapping(dev, skb); + else + txq = netdev_core_pick_tx(dev, skb, sb_dev); + q = rcu_dereference_bh(txq->qdisc); trace_net_dev_queue(skb); diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c index ceba11b198bb..48504ed3b280 100644 --- a/net/sched/act_skbedit.c +++ b/net/sched/act_skbedit.c @@ -58,8 +58,10 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a, } } if (params->flags & SKBEDIT_F_QUEUE_MAPPING && - skb->dev->real_num_tx_queues > params->queue_mapping) + skb->dev->real_num_tx_queues > params->queue_mapping) { + netdev_xmit_skip_txqueue(true); skb_set_queue_mapping(skb, params->queue_mapping); + } if (params->flags & SKBEDIT_F_MARK) { skb->mark &= ~params->mask; skb->mark |= params->mark & params->mask; -- 2.27.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-20 12:38 ` [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue xiangxia.m.yue @ 2021-12-20 13:57 ` Eric Dumazet 2021-12-20 14:21 ` Tonghao Zhang 2021-12-20 17:51 ` Cong Wang 2021-12-29 19:13 ` Vinicius Costa Gomes 2 siblings, 1 reply; 18+ messages in thread From: Eric Dumazet @ 2021-12-20 13:57 UTC (permalink / raw) To: Tonghao Zhang Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Mon, Dec 20, 2021 at 4:38 AM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > This patch fixes issue: > * If we install tc filters with act_skbedit in clsact hook. > It doesn't work, because netdev_core_pick_tx() overwrites > queue_mapping. > > $ tc filter ... action skbedit queue_mapping 1 > > And this patch is useful: > * We can use FQ + EDT to implement efficient policies. Tx queues > are picked by xps, ndo_select_queue of netdev driver, or skb hash > in netdev_core_pick_tx(). In fact, the netdev driver, and skb > hash are _not_ under control. xps uses the CPUs map to select Tx > queues, but we can't figure out which task_struct of pod/containter > running on this cpu in most case. We can use clsact filters to classify > one pod/container traffic to one Tx queue. Why ? > > In containter networking environment, there are two kinds of pod/ > containter/net-namespace. One kind (e.g. P1, P2), the high throughput > is key in these applications. But avoid running out of network resource, > the outbound traffic of these pods is limited, using or sharing one > dedicated Tx queues assigned HTB/TBF/FQ Qdisc. Other kind of pods > (e.g. Pn), the low latency of data access is key. And the traffic is not > limited. Pods use or share other dedicated Tx queues assigned FIFO Qdisc. > This choice provides two benefits. First, contention on the HTB/FQ Qdisc > lock is significantly reduced since fewer CPUs contend for the same queue. > More importantly, Qdisc contention can be eliminated completely if each > CPU has its own FIFO Qdisc for the second kind of pods. > > There must be a mechanism in place to support classifying traffic based on > pods/container to different Tx queues. Note that clsact is outside of Qdisc > while Qdisc can run a classifier to select a sub-queue under the lock. > > In general recording the decision in the skb seems a little heavy handed. > This patch introduces a per-CPU variable, suggested by Eric. > > The xmit.skip_txqueue flag is firstly cleared in __dev_queue_xmit(). > - Tx Qdisc may install that skbedit actions, then xmit.skip_txqueue flag > is set in qdisc->enqueue() though tx queue has been selected in > netdev_tx_queue_mapping() or netdev_core_pick_tx(). That flag is cleared > firstly in __dev_queue_xmit(), is useful: > - Avoid picking Tx queue with netdev_tx_queue_mapping() in next netdev > in such case: eth0 macvlan - eth0.3 vlan - eth0 ixgbe-phy: > For example, eth0, macvlan in pod, which root Qdisc install skbedit > queue_mapping, send packets to eth0.3, vlan in host. In __dev_queue_xmit() of > eth0.3, clear the flag, does not select tx queue according to skb->queue_mapping > because there is no filters in clsact or tx Qdisc of this netdev. > Same action taked in eth0, ixgbe in Host. > - Avoid picking Tx queue for next packet. If we set xmit.skip_txqueue > in tx Qdisc (qdisc->enqueue()), the proper way to clear it is clearing it > in __dev_queue_xmit when processing next packets. > > +----+ +----+ +----+ > | P1 | | P2 | | Pn | > +----+ +----+ +----+ > | | | > +-----------+-----------+ > | > | clsact/skbedit > | MQ > v > +-----------+-----------+ > | q0 | q1 | qn > v v v > HTB/FQ HTB/FQ ... FIFO > > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > Cc: Cong Wang <xiyou.wangcong@gmail.com> > Cc: Jiri Pirko <jiri@resnulli.us> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Jonathan Lemon <jonathan.lemon@gmail.com> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Alexander Lobakin <alobakin@pm.me> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Talal Ahmad <talalahmad@google.com> > Cc: Kevin Hao <haokexin@gmail.com> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Cc: Kees Cook <keescook@chromium.org> > Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > Cc: Antoine Tenart <atenart@kernel.org> > Cc: Wei Wang <weiwan@google.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Suggested-by: Eric Dumazet <edumazet@google.com> I have not suggested this patch, only to not add yet another bit in sk_buff. > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > --- > include/linux/netdevice.h | 19 +++++++++++++++++++ > net/core/dev.c | 7 ++++++- > net/sched/act_skbedit.c | 4 +++- > 3 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 8b0bdeb4734e..8d02dafb32ba 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -3009,6 +3009,7 @@ struct softnet_data { > /* written and read only by owning cpu: */ > struct { > u16 recursion; > + u8 skip_txqueue; > u8 more; > } xmit; > #ifdef CONFIG_RPS > @@ -4696,6 +4697,24 @@ static inline netdev_tx_t netdev_start_xmit(struct sk_buff *skb, struct net_devi > return rc; > } > > +static inline void netdev_xmit_skip_txqueue(bool skip) > +{ > + __this_cpu_write(softnet_data.xmit.skip_txqueue, skip); > +} > + > +static inline bool netdev_xmit_txqueue_skipped(void) > +{ > + return __this_cpu_read(softnet_data.xmit.skip_txqueue); > +} > + > +static inline struct netdev_queue * > +netdev_tx_queue_mapping(struct net_device *dev, struct sk_buff *skb) > +{ > + int qm = skb_get_queue_mapping(skb); > + > + return netdev_get_tx_queue(dev, netdev_cap_txqueue(dev, qm)); > +} > + > int netdev_class_create_file_ns(const struct class_attribute *class_attr, > const void *ns); > void netdev_class_remove_file_ns(const struct class_attribute *class_attr, > diff --git a/net/core/dev.c b/net/core/dev.c > index a855e41bbe39..e3f548c54dda 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -4048,6 +4048,7 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > skb_update_prio(skb); > > qdisc_pkt_len_init(skb); > + netdev_xmit_skip_txqueue(false); > #ifdef CONFIG_NET_CLS_ACT > skb->tc_at_ingress = 0; > #endif > @@ -4073,7 +4074,11 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > else > skb_dst_force(skb); > > - txq = netdev_core_pick_tx(dev, skb, sb_dev); > + if (netdev_xmit_txqueue_skipped()) > + txq = netdev_tx_queue_mapping(dev, skb); > + else > + txq = netdev_core_pick_tx(dev, skb, sb_dev); > + If we really need to add yet another conditional in fast path, I would suggest using a static key. Only hosts where SKBEDIT_F_QUEUE_MAPPING is requested would pay the price. > q = rcu_dereference_bh(txq->qdisc); > > trace_net_dev_queue(skb); > diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c > index ceba11b198bb..48504ed3b280 100644 > --- a/net/sched/act_skbedit.c > +++ b/net/sched/act_skbedit.c > @@ -58,8 +58,10 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a, > } > } > if (params->flags & SKBEDIT_F_QUEUE_MAPPING && > - skb->dev->real_num_tx_queues > params->queue_mapping) > + skb->dev->real_num_tx_queues > params->queue_mapping) { > + netdev_xmit_skip_txqueue(true); > skb_set_queue_mapping(skb, params->queue_mapping); > + } > if (params->flags & SKBEDIT_F_MARK) { > skb->mark &= ~params->mask; > skb->mark |= params->mark & params->mask; > -- > 2.27.0 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-20 13:57 ` Eric Dumazet @ 2021-12-20 14:21 ` Tonghao Zhang 2021-12-20 14:31 ` Tonghao Zhang 0 siblings, 1 reply; 18+ messages in thread From: Tonghao Zhang @ 2021-12-20 14:21 UTC (permalink / raw) To: Eric Dumazet Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Mon, Dec 20, 2021 at 9:58 PM Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Dec 20, 2021 at 4:38 AM <xiangxia.m.yue@gmail.com> wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > This patch fixes issue: > > * If we install tc filters with act_skbedit in clsact hook. > > It doesn't work, because netdev_core_pick_tx() overwrites > > queue_mapping. > > > > $ tc filter ... action skbedit queue_mapping 1 > > > > And this patch is useful: > > * We can use FQ + EDT to implement efficient policies. Tx queues > > are picked by xps, ndo_select_queue of netdev driver, or skb hash > > in netdev_core_pick_tx(). In fact, the netdev driver, and skb > > hash are _not_ under control. xps uses the CPUs map to select Tx > > queues, but we can't figure out which task_struct of pod/containter > > running on this cpu in most case. We can use clsact filters to classify > > one pod/container traffic to one Tx queue. Why ? > > > > In containter networking environment, there are two kinds of pod/ > > containter/net-namespace. One kind (e.g. P1, P2), the high throughput > > is key in these applications. But avoid running out of network resource, > > the outbound traffic of these pods is limited, using or sharing one > > dedicated Tx queues assigned HTB/TBF/FQ Qdisc. Other kind of pods > > (e.g. Pn), the low latency of data access is key. And the traffic is not > > limited. Pods use or share other dedicated Tx queues assigned FIFO Qdisc. > > This choice provides two benefits. First, contention on the HTB/FQ Qdisc > > lock is significantly reduced since fewer CPUs contend for the same queue. > > More importantly, Qdisc contention can be eliminated completely if each > > CPU has its own FIFO Qdisc for the second kind of pods. > > > > There must be a mechanism in place to support classifying traffic based on > > pods/container to different Tx queues. Note that clsact is outside of Qdisc > > while Qdisc can run a classifier to select a sub-queue under the lock. > > > > In general recording the decision in the skb seems a little heavy handed. > > This patch introduces a per-CPU variable, suggested by Eric. > > > > The xmit.skip_txqueue flag is firstly cleared in __dev_queue_xmit(). > > - Tx Qdisc may install that skbedit actions, then xmit.skip_txqueue flag > > is set in qdisc->enqueue() though tx queue has been selected in > > netdev_tx_queue_mapping() or netdev_core_pick_tx(). That flag is cleared > > firstly in __dev_queue_xmit(), is useful: > > - Avoid picking Tx queue with netdev_tx_queue_mapping() in next netdev > > in such case: eth0 macvlan - eth0.3 vlan - eth0 ixgbe-phy: > > For example, eth0, macvlan in pod, which root Qdisc install skbedit > > queue_mapping, send packets to eth0.3, vlan in host. In __dev_queue_xmit() of > > eth0.3, clear the flag, does not select tx queue according to skb->queue_mapping > > because there is no filters in clsact or tx Qdisc of this netdev. > > Same action taked in eth0, ixgbe in Host. > > - Avoid picking Tx queue for next packet. If we set xmit.skip_txqueue > > in tx Qdisc (qdisc->enqueue()), the proper way to clear it is clearing it > > in __dev_queue_xmit when processing next packets. > > > > +----+ +----+ +----+ > > | P1 | | P2 | | Pn | > > +----+ +----+ +----+ > > | | | > > +-----------+-----------+ > > | > > | clsact/skbedit > > | MQ > > v > > +-----------+-----------+ > > | q0 | q1 | qn > > v v v > > HTB/FQ HTB/FQ ... FIFO > > > > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > > Cc: Cong Wang <xiyou.wangcong@gmail.com> > > Cc: Jiri Pirko <jiri@resnulli.us> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Jonathan Lemon <jonathan.lemon@gmail.com> > > Cc: Eric Dumazet <edumazet@google.com> > > Cc: Alexander Lobakin <alobakin@pm.me> > > Cc: Paolo Abeni <pabeni@redhat.com> > > Cc: Talal Ahmad <talalahmad@google.com> > > Cc: Kevin Hao <haokexin@gmail.com> > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > Cc: Antoine Tenart <atenart@kernel.org> > > Cc: Wei Wang <weiwan@google.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Suggested-by: Eric Dumazet <edumazet@google.com> > > I have not suggested this patch, only to not add yet another bit in sk_buff. sorry for that > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > --- > > include/linux/netdevice.h | 19 +++++++++++++++++++ > > net/core/dev.c | 7 ++++++- > > net/sched/act_skbedit.c | 4 +++- > > 3 files changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > index 8b0bdeb4734e..8d02dafb32ba 100644 > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -3009,6 +3009,7 @@ struct softnet_data { > > /* written and read only by owning cpu: */ > > struct { > > u16 recursion; > > + u8 skip_txqueue; > > u8 more; > > } xmit; > > #ifdef CONFIG_RPS > > @@ -4696,6 +4697,24 @@ static inline netdev_tx_t netdev_start_xmit(struct sk_buff *skb, struct net_devi > > return rc; > > } > > > > +static inline void netdev_xmit_skip_txqueue(bool skip) > > +{ > > + __this_cpu_write(softnet_data.xmit.skip_txqueue, skip); > > +} > > + > > +static inline bool netdev_xmit_txqueue_skipped(void) > > +{ > > + return __this_cpu_read(softnet_data.xmit.skip_txqueue); > > +} > > + > > +static inline struct netdev_queue * > > +netdev_tx_queue_mapping(struct net_device *dev, struct sk_buff *skb) > > +{ > > + int qm = skb_get_queue_mapping(skb); > > + > > + return netdev_get_tx_queue(dev, netdev_cap_txqueue(dev, qm)); > > +} > > + > > int netdev_class_create_file_ns(const struct class_attribute *class_attr, > > const void *ns); > > void netdev_class_remove_file_ns(const struct class_attribute *class_attr, > > diff --git a/net/core/dev.c b/net/core/dev.c > > index a855e41bbe39..e3f548c54dda 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -4048,6 +4048,7 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > skb_update_prio(skb); > > > > qdisc_pkt_len_init(skb); > > + netdev_xmit_skip_txqueue(false); > > #ifdef CONFIG_NET_CLS_ACT > > skb->tc_at_ingress = 0; > > #endif > > @@ -4073,7 +4074,11 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > else > > skb_dst_force(skb); > > > > - txq = netdev_core_pick_tx(dev, skb, sb_dev); > > + if (netdev_xmit_txqueue_skipped()) > > + txq = netdev_tx_queue_mapping(dev, skb); > > + else > > + txq = netdev_core_pick_tx(dev, skb, sb_dev); > > + > > If we really need to add yet another conditional in fast path, I would > suggest using a static key. Thanks, I will add a static key for this patch. > Only hosts where SKBEDIT_F_QUEUE_MAPPING is requested would pay the price. > > > > q = rcu_dereference_bh(txq->qdisc); > > > > trace_net_dev_queue(skb); > > diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c > > index ceba11b198bb..48504ed3b280 100644 > > --- a/net/sched/act_skbedit.c > > +++ b/net/sched/act_skbedit.c > > @@ -58,8 +58,10 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a, > > } > > } > > if (params->flags & SKBEDIT_F_QUEUE_MAPPING && > > - skb->dev->real_num_tx_queues > params->queue_mapping) > > + skb->dev->real_num_tx_queues > params->queue_mapping) { > > + netdev_xmit_skip_txqueue(true); > > skb_set_queue_mapping(skb, params->queue_mapping); > > + } > > if (params->flags & SKBEDIT_F_MARK) { > > skb->mark &= ~params->mask; > > skb->mark |= params->mark & params->mask; > > -- > > 2.27.0 > > -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-20 14:21 ` Tonghao Zhang @ 2021-12-20 14:31 ` Tonghao Zhang 0 siblings, 0 replies; 18+ messages in thread From: Tonghao Zhang @ 2021-12-20 14:31 UTC (permalink / raw) To: Eric Dumazet Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Mon, Dec 20, 2021 at 10:21 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Mon, Dec 20, 2021 at 9:58 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Mon, Dec 20, 2021 at 4:38 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > This patch fixes issue: > > > * If we install tc filters with act_skbedit in clsact hook. > > > It doesn't work, because netdev_core_pick_tx() overwrites > > > queue_mapping. > > > > > > $ tc filter ... action skbedit queue_mapping 1 > > > > > > And this patch is useful: > > > * We can use FQ + EDT to implement efficient policies. Tx queues > > > are picked by xps, ndo_select_queue of netdev driver, or skb hash > > > in netdev_core_pick_tx(). In fact, the netdev driver, and skb > > > hash are _not_ under control. xps uses the CPUs map to select Tx > > > queues, but we can't figure out which task_struct of pod/containter > > > running on this cpu in most case. We can use clsact filters to classify > > > one pod/container traffic to one Tx queue. Why ? > > > > > > In containter networking environment, there are two kinds of pod/ > > > containter/net-namespace. One kind (e.g. P1, P2), the high throughput > > > is key in these applications. But avoid running out of network resource, > > > the outbound traffic of these pods is limited, using or sharing one > > > dedicated Tx queues assigned HTB/TBF/FQ Qdisc. Other kind of pods > > > (e.g. Pn), the low latency of data access is key. And the traffic is not > > > limited. Pods use or share other dedicated Tx queues assigned FIFO Qdisc. > > > This choice provides two benefits. First, contention on the HTB/FQ Qdisc > > > lock is significantly reduced since fewer CPUs contend for the same queue. > > > More importantly, Qdisc contention can be eliminated completely if each > > > CPU has its own FIFO Qdisc for the second kind of pods. > > > > > > There must be a mechanism in place to support classifying traffic based on > > > pods/container to different Tx queues. Note that clsact is outside of Qdisc > > > while Qdisc can run a classifier to select a sub-queue under the lock. > > > > > > In general recording the decision in the skb seems a little heavy handed. > > > This patch introduces a per-CPU variable, suggested by Eric. > > > > > > The xmit.skip_txqueue flag is firstly cleared in __dev_queue_xmit(). > > > - Tx Qdisc may install that skbedit actions, then xmit.skip_txqueue flag > > > is set in qdisc->enqueue() though tx queue has been selected in > > > netdev_tx_queue_mapping() or netdev_core_pick_tx(). That flag is cleared > > > firstly in __dev_queue_xmit(), is useful: > > > - Avoid picking Tx queue with netdev_tx_queue_mapping() in next netdev > > > in such case: eth0 macvlan - eth0.3 vlan - eth0 ixgbe-phy: > > > For example, eth0, macvlan in pod, which root Qdisc install skbedit > > > queue_mapping, send packets to eth0.3, vlan in host. In __dev_queue_xmit() of > > > eth0.3, clear the flag, does not select tx queue according to skb->queue_mapping > > > because there is no filters in clsact or tx Qdisc of this netdev. > > > Same action taked in eth0, ixgbe in Host. > > > - Avoid picking Tx queue for next packet. If we set xmit.skip_txqueue > > > in tx Qdisc (qdisc->enqueue()), the proper way to clear it is clearing it > > > in __dev_queue_xmit when processing next packets. > > > > > > +----+ +----+ +----+ > > > | P1 | | P2 | | Pn | > > > +----+ +----+ +----+ > > > | | | > > > +-----------+-----------+ > > > | > > > | clsact/skbedit > > > | MQ > > > v > > > +-----------+-----------+ > > > | q0 | q1 | qn > > > v v v > > > HTB/FQ HTB/FQ ... FIFO > > > > > > Cc: Jamal Hadi Salim <jhs@mojatatu.com> > > > Cc: Cong Wang <xiyou.wangcong@gmail.com> > > > Cc: Jiri Pirko <jiri@resnulli.us> > > > Cc: "David S. Miller" <davem@davemloft.net> > > > Cc: Jakub Kicinski <kuba@kernel.org> > > > Cc: Jonathan Lemon <jonathan.lemon@gmail.com> > > > Cc: Eric Dumazet <edumazet@google.com> > > > Cc: Alexander Lobakin <alobakin@pm.me> > > > Cc: Paolo Abeni <pabeni@redhat.com> > > > Cc: Talal Ahmad <talalahmad@google.com> > > > Cc: Kevin Hao <haokexin@gmail.com> > > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > Cc: Antoine Tenart <atenart@kernel.org> > > > Cc: Wei Wang <weiwan@google.com> > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > Suggested-by: Eric Dumazet <edumazet@google.com> > > > > I have not suggested this patch, only to not add yet another bit in sk_buff. > sorry for that > > > > > Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > --- > > > include/linux/netdevice.h | 19 +++++++++++++++++++ > > > net/core/dev.c | 7 ++++++- > > > net/sched/act_skbedit.c | 4 +++- > > > 3 files changed, 28 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > > index 8b0bdeb4734e..8d02dafb32ba 100644 > > > --- a/include/linux/netdevice.h > > > +++ b/include/linux/netdevice.h > > > @@ -3009,6 +3009,7 @@ struct softnet_data { > > > /* written and read only by owning cpu: */ > > > struct { > > > u16 recursion; > > > + u8 skip_txqueue; > > > u8 more; > > > } xmit; > > > #ifdef CONFIG_RPS > > > @@ -4696,6 +4697,24 @@ static inline netdev_tx_t netdev_start_xmit(struct sk_buff *skb, struct net_devi > > > return rc; > > > } > > > > > > +static inline void netdev_xmit_skip_txqueue(bool skip) > > > +{ > > > + __this_cpu_write(softnet_data.xmit.skip_txqueue, skip); > > > +} > > > + > > > +static inline bool netdev_xmit_txqueue_skipped(void) > > > +{ > > > + return __this_cpu_read(softnet_data.xmit.skip_txqueue); > > > +} > > > + > > > +static inline struct netdev_queue * > > > +netdev_tx_queue_mapping(struct net_device *dev, struct sk_buff *skb) > > > +{ > > > + int qm = skb_get_queue_mapping(skb); > > > + > > > + return netdev_get_tx_queue(dev, netdev_cap_txqueue(dev, qm)); > > > +} > > > + > > > int netdev_class_create_file_ns(const struct class_attribute *class_attr, > > > const void *ns); > > > void netdev_class_remove_file_ns(const struct class_attribute *class_attr, > > > diff --git a/net/core/dev.c b/net/core/dev.c > > > index a855e41bbe39..e3f548c54dda 100644 > > > --- a/net/core/dev.c > > > +++ b/net/core/dev.c > > > @@ -4048,6 +4048,7 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > > skb_update_prio(skb); > > > > > > qdisc_pkt_len_init(skb); > > > + netdev_xmit_skip_txqueue(false); > > > #ifdef CONFIG_NET_CLS_ACT > > > skb->tc_at_ingress = 0; > > > #endif > > > @@ -4073,7 +4074,11 @@ static int __dev_queue_xmit(struct sk_buff *skb, struct net_device *sb_dev) > > > else > > > skb_dst_force(skb); > > > > > > - txq = netdev_core_pick_tx(dev, skb, sb_dev); > > > + if (netdev_xmit_txqueue_skipped()) > > > + txq = netdev_tx_queue_mapping(dev, skb); > > > + else > > > + txq = netdev_core_pick_tx(dev, skb, sb_dev); > > > + > > > > If we really need to add yet another conditional in fast path, I would > > suggest using a static key. > Thanks, I will add a static key for this patch. > > Only hosts where SKBEDIT_F_QUEUE_MAPPING is requested would pay the price. Hi Eirc Do you mean we need a static key, such as, egress_needed_key ? static DEFINE_STATIC_KEY_FALSE(egress_needed_key); and we also should use the per-cpu var to decide to select tx from netdev_tx_queue_mapping or netdev_core_pick_tx. > > > > > q = rcu_dereference_bh(txq->qdisc); > > > > > > trace_net_dev_queue(skb); > > > diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c > > > index ceba11b198bb..48504ed3b280 100644 > > > --- a/net/sched/act_skbedit.c > > > +++ b/net/sched/act_skbedit.c > > > @@ -58,8 +58,10 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a, > > > } > > > } > > > if (params->flags & SKBEDIT_F_QUEUE_MAPPING && > > > - skb->dev->real_num_tx_queues > params->queue_mapping) > > > + skb->dev->real_num_tx_queues > params->queue_mapping) { > > > + netdev_xmit_skip_txqueue(true); > > > skb_set_queue_mapping(skb, params->queue_mapping); > > > + } > > > if (params->flags & SKBEDIT_F_MARK) { > > > skb->mark &= ~params->mask; > > > skb->mark |= params->mark & params->mask; > > > -- > > > 2.27.0 > > > > > > > -- > Best regards, Tonghao -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-20 12:38 ` [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue xiangxia.m.yue 2021-12-20 13:57 ` Eric Dumazet @ 2021-12-20 17:51 ` Cong Wang 2021-12-22 1:19 ` Tonghao Zhang 2021-12-29 19:13 ` Vinicius Costa Gomes 2 siblings, 1 reply; 18+ messages in thread From: Cong Wang @ 2021-12-20 17:51 UTC (permalink / raw) To: Tonghao Zhang Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Mon, Dec 20, 2021 at 4:38 AM <xiangxia.m.yue@gmail.com> wrote: > In general recording the decision in the skb seems a little heavy handed. > This patch introduces a per-CPU variable, suggested by Eric. > > The xmit.skip_txqueue flag is firstly cleared in __dev_queue_xmit(). > - Tx Qdisc may install that skbedit actions, then xmit.skip_txqueue flag > is set in qdisc->enqueue() though tx queue has been selected in > netdev_tx_queue_mapping() or netdev_core_pick_tx(). That flag is cleared > firstly in __dev_queue_xmit(), is useful: > - Avoid picking Tx queue with netdev_tx_queue_mapping() in next netdev > in such case: eth0 macvlan - eth0.3 vlan - eth0 ixgbe-phy: > For example, eth0, macvlan in pod, which root Qdisc install skbedit > queue_mapping, send packets to eth0.3, vlan in host. In __dev_queue_xmit() of > eth0.3, clear the flag, does not select tx queue according to skb->queue_mapping > because there is no filters in clsact or tx Qdisc of this netdev. > Same action taked in eth0, ixgbe in Host. > - Avoid picking Tx queue for next packet. If we set xmit.skip_txqueue > in tx Qdisc (qdisc->enqueue()), the proper way to clear it is clearing it > in __dev_queue_xmit when processing next packets. Any reason why we can't just move sch_handle_egress() down after netdev_core_pick_tx() and recalculate txq with skb->queue_mapping? Thanks. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-20 17:51 ` Cong Wang @ 2021-12-22 1:19 ` Tonghao Zhang 0 siblings, 0 replies; 18+ messages in thread From: Tonghao Zhang @ 2021-12-22 1:19 UTC (permalink / raw) To: Cong Wang Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Tue, Dec 21, 2021 at 1:51 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Dec 20, 2021 at 4:38 AM <xiangxia.m.yue@gmail.com> wrote: > > In general recording the decision in the skb seems a little heavy handed. > > This patch introduces a per-CPU variable, suggested by Eric. > > > > The xmit.skip_txqueue flag is firstly cleared in __dev_queue_xmit(). > > - Tx Qdisc may install that skbedit actions, then xmit.skip_txqueue flag > > is set in qdisc->enqueue() though tx queue has been selected in > > netdev_tx_queue_mapping() or netdev_core_pick_tx(). That flag is cleared > > firstly in __dev_queue_xmit(), is useful: > > - Avoid picking Tx queue with netdev_tx_queue_mapping() in next netdev > > in such case: eth0 macvlan - eth0.3 vlan - eth0 ixgbe-phy: > > For example, eth0, macvlan in pod, which root Qdisc install skbedit > > queue_mapping, send packets to eth0.3, vlan in host. In __dev_queue_xmit() of > > eth0.3, clear the flag, does not select tx queue according to skb->queue_mapping > > because there is no filters in clsact or tx Qdisc of this netdev. > > Same action taked in eth0, ixgbe in Host. > > - Avoid picking Tx queue for next packet. If we set xmit.skip_txqueue > > in tx Qdisc (qdisc->enqueue()), the proper way to clear it is clearing it > > in __dev_queue_xmit when processing next packets. > > Any reason why we can't just move sch_handle_egress() down after > netdev_core_pick_tx() and recalculate txq with skb->queue_mapping? If we sch_handle_egress() down after netdev_core_pick_tx(), and we overwrite the txqueue, the overhead of netdev_core_pick_tx() is unnecessary, so I don't do that . If eth0(macvlan) -> eth0.3(vlan) -> eth0(ixgbe), skb-> queue_mapping is not cleared, skb-> queue_mapping affect every netdevice to pick the tx queue. > Thanks. -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-20 12:38 ` [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue xiangxia.m.yue 2021-12-20 13:57 ` Eric Dumazet 2021-12-20 17:51 ` Cong Wang @ 2021-12-29 19:13 ` Vinicius Costa Gomes 2021-12-30 5:02 ` Tonghao Zhang 2 siblings, 1 reply; 18+ messages in thread From: Vinicius Costa Gomes @ 2021-12-29 19:13 UTC (permalink / raw) To: xiangxia.m.yue, netdev Cc: Tonghao Zhang, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann xiangxia.m.yue@gmail.com writes: > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > This patch fixes issue: > * If we install tc filters with act_skbedit in clsact hook. > It doesn't work, because netdev_core_pick_tx() overwrites > queue_mapping. > > $ tc filter ... action skbedit queue_mapping 1 > > And this patch is useful: > * We can use FQ + EDT to implement efficient policies. Tx queues > are picked by xps, ndo_select_queue of netdev driver, or skb hash > in netdev_core_pick_tx(). In fact, the netdev driver, and skb > hash are _not_ under control. xps uses the CPUs map to select Tx > queues, but we can't figure out which task_struct of pod/containter > running on this cpu in most case. We can use clsact filters to classify > one pod/container traffic to one Tx queue. Why ? > > In containter networking environment, there are two kinds of pod/ > containter/net-namespace. One kind (e.g. P1, P2), the high throughput > is key in these applications. But avoid running out of network resource, > the outbound traffic of these pods is limited, using or sharing one > dedicated Tx queues assigned HTB/TBF/FQ Qdisc. Other kind of pods > (e.g. Pn), the low latency of data access is key. And the traffic is not > limited. Pods use or share other dedicated Tx queues assigned FIFO Qdisc. > This choice provides two benefits. First, contention on the HTB/FQ Qdisc > lock is significantly reduced since fewer CPUs contend for the same queue. > More importantly, Qdisc contention can be eliminated completely if each > CPU has its own FIFO Qdisc for the second kind of pods. > > There must be a mechanism in place to support classifying traffic based on > pods/container to different Tx queues. Note that clsact is outside of Qdisc > while Qdisc can run a classifier to select a sub-queue under the > lock. One alternative, I don't know if it would work for you, it to use the net_prio cgroup + mqprio. Something like this: * create the cgroup $ mkdir -p /sys/fs/cgroup/net_prio/<CGROUP_NAME> * assign priorities to the cgroup (per interface) $ echo "<IFACE> <PRIO>" >> /sys/fs/cgroup/net_prio/<CGROUP_NAME>/net_prio.ifpriomap" * use the cgroup in applications that do not set SO_PRIORITY $ cgexec -g net_prio:<CGROUP_NAME> <application> * configure mqprio $ tc qdisc replace dev $IFACE parent root handle 100 mqprio \ num_tc 3 \ map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ queues 1@0 1@1 2@2 \ hw 0 This would map all traffic with SO_PRIORITY 3 to TX queue 0, for example. But I agree that skbedit's queue_mapping not working is unexpected and should be fixed. Cheers, -- Vinicius ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-29 19:13 ` Vinicius Costa Gomes @ 2021-12-30 5:02 ` Tonghao Zhang 2022-01-26 19:58 ` Cong Wang 0 siblings, 1 reply; 18+ messages in thread From: Tonghao Zhang @ 2021-12-30 5:02 UTC (permalink / raw) To: Vinicius Costa Gomes Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Thu, Dec 30, 2021 at 3:14 AM Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote: > > xiangxia.m.yue@gmail.com writes: > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > This patch fixes issue: > > * If we install tc filters with act_skbedit in clsact hook. > > It doesn't work, because netdev_core_pick_tx() overwrites > > queue_mapping. > > > > $ tc filter ... action skbedit queue_mapping 1 > > > > And this patch is useful: > > * We can use FQ + EDT to implement efficient policies. Tx queues > > are picked by xps, ndo_select_queue of netdev driver, or skb hash > > in netdev_core_pick_tx(). In fact, the netdev driver, and skb > > hash are _not_ under control. xps uses the CPUs map to select Tx > > queues, but we can't figure out which task_struct of pod/containter > > running on this cpu in most case. We can use clsact filters to classify > > one pod/container traffic to one Tx queue. Why ? > > > > In containter networking environment, there are two kinds of pod/ > > containter/net-namespace. One kind (e.g. P1, P2), the high throughput > > is key in these applications. But avoid running out of network resource, > > the outbound traffic of these pods is limited, using or sharing one > > dedicated Tx queues assigned HTB/TBF/FQ Qdisc. Other kind of pods > > (e.g. Pn), the low latency of data access is key. And the traffic is not > > limited. Pods use or share other dedicated Tx queues assigned FIFO Qdisc. > > This choice provides two benefits. First, contention on the HTB/FQ Qdisc > > lock is significantly reduced since fewer CPUs contend for the same queue. > > More importantly, Qdisc contention can be eliminated completely if each > > CPU has its own FIFO Qdisc for the second kind of pods. > > > > There must be a mechanism in place to support classifying traffic based on > > pods/container to different Tx queues. Note that clsact is outside of Qdisc > > while Qdisc can run a classifier to select a sub-queue under the > > lock. > > One alternative, I don't know if it would work for you, it to use the > net_prio cgroup + mqprio. > > Something like this: > > * create the cgroup > $ mkdir -p /sys/fs/cgroup/net_prio/<CGROUP_NAME> > * assign priorities to the cgroup (per interface) > $ echo "<IFACE> <PRIO>" >> /sys/fs/cgroup/net_prio/<CGROUP_NAME>/net_prio.ifpriomap" > * use the cgroup in applications that do not set SO_PRIORITY Yes, I think this is key. In k8s, we can not control the priority of applications in Pod. and I think 2/2 patch can provide more mechanisms to select queues from a range. If possible, you can help me review it. https://patchwork.kernel.org/project/netdevbpf/patch/20211224164926.80733-3-xiangxia.m.yue@gmail.com/ > $ cgexec -g net_prio:<CGROUP_NAME> <application> > * configure mqprio > $ tc qdisc replace dev $IFACE parent root handle 100 mqprio \ > num_tc 3 \ > map 2 2 1 0 2 2 2 2 2 2 2 2 2 2 2 2 \ > queues 1@0 1@1 2@2 \ > hw 0 > > This would map all traffic with SO_PRIORITY 3 to TX queue 0, for example. > But I agree that skbedit's queue_mapping not working is unexpected and > should be fixed. > > > Cheers, > -- > Vinicius -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2021-12-30 5:02 ` Tonghao Zhang @ 2022-01-26 19:58 ` Cong Wang 2022-01-27 1:35 ` Tonghao Zhang 0 siblings, 1 reply; 18+ messages in thread From: Cong Wang @ 2022-01-26 19:58 UTC (permalink / raw) To: Tonghao Zhang Cc: Vinicius Costa Gomes, Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Wed, Dec 29, 2021 at 9:03 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > Yes, I think this is key. In k8s, we can not control the priority of > applications in Pod. and I think 2/2 patch Why not? Please be more specific. > can provide more mechanisms to select queues from a range. Well, you just keep ignoring eBPF action which provides literally infinitely more choices than yours. So, you want more choices but still refuse using eBPF action, what point are you trying to make? Thanks. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue 2022-01-26 19:58 ` Cong Wang @ 2022-01-27 1:35 ` Tonghao Zhang 0 siblings, 0 replies; 18+ messages in thread From: Tonghao Zhang @ 2022-01-27 1:35 UTC (permalink / raw) To: Cong Wang Cc: Vinicius Costa Gomes, Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Thu, Jan 27, 2022 at 3:59 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Wed, Dec 29, 2021 at 9:03 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > Yes, I think this is key. In k8s, we can not control the priority of > > applications in Pod. and I think 2/2 patch > > Why not? Please be more specific. I mean the priority of skb can be set via the socket from applications. the application is running for a long time, There are so many applications in the company. we can't ask them to do that. there is a lot of workload. > > can provide more mechanisms to select queues from a range. > > Well, you just keep ignoring eBPF action which provides > literally infinitely more choices than yours. So, you want more > choices but still refuse using eBPF action, what point are you > trying to make? Maybe I can answer your question in another thread. In brief, this patch is only enhanced the tc-skbedit. > Thanks. -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
* [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting tx queue 2021-12-20 12:38 [net-next v5 0/2] net: sched: allow user to select txqueue xiangxia.m.yue 2021-12-20 12:38 ` [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue xiangxia.m.yue @ 2021-12-20 12:38 ` xiangxia.m.yue 2021-12-20 17:57 ` Cong Wang 1 sibling, 1 reply; 18+ messages in thread From: xiangxia.m.yue @ 2021-12-20 12:38 UTC (permalink / raw) To: netdev Cc: Tonghao Zhang, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann From: Tonghao Zhang <xiangxia.m.yue@gmail.com> This patch allows user to select queue_mapping, range from A to B. And user can use skbhash, cgroup classid and cpuid to select Tx queues. Then we can load balance packets from A to B queue. The range is an unsigned 16bit value in decimal format. $ tc filter ... action skbedit queue_mapping skbhash A B "skbedit queue_mapping QUEUE_MAPPING" (from "man 8 tc-skbedit") is enhanced with flags: * SKBEDIT_F_TXQ_SKBHASH * SKBEDIT_F_TXQ_CLASSID * SKBEDIT_F_TXQ_CPUID Use skb->hash, cgroup classid, or cpuid to distribute packets. Then same range of tx queues can be shared for different flows, cgroups, or CPUs in a variety of scenarios. For example, F1 may share range R1 with F2. The best way to do that is to set flag to SKBEDIT_F_TXQ_HASH, using skb->hash to share the queues. If cgroup C1 want to share the R1 with cgroup C2 .. Cn, use the SKBEDIT_F_TXQ_CLASSID. Of course, in some other scenario, C1 use R1, while Cn can use the Rn. Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Jiri Pirko <jiri@resnulli.us> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Jonathan Lemon <jonathan.lemon@gmail.com> Cc: Eric Dumazet <edumazet@google.com> Cc: Alexander Lobakin <alobakin@pm.me> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Talal Ahmad <talalahmad@google.com> Cc: Kevin Hao <haokexin@gmail.com> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> Cc: Kees Cook <keescook@chromium.org> Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> Cc: Antoine Tenart <atenart@kernel.org> Cc: Wei Wang <weiwan@google.com> Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com> --- include/net/tc_act/tc_skbedit.h | 1 + include/uapi/linux/tc_act/tc_skbedit.h | 8 +++ net/sched/act_skbedit.c | 81 ++++++++++++++++++++++++-- 3 files changed, 85 insertions(+), 5 deletions(-) diff --git a/include/net/tc_act/tc_skbedit.h b/include/net/tc_act/tc_skbedit.h index 00bfee70609e..ee96e0fa6566 100644 --- a/include/net/tc_act/tc_skbedit.h +++ b/include/net/tc_act/tc_skbedit.h @@ -17,6 +17,7 @@ struct tcf_skbedit_params { u32 mark; u32 mask; u16 queue_mapping; + u16 mapping_mod; u16 ptype; struct rcu_head rcu; }; diff --git a/include/uapi/linux/tc_act/tc_skbedit.h b/include/uapi/linux/tc_act/tc_skbedit.h index 800e93377218..5ea1438a4d88 100644 --- a/include/uapi/linux/tc_act/tc_skbedit.h +++ b/include/uapi/linux/tc_act/tc_skbedit.h @@ -29,6 +29,13 @@ #define SKBEDIT_F_PTYPE 0x8 #define SKBEDIT_F_MASK 0x10 #define SKBEDIT_F_INHERITDSFIELD 0x20 +#define SKBEDIT_F_TXQ_SKBHASH 0x40 +#define SKBEDIT_F_TXQ_CLASSID 0x80 +#define SKBEDIT_F_TXQ_CPUID 0x100 + +#define SKBEDIT_F_TXQ_HASH_MASK (SKBEDIT_F_TXQ_SKBHASH | \ + SKBEDIT_F_TXQ_CLASSID | \ + SKBEDIT_F_TXQ_CPUID) struct tc_skbedit { tc_gen; @@ -45,6 +52,7 @@ enum { TCA_SKBEDIT_PTYPE, TCA_SKBEDIT_MASK, TCA_SKBEDIT_FLAGS, + TCA_SKBEDIT_QUEUE_MAPPING_MAX, __TCA_SKBEDIT_MAX }; #define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1) diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c index 48504ed3b280..2a7a1f8b2dfc 100644 --- a/net/sched/act_skbedit.c +++ b/net/sched/act_skbedit.c @@ -10,6 +10,7 @@ #include <linux/kernel.h> #include <linux/skbuff.h> #include <linux/rtnetlink.h> +#include <net/cls_cgroup.h> #include <net/netlink.h> #include <net/pkt_sched.h> #include <net/ip.h> @@ -23,6 +24,38 @@ static unsigned int skbedit_net_id; static struct tc_action_ops act_skbedit_ops; +static u16 tcf_skbedit_hash(struct tcf_skbedit_params *params, + struct sk_buff *skb) +{ + u32 mapping_hash_type = params->flags & SKBEDIT_F_TXQ_HASH_MASK; + u16 queue_mapping = params->queue_mapping; + u16 mapping_mod = params->mapping_mod; + u32 hash = 0; + + switch (mapping_hash_type) { + case SKBEDIT_F_TXQ_CLASSID: + hash = task_get_classid(skb); + break; + case SKBEDIT_F_TXQ_SKBHASH: + hash = skb_get_hash(skb); + break; + case SKBEDIT_F_TXQ_CPUID: + hash = raw_smp_processor_id(); + break; + case 0: + /* Hash type isn't specified. In this case: + * hash % mapping_mod == 0 + */ + break; + default: + net_warn_ratelimited("The type of queue_mapping hash is not supported. 0x%x\n", + mapping_hash_type); + } + + queue_mapping = queue_mapping + hash % mapping_mod; + return netdev_cap_txqueue(skb->dev, queue_mapping); +} + static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a, struct tcf_result *res) { @@ -57,10 +90,9 @@ static int tcf_skbedit_act(struct sk_buff *skb, const struct tc_action *a, break; } } - if (params->flags & SKBEDIT_F_QUEUE_MAPPING && - skb->dev->real_num_tx_queues > params->queue_mapping) { + if (params->flags & SKBEDIT_F_QUEUE_MAPPING) { netdev_xmit_skip_txqueue(true); - skb_set_queue_mapping(skb, params->queue_mapping); + skb_set_queue_mapping(skb, tcf_skbedit_hash(params, skb)); } if (params->flags & SKBEDIT_F_MARK) { skb->mark &= ~params->mask; @@ -94,6 +126,7 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = { [TCA_SKBEDIT_PTYPE] = { .len = sizeof(u16) }, [TCA_SKBEDIT_MASK] = { .len = sizeof(u32) }, [TCA_SKBEDIT_FLAGS] = { .len = sizeof(u64) }, + [TCA_SKBEDIT_QUEUE_MAPPING_MAX] = { .len = sizeof(u16) }, }; static int tcf_skbedit_init(struct net *net, struct nlattr *nla, @@ -110,6 +143,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, struct tcf_skbedit *d; u32 flags = 0, *priority = NULL, *mark = NULL, *mask = NULL; u16 *queue_mapping = NULL, *ptype = NULL; + u16 mapping_mod = 1; bool exists = false; int ret = 0, err; u32 index; @@ -154,7 +188,34 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, if (tb[TCA_SKBEDIT_FLAGS] != NULL) { u64 *pure_flags = nla_data(tb[TCA_SKBEDIT_FLAGS]); - + u64 mapping_hash_type; + + mapping_hash_type = *pure_flags & SKBEDIT_F_TXQ_HASH_MASK; + if (mapping_hash_type) { + u16 *queue_mapping_max; + + /* Hash types are mutually exclusive. */ + if (mapping_hash_type & (mapping_hash_type - 1)) { + NL_SET_ERR_MSG_MOD(extack, "Multi types of hash are specified."); + return -EINVAL; + } + + if (!tb[TCA_SKBEDIT_QUEUE_MAPPING] || + !tb[TCA_SKBEDIT_QUEUE_MAPPING_MAX]) { + NL_SET_ERR_MSG_MOD(extack, "Missing required range of queue_mapping."); + return -EINVAL; + } + + queue_mapping_max = + nla_data(tb[TCA_SKBEDIT_QUEUE_MAPPING_MAX]); + if (*queue_mapping_max < *queue_mapping) { + NL_SET_ERR_MSG_MOD(extack, "The range of queue_mapping is invalid, max < min."); + return -EINVAL; + } + + mapping_mod = *queue_mapping_max - *queue_mapping + 1; + flags |= mapping_hash_type; + } if (*pure_flags & SKBEDIT_F_INHERITDSFIELD) flags |= SKBEDIT_F_INHERITDSFIELD; } @@ -206,8 +267,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla, params_new->flags = flags; if (flags & SKBEDIT_F_PRIORITY) params_new->priority = *priority; - if (flags & SKBEDIT_F_QUEUE_MAPPING) + if (flags & SKBEDIT_F_QUEUE_MAPPING) { params_new->queue_mapping = *queue_mapping; + params_new->mapping_mod = mapping_mod; + } if (flags & SKBEDIT_F_MARK) params_new->mark = *mark; if (flags & SKBEDIT_F_PTYPE) @@ -274,6 +337,13 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a, goto nla_put_failure; if (params->flags & SKBEDIT_F_INHERITDSFIELD) pure_flags |= SKBEDIT_F_INHERITDSFIELD; + if (params->flags & SKBEDIT_F_TXQ_HASH_MASK) { + if (nla_put_u16(skb, TCA_SKBEDIT_QUEUE_MAPPING_MAX, + params->queue_mapping + params->mapping_mod - 1)) + goto nla_put_failure; + + pure_flags |= params->flags & SKBEDIT_F_TXQ_HASH_MASK; + } if (pure_flags != 0 && nla_put(skb, TCA_SKBEDIT_FLAGS, sizeof(pure_flags), &pure_flags)) goto nla_put_failure; @@ -323,6 +393,7 @@ static size_t tcf_skbedit_get_fill_size(const struct tc_action *act) return nla_total_size(sizeof(struct tc_skbedit)) + nla_total_size(sizeof(u32)) /* TCA_SKBEDIT_PRIORITY */ + nla_total_size(sizeof(u16)) /* TCA_SKBEDIT_QUEUE_MAPPING */ + + nla_total_size(sizeof(u16)) /* TCA_SKBEDIT_QUEUE_MAPPING_MAX */ + nla_total_size(sizeof(u32)) /* TCA_SKBEDIT_MARK */ + nla_total_size(sizeof(u16)) /* TCA_SKBEDIT_PTYPE */ + nla_total_size(sizeof(u32)) /* TCA_SKBEDIT_MASK */ -- 2.27.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting tx queue 2021-12-20 12:38 ` [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting " xiangxia.m.yue @ 2021-12-20 17:57 ` Cong Wang 2021-12-22 1:23 ` Tonghao Zhang 0 siblings, 1 reply; 18+ messages in thread From: Cong Wang @ 2021-12-20 17:57 UTC (permalink / raw) To: Tonghao Zhang Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Mon, Dec 20, 2021 at 4:39 AM <xiangxia.m.yue@gmail.com> wrote: > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > This patch allows user to select queue_mapping, range > from A to B. And user can use skbhash, cgroup classid > and cpuid to select Tx queues. Then we can load balance > packets from A to B queue. The range is an unsigned 16bit > value in decimal format. > > $ tc filter ... action skbedit queue_mapping skbhash A B > > "skbedit queue_mapping QUEUE_MAPPING" (from "man 8 tc-skbedit") > is enhanced with flags: > * SKBEDIT_F_TXQ_SKBHASH > * SKBEDIT_F_TXQ_CLASSID > * SKBEDIT_F_TXQ_CPUID Once again, you are enforcing policies in kernel, which is not good. Kernel should just set whatever you give to it, not selecting policies like a menu. Any reason why you can't obtain these values in user-space? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting tx queue 2021-12-20 17:57 ` Cong Wang @ 2021-12-22 1:23 ` Tonghao Zhang 2021-12-24 19:26 ` Cong Wang 0 siblings, 1 reply; 18+ messages in thread From: Tonghao Zhang @ 2021-12-22 1:23 UTC (permalink / raw) To: Cong Wang Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Tue, Dec 21, 2021 at 1:57 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Mon, Dec 20, 2021 at 4:39 AM <xiangxia.m.yue@gmail.com> wrote: > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > This patch allows user to select queue_mapping, range > > from A to B. And user can use skbhash, cgroup classid > > and cpuid to select Tx queues. Then we can load balance > > packets from A to B queue. The range is an unsigned 16bit > > value in decimal format. > > > > $ tc filter ... action skbedit queue_mapping skbhash A B > > > > "skbedit queue_mapping QUEUE_MAPPING" (from "man 8 tc-skbedit") > > is enhanced with flags: > > * SKBEDIT_F_TXQ_SKBHASH > > * SKBEDIT_F_TXQ_CLASSID > > * SKBEDIT_F_TXQ_CPUID > > Once again, you are enforcing policies in kernel, which is not good. > Kernel should just set whatever you give to it, not selecting policies > like a menu. I agree that, but for tc/net-sched layer, there are a lot of networking policies, for example , cls_A, act_B, sch_C. > Any reason why you can't obtain these values in user-space? Did you mean that we add this flags to iproute2 tools? This patch for iproute2, is not post. If the kerenl patches are accepted, I will send them. -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting tx queue 2021-12-22 1:23 ` Tonghao Zhang @ 2021-12-24 19:26 ` Cong Wang 2021-12-25 1:41 ` Tonghao Zhang 0 siblings, 1 reply; 18+ messages in thread From: Cong Wang @ 2021-12-24 19:26 UTC (permalink / raw) To: Tonghao Zhang Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Tue, Dec 21, 2021 at 5:24 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Tue, Dec 21, 2021 at 1:57 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Mon, Dec 20, 2021 at 4:39 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > This patch allows user to select queue_mapping, range > > > from A to B. And user can use skbhash, cgroup classid > > > and cpuid to select Tx queues. Then we can load balance > > > packets from A to B queue. The range is an unsigned 16bit > > > value in decimal format. > > > > > > $ tc filter ... action skbedit queue_mapping skbhash A B > > > > > > "skbedit queue_mapping QUEUE_MAPPING" (from "man 8 tc-skbedit") > > > is enhanced with flags: > > > * SKBEDIT_F_TXQ_SKBHASH > > > * SKBEDIT_F_TXQ_CLASSID > > > * SKBEDIT_F_TXQ_CPUID > > > > Once again, you are enforcing policies in kernel, which is not good. > > Kernel should just set whatever you give to it, not selecting policies > > like a menu. > I agree that, but for tc/net-sched layer, there are a lot of If you agree, why still move on with this patch? Apparently you don't. ;) > networking policies, for example , cls_A, act_B, sch_C. You are justifying your logic by shifting the topics here. The qdisc algorithm is very different from your case, it is essentially hard, if not impossible, to completely move to user-space. Even if we had eBPF based Qdisc, its programmablility is still very limited. Your code is much much much easier, which is essentially one-line, hence you have to reason to compare it with this, nor you can even justify it. > > Any reason why you can't obtain these values in user-space? > Did you mean that we add this flags to iproute2 tools? This patch for > iproute2, is not post. If the kerenl patches are accepted, I will send > them. Nope, I mean you can for example, obtain the CPU ID in user-space and pass it directly to _current_ act_skbedit as it is. Thanks. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting tx queue 2021-12-24 19:26 ` Cong Wang @ 2021-12-25 1:41 ` Tonghao Zhang 2021-12-29 12:50 ` Tonghao Zhang 0 siblings, 1 reply; 18+ messages in thread From: Tonghao Zhang @ 2021-12-25 1:41 UTC (permalink / raw) To: Cong Wang Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Sat, Dec 25, 2021 at 3:26 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Tue, Dec 21, 2021 at 5:24 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > On Tue, Dec 21, 2021 at 1:57 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > On Mon, Dec 20, 2021 at 4:39 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > > > This patch allows user to select queue_mapping, range > > > > from A to B. And user can use skbhash, cgroup classid > > > > and cpuid to select Tx queues. Then we can load balance > > > > packets from A to B queue. The range is an unsigned 16bit > > > > value in decimal format. > > > > > > > > $ tc filter ... action skbedit queue_mapping skbhash A B > > > > > > > > "skbedit queue_mapping QUEUE_MAPPING" (from "man 8 tc-skbedit") > > > > is enhanced with flags: > > > > * SKBEDIT_F_TXQ_SKBHASH > > > > * SKBEDIT_F_TXQ_CLASSID > > > > * SKBEDIT_F_TXQ_CPUID > > > > > > Once again, you are enforcing policies in kernel, which is not good. > > > Kernel should just set whatever you give to it, not selecting policies > > > like a menu. > > I agree that, but for tc/net-sched layer, there are a lot of > > If you agree, why still move on with this patch? Apparently > you don't. ;) > > > networking policies, for example , cls_A, act_B, sch_C. > > You are justifying your logic by shifting the topics here. Hi Cong, I mean that if the "policies" are networking policies, I don't agree with you, because the TC(or net sched layer), are networking policies, right ? and this patch enhances the original feature of TC. > The qdisc algorithm is very different from your case, it is essentially > hard, if not impossible, to completely move to user-space. Even if we > had eBPF based Qdisc, its programmablility is still very limited. > Your code is much much much easier, which is essentially one-line, > hence you have to reason to compare it with this, nor you can even > justify it. I think we have talked about this in another thread. And other maintainers comment on this. > > > Any reason why you can't obtain these values in user-space? > > Did you mean that we add this flags to iproute2 tools? This patch for > > iproute2, is not post. If the kerenl patches are accepted, I will send > > them. > > Nope, I mean you can for example, obtain the CPU ID in user-space > and pass it directly to _current_ act_skbedit as it is. This flag is passed from userspace. Do you mean the cpuid value should be passed from userspace? If so, this is not what we need. because we don't know which one cpu the pod(containers) will use. and for skb-hash/classid this helps us to pick the tx queue from a range. I don't understand why this value should be passed from userspace. How ? can you give me examples ? BTW, this patch is the core of patchset. > Thanks. -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting tx queue 2021-12-25 1:41 ` Tonghao Zhang @ 2021-12-29 12:50 ` Tonghao Zhang 2022-01-06 1:28 ` Tonghao Zhang 0 siblings, 1 reply; 18+ messages in thread From: Tonghao Zhang @ 2021-12-29 12:50 UTC (permalink / raw) To: Cong Wang Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Sat, Dec 25, 2021 at 9:41 AM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Sat, Dec 25, 2021 at 3:26 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > On Tue, Dec 21, 2021 at 5:24 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > > > On Tue, Dec 21, 2021 at 1:57 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > > > On Mon, Dec 20, 2021 at 4:39 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > > > > > This patch allows user to select queue_mapping, range > > > > > from A to B. And user can use skbhash, cgroup classid > > > > > and cpuid to select Tx queues. Then we can load balance > > > > > packets from A to B queue. The range is an unsigned 16bit > > > > > value in decimal format. > > > > > > > > > > $ tc filter ... action skbedit queue_mapping skbhash A B > > > > > > > > > > "skbedit queue_mapping QUEUE_MAPPING" (from "man 8 tc-skbedit") > > > > > is enhanced with flags: > > > > > * SKBEDIT_F_TXQ_SKBHASH > > > > > * SKBEDIT_F_TXQ_CLASSID > > > > > * SKBEDIT_F_TXQ_CPUID > > > > > > > > Once again, you are enforcing policies in kernel, which is not good. > > > > Kernel should just set whatever you give to it, not selecting policies > > > > like a menu. > > > I agree that, but for tc/net-sched layer, there are a lot of > > > > If you agree, why still move on with this patch? Apparently > > you don't. ;) > > > > > networking policies, for example , cls_A, act_B, sch_C. > > > > You are justifying your logic by shifting the topics here. > Hi Cong, > I mean that if the "policies" are networking policies, I don't agree > with you, because > the TC(or net sched layer), are networking policies, right ? and this > patch enhances the original feature of TC. > > > > The qdisc algorithm is very different from your case, it is essentially > > hard, if not impossible, to completely move to user-space. Even if we > > had eBPF based Qdisc, its programmablility is still very limited. > > Your code is much much much easier, which is essentially one-line, > > hence you have to reason to compare it with this, nor you can even > > justify it. > I think we have talked about this in another thread. And other > maintainers comment on this. > > > > Any reason why you can't obtain these values in user-space? > > > Did you mean that we add this flags to iproute2 tools? This patch for > > > iproute2, is not post. If the kerenl patches are accepted, I will send > > > them. > > > > Nope, I mean you can for example, obtain the CPU ID in user-space > > and pass it directly to _current_ act_skbedit as it is. > This flag is passed from userspace. Do you mean the cpuid value should > be passed from userspace? > If so, this is not what we need. because we don't know which one cpu > the pod(containers) will use. and for skb-hash/classid > this helps us to pick the tx queue from a range. I don't understand > why this value should be passed from userspace. How ? > can you give me examples ? friendly ping > BTW, this patch is the core of patchset. > > Thanks. > > > > -- > Best regards, Tonghao -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting tx queue 2021-12-29 12:50 ` Tonghao Zhang @ 2022-01-06 1:28 ` Tonghao Zhang 0 siblings, 0 replies; 18+ messages in thread From: Tonghao Zhang @ 2022-01-06 1:28 UTC (permalink / raw) To: Cong Wang Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, Jonathan Lemon, Eric Dumazet, Alexander Lobakin, Paolo Abeni, Talal Ahmad, Kevin Hao, Ilias Apalodimas, Kees Cook, Kumar Kartikeya Dwivedi, Antoine Tenart, Wei Wang, Arnd Bergmann On Wed, Dec 29, 2021 at 8:50 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > On Sat, Dec 25, 2021 at 9:41 AM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > On Sat, Dec 25, 2021 at 3:26 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > On Tue, Dec 21, 2021 at 5:24 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > On Tue, Dec 21, 2021 at 1:57 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > > > > > > > > > On Mon, Dec 20, 2021 at 4:39 AM <xiangxia.m.yue@gmail.com> wrote: > > > > > > > > > > > > From: Tonghao Zhang <xiangxia.m.yue@gmail.com> > > > > > > > > > > > > This patch allows user to select queue_mapping, range > > > > > > from A to B. And user can use skbhash, cgroup classid > > > > > > and cpuid to select Tx queues. Then we can load balance > > > > > > packets from A to B queue. The range is an unsigned 16bit > > > > > > value in decimal format. > > > > > > > > > > > > $ tc filter ... action skbedit queue_mapping skbhash A B > > > > > > > > > > > > "skbedit queue_mapping QUEUE_MAPPING" (from "man 8 tc-skbedit") > > > > > > is enhanced with flags: > > > > > > * SKBEDIT_F_TXQ_SKBHASH > > > > > > * SKBEDIT_F_TXQ_CLASSID > > > > > > * SKBEDIT_F_TXQ_CPUID > > > > > > > > > > Once again, you are enforcing policies in kernel, which is not good. > > > > > Kernel should just set whatever you give to it, not selecting policies > > > > > like a menu. > > > > I agree that, but for tc/net-sched layer, there are a lot of > > > > > > If you agree, why still move on with this patch? Apparently > > > you don't. ;) > > > > > > > networking policies, for example , cls_A, act_B, sch_C. > > > > > > You are justifying your logic by shifting the topics here. > > Hi Cong, > > I mean that if the "policies" are networking policies, I don't agree > > with you, because > > the TC(or net sched layer), are networking policies, right ? and this > > patch enhances the original feature of TC. > > > > > > > The qdisc algorithm is very different from your case, it is essentially > > > hard, if not impossible, to completely move to user-space. Even if we > > > had eBPF based Qdisc, its programmablility is still very limited. > > > Your code is much much much easier, which is essentially one-line, > > > hence you have to reason to compare it with this, nor you can even > > > justify it. > > I think we have talked about this in another thread. And other > > maintainers comment on this. > > > > > Any reason why you can't obtain these values in user-space? > > > > Did you mean that we add this flags to iproute2 tools? This patch for > > > > iproute2, is not post. If the kerenl patches are accepted, I will send > > > > them. > > > > > > Nope, I mean you can for example, obtain the CPU ID in user-space > > > and pass it directly to _current_ act_skbedit as it is. > > This flag is passed from userspace. Do you mean the cpuid value should > > be passed from userspace? > > If so, this is not what we need. because we don't know which one cpu > > the pod(containers) will use. and for skb-hash/classid > > this helps us to pick the tx queue from a range. I don't understand > > why this value should be passed from userspace. How ? > > can you give me examples ? > friendly ping still waiting for your reply, and Jakub's questions in other thread. If possible, I plan to resend the patchset. > > BTW, this patch is the core of patchset. > > > Thanks. > > > > > > > > -- > > Best regards, Tonghao > > > > -- > Best regards, Tonghao -- Best regards, Tonghao ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-01-27 1:35 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-20 12:38 [net-next v5 0/2] net: sched: allow user to select txqueue xiangxia.m.yue 2021-12-20 12:38 ` [net-next v5 1/2] net: sched: use queue_mapping to pick tx queue xiangxia.m.yue 2021-12-20 13:57 ` Eric Dumazet 2021-12-20 14:21 ` Tonghao Zhang 2021-12-20 14:31 ` Tonghao Zhang 2021-12-20 17:51 ` Cong Wang 2021-12-22 1:19 ` Tonghao Zhang 2021-12-29 19:13 ` Vinicius Costa Gomes 2021-12-30 5:02 ` Tonghao Zhang 2022-01-26 19:58 ` Cong Wang 2022-01-27 1:35 ` Tonghao Zhang 2021-12-20 12:38 ` [net-next v5 2/2] net: sched: support hash/classid/cpuid selecting " xiangxia.m.yue 2021-12-20 17:57 ` Cong Wang 2021-12-22 1:23 ` Tonghao Zhang 2021-12-24 19:26 ` Cong Wang 2021-12-25 1:41 ` Tonghao Zhang 2021-12-29 12:50 ` Tonghao Zhang 2022-01-06 1:28 ` Tonghao Zhang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.