[net-next PATCH v3 0/3] tc software only

[net-next PATCH v3 0/3] tc software only

[John Fastabend]

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 and named the flag
TCA_CLS_FLAGS_SKIP_HW (Thanks Jiri ;)

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(-)

[net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[John Fastabend]

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;

[net-next PATCH v3 2/3] net: cls_u32: move TC offload feature bit into cls_u32 offload logic

[John Fastabend]

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;
 }
 

[net-next PATCH v3 3/3] net: sched: cls_u32 add bit to specify software only rules

[John Fastabend]

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..bea14ee 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_SKIP_HW 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_SKIP_HW)
+		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..563cdad 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]		= { .type = 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,

Re: [net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[Jiri Pirko]

Fri, Feb 26, 2016 at 04:53:49PM CET, john.fastabend@gmail.com wrote:
>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>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [net-next PATCH v3 2/3] net: cls_u32: move TC offload feature bit into cls_u32 offload logic

[Jiri Pirko]

Fri, Feb 26, 2016 at 04:54:13PM CET, john.fastabend@gmail.com wrote:
>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>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [net-next PATCH v3 3/3] net: sched: cls_u32 add bit to specify software only rules

[Jiri Pirko]

Fri, Feb 26, 2016 at 04:54:39PM 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>

Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[Cong Wang]

On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend
<john.fastabend@gmail.com> wrote:
> 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;
> +}
> +

These should be protected by CONFIG_NET_CLS_U32, no?

Re: [net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[John Fastabend]

On 16-02-26 09:39 AM, Cong Wang wrote:
> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> 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;
>> +}
>> +
> 
> These should be protected by CONFIG_NET_CLS_U32, no?
> 

Its not necessary it is a completely general function and I only
lifted it out of cls_u32 so that the cls_flower classifier could
also use it.

I don't see the need off-hand to have it wrapped in an ORd ifdef
statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...).
Any particular reason you were thnking it should be wrapped in ifdefs?

Thanks for taking a look at the patches.

.John

Re: [net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[Cong Wang]

On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 16-02-26 09:39 AM, Cong Wang wrote:
>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend
>> <john.fastabend@gmail.com> wrote:
>>> 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;
>>> +}
>>> +
>>
>> These should be protected by CONFIG_NET_CLS_U32, no?
>>
>
> Its not necessary it is a completely general function and I only
> lifted it out of cls_u32 so that the cls_flower classifier could
> also use it.
>
> I don't see the need off-hand to have it wrapped in an ORd ifdef
> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...).
> Any particular reason you were thnking it should be wrapped in ifdefs?
>

Not a big deal.

I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n.

Thanks.

Re: [net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[John Fastabend]

On 16-02-27 08:28 PM, Cong Wang wrote:
> On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> On 16-02-26 09:39 AM, Cong Wang wrote:
>>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend
>>> <john.fastabend@gmail.com> wrote:
>>>> 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;
>>>> +}
>>>> +
>>>
>>> These should be protected by CONFIG_NET_CLS_U32, no?
>>>
>>
>> Its not necessary it is a completely general function and I only
>> lifted it out of cls_u32 so that the cls_flower classifier could
>> also use it.
>>
>> I don't see the need off-hand to have it wrapped in an ORd ifdef
>> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...).
>> Any particular reason you were thnking it should be wrapped in ifdefs?
>>
> 
> Not a big deal.
> 
> I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n.
> 
> Thanks.
> 

Well because this is 'static inline' gcc should just remove it
if it is not used. Assuming non-ancient gcc and normal compile
flags, e.g. you are not including -fkeep-inline-functions or
something.

So just to keep it readable I would prefer to just leave it
as is.

Thanks,
John

Re: [net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[Jiri Pirko]

Mon, Feb 29, 2016 at 07:40:53PM CET, john.fastabend@gmail.com wrote:
>On 16-02-27 08:28 PM, Cong Wang wrote:
>> On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend
>> <john.fastabend@gmail.com> wrote:
>>> On 16-02-26 09:39 AM, Cong Wang wrote:
>>>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend
>>>> <john.fastabend@gmail.com> wrote:
>>>>> 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;
>>>>> +}
>>>>> +
>>>>
>>>> These should be protected by CONFIG_NET_CLS_U32, no?
>>>>
>>>
>>> Its not necessary it is a completely general function and I only
>>> lifted it out of cls_u32 so that the cls_flower classifier could
>>> also use it.
>>>
>>> I don't see the need off-hand to have it wrapped in an ORd ifdef
>>> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...).
>>> Any particular reason you were thnking it should be wrapped in ifdefs?
>>>
>> 
>> Not a big deal.
>> 
>> I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n.
>> 
>> Thanks.
>> 
>
>Well because this is 'static inline' gcc should just remove it
>if it is not used. Assuming non-ancient gcc and normal compile
>flags, e.g. you are not including -fkeep-inline-functions or
>something.
>
>So just to keep it readable I would prefer to just leave it
>as is.

Definitelly. cls_flower will use it in very near future. Making it
dependent on CONFIG_NET_CLS_U32 makes 0 sense to me.

Re: [net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[Cong Wang]

On Mon, Feb 29, 2016 at 10:58 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Mon, Feb 29, 2016 at 07:40:53PM CET, john.fastabend@gmail.com wrote:
>>On 16-02-27 08:28 PM, Cong Wang wrote:
>>> On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend
>>> <john.fastabend@gmail.com> wrote:
>>>> On 16-02-26 09:39 AM, Cong Wang wrote:
>>>>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend
>>>>> <john.fastabend@gmail.com> wrote:
>>>>>> 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;
>>>>>> +}
>>>>>> +
>>>>>
>>>>> These should be protected by CONFIG_NET_CLS_U32, no?
>>>>>
>>>>
>>>> Its not necessary it is a completely general function and I only
>>>> lifted it out of cls_u32 so that the cls_flower classifier could
>>>> also use it.
>>>>
>>>> I don't see the need off-hand to have it wrapped in an ORd ifdef
>>>> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...).
>>>> Any particular reason you were thnking it should be wrapped in ifdefs?
>>>>
>>>
>>> Not a big deal.
>>>
>>> I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n.
>>>
>>> Thanks.
>>>
>>
>>Well because this is 'static inline' gcc should just remove it
>>if it is not used. Assuming non-ancient gcc and normal compile
>>flags, e.g. you are not including -fkeep-inline-functions or
>>something.
>>
>>So just to keep it readable I would prefer to just leave it
>>as is.
>
> Definitelly. cls_flower will use it in very near future. Making it
> dependent on CONFIG_NET_CLS_U32 makes 0 sense to me.

Oh, why then do you have u32 in the struct name tc_cls_u32_offload?

(Note that in the above I said "these" not "this", so I never only refer
to tc_should_offload)

Re: [net-next PATCH v3 1/3] net: sched: consolidate offload decision in cls_u32

[John Fastabend]

On 16-02-29 01:25 PM, Cong Wang wrote:
> On Mon, Feb 29, 2016 at 10:58 AM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Mon, Feb 29, 2016 at 07:40:53PM CET, john.fastabend@gmail.com wrote:
>>> On 16-02-27 08:28 PM, Cong Wang wrote:
>>>> On Fri, Feb 26, 2016 at 8:24 PM, John Fastabend
>>>> <john.fastabend@gmail.com> wrote:
>>>>> On 16-02-26 09:39 AM, Cong Wang wrote:
>>>>>> On Fri, Feb 26, 2016 at 7:53 AM, John Fastabend
>>>>>> <john.fastabend@gmail.com> wrote:
>>>>>>> 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;
>>>>>>> +}
>>>>>>> +
>>>>>>
>>>>>> These should be protected by CONFIG_NET_CLS_U32, no?
>>>>>>
>>>>>
>>>>> Its not necessary it is a completely general function and I only
>>>>> lifted it out of cls_u32 so that the cls_flower classifier could
>>>>> also use it.
>>>>>
>>>>> I don't see the need off-hand to have it wrapped in an ORd ifdef
>>>>> statement where its (CONFIG_NET_CLS_U32 | CONFIG_NET_CLS_X ...).
>>>>> Any particular reason you were thnking it should be wrapped in ifdefs?
>>>>>
>>>>
>>>> Not a big deal.
>>>>
>>>> I just feel these don't need to compile when I have CONFIG_NET_CLS_U32=n.
>>>>
>>>> Thanks.
>>>>
>>>
>>> Well because this is 'static inline' gcc should just remove it
>>> if it is not used. Assuming non-ancient gcc and normal compile
>>> flags, e.g. you are not including -fkeep-inline-functions or
>>> something.
>>>
>>> So just to keep it readable I would prefer to just leave it
>>> as is.
>>
>> Definitelly. cls_flower will use it in very near future. Making it
>> dependent on CONFIG_NET_CLS_U32 makes 0 sense to me.
> 
> Oh, why then do you have u32 in the struct name tc_cls_u32_offload?
> 
> (Note that in the above I said "these" not "this", so I never only refer
> to tc_should_offload)
> 

hmm yeah that likely wont be needed by flower although it could be used.
I still think its best to leave this as is there doesn't seem to be a
very strong precedent to wrap any of the other structs/fields/etc in
pkt_cls.h into their respective ifdef/endif blocks. And I think it
starts to get a bit much if we do. I'm trusting gcc here can do the
right thing when these are included but never used.

Thanks,
John

Re: [net-next PATCH v3 0/3] tc software only

[David Miller]

From: John Fastabend <john.fastabend@gmail.com>
Date: Fri, 26 Feb 2016 07:53:26 -0800

> 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 and named the flag
> TCA_CLS_FLAGS_SKIP_HW (Thanks Jiri ;)
> 
> 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.

Series applied, thanks John.