| Message ID | 20211220123839.54664-3-xiangxia.m.yue@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sched: allow user to select txqueue | expand |
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?
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.
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.
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
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
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
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 */