| Message ID | 20180109140731.1022-10-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: sched: allow qdiscs to share filter block instances | expand |
On 18-01-09 09:07 AM, Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Benefit from the previously introduced shared filter blocks > infrastructure and allow ingress and clsact qdisc instances to share > filter blocks. The block index is coming from userspace as qdisc option. Didnt quiet follow why ingress is special and needs attributes to set the block but other qdiscs didnt. Will check again later after some coffee.. cheers, jamal
Thu, Jan 11, 2018 at 02:36:01PM CET, jhs@mojatatu.com wrote: >On 18-01-09 09:07 AM, Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Benefit from the previously introduced shared filter blocks >> infrastructure and allow ingress and clsact qdisc instances to share >> filter blocks. The block index is coming from userspace as qdisc option. > >Didnt quiet follow why ingress is special and needs attributes to >set the block but other qdiscs didnt. Jamal, again, other qdiscs does not support block sharing. This patchset only adds support for sharing of block for ingress and clsact qdiscs. Later on, other qdiscs could also support block sharing. >Will check again later after some coffee.. > >cheers, >jamal
On 18-01-11 09:24 AM, Jiri Pirko wrote: > Thu, Jan 11, 2018 at 02:36:01PM CET, jhs@mojatatu.com wrote: >> On 18-01-09 09:07 AM, Jiri Pirko wrote: >>> From: Jiri Pirko <jiri@mellanox.com> >>> >>> Benefit from the previously introduced shared filter blocks >>> infrastructure and allow ingress and clsact qdisc instances to share >>> filter blocks. The block index is coming from userspace as qdisc option. >> >> Didnt quiet follow why ingress is special and needs attributes to >> set the block but other qdiscs didnt. > > Jamal, again, other qdiscs does not support block sharing. This patchset > only adds support for sharing of block for ingress and clsact qdiscs. > Later on, other qdiscs could also support block sharing. > Can you stop a config which says: tc qdisc add dev ens9 root block 22 handle 1:0 prio ? cheers, jamal
Thu, Jan 11, 2018 at 03:37:08PM CET, jhs@mojatatu.com wrote: >On 18-01-11 09:24 AM, Jiri Pirko wrote: >> Thu, Jan 11, 2018 at 02:36:01PM CET, jhs@mojatatu.com wrote: >> > On 18-01-09 09:07 AM, Jiri Pirko wrote: >> > > From: Jiri Pirko <jiri@mellanox.com> >> > > >> > > Benefit from the previously introduced shared filter blocks >> > > infrastructure and allow ingress and clsact qdisc instances to share >> > > filter blocks. The block index is coming from userspace as qdisc option. >> > >> > Didnt quiet follow why ingress is special and needs attributes to >> > set the block but other qdiscs didnt. >> >> Jamal, again, other qdiscs does not support block sharing. This patchset >> only adds support for sharing of block for ingress and clsact qdiscs. >> Later on, other qdiscs could also support block sharing. >> > >Can you stop a config which says: >tc qdisc add dev ens9 root block 22 handle 1:0 prio ? Please see the iproute2 patches. Parsing of "block" command line option is done inside q_ingress.c
On 18-01-11 09:41 AM, Jiri Pirko wrote: > Thu, Jan 11, 2018 at 03:37:08PM CET, jhs@mojatatu.com wrote: >> On 18-01-11 09:24 AM, Jiri Pirko wrote: >>> Thu, Jan 11, 2018 at 02:36:01PM CET, jhs@mojatatu.com wrote: >>>> On 18-01-09 09:07 AM, Jiri Pirko wrote: >>>>> From: Jiri Pirko <jiri@mellanox.com> >>>>> >>>>> Benefit from the previously introduced shared filter blocks >>>>> infrastructure and allow ingress and clsact qdisc instances to share >>>>> filter blocks. The block index is coming from userspace as qdisc option. >>>> >>>> Didnt quiet follow why ingress is special and needs attributes to >>>> set the block but other qdiscs didnt. >>> >>> Jamal, again, other qdiscs does not support block sharing. This patchset >>> only adds support for sharing of block for ingress and clsact qdiscs. >>> Later on, other qdiscs could also support block sharing. >>> >> >> Can you stop a config which says: >> tc qdisc add dev ens9 root block 22 handle 1:0 prio ? > > Please see the iproute2 patches. Parsing of "block" command line option > is done inside q_ingress.c > I only looked at the kernel code. Good you can stop it at tc but the API does not stop it (unless you expect the rest of the world to only use tc). Really - there is no reason for this API to be only via ingress qdisc attributes. You can add a check in cls api to reject any parent that is not either of the clsacts + ingress (depending on tc doesnt sound right). cheers, jamal
Thu, Jan 11, 2018 at 03:46:09PM CET, jhs@mojatatu.com wrote: >On 18-01-11 09:41 AM, Jiri Pirko wrote: >> Thu, Jan 11, 2018 at 03:37:08PM CET, jhs@mojatatu.com wrote: >> > On 18-01-11 09:24 AM, Jiri Pirko wrote: >> > > Thu, Jan 11, 2018 at 02:36:01PM CET, jhs@mojatatu.com wrote: >> > > > On 18-01-09 09:07 AM, Jiri Pirko wrote: >> > > > > From: Jiri Pirko <jiri@mellanox.com> >> > > > > >> > > > > Benefit from the previously introduced shared filter blocks >> > > > > infrastructure and allow ingress and clsact qdisc instances to share >> > > > > filter blocks. The block index is coming from userspace as qdisc option. >> > > > >> > > > Didnt quiet follow why ingress is special and needs attributes to >> > > > set the block but other qdiscs didnt. >> > > >> > > Jamal, again, other qdiscs does not support block sharing. This patchset >> > > only adds support for sharing of block for ingress and clsact qdiscs. >> > > Later on, other qdiscs could also support block sharing. >> > > >> > >> > Can you stop a config which says: >> > tc qdisc add dev ens9 root block 22 handle 1:0 prio ? >> >> Please see the iproute2 patches. Parsing of "block" command line option >> is done inside q_ingress.c >> > >I only looked at the kernel code. Good you can stop it at tc >but the API does not stop it (unless you expect the rest of the >world to only use tc). Jamal, apparently, you did not looked at the kernel code either :) Look at the changes done in net/sched/sch_ingress.c - there is where the parsing of block attr takes place. >Really - there is no reason for this API to be only via ingress qdisc >attributes. You can add a check in cls api to reject any parent that is >not either of the clsacts + ingress (depending on tc doesnt sound >right). I was thinking to take this direction originally. To have another generic attr called TCA_BLOCK or something that would be used when qdisc is created. For ingress, what would work. But for clsact, you need to be able to specify 2 block during qdisc creation - one for ingress, one for egress. That's when I realized this has to be per-qdisc-type attr.
On Thu, Jan 11, 2018 at 7:07 AM, Jiri Pirko <jiri@resnulli.us> wrote: > Thu, Jan 11, 2018 at 03:46:09PM CET, jhs@mojatatu.com wrote: >>On 18-01-11 09:41 AM, Jiri Pirko wrote: >>> Thu, Jan 11, 2018 at 03:37:08PM CET, jhs@mojatatu.com wrote: >>> > On 18-01-11 09:24 AM, Jiri Pirko wrote: >>> > > Thu, Jan 11, 2018 at 02:36:01PM CET, jhs@mojatatu.com wrote: >>> > > > On 18-01-09 09:07 AM, Jiri Pirko wrote: >>> > > > > From: Jiri Pirko <jiri@mellanox.com> >>> > > > > >>> > > > > Benefit from the previously introduced shared filter blocks >>> > > > > infrastructure and allow ingress and clsact qdisc instances to share >>> > > > > filter blocks. The block index is coming from userspace as qdisc option. >>> > > > >>> > > > Didnt quiet follow why ingress is special and needs attributes to >>> > > > set the block but other qdiscs didnt. >>> > > >>> > > Jamal, again, other qdiscs does not support block sharing. This patchset >>> > > only adds support for sharing of block for ingress and clsact qdiscs. >>> > > Later on, other qdiscs could also support block sharing. >>> > > >>> > >>> > Can you stop a config which says: >>> > tc qdisc add dev ens9 root block 22 handle 1:0 prio ? >>> >>> Please see the iproute2 patches. Parsing of "block" command line option >>> is done inside q_ingress.c >>> >> >>I only looked at the kernel code. Good you can stop it at tc >>but the API does not stop it (unless you expect the rest of the >>world to only use tc). > > Jamal, apparently, you did not looked at the kernel code either :) > Look at the changes done in net/sched/sch_ingress.c - there is where the > parsing of block attr takes place. > > >>Really - there is no reason for this API to be only via ingress qdisc >>attributes. You can add a check in cls api to reject any parent that is >>not either of the clsacts + ingress (depending on tc doesnt sound >>right). > > I was thinking to take this direction originally. To have another > generic attr called TCA_BLOCK or something that would be used when qdisc > is created. For ingress, what would work. But for clsact, you need to be > able to specify 2 block during qdisc creation - one for ingress, one for > egress. That's when I realized this has to be per-qdisc-type attr. yeah, see the problem...but.., would it help if we just introduce two generic attrs TCA_BLOCK_INGRESS and TCA_BLOCK_EGRESS instead of having to duplicate these attrs at every qdisc ?. and add proper validation depending on qdisc type..
On 18-01-11 10:07 AM, Jiri Pirko wrote: > Thu, Jan 11, 2018 at 03:46:09PM CET, jhs@mojatatu.com wrote: >> On 18-01-11 09:41 AM, Jiri Pirko wrote: >>> Thu, Jan 11, 2018 at 03:37:08PM CET, jhs@mojatatu.com wrote: >> >> I only looked at the kernel code. Good you can stop it at tc >> but the API does not stop it (unless you expect the rest of the >> world to only use tc). > > Jamal, apparently, you did not looked at the kernel code either :) > Look at the changes done in net/sched/sch_ingress.c - there is where the > parsing of block attr takes place. > reason i raised it is from looking at tc_ctl_tfilter(). If i specify ifindex != TCM_IFINDEX_MAGIC_BLOCK, parent = 0XFFFF.... and block = 22 that should work, no? i.e regardless of whether parent is INGRESS etc. And so i was confused why you had attributes in sch_ingress.c > >> Really - there is no reason for this API to be only via ingress qdisc >> attributes. You can add a check in cls api to reject any parent that is >> not either of the clsacts + ingress (depending on tc doesnt sound >> right). > > I was thinking to take this direction originally. To have another > generic attr called TCA_BLOCK or something that would be used when qdisc > is created. For ingress, what would work. But for clsact, you need to be > able to specify 2 block during qdisc creation - one for ingress, one for > egress. That's when I realized this has to be per-qdisc-type attr. > ok for clsact - i can see that we dont have enough fields in the tcm message. TCA_BLOCK sounds appealing - could be a speacial tlv with many block ids maybe? I really would like to use this for egress as well - and what i described earlier should work for me. cheers, jamal
Thu, Jan 11, 2018 at 04:41:27PM CET, roopa@cumulusnetworks.com wrote: >On Thu, Jan 11, 2018 at 7:07 AM, Jiri Pirko <jiri@resnulli.us> wrote: >> Thu, Jan 11, 2018 at 03:46:09PM CET, jhs@mojatatu.com wrote: >>>On 18-01-11 09:41 AM, Jiri Pirko wrote: >>>> Thu, Jan 11, 2018 at 03:37:08PM CET, jhs@mojatatu.com wrote: >>>> > On 18-01-11 09:24 AM, Jiri Pirko wrote: >>>> > > Thu, Jan 11, 2018 at 02:36:01PM CET, jhs@mojatatu.com wrote: >>>> > > > On 18-01-09 09:07 AM, Jiri Pirko wrote: >>>> > > > > From: Jiri Pirko <jiri@mellanox.com> >>>> > > > > >>>> > > > > Benefit from the previously introduced shared filter blocks >>>> > > > > infrastructure and allow ingress and clsact qdisc instances to share >>>> > > > > filter blocks. The block index is coming from userspace as qdisc option. >>>> > > > >>>> > > > Didnt quiet follow why ingress is special and needs attributes to >>>> > > > set the block but other qdiscs didnt. >>>> > > >>>> > > Jamal, again, other qdiscs does not support block sharing. This patchset >>>> > > only adds support for sharing of block for ingress and clsact qdiscs. >>>> > > Later on, other qdiscs could also support block sharing. >>>> > > >>>> > >>>> > Can you stop a config which says: >>>> > tc qdisc add dev ens9 root block 22 handle 1:0 prio ? >>>> >>>> Please see the iproute2 patches. Parsing of "block" command line option >>>> is done inside q_ingress.c >>>> >>> >>>I only looked at the kernel code. Good you can stop it at tc >>>but the API does not stop it (unless you expect the rest of the >>>world to only use tc). >> >> Jamal, apparently, you did not looked at the kernel code either :) >> Look at the changes done in net/sched/sch_ingress.c - there is where the >> parsing of block attr takes place. >> >> >>>Really - there is no reason for this API to be only via ingress qdisc >>>attributes. You can add a check in cls api to reject any parent that is >>>not either of the clsacts + ingress (depending on tc doesnt sound >>>right). >> >> I was thinking to take this direction originally. To have another >> generic attr called TCA_BLOCK or something that would be used when qdisc >> is created. For ingress, what would work. But for clsact, you need to be >> able to specify 2 block during qdisc creation - one for ingress, one for >> egress. That's when I realized this has to be per-qdisc-type attr. > > >yeah, see the problem...but.., would it help if we just introduce two >generic attrs TCA_BLOCK_INGRESS and TCA_BLOCK_EGRESS instead of having >to duplicate these attrs at every qdisc ?. >and add proper validation depending on qdisc type.. I guess it would make sense. I just did not want to introduce this limitation. But allright. Will do this.
Thu, Jan 11, 2018 at 04:44:32PM CET, jhs@mojatatu.com wrote: >On 18-01-11 10:07 AM, Jiri Pirko wrote: >> Thu, Jan 11, 2018 at 03:46:09PM CET, jhs@mojatatu.com wrote: >> > On 18-01-11 09:41 AM, Jiri Pirko wrote: >> > > Thu, Jan 11, 2018 at 03:37:08PM CET, jhs@mojatatu.com wrote: > >> > >> > I only looked at the kernel code. Good you can stop it at tc >> > but the API does not stop it (unless you expect the rest of the >> > world to only use tc). >> >> Jamal, apparently, you did not looked at the kernel code either :) >> Look at the changes done in net/sched/sch_ingress.c - there is where the >> parsing of block attr takes place. >> > >reason i raised it is from looking at tc_ctl_tfilter(). >If i specify ifindex != TCM_IFINDEX_MAGIC_BLOCK, >parent = 0XFFFF.... and block = 22 that should work, no? >i.e regardless of whether parent is INGRESS etc. No, the block needs to be created first by qdisc instance. Seems to me that you are mixing apples and oranges a bit. > >And so i was confused why you had attributes in sch_ingress.c > >> >> > Really - there is no reason for this API to be only via ingress qdisc >> > attributes. You can add a check in cls api to reject any parent that is >> > not either of the clsacts + ingress (depending on tc doesnt sound >> > right). >> >> I was thinking to take this direction originally. To have another >> generic attr called TCA_BLOCK or something that would be used when qdisc >> is created. For ingress, what would work. But for clsact, you need to be >> able to specify 2 block during qdisc creation - one for ingress, one for >> egress. That's when I realized this has to be per-qdisc-type attr. >> > >ok for clsact - i can see that we dont have enough fields in the tcm >message. That is not a problem. Again, don't mix filter manipulation with qdisc manipulation. > >TCA_BLOCK sounds appealing - could be a speacial tlv with many block ids >maybe? I really would like to use this for egress as well - and what >i described earlier should work for me. I don't get what you mean by "tlv with many block ids". What is it good for? :O > >cheers, >jamal
On 18-01-11 11:15 AM, Jiri Pirko wrote: > Thu, Jan 11, 2018 at 04:44:32PM CET, jhs@mojatatu.com wrote: >> On 18-01-11 10:07 AM, Jiri Pirko wrote: >>> Thu, Jan 11, 2018 at 03:46:09PM CET, jhs@mojatatu.com wrote: >>>> On 18-01-11 09:41 AM, Jiri Pirko wrote: >>>>> Thu, Jan 11, 2018 at 03:37:08PM CET, jhs@mojatatu.com wrote: >> >>>> >>>> I only looked at the kernel code. Good you can stop it at tc >>>> but the API does not stop it (unless you expect the rest of the >>>> world to only use tc). >>> >>> Jamal, apparently, you did not looked at the kernel code either :) >>> Look at the changes done in net/sched/sch_ingress.c - there is where the >>> parsing of block attr takes place. >>> >> >> reason i raised it is from looking at tc_ctl_tfilter(). >> If i specify ifindex != TCM_IFINDEX_MAGIC_BLOCK, >> parent = 0XFFFF.... and block = 22 that should work, no? >> i.e regardless of whether parent is INGRESS etc. > > No, the block needs to be created first by qdisc instance. > Seems to me that you are mixing apples and oranges a bit. > You are correct, the qdisc attachment must exist first ;-> >> TCA_BLOCK sounds appealing - could be a speacial tlv with many block ids >> maybe? I really would like to use this for egress as well - and what >> i described earlier should work for me. > > I don't get what you mean by "tlv with many block ids". What is it good > for? :O > I meant A TLV with a bunch of 32 bit values, so you can have more than one block id in it. But what Roopa suggested is more explicit (and better). cheers, jamal
diff --git a/include/uapi/linux/pkt_sched.h b/include/uapi/linux/pkt_sched.h index 37b5096..8cc554a 100644 --- a/include/uapi/linux/pkt_sched.h +++ b/include/uapi/linux/pkt_sched.h @@ -934,4 +934,15 @@ enum { #define TCA_CBS_MAX (__TCA_CBS_MAX - 1) +/* Ingress/clsact */ + +enum { + TCA_CLSACT_UNSPEC, + TCA_CLSACT_INGRESS_BLOCK, + TCA_CLSACT_EGRESS_BLOCK, + __TCA_CLSACT_MAX +}; + +#define TCA_CLSACT_MAX (__TCA_CLSACT_MAX - 1) + #endif diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c index 7ca2be2..1bef8d4 100644 --- a/net/sched/sch_ingress.c +++ b/net/sched/sch_ingress.c @@ -61,6 +61,32 @@ static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv) struct mini_Qdisc_pair *miniqp = priv; mini_qdisc_pair_swap(miniqp, tp_head); +}; + +static const struct nla_policy ingress_policy[TCA_CLSACT_MAX + 1] = { + [TCA_CLSACT_INGRESS_BLOCK] = { .type = NLA_U32 }, +}; + +static int ingress_parse_opt(struct nlattr *opt, struct tcf_block_ext_info *ei, + struct netlink_ext_ack *extack) +{ + struct nlattr *tb[TCA_CLSACT_MAX + 1]; + int err; + + if (!opt) + return 0; + err = nla_parse_nested(tb, TCA_CLSACT_MAX, opt, ingress_policy, NULL); + if (err) + return err; + + if (tb[TCA_CLSACT_INGRESS_BLOCK]) { + ei->block_index = nla_get_u32(tb[TCA_CLSACT_INGRESS_BLOCK]); + if (!ei->block_index) { + NL_SET_ERR_MSG(extack, "Block index cannot be 0"); + return -EINVAL; + } + } + return 0; } static int ingress_init(struct Qdisc *sch, struct nlattr *opt, @@ -74,6 +100,10 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt, mini_qdisc_pair_init(&q->miniqp, sch, &dev->miniq_ingress); + err = ingress_parse_opt(opt, &q->block_info, extack); + if (err) + return err; + q->block_info.binder_type = TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS; q->block_info.chain_head_change = clsact_chain_head_change; q->block_info.chain_head_change_priv = &q->miniqp; @@ -97,11 +127,15 @@ static void ingress_destroy(struct Qdisc *sch) static int ingress_dump(struct Qdisc *sch, struct sk_buff *skb) { + struct ingress_sched_data *q = qdisc_priv(sch); struct nlattr *nest; nest = nla_nest_start(skb, TCA_OPTIONS); if (nest == NULL) goto nla_put_failure; + if (q->block->index && + nla_put_u32(skb, TCA_CLSACT_INGRESS_BLOCK, q->block->index)) + goto nla_put_failure; return nla_nest_end(skb, nest); @@ -170,6 +204,44 @@ static struct tcf_block *clsact_tcf_block(struct Qdisc *sch, unsigned long cl, } } +static const struct nla_policy clsact_policy[TCA_CLSACT_MAX + 1] = { + [TCA_CLSACT_INGRESS_BLOCK] = { .type = NLA_U32 }, + [TCA_CLSACT_EGRESS_BLOCK] = { .type = NLA_U32 }, +}; + +static int clsact_parse_opt(struct nlattr *opt, + struct tcf_block_ext_info *ei_ingress, + struct tcf_block_ext_info *ei_egress, + struct netlink_ext_ack *extack) +{ + struct nlattr *tb[TCA_CLSACT_MAX + 1]; + int err; + + if (!opt) + return 0; + err = nla_parse_nested(tb, TCA_CLSACT_MAX, opt, clsact_policy, NULL); + if (err) + return err; + + if (tb[TCA_CLSACT_INGRESS_BLOCK]) { + ei_ingress->block_index = + nla_get_u32(tb[TCA_CLSACT_INGRESS_BLOCK]); + if (!ei_ingress->block_index) { + NL_SET_ERR_MSG(extack, "Block index cannot be 0"); + return -EINVAL; + } + } + if (tb[TCA_CLSACT_EGRESS_BLOCK]) { + ei_egress->block_index = + nla_get_u32(tb[TCA_CLSACT_EGRESS_BLOCK]); + if (!ei_egress->block_index) { + NL_SET_ERR_MSG(extack, "Block index cannot be 0"); + return -EINVAL; + } + } + return 0; +} + static int clsact_init(struct Qdisc *sch, struct nlattr *opt, struct netlink_ext_ack *extack) { @@ -182,6 +254,11 @@ static int clsact_init(struct Qdisc *sch, struct nlattr *opt, mini_qdisc_pair_init(&q->miniqp_ingress, sch, &dev->miniq_ingress); + err = clsact_parse_opt(opt, &q->ingress_block_info, + &q->egress_block_info, extack); + if (err) + return err; + q->ingress_block_info.binder_type = TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS; q->ingress_block_info.chain_head_change = clsact_chain_head_change; q->ingress_block_info.chain_head_change_priv = &q->miniqp_ingress; @@ -218,6 +295,28 @@ static void clsact_destroy(struct Qdisc *sch) net_dec_egress_queue(); } +static int clsact_dump(struct Qdisc *sch, struct sk_buff *skb) +{ + struct clsact_sched_data *q = qdisc_priv(sch); + struct nlattr *nest; + + nest = nla_nest_start(skb, TCA_OPTIONS); + if (!nest) + goto nla_put_failure; + if (q->ingress_block->index && + nla_put_u32(skb, TCA_CLSACT_INGRESS_BLOCK, q->ingress_block->index)) + goto nla_put_failure; + if (q->egress_block->index && + nla_put_u32(skb, TCA_CLSACT_EGRESS_BLOCK, q->egress_block->index)) + goto nla_put_failure; + + return nla_nest_end(skb, nest); + +nla_put_failure: + nla_nest_cancel(skb, nest); + return -1; +} + static const struct Qdisc_class_ops clsact_class_ops = { .leaf = ingress_leaf, .find = clsact_find, @@ -233,7 +332,7 @@ static struct Qdisc_ops clsact_qdisc_ops __read_mostly = { .priv_size = sizeof(struct clsact_sched_data), .init = clsact_init, .destroy = clsact_destroy, - .dump = ingress_dump, + .dump = clsact_dump, .owner = THIS_MODULE, };