[PATCH net 0/2] cls_u32 hardware offload fixes

Hi!

This set fixes two small issues with error codes I noticed
in cls_u32.  Second patch could be viewed as user space API
change but that portion of API is not part of any release,
yet.

Compile tested only.

Jakub Kicinski (2):
  net: cls_u32: fix error code for invalid flags
  net: cls_u32: be more strict about skip-sw flag

 net/sched/cls_u32.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

-- 
1.9.1

'err' variable is not set in this test, we would return whatever
previous test set 'err' to.

Signed-off-by: Jakub Kicinski <redacted>
---
 net/sched/cls_u32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 079b43b3c5d2..b17e090f2fe1 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c

@@ -863,7 +863,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	if (tb[TCA_U32_FLAGS]) {
 		flags = nla_get_u32(tb[TCA_U32_FLAGS]);
 		if (!tc_flags_valid(flags))
-			return err;
+			return -EINVAL;
 	}
 
 	n = (struct tc_u_knode *)*arg;

-- 
1.9.1

Return an error if user requested skip-sw and the underlaying
hardware cannot handle tc offloads (or offloads are disabled).

Signed-off-by: Jakub Kicinski <redacted>
---
 net/sched/cls_u32.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index b17e090f2fe1..fe05449537a3 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c

@@ -457,20 +457,21 @@ static int u32_replace_hw_hnode(struct tcf_proto *tp,
 	struct tc_to_netdev offload;
 	int err;
 
+	if (!tc_should_offload(dev, flags))
+		return tc_skip_sw(flags) ? -EINVAL : 0;
+
 	offload.type = TC_SETUP_CLSU32;
 	offload.cls_u32 = &u32_offload;
 
-	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;
-		offload.cls_u32->hnode.prio = h->prio;
+	offload.cls_u32->command = TC_CLSU32_NEW_HNODE;
+	offload.cls_u32->hnode.divisor = h->divisor;
+	offload.cls_u32->hnode.handle = h->handle;
+	offload.cls_u32->hnode.prio = h->prio;
 
-		err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
-						    tp->protocol, &offload);
-		if (tc_skip_sw(flags))
-			return err;
-	}
+	err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
+					    tp->protocol, &offload);
+	if (tc_skip_sw(flags))
+		return err;
 
 	return 0;
 }

-- 
1.9.1

On 6/6/2016 8:16 AM, Jakub Kicinski wrote:

'err' variable is not set in this test, we would return whatever
previous test set 'err' to.

Signed-off-by: Jakub Kicinski <redacted>

Acked-by: Sridhar Samudrala <sridhar.samudrala@intel.com>

On 6/6/2016 8:16 AM, Jakub Kicinski wrote:

Return an error if user requested skip-sw and the underlaying
hardware cannot handle tc offloads (or offloads are disabled).

Signed-off-by: Jakub Kicinski <redacted>

looks good. I think we need similar checks in u32_replace_hw_knode() too.

Hi!

This set fixes two small issues with error codes I noticed
in cls_u32.  Second patch could be viewed as user space API
change but that portion of API is not part of any release,
yet.

Compile tested only.

Jakub Kicinski (2):
  net: cls_u32: fix error code for invalid flags
  net: cls_u32: be more strict about skip-sw flag

 net/sched/cls_u32.c | 60 +++++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

-- 
1.9.1

'err' variable is not set in this test, we would return whatever
previous test set 'err' to.

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Dinan Gunawardena <redacted>
Reviewed-by: Simon Horman <redacted>
Acked-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
 net/sched/cls_u32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 079b43b3c5d2..b17e090f2fe1 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c

@@ -863,7 +863,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	if (tb[TCA_U32_FLAGS]) {
 		flags = nla_get_u32(tb[TCA_U32_FLAGS]);
 		if (!tc_flags_valid(flags))
-			return err;
+			return -EINVAL;
 	}
 
 	n = (struct tc_u_knode *)*arg;

-- 
1.9.1

Return an error if user requested skip-sw and the underlaying
hardware cannot handle tc offloads (or offloads are disabled).

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Dinan Gunawardena <redacted>
Reviewed-by: Simon Horman <redacted>
---
v2:
  - handle both knode and hnodes
---
 net/sched/cls_u32.c | 58 +++++++++++++++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index b17e090f2fe1..0fc1d47885f8 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c

@@ -457,20 +457,21 @@ static int u32_replace_hw_hnode(struct tcf_proto *tp,
 	struct tc_to_netdev offload;
 	int err;
 
+	if (!tc_should_offload(dev, flags))
+		return tc_skip_sw(flags) ? -EINVAL : 0;
+
 	offload.type = TC_SETUP_CLSU32;
 	offload.cls_u32 = &u32_offload;
 
-	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;
-		offload.cls_u32->hnode.prio = h->prio;
+	offload.cls_u32->command = TC_CLSU32_NEW_HNODE;
+	offload.cls_u32->hnode.divisor = h->divisor;
+	offload.cls_u32->hnode.handle = h->handle;
+	offload.cls_u32->hnode.prio = h->prio;
 
-		err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
-						    tp->protocol, &offload);
-		if (tc_skip_sw(flags))
-			return err;
-	}
+	err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
+					    tp->protocol, &offload);
+	if (tc_skip_sw(flags))
+		return err;
 
 	return 0;
 }

@@ -507,27 +508,28 @@ static int u32_replace_hw_knode(struct tcf_proto *tp,
 	offload.type = TC_SETUP_CLSU32;
 	offload.cls_u32 = &u32_offload;
 
-	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;
+	if (!tc_should_offload(dev, flags))
+		return tc_skip_sw(flags) ? -EINVAL : 0;
+
+	offload.cls_u32->command = TC_CLSU32_REPLACE_KNODE;
+	offload.cls_u32->knode.handle = n->handle;
+	offload.cls_u32->knode.fshift = n->fshift;
 #ifdef CONFIG_CLS_U32_MARK
-		offload.cls_u32->knode.val = n->val;
-		offload.cls_u32->knode.mask = n->mask;
+	offload.cls_u32->knode.val = n->val;
+	offload.cls_u32->knode.mask = n->mask;
 #else
-		offload.cls_u32->knode.val = 0;
-		offload.cls_u32->knode.mask = 0;
+	offload.cls_u32->knode.val = 0;
+	offload.cls_u32->knode.mask = 0;
 #endif
-		offload.cls_u32->knode.sel = &n->sel;
-		offload.cls_u32->knode.exts = &n->exts;
-		if (n->ht_down)
-			offload.cls_u32->knode.link_handle = n->ht_down->handle;
-
-		err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
-						    tp->protocol, &offload);
-		if (tc_skip_sw(flags))
-			return err;
-	}
+	offload.cls_u32->knode.sel = &n->sel;
+	offload.cls_u32->knode.exts = &n->exts;
+	if (n->ht_down)
+		offload.cls_u32->knode.link_handle = n->ht_down->handle;
+
+	err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
+					    tp->protocol, &offload);
+	if (tc_skip_sw(flags))
+		return err;
 
 	return 0;
 }

-- 
1.9.1

On 16-06-06 08:16 AM, Jakub Kicinski wrote:

Yep, I agree it is nice to get an error on this case.

Acked-by: John Fastabend <redacted>

On 16-06-07 03:46 AM, Jakub Kicinski wrote:

Acking the v2 now -- seems nice to throw an error in this case.

Acked-by: John Fastabend <redacted>

On 16-06-07 03:46 AM, Jakub Kicinski wrote:

Looks like we also need to catch the error at u32_replace_hw_hnode call
sites?


                u32_replace_hw_hnode(tp, ht, flags);
                return 0;
        }

should be

		return replace_hw_hnode(tp, ht,flags)


Thanks,
John

On Tue, 7 Jun 2016 08:53:35 -0700, John Fastabend wrote:

On 16-06-07 03:46 AM, Jakub Kicinski wrote:

quoted

Return an error if user requested skip-sw and the underlaying
hardware cannot handle tc offloads (or offloads are disabled).

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Dinan Gunawardena <redacted>
Reviewed-by: Simon Horman <redacted>
---
v2:
  - handle both knode and hnodes
---
 net/sched/cls_u32.c | 58 +++++++++++++++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index b17e090f2fe1..0fc1d47885f8 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c

@@ -457,20 +457,21 @@ static int u32_replace_hw_hnode(struct tcf_proto *tp,
 	struct tc_to_netdev offload;
 	int err;
 
+	if (!tc_should_offload(dev, flags))
+		return tc_skip_sw(flags) ? -EINVAL : 0;
+
 	offload.type = TC_SETUP_CLSU32;
 	offload.cls_u32 = &u32_offload;
 
-	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;
-		offload.cls_u32->hnode.prio = h->prio;
+	offload.cls_u32->command = TC_CLSU32_NEW_HNODE;
+	offload.cls_u32->hnode.divisor = h->divisor;
+	offload.cls_u32->hnode.handle = h->handle;
+	offload.cls_u32->hnode.prio = h->prio;
 
-		err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
-						    tp->protocol, &offload);
-		if (tc_skip_sw(flags))
-			return err;
-	}
+	err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
+					    tp->protocol, &offload);
+	if (tc_skip_sw(flags))
+		return err;
 
 	return 0;
 }  

Looks like we also need to catch the error at u32_replace_hw_hnode call
sites?


                u32_replace_hw_hnode(tp, ht, flags);
                return 0;
        }

should be

		return replace_hw_hnode(tp, ht,flags)

Indeed. I'll add a third patch to the series, seems like a separate bug.

Hi!

This set fixes three small issues with error codes I noticed
in cls_u32.  Second patch could be viewed as user space API
change but that portion of API is not part of any release,
yet.

Very lightly tested.

Jakub Kicinski (3):
  net: cls_u32: fix error code for invalid flags
  net: cls_u32: be more strict about skip-sw flag
  net: cls_u32: catch all hardware offload errors

 net/sched/cls_u32.c | 68 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 30 deletions(-)

-- 
1.9.1

'err' variable is not set in this test, we would return whatever
previous test set 'err' to.

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Dinan Gunawardena <redacted>
Reviewed-by: Simon Horman <redacted>
Acked-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
Acked-by: John Fastabend <redacted>
---
 net/sched/cls_u32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 079b43b3c5d2..b17e090f2fe1 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c

@@ -863,7 +863,7 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 	if (tb[TCA_U32_FLAGS]) {
 		flags = nla_get_u32(tb[TCA_U32_FLAGS]);
 		if (!tc_flags_valid(flags))
-			return err;
+			return -EINVAL;
 	}
 
 	n = (struct tc_u_knode *)*arg;

-- 
1.9.1

Return an error if user requested skip-sw and the underlaying
hardware cannot handle tc offloads (or offloads are disabled).

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Dinan Gunawardena <redacted>
Reviewed-by: Simon Horman <redacted>
---
v2:
  - handle both knode and hnodes
---
 net/sched/cls_u32.c | 58 +++++++++++++++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index b17e090f2fe1..0fc1d47885f8 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c

@@ -457,20 +457,21 @@ static int u32_replace_hw_hnode(struct tcf_proto *tp,
 	struct tc_to_netdev offload;
 	int err;
 
+	if (!tc_should_offload(dev, flags))
+		return tc_skip_sw(flags) ? -EINVAL : 0;
+
 	offload.type = TC_SETUP_CLSU32;
 	offload.cls_u32 = &u32_offload;
 
-	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;
-		offload.cls_u32->hnode.prio = h->prio;
+	offload.cls_u32->command = TC_CLSU32_NEW_HNODE;
+	offload.cls_u32->hnode.divisor = h->divisor;
+	offload.cls_u32->hnode.handle = h->handle;
+	offload.cls_u32->hnode.prio = h->prio;
 
-		err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
-						    tp->protocol, &offload);
-		if (tc_skip_sw(flags))
-			return err;
-	}
+	err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
+					    tp->protocol, &offload);
+	if (tc_skip_sw(flags))
+		return err;
 
 	return 0;
 }

@@ -507,27 +508,28 @@ static int u32_replace_hw_knode(struct tcf_proto *tp,
 	offload.type = TC_SETUP_CLSU32;
 	offload.cls_u32 = &u32_offload;
 
-	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;
+	if (!tc_should_offload(dev, flags))
+		return tc_skip_sw(flags) ? -EINVAL : 0;
+
+	offload.cls_u32->command = TC_CLSU32_REPLACE_KNODE;
+	offload.cls_u32->knode.handle = n->handle;
+	offload.cls_u32->knode.fshift = n->fshift;
 #ifdef CONFIG_CLS_U32_MARK
-		offload.cls_u32->knode.val = n->val;
-		offload.cls_u32->knode.mask = n->mask;
+	offload.cls_u32->knode.val = n->val;
+	offload.cls_u32->knode.mask = n->mask;
 #else
-		offload.cls_u32->knode.val = 0;
-		offload.cls_u32->knode.mask = 0;
+	offload.cls_u32->knode.val = 0;
+	offload.cls_u32->knode.mask = 0;
 #endif
-		offload.cls_u32->knode.sel = &n->sel;
-		offload.cls_u32->knode.exts = &n->exts;
-		if (n->ht_down)
-			offload.cls_u32->knode.link_handle = n->ht_down->handle;
-
-		err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
-						    tp->protocol, &offload);
-		if (tc_skip_sw(flags))
-			return err;
-	}
+	offload.cls_u32->knode.sel = &n->sel;
+	offload.cls_u32->knode.exts = &n->exts;
+	if (n->ht_down)
+		offload.cls_u32->knode.link_handle = n->ht_down->handle;
+
+	err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
+					    tp->protocol, &offload);
+	if (tc_skip_sw(flags))
+		return err;
 
 	return 0;
 }

-- 
1.9.1

Errors reported by u32_replace_hw_hnode() were not propagated.

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Dinan Gunawardena <redacted>
---
v3:
 - new patch

 net/sched/cls_u32.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 0fc1d47885f8..b9c3875fddc6 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c

@@ -923,11 +923,17 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		ht->divisor = divisor;
 		ht->handle = handle;
 		ht->prio = tp->prio;
+
+		err = u32_replace_hw_hnode(tp, ht, flags);
+		if (err) {
+			kfree(ht);
+			return err;
+		}
+
 		RCU_INIT_POINTER(ht->next, tp_c->hlist);
 		rcu_assign_pointer(tp_c->hlist, ht);
 		*arg = (unsigned long)ht;
 
-		u32_replace_hw_hnode(tp, ht, flags);
 		return 0;
 	}
 

-- 
1.9.1

On 6/7/2016 3:17 PM, Jakub Kicinski wrote:

Return an error if user requested skip-sw and the underlaying
hardware cannot handle tc offloads (or offloads are disabled).

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Dinan Gunawardena <redacted>
Reviewed-by: Simon Horman <redacted>
---
v2:
   - handle both knode and hnodes
---

Acked-by: Sridhar Samudrala <sridhar.samudrala@intel.com>

On 6/7/2016 3:17 PM, Jakub Kicinski wrote:

Errors reported by u32_replace_hw_hnode() were not propagated.

Signed-off-by: Jakub Kicinski <redacted>
Reviewed-by: Dinan Gunawardena <redacted>
---
v3:
  - new patch

Acked-by: Sridhar Samudrala <sridhar.samudrala@intel.com>

From: Jakub Kicinski <redacted>
Date: Mon,  6 Jun 2016 16:16:46 +0100

This set fixes two small issues with error codes I noticed
in cls_u32.  Second patch could be viewed as user space API
change but that portion of API is not part of any release,
yet.

Compile tested only.

Applied, thanks.

On Tue, 07 Jun 2016 16:27:31 -0700 (PDT), David Miller wrote:

From: Jakub Kicinski <redacted>
Date: Mon,  6 Jun 2016 16:16:46 +0100

quoted

This set fixes two small issues with error codes I noticed
in cls_u32.  Second patch could be viewed as user space API
change but that portion of API is not part of any release,
yet.

Compile tested only.  

Applied, thanks.

I think you applied v1 instead of v3 (which is still in patchwork) :S
Should I post an incremental patch to bring the code to v3 state?

From: Jakub Kicinski <redacted>
Date: Wed, 8 Jun 2016 11:18:30 +0100

On Tue, 07 Jun 2016 16:27:31 -0700 (PDT), David Miller wrote:

quoted

From: Jakub Kicinski <redacted>
Date: Mon,  6 Jun 2016 16:16:46 +0100

quoted

This set fixes two small issues with error codes I noticed
in cls_u32.  Second patch could be viewed as user space API
change but that portion of API is not part of any release,
yet.

Compile tested only.  

Applied, thanks.

I think you applied v1 instead of v3 (which is still in patchwork) :S
Should I post an incremental patch to bring the code to v3 state?

Yeah please do, sorry about that :-/

On Wed, 08 Jun 2016 11:09:36 -0700 (PDT), David Miller wrote:

From: Jakub Kicinski <redacted>
Date: Wed, 8 Jun 2016 11:18:30 +0100

quoted

On Tue, 07 Jun 2016 16:27:31 -0700 (PDT), David Miller wrote:  

quoted

From: Jakub Kicinski <redacted>
Date: Mon,  6 Jun 2016 16:16:46 +0100
  

quoted

This set fixes two small issues with error codes I noticed
in cls_u32.  Second patch could be viewed as user space API
change but that portion of API is not part of any release,
yet.

Compile tested only.    

Applied, thanks.  

I think you applied v1 instead of v3 (which is still in patchwork) :S
Should I post an incremental patch to bring the code to v3 state?  

Yeah please do, sorry about that :-/

Done and marked v3 as Superseded, hope that's OK!