Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules
On 16-02-26 02:29 AM, Jiri Pirko wrote: > Fri, Feb 26, 2016 at 12:20:45AM CET, john.fastab...@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 >> --- >> 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.
Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules
Fri, Feb 26, 2016 at 12:20:45AM CET, john.fastab...@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 >--- > 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?
Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules
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. >> >
Re: [net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules
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 --- 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(-) @@ -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
[net-next PATCH v2 3/3] net: sched: cls_u32 add bit to specify software only rules
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 --- 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