* [net-next PATCH v2 0/3] tc software only flag
@ 2016-02-25 23:19 John Fastabend
2016-02-25 23:19 ` [net-next PATCH v2 1/3] net: sched: consolidate offload decision in cls_u32 John Fastabend
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: John Fastabend @ 2016-02-25 23:19 UTC (permalink / raw)
To: jiri, john.fastabend, daniel, simon.horman
Cc: netdev, alexei.starovoitov, davem, jhs
This adds a software only flag to tc but incorporates a bunch of comments
from the original attempt at this.
First instead of having the offload decision logic be embedded in cls_u32
I lifted into cls_pkt.h so it can be used anywhere.
In order to do this I put the flag defines in pkt_cls.h as well. However
it was suggested that perhaps these flags could be lifted into the
upper layer of TCA_ as well but I'm afraid this can not be done with
existing tc design as far as I can tell. The problem is the filters are
packed and unpacked in the classifier specific code and pushing the flags
through the high level doesn't seem easily doable. And we already have
this design where classifiers handle generic options such as actions and
policers. So I think adding one more thing here is OK as 'tc', et. al.
already know how to handle this type of thing.
Thanks,
.John
---
John Fastabend (3):
net: sched: consolidate offload decision in cls_u32
net: cls_u32: move TC offload feature bit into cls_u32 offload logic
net: sched: cls_u32 add bit to specify software only rules
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 --
include/net/pkt_cls.h | 17 +++++++++++
include/uapi/linux/pkt_cls.h | 1 +
net/sched/cls_u32.c | 37 ++++++++++++++++++-------
4 files changed, 45 insertions(+), 13 deletions(-)
^ permalink raw reply [flat|nested] 9+ messages in thread* [net-next PATCH v2 1/3] net: sched: consolidate offload decision in cls_u32 2016-02-25 23:19 [net-next PATCH v2 0/3] tc software only flag John Fastabend @ 2016-02-25 23:19 ` John Fastabend 2016-02-25 23:20 ` [net-next PATCH v2 2/3] net: cls_u32: move TC offload feature bit into cls_u32 offload logic John Fastabend ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: John Fastabend @ 2016-02-25 23:19 UTC (permalink / raw) To: jiri, john.fastabend, daniel, simon.horman Cc: netdev, alexei.starovoitov, davem, jhs The offload decision was originally very basic and tied to if the dev implemented the appropriate ndo op hook. The next step is to allow the user to more flexibly define if any paticular rule should be offloaded or not. In order to have this logic in one function lift the current check into a helper routine tc_should_offload(). Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- include/net/pkt_cls.h | 5 +++++ net/sched/cls_u32.c | 8 ++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 2121df5..e64d20b 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -392,4 +392,9 @@ struct tc_cls_u32_offload { }; }; +static inline bool tc_should_offload(struct net_device *dev) +{ + return dev->netdev_ops->ndo_setup_tc; +} + #endif diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index d54bc94..24e888b 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -434,7 +434,7 @@ static void u32_remove_hw_knode(struct tcf_proto *tp, u32 handle) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (dev->netdev_ops->ndo_setup_tc) { + if (tc_should_offload(dev)) { offload.cls_u32->command = TC_CLSU32_DELETE_KNODE; offload.cls_u32->knode.handle = handle; dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle, @@ -451,7 +451,7 @@ static void u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (dev->netdev_ops->ndo_setup_tc) { + if (tc_should_offload(dev)) { offload.cls_u32->command = TC_CLSU32_NEW_HNODE; offload.cls_u32->hnode.divisor = h->divisor; offload.cls_u32->hnode.handle = h->handle; @@ -471,7 +471,7 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (dev->netdev_ops->ndo_setup_tc) { + if (tc_should_offload(dev)) { offload.cls_u32->command = TC_CLSU32_DELETE_HNODE; offload.cls_u32->hnode.divisor = h->divisor; offload.cls_u32->hnode.handle = h->handle; @@ -491,7 +491,7 @@ static void u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (dev->netdev_ops->ndo_setup_tc) { + if (tc_should_offload(dev)) { offload.cls_u32->command = TC_CLSU32_REPLACE_KNODE; offload.cls_u32->knode.handle = n->handle; offload.cls_u32->knode.fshift = n->fshift; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [net-next PATCH v2 2/3] net: cls_u32: move TC offload feature bit into cls_u32 offload logic 2016-02-25 23:19 [net-next PATCH v2 0/3] tc software only flag John Fastabend 2016-02-25 23:19 ` [net-next PATCH v2 1/3] net: sched: consolidate offload decision in cls_u32 John Fastabend @ 2016-02-25 23:20 ` John Fastabend 2016-02-25 23:20 ` [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules John Fastabend 2016-02-25 23:24 ` [net-next PATCH v2 0/3] tc software only flag John Fastabend 3 siblings, 0 replies; 9+ messages in thread From: John Fastabend @ 2016-02-25 23:20 UTC (permalink / raw) To: jiri, john.fastabend, daniel, simon.horman Cc: netdev, alexei.starovoitov, davem, jhs In the original series drivers would get offload requests for cls_u32 rules even if the feature bit is disabled. This meant the driver had to do a boiler plate check on the feature bit before adding/deleting the rule. This patch lifts the check into the core code and removes it from the driver specific case. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 --- include/net/pkt_cls.h | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index cf4b729..b893ff8 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -8400,9 +8400,6 @@ int __ixgbe_setup_tc(struct net_device *dev, u32 handle, __be16 proto, if (TC_H_MAJ(handle) == TC_H_MAJ(TC_H_INGRESS) && tc->type == TC_SETUP_CLSU32) { - if (!(dev->features & NETIF_F_HW_TC)) - return -EINVAL; - switch (tc->cls_u32->command) { case TC_CLSU32_NEW_KNODE: case TC_CLSU32_REPLACE_KNODE: diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index e64d20b..6096e96 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -394,6 +394,9 @@ struct tc_cls_u32_offload { static inline bool tc_should_offload(struct net_device *dev) { + if (!(dev->features & NETIF_F_HW_TC)) + return false; + return dev->netdev_ops->ndo_setup_tc; } ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules 2016-02-25 23:19 [net-next PATCH v2 0/3] tc software only flag John Fastabend 2016-02-25 23:19 ` [net-next PATCH v2 1/3] net: sched: consolidate offload decision in cls_u32 John Fastabend 2016-02-25 23:20 ` [net-next PATCH v2 2/3] net: cls_u32: move TC offload feature bit into cls_u32 offload logic John Fastabend @ 2016-02-25 23:20 ` John Fastabend 2016-02-26 7:02 ` Samudrala, Sridhar 2016-02-26 10:29 ` Jiri Pirko 2016-02-25 23:24 ` [net-next PATCH v2 0/3] tc software only flag John Fastabend 3 siblings, 2 replies; 9+ messages in thread From: John Fastabend @ 2016-02-25 23:20 UTC (permalink / raw) To: jiri, john.fastabend, daniel, simon.horman Cc: netdev, alexei.starovoitov, davem, jhs In the initial implementation the only way to stop a rule from being inserted into the hardware table was via the device feature flag. However this doesn't work well when working on an end host system where packets are expect to hit both the hardware and software datapaths. For example we can imagine a rule that will match an IP address and increment a field. If we install this rule in both hardware and software we may increment the field twice. To date we have only added support for the drop action so we have been able to ignore these cases. But as we extend the action support we will hit this example plus more such cases. Arguably these are not even corner cases in many working systems these cases will be common. To avoid forcing the driver to always abort (i.e. the above example) this patch adds a flag to add a rule in software only. A careful user can use this flag to build software and hardware datapaths that work together. One example we have found particularly useful is to use hardware resources to set the skb->mark on the skb when the match may be expensive to run in software but a mark lookup in a hash table is cheap. The idea here is hardware can do in one lookup what the u32 classifier may need to traverse multiple lists and hash tables to compute. The flag is only passed down on inserts on deletion to avoid stale references in hardware we always try to remove a rule if it exists. The flags field is part of the classifier specific options. Although it is tempting to lift this into the generic structure doing this proves difficult do to how the tc netlink attributes are implemented along with how the dump/change routines are called. There is also precedence for putting seemingly generic pieces in the specific classifier options such as TCA_U32_POLICE, TCA_U32_ACT, etc. So although not ideal I've left FLAGS in the u32 options as well as it simplifies the code greatly and user space has already learned how to manage these bits ala 'tc' tool. Another thing if trying to update a rule we require the flags to be unchanged. This is to force user space, software u32 and the hardware u32 to keep in sync. Thanks to Simon Horman for catching this case. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- include/net/pkt_cls.h | 13 +++++++++++-- include/uapi/linux/pkt_cls.h | 1 + net/sched/cls_u32.c | 37 +++++++++++++++++++++++++++---------- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index 6096e96..42dc412 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -392,12 +392,21 @@ struct tc_cls_u32_offload { }; }; -static inline bool tc_should_offload(struct net_device *dev) +/* tca flags definitions */ +#define TCA_CLS_FLAGS_SOFTWARE 1 + +static inline bool tc_should_offload(struct net_device *dev, u32 flags) { if (!(dev->features & NETIF_F_HW_TC)) return false; - return dev->netdev_ops->ndo_setup_tc; + if (flags & TCA_CLS_FLAGS_SOFTWARE) + return false; + + if (!dev->netdev_ops->ndo_setup_tc) + return false; + + return true; } #endif diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index 4398737..9874f568 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -172,6 +172,7 @@ enum { TCA_U32_INDEV, TCA_U32_PCNT, TCA_U32_MARK, + TCA_U32_FLAGS, __TCA_U32_MAX }; diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c index 24e888b..6d4450d 100644 --- a/net/sched/cls_u32.c +++ b/net/sched/cls_u32.c @@ -59,6 +59,7 @@ struct tc_u_knode { #ifdef CONFIG_CLS_U32_PERF struct tc_u32_pcnt __percpu *pf; #endif + u32 flags; #ifdef CONFIG_CLS_U32_MARK u32 val; u32 mask; @@ -434,7 +435,7 @@ static void u32_remove_hw_knode(struct tcf_proto *tp, u32 handle) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (tc_should_offload(dev)) { + if (tc_should_offload(dev, 0)) { offload.cls_u32->command = TC_CLSU32_DELETE_KNODE; offload.cls_u32->knode.handle = handle; dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle, @@ -442,7 +443,9 @@ static void u32_remove_hw_knode(struct tcf_proto *tp, u32 handle) } } -static void u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) +static void u32_replace_hw_hnode(struct tcf_proto *tp, + struct tc_u_hnode *h, + u32 flags) { struct net_device *dev = tp->q->dev_queue->dev; struct tc_cls_u32_offload u32_offload = {0}; @@ -451,7 +454,7 @@ static void u32_replace_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (tc_should_offload(dev)) { + if (tc_should_offload(dev, flags)) { offload.cls_u32->command = TC_CLSU32_NEW_HNODE; offload.cls_u32->hnode.divisor = h->divisor; offload.cls_u32->hnode.handle = h->handle; @@ -471,7 +474,7 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (tc_should_offload(dev)) { + if (tc_should_offload(dev, 0)) { offload.cls_u32->command = TC_CLSU32_DELETE_HNODE; offload.cls_u32->hnode.divisor = h->divisor; offload.cls_u32->hnode.handle = h->handle; @@ -482,7 +485,9 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) } } -static void u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n) +static void u32_replace_hw_knode(struct tcf_proto *tp, + struct tc_u_knode *n, + u32 flags) { struct net_device *dev = tp->q->dev_queue->dev; struct tc_cls_u32_offload u32_offload = {0}; @@ -491,7 +496,7 @@ static void u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n) offload.type = TC_SETUP_CLSU32; offload.cls_u32 = &u32_offload; - if (tc_should_offload(dev)) { + if (tc_should_offload(dev, flags)) { offload.cls_u32->command = TC_CLSU32_REPLACE_KNODE; offload.cls_u32->knode.handle = n->handle; offload.cls_u32->knode.fshift = n->fshift; @@ -679,6 +684,7 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = { [TCA_U32_SEL] = { .len = sizeof(struct tc_u32_sel) }, [TCA_U32_INDEV] = { .type = NLA_STRING, .len = IFNAMSIZ }, [TCA_U32_MARK] = { .len = sizeof(struct tc_u32_mark) }, + [TCA_U32_FLAGS] = { .len = NLA_U32 }, }; static int u32_set_parms(struct net *net, struct tcf_proto *tp, @@ -786,6 +792,7 @@ static struct tc_u_knode *u32_init_knode(struct tcf_proto *tp, #endif new->fshift = n->fshift; new->res = n->res; + new->flags = n->flags; RCU_INIT_POINTER(new->ht_down, n->ht_down); /* bump reference count as long as we hold pointer to structure */ @@ -825,7 +832,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb, struct tc_u32_sel *s; struct nlattr *opt = tca[TCA_OPTIONS]; struct nlattr *tb[TCA_U32_MAX + 1]; - u32 htid; + u32 htid, flags = 0; int err; #ifdef CONFIG_CLS_U32_PERF size_t size; @@ -838,6 +845,9 @@ static int u32_change(struct net *net, struct sk_buff *in_skb, if (err < 0) return err; + if (tb[TCA_U32_FLAGS]) + flags = nla_get_u32(tb[TCA_U32_FLAGS]); + n = (struct tc_u_knode *)*arg; if (n) { struct tc_u_knode *new; @@ -845,6 +855,9 @@ static int u32_change(struct net *net, struct sk_buff *in_skb, if (TC_U32_KEY(n->handle) == 0) return -EINVAL; + if (n->flags != flags) + return -EINVAL; + new = u32_init_knode(tp, n); if (!new) return -ENOMEM; @@ -861,7 +874,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb, u32_replace_knode(tp, tp_c, new); tcf_unbind_filter(tp, &n->res); call_rcu(&n->rcu, u32_delete_key_rcu); - u32_replace_hw_knode(tp, new); + u32_replace_hw_knode(tp, new, flags); return 0; } @@ -889,7 +902,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb, rcu_assign_pointer(tp_c->hlist, ht); *arg = (unsigned long)ht; - u32_replace_hw_hnode(tp, ht); + u32_replace_hw_hnode(tp, ht, flags); return 0; } @@ -940,6 +953,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb, RCU_INIT_POINTER(n->ht_up, ht); n->handle = handle; n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0; + n->flags = flags; tcf_exts_init(&n->exts, TCA_U32_ACT, TCA_U32_POLICE); n->tp = tp; @@ -972,7 +986,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb, RCU_INIT_POINTER(n->next, pins); rcu_assign_pointer(*ins, n); - u32_replace_hw_knode(tp, n); + u32_replace_hw_knode(tp, n, flags); *arg = (unsigned long)n; return 0; } @@ -1077,6 +1091,9 @@ static int u32_dump(struct net *net, struct tcf_proto *tp, unsigned long fh, nla_put_u32(skb, TCA_U32_LINK, ht_down->handle)) goto nla_put_failure; + if (n->flags && nla_put_u32(skb, TCA_U32_FLAGS, n->flags)) + goto nla_put_failure; + #ifdef CONFIG_CLS_U32_MARK if ((n->val || n->mask)) { struct tc_u32_mark mark = {.val = n->val, ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules 2016-02-25 23:20 ` [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules John Fastabend @ 2016-02-26 7:02 ` Samudrala, Sridhar 2016-02-26 7:21 ` John Fastabend 2016-02-26 10:29 ` Jiri Pirko 1 sibling, 1 reply; 9+ messages in thread From: Samudrala, Sridhar @ 2016-02-26 7:02 UTC (permalink / raw) To: John Fastabend, jiri, daniel, simon.horman Cc: netdev, alexei.starovoitov, davem, jhs On 2/25/2016 3:20 PM, John Fastabend wrote: > In the initial implementation the only way to stop a rule from being > inserted into the hardware table was via the device feature flag. > However this doesn't work well when working on an end host system > where packets are expect to hit both the hardware and software > datapaths. > > For example we can imagine a rule that will match an IP address and > increment a field. If we install this rule in both hardware and > software we may increment the field twice. To date we have only > added support for the drop action so we have been able to ignore > these cases. But as we extend the action support we will hit this > example plus more such cases. Arguably these are not even corner > cases in many working systems these cases will be common. > > To avoid forcing the driver to always abort (i.e. the above example) > this patch adds a flag to add a rule in software only. A careful > user can use this flag to build software and hardware datapaths > that work together. One example we have found particularly useful > is to use hardware resources to set the skb->mark on the skb when > the match may be expensive to run in software but a mark lookup > in a hash table is cheap. The idea here is hardware can do in one > lookup what the u32 classifier may need to traverse multiple lists > and hash tables to compute. The flag is only passed down on inserts > on deletion to avoid stale references in hardware we always try I think this is supposed to be a new sentence starting with 'On deletion' > to remove a rule if it exists. > > The flags field is part of the classifier specific options. Although > it is tempting to lift this into the generic structure doing this > proves difficult do to how the tc netlink attributes are implemented > along with how the dump/change routines are called. There is also > precedence for putting seemingly generic pieces in the specific > classifier options such as TCA_U32_POLICE, TCA_U32_ACT, etc. So > although not ideal I've left FLAGS in the u32 options as well as it > simplifies the code greatly and user space has already learned how > to manage these bits ala 'tc' tool. > > Another thing if trying to update a rule we require the flags to > be unchanged. This is to force user space, software u32 and > the hardware u32 to keep in sync. Thanks to Simon Horman for > catching this case. > > Signed-off-by: John Fastabend <john.r.fastabend@intel.com> > --- > include/net/pkt_cls.h | 13 +++++++++++-- > include/uapi/linux/pkt_cls.h | 1 + > net/sched/cls_u32.c | 37 +++++++++++++++++++++++++++---------- > 3 files changed, 39 insertions(+), 12 deletions(-) <snip> > > @@ -482,7 +485,9 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h) > } > } > > -static void u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n) > +static void u32_replace_hw_knode(struct tcf_proto *tp, > + struct tc_u_knode *n, > + u32 flags) > { > struct net_device *dev = tp->q->dev_queue->dev; > struct tc_cls_u32_offload u32_offload = {0}; > @@ -491,7 +496,7 @@ static void u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n) > offload.type = TC_SETUP_CLSU32; > offload.cls_u32 = &u32_offload; > > - if (tc_should_offload(dev)) { > + if (tc_should_offload(dev, flags)) { > offload.cls_u32->command = TC_CLSU32_REPLACE_KNODE; > offload.cls_u32->knode.handle = n->handle; > offload.cls_u32->knode.fshift = n->fshift; > @@ -679,6 +684,7 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = { > [TCA_U32_SEL] = { .len = sizeof(struct tc_u32_sel) }, > [TCA_U32_INDEV] = { .type = NLA_STRING, .len = IFNAMSIZ }, > [TCA_U32_MARK] = { .len = sizeof(struct tc_u32_mark) }, > + [TCA_U32_FLAGS] = { .len = NLA_U32 }, should be .type = NLA_U32 > <snip> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules 2016-02-26 7:02 ` Samudrala, Sridhar @ 2016-02-26 7:21 ` John Fastabend 0 siblings, 0 replies; 9+ messages in thread From: John Fastabend @ 2016-02-26 7:21 UTC (permalink / raw) To: Samudrala, Sridhar, jiri, daniel, simon.horman Cc: netdev, alexei.starovoitov, davem, jhs On 16-02-25 11:02 PM, Samudrala, Sridhar wrote: > On 2/25/2016 3:20 PM, John Fastabend wrote: >> In the initial implementation the only way to stop a rule from being >> inserted into the hardware table was via the device feature flag. >> However this doesn't work well when working on an end host system >> where packets are expect to hit both the hardware and software >> datapaths. >> >> For example we can imagine a rule that will match an IP address and >> increment a field. If we install this rule in both hardware and >> software we may increment the field twice. To date we have only >> added support for the drop action so we have been able to ignore >> these cases. But as we extend the action support we will hit this >> example plus more such cases. Arguably these are not even corner >> cases in many working systems these cases will be common. >> >> To avoid forcing the driver to always abort (i.e. the above example) >> this patch adds a flag to add a rule in software only. A careful >> user can use this flag to build software and hardware datapaths >> that work together. One example we have found particularly useful >> is to use hardware resources to set the skb->mark on the skb when >> the match may be expensive to run in software but a mark lookup >> in a hash table is cheap. The idea here is hardware can do in one >> lookup what the u32 classifier may need to traverse multiple lists >> and hash tables to compute. The flag is only passed down on inserts >> on deletion to avoid stale references in hardware we always try > > I think this is supposed to be a new sentence starting with 'On deletion' Yep. >> to remove a rule if it exists. >> >> The flags field is part of the classifier specific options. Although >> it is tempting to lift this into the generic structure doing this >> proves difficult do to how the tc netlink attributes are implemented >> along with how the dump/change routines are called. There is also >> precedence for putting seemingly generic pieces in the specific >> classifier options such as TCA_U32_POLICE, TCA_U32_ACT, etc. So >> although not ideal I've left FLAGS in the u32 options as well as it >> simplifies the code greatly and user space has already learned how >> to manage these bits ala 'tc' tool. >> >> Another thing if trying to update a rule we require the flags to >> be unchanged. This is to force user space, software u32 and >> the hardware u32 to keep in sync. Thanks to Simon Horman for >> catching this case. >> [...] >> u32_policy[TCA_U32_MAX + 1] = { >> [TCA_U32_SEL] = { .len = sizeof(struct tc_u32_sel) }, >> [TCA_U32_INDEV] = { .type = NLA_STRING, .len = IFNAMSIZ }, >> [TCA_U32_MARK] = { .len = sizeof(struct tc_u32_mark) }, >> + [TCA_U32_FLAGS] = { .len = NLA_U32 }, > should be .type = NLA_U32 > Yep stupid typo. I think I'm going to write some smatch files to catch these sorts of things they should be detectable pragmatically. Thanks. >> <snip> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules 2016-02-25 23:20 ` [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules John Fastabend 2016-02-26 7:02 ` Samudrala, Sridhar @ 2016-02-26 10:29 ` Jiri Pirko 2016-02-26 14:29 ` John Fastabend 1 sibling, 1 reply; 9+ messages in thread From: Jiri Pirko @ 2016-02-26 10:29 UTC (permalink / raw) To: John Fastabend Cc: daniel, simon.horman, netdev, alexei.starovoitov, davem, jhs Fri, Feb 26, 2016 at 12:20:45AM CET, john.fastabend@gmail.com wrote: >In the initial implementation the only way to stop a rule from being >inserted into the hardware table was via the device feature flag. >However this doesn't work well when working on an end host system >where packets are expect to hit both the hardware and software >datapaths. > >For example we can imagine a rule that will match an IP address and >increment a field. If we install this rule in both hardware and >software we may increment the field twice. To date we have only >added support for the drop action so we have been able to ignore >these cases. But as we extend the action support we will hit this >example plus more such cases. Arguably these are not even corner >cases in many working systems these cases will be common. > >To avoid forcing the driver to always abort (i.e. the above example) >this patch adds a flag to add a rule in software only. A careful >user can use this flag to build software and hardware datapaths >that work together. One example we have found particularly useful >is to use hardware resources to set the skb->mark on the skb when >the match may be expensive to run in software but a mark lookup >in a hash table is cheap. The idea here is hardware can do in one >lookup what the u32 classifier may need to traverse multiple lists >and hash tables to compute. The flag is only passed down on inserts >on deletion to avoid stale references in hardware we always try >to remove a rule if it exists. > >The flags field is part of the classifier specific options. Although >it is tempting to lift this into the generic structure doing this >proves difficult do to how the tc netlink attributes are implemented >along with how the dump/change routines are called. There is also >precedence for putting seemingly generic pieces in the specific >classifier options such as TCA_U32_POLICE, TCA_U32_ACT, etc. So >although not ideal I've left FLAGS in the u32 options as well as it >simplifies the code greatly and user space has already learned how >to manage these bits ala 'tc' tool. > >Another thing if trying to update a rule we require the flags to >be unchanged. This is to force user space, software u32 and >the hardware u32 to keep in sync. Thanks to Simon Horman for >catching this case. > >Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >--- > include/net/pkt_cls.h | 13 +++++++++++-- > include/uapi/linux/pkt_cls.h | 1 + > net/sched/cls_u32.c | 37 +++++++++++++++++++++++++++---------- > 3 files changed, 39 insertions(+), 12 deletions(-) > >diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >index 6096e96..42dc412 100644 >--- a/include/net/pkt_cls.h >+++ b/include/net/pkt_cls.h >@@ -392,12 +392,21 @@ struct tc_cls_u32_offload { > }; > }; > >-static inline bool tc_should_offload(struct net_device *dev) >+/* tca flags definitions */ >+#define TCA_CLS_FLAGS_SOFTWARE 1 I'm sorry, the flag name is misleading to me. We have by default, both SW and HW. Now this flag should say: "do not push to HW". In future, there will be another flag saying: "do not push to SW". So I suggest what I already suggested before: TCA_CLS_FLAGS_SKIP_HW for this one and TCA_CLS_FLAGS_SKIP_KERNEL for the future one. Sounds sane? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules 2016-02-26 10:29 ` Jiri Pirko @ 2016-02-26 14:29 ` John Fastabend 0 siblings, 0 replies; 9+ messages in thread From: John Fastabend @ 2016-02-26 14:29 UTC (permalink / raw) To: Jiri Pirko; +Cc: daniel, simon.horman, netdev, alexei.starovoitov, davem, jhs On 16-02-26 02:29 AM, Jiri Pirko wrote: > Fri, Feb 26, 2016 at 12:20:45AM CET, john.fastabend@gmail.com wrote: >> In the initial implementation the only way to stop a rule from being >> inserted into the hardware table was via the device feature flag. >> However this doesn't work well when working on an end host system >> where packets are expect to hit both the hardware and software >> datapaths. >> >> For example we can imagine a rule that will match an IP address and >> increment a field. If we install this rule in both hardware and >> software we may increment the field twice. To date we have only >> added support for the drop action so we have been able to ignore >> these cases. But as we extend the action support we will hit this >> example plus more such cases. Arguably these are not even corner >> cases in many working systems these cases will be common. >> >> To avoid forcing the driver to always abort (i.e. the above example) >> this patch adds a flag to add a rule in software only. A careful >> user can use this flag to build software and hardware datapaths >> that work together. One example we have found particularly useful >> is to use hardware resources to set the skb->mark on the skb when >> the match may be expensive to run in software but a mark lookup >> in a hash table is cheap. The idea here is hardware can do in one >> lookup what the u32 classifier may need to traverse multiple lists >> and hash tables to compute. The flag is only passed down on inserts >> on deletion to avoid stale references in hardware we always try >> to remove a rule if it exists. >> >> The flags field is part of the classifier specific options. Although >> it is tempting to lift this into the generic structure doing this >> proves difficult do to how the tc netlink attributes are implemented >> along with how the dump/change routines are called. There is also >> precedence for putting seemingly generic pieces in the specific >> classifier options such as TCA_U32_POLICE, TCA_U32_ACT, etc. So >> although not ideal I've left FLAGS in the u32 options as well as it >> simplifies the code greatly and user space has already learned how >> to manage these bits ala 'tc' tool. >> >> Another thing if trying to update a rule we require the flags to >> be unchanged. This is to force user space, software u32 and >> the hardware u32 to keep in sync. Thanks to Simon Horman for >> catching this case. >> >> Signed-off-by: John Fastabend <john.r.fastabend@intel.com> >> --- >> include/net/pkt_cls.h | 13 +++++++++++-- >> include/uapi/linux/pkt_cls.h | 1 + >> net/sched/cls_u32.c | 37 +++++++++++++++++++++++++++---------- >> 3 files changed, 39 insertions(+), 12 deletions(-) >> >> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h >> index 6096e96..42dc412 100644 >> --- a/include/net/pkt_cls.h >> +++ b/include/net/pkt_cls.h >> @@ -392,12 +392,21 @@ struct tc_cls_u32_offload { >> }; >> }; >> >> -static inline bool tc_should_offload(struct net_device *dev) >> +/* tca flags definitions */ >> +#define TCA_CLS_FLAGS_SOFTWARE 1 > > I'm sorry, the flag name is misleading to me. > We have by default, both SW and HW. > > Now this flag should say: "do not push to HW". > > In future, there will be another flag saying: "do not push to SW". > > So I suggest what I already suggested before: > > TCA_CLS_FLAGS_SKIP_HW for this one and > TCA_CLS_FLAGS_SKIP_KERNEL for the future one. > > Sounds sane? > Sounds reasonable I'll change it in the next revision. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [net-next PATCH v2 0/3] tc software only flag 2016-02-25 23:19 [net-next PATCH v2 0/3] tc software only flag John Fastabend ` (2 preceding siblings ...) 2016-02-25 23:20 ` [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules John Fastabend @ 2016-02-25 23:24 ` John Fastabend 3 siblings, 0 replies; 9+ messages in thread From: John Fastabend @ 2016-02-25 23:24 UTC (permalink / raw) To: jiri, daniel, simon.horman; +Cc: netdev, alexei.starovoitov, davem, jhs On 16-02-25 03:19 PM, John Fastabend wrote: > This adds a software only flag to tc but incorporates a bunch of comments > from the original attempt at this. In case its not entirely obvious I dropped the hardware only case for now because I'm investigating Jiri's comment. But just having the software only case lets me handle many use cases. Now in the u32 classifier I can build graphs where part of the graph is software only and other parts are sw/hw. This lets me use part of the mark bit or queue information to learn if the hardware has already acted on the skb and then parse it differently in the classifier. > > First instead of having the offload decision logic be embedded in cls_u32 > I lifted into cls_pkt.h so it can be used anywhere. > > In order to do this I put the flag defines in pkt_cls.h as well. However > it was suggested that perhaps these flags could be lifted into the > upper layer of TCA_ as well but I'm afraid this can not be done with > existing tc design as far as I can tell. The problem is the filters are > packed and unpacked in the classifier specific code and pushing the flags > through the high level doesn't seem easily doable. And we already have > this design where classifiers handle generic options such as actions and > policers. So I think adding one more thing here is OK as 'tc', et. al. > already know how to handle this type of thing. > > Thanks, > .John > > --- > > John Fastabend (3): > net: sched: consolidate offload decision in cls_u32 > net: cls_u32: move TC offload feature bit into cls_u32 offload logic > net: sched: cls_u32 add bit to specify software only rules > > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 -- > include/net/pkt_cls.h | 17 +++++++++++ > include/uapi/linux/pkt_cls.h | 1 + > net/sched/cls_u32.c | 37 ++++++++++++++++++------- > 4 files changed, 45 insertions(+), 13 deletions(-) > > -- > Signature > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-02-26 14:29 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-02-25 23:19 [net-next PATCH v2 0/3] tc software only flag John Fastabend 2016-02-25 23:19 ` [net-next PATCH v2 1/3] net: sched: consolidate offload decision in cls_u32 John Fastabend 2016-02-25 23:20 ` [net-next PATCH v2 2/3] net: cls_u32: move TC offload feature bit into cls_u32 offload logic John Fastabend 2016-02-25 23:20 ` [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules John Fastabend 2016-02-26 7:02 ` Samudrala, Sridhar 2016-02-26 7:21 ` John Fastabend 2016-02-26 10:29 ` Jiri Pirko 2016-02-26 14:29 ` John Fastabend 2016-02-25 23:24 ` [net-next PATCH v2 0/3] tc software only flag John Fastabend
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.