[PATCH net-next 0/4] net/sched: Use TC skip flags to reflect HW offload status

[PATCH net-next 0/4] net/sched: Use TC skip flags to reflect HW offload status

[Or Gerlitz]

Currently there is no way of querying whether a filter is
offloaded to HW or not when using both policy (no flag).

Reuse the skip flags to show the insertion status by setting
the skip_hw flag in case the filter wasn't offloaded.

The bpf patch is compile tested only, Daniel/Jakub, will 
appreciate your review/ack.

I was able to test the u32 patch (as shown below), just one thing
I wasn't sure on is if this matter w.r.t hnodes (vs knodes), John,
could you look and tell me if something is missing?

As an example, add two vlan push + fwd rules over mlx5 SRIOV VF rep, 
one match all and one u32 rule without any flags, which would cause
the TC subsystem to attempt and offload all of them:

#tc filter add dev eth2_0 protocol ip parent ffff: 
  flower indev eth2_0 src_mac e4:11:22:33:44:50 dst_mac e4:1d:2d:a5:f3:9d 
  action vlan push id 52 action mirred egress redirect dev eth2

#tc filter add dev eth2_0 protocol ip parent ffff: 
  flower indev eth2_0 src_mac e4:11:22:33:44:50 dst_mac e4:11:22:33:44:51
  action vlan push id 53 action mirred egress redirect dev eth2

#tc filter add dev eth2_0 parent ffff: matchall action mirred egress mirror dev veth1

Currenly we can only offload one vlan push per VF vport, so the 2nd flower rule is 
not offloaded, same for the matchall and the u32 rules. With this series, user-space 
can note that as the skip_hw flag is set for this rule when they dump it.

#tc  filter show dev eth2_0 parent ffff:

filter protocol ip pref 99 u32 
filter protocol ip pref 99 u32 fh 800: ht divisor 1 
filter protocol ip pref 99 u32 fh 800::1 order 1 key ht 800 bkt 0 flowid 800:1 skip_hw 
  match c0a80100/ffffff00 at 12
	action order 1: gact action drop
	 random type none pass val 0
	 index 17 ref 1 bind 1
 
filter protocol all pref 49150 matchall 
filter protocol all pref 49150 matchall handle 0x1 
  skip_hw
	action order 1: mirred (Egress Mirror to device veth1) pipe
 	index 119 ref 1 bind 1
 
filter protocol ip pref 49151 flower 
filter protocol ip pref 49151 flower handle 0x1 
  indev eth2_0
  dst_mac e4:11:22:33:44:51
  src_mac e4:11:22:33:44:50
  eth_type ipv4
  skip_hw
	action order 1:  vlan push id 53 protocol 802.1Q priority 0 pipe
	 index 56 ref 1 bind 1
 
	action order 2: mirred (Egress Redirect to device eth2) stolen
 	index 118 ref 1 bind 1
 
filter protocol ip pref 49152 flower 
filter protocol ip pref 49152 flower handle 0x1 
  indev eth2_0
  dst_mac e4:1d:2d:a5:f3:9d
  src_mac e4:11:22:33:44:50
  eth_type ipv4
	action order 1:  vlan push id 52 protocol 802.1Q priority 0 pipe
	 index 55 ref 1 bind 1
 
	action order 2: mirred (Egress Redirect to device eth2) stolen
 	index 117 ref 1 bind 1
 
Or Gerlitz (3):
  net/sched: cls_matchall: Dump skip flags and use them to reflect HW offload status
  net/sched: cls_u32: Use skip flags to reflect HW offload status
  net/sched: cls_bpf: Use skip flags to reflect HW offload status

Paul Blakey (1):
  net/sched: cls_flower: Use skip flags to reflect HW offload status

 net/sched/cls_bpf.c      | 17 +++++++++++++----
 net/sched/cls_flower.c   |  7 ++++++-
 net/sched/cls_matchall.c |  6 +++++-
 net/sched/cls_u32.c      | 20 ++++++++++++++------
 4 files changed, 38 insertions(+), 12 deletions(-)

-- 
2.3.7

[PATCH net-next 1/4] net/sched: cls_flower: Use skip flags to reflect HW offload status

[Or Gerlitz]

From: Paul Blakey <paulb@mellanox.com>

Currently there is no way of querying whether a filter is
offloaded to HW or not when using both policy (no flag).

Reuse the skip flags to show the insertion status by setting
the skip_hw flag in case the filter wasn't offloaded.

Signed-off-by: Paul Blakey <paulb@mellanox.com>
Signed-off-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Or Gerlitz <ogerlitz@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_flower.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 0826c8e..90e4490 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -252,7 +252,10 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
 		if (tcf_exts_get_dev(dev, &f->exts, &f->hw_dev) ||
 		    (f->hw_dev && !tc_can_offload(f->hw_dev, tp))) {
 			f->hw_dev = dev;
-			return tc_skip_sw(f->flags) ? -EINVAL : 0;
+			if (tc_skip_sw(f->flags))
+				return -EINVAL;
+			f->flags |= TCA_CLS_FLAGS_SKIP_HW;
+			return 0;
 		}
 		dev = f->hw_dev;
 		tc->egress_dev = true;
@@ -276,6 +279,8 @@ static int fl_hw_replace_filter(struct tcf_proto *tp,
 
 	if (tc_skip_sw(f->flags))
 		return err;
+	if (err)
+		f->flags |= TCA_CLS_FLAGS_SKIP_HW;
 	return 0;
 }
 
-- 
2.3.7

[PATCH net-next 2/4] net/sched: cls_matchall: Dump skip flags and use them to reflect HW offload status

[Or Gerlitz]

Currently there is no way of querying whether a filter is offloaded
to HW or not, as the flags are not dumped to user-space, fix that.

Also, reuse the skip flags to show the insertion status by
setting the skip_hw flag in case the filter wasn't offloaded.

Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_matchall.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index f2141cb..9dda12d 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -189,8 +189,10 @@ static int mall_change(struct net *net, struct sk_buff *in_skb,
 		if (err) {
 			if (tc_skip_sw(flags))
 				goto err_replace_hw_filter;
-			else
+			else {
+				new->flags |= TCA_CLS_FLAGS_SKIP_HW;
 				err = 0;
+			}
 		}
 	}
 
@@ -244,6 +246,8 @@ static int mall_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	    nla_put_u32(skb, TCA_MATCHALL_CLASSID, head->res.classid))
 		goto nla_put_failure;
 
+	nla_put_u32(skb, TCA_MATCHALL_FLAGS, head->flags);
+
 	if (tcf_exts_dump(skb, &head->exts))
 		goto nla_put_failure;
 
-- 
2.3.7

[PATCH net-next 3/4] net/sched: cls_u32: Use skip flags to reflect HW offload status

[Or Gerlitz]

Currently there is no way of querying whether a filter is
offloaded to HW or not when using both policy (no flag).

Reuse the skip flags to show the insertion status by setting
the skip_hw flag in case the filter wasn't offloaded.

Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 net/sched/cls_u32.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index a6ec3e4b..97411d5 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -493,7 +493,7 @@ static void u32_clear_hw_hnode(struct tcf_proto *tp, struct tc_u_hnode *h)
 }
 
 static int u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n,
-				u32 flags)
+				u32 *flags)
 {
 	struct net_device *dev = tp->q->dev_queue->dev;
 	struct tc_cls_u32_offload u32_offload = {0};
@@ -503,8 +503,12 @@ static int 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, tp, flags))
-		return tc_skip_sw(flags) ? -EINVAL : 0;
+	if (!tc_should_offload(dev, tp, *flags)) {
+		if (tc_skip_sw(*flags))
+			return -EINVAL;
+		*flags = TCA_CLS_FLAGS_SKIP_HW;
+		return 0;
+	}
 
 	offload.cls_u32->command = TC_CLSU32_REPLACE_KNODE;
 	offload.cls_u32->knode.handle = n->handle;
@@ -523,9 +527,11 @@ static int u32_replace_hw_knode(struct tcf_proto *tp, struct tc_u_knode *n,
 
 	err = dev->netdev_ops->ndo_setup_tc(dev, tp->q->handle,
 					    tp->protocol, &offload);
-	if (tc_skip_sw(flags))
+	if (tc_skip_sw(*flags))
 		return err;
 
+	if (err)
+		*flags = TCA_CLS_FLAGS_SKIP_HW;
 	return 0;
 }
 
@@ -889,12 +895,13 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 			return err;
 		}
 
-		err = u32_replace_hw_knode(tp, new, flags);
+		err = u32_replace_hw_knode(tp, new, &flags);
 		if (err) {
 			u32_destroy_key(tp, new, false);
 			return err;
 		}
 
+		n->flags |= flags;
 		u32_replace_knode(tp, tp_c, new);
 		tcf_unbind_filter(tp, &n->res);
 		call_rcu(&n->rcu, u32_delete_key_rcu);
@@ -1010,10 +1017,11 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
 		struct tc_u_knode __rcu **ins;
 		struct tc_u_knode *pins;
 
-		err = u32_replace_hw_knode(tp, n, flags);
+		err = u32_replace_hw_knode(tp, n, &flags);
 		if (err)
 			goto errhw;
 
+		n->flags |= flags;
 		ins = &ht->ht[TC_U32_HASH(handle)];
 		for (pins = rtnl_dereference(*ins); pins;
 		     ins = &pins->next, pins = rtnl_dereference(*ins))
-- 
2.3.7

[PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

[Or Gerlitz]

Currently there is no way of querying whether a filter is
offloaded to HW or not when using both policy (no flag).

Reuse the skip flags to show the insertion status by setting
the skip_hw flag in case the filter wasn't offloaded.

Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 net/sched/cls_bpf.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index d9c9701..91ba90d 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -185,14 +185,23 @@ static int cls_bpf_offload(struct tcf_proto *tp, struct cls_bpf_prog *prog,
 			return -EINVAL;
 		}
 	} else {
-		if (!tc_should_offload(dev, tp, prog->gen_flags))
-			return skip_sw ? -EINVAL : 0;
+		if (!tc_should_offload(dev, tp, prog->gen_flags)) {
+			if (tc_skip_sw(prog->gen_flags))
+				return -EINVAL;
+			prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
+			return 0;
+		}
 		cmd = TC_CLSBPF_ADD;
 	}
 
 	ret = cls_bpf_offload_cmd(tp, obj, cmd);
-	if (ret)
-		return skip_sw ? ret : 0;
+
+	if (ret) {
+		if (skip_sw)
+			return ret;
+		prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
+		return 0;
+	}
 
 	obj->offloaded = true;
 	if (oldprog)
-- 
2.3.7

Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

[Jakub Kicinski]

On Thu,  9 Feb 2017 16:18:08 +0200, Or Gerlitz wrote:
> Currently there is no way of querying whether a filter is
> offloaded to HW or not when using both policy (no flag).
> 
> Reuse the skip flags to show the insertion status by setting
> the skip_hw flag in case the filter wasn't offloaded.
> 
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
>  net/sched/cls_bpf.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index d9c9701..91ba90d 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -185,14 +185,23 @@ static int cls_bpf_offload(struct tcf_proto *tp, struct cls_bpf_prog *prog,
>  			return -EINVAL;
>  		}
>  	} else {
> -		if (!tc_should_offload(dev, tp, prog->gen_flags))
> -			return skip_sw ? -EINVAL : 0;
> +		if (!tc_should_offload(dev, tp, prog->gen_flags)) {
> +			if (tc_skip_sw(prog->gen_flags))
> +				return -EINVAL;
> +			prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
> +			return 0;
> +		}
>  		cmd = TC_CLSBPF_ADD;
>  	}
>  
>  	ret = cls_bpf_offload_cmd(tp, obj, cmd);
> -	if (ret)
> -		return skip_sw ? ret : 0;
> +
> +	if (ret) {
> +		if (skip_sw)
> +			return ret;
> +		prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
> +		return 0;
> +	}
>  
>  	obj->offloaded = true;

In cls_bpf we do store information about whether program is offloaded or
not already (see the @offloaded member).  Could we simplify the code
thanks to this?

I'm obviously all for reporting whether tc objects are offloaded or not
but let me ask perhaps the silly question of why reuse the SKIP_HW flag?
We don't have to worry about flag bits running out, could it be clearer
to users to report whether object is present in HW using a new flag?  Or
even two flags for present/non-present so user doesn't have to ponder
what no flag means (old kernel or not offloaded?). I don't really mind
either way I'm just wondering what the motivation was and maybe how
others feel.

Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

[Or Gerlitz]

On Fri, Feb 10, 2017 at 3:22 AM, Jakub Kicinski <kubakici@wp.pl> wrote:
> On Thu,  9 Feb 2017 16:18:08 +0200, Or Gerlitz wrote:
>> Currently there is no way of querying whether a filter is
>> offloaded to HW or not when using both policy (no flag).
>>
>> Reuse the skip flags to show the insertion status by setting
>> the skip_hw flag in case the filter wasn't offloaded.
>>
>> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
>> ---
>>  net/sched/cls_bpf.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
>> index d9c9701..91ba90d 100644
>> --- a/net/sched/cls_bpf.c
>> +++ b/net/sched/cls_bpf.c
>> @@ -185,14 +185,23 @@ static int cls_bpf_offload(struct tcf_proto *tp, struct cls_bpf_prog *prog,
>>                       return -EINVAL;
>>               }
>>       } else {
>> -             if (!tc_should_offload(dev, tp, prog->gen_flags))
>> -                     return skip_sw ? -EINVAL : 0;
>> +             if (!tc_should_offload(dev, tp, prog->gen_flags)) {
>> +                     if (tc_skip_sw(prog->gen_flags))
>> +                             return -EINVAL;
>> +                     prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
>> +                     return 0;
>> +             }
>>               cmd = TC_CLSBPF_ADD;
>>       }
>>
>>       ret = cls_bpf_offload_cmd(tp, obj, cmd);
>> -     if (ret)
>> -             return skip_sw ? ret : 0;
>> +
>> +     if (ret) {
>> +             if (skip_sw)
>> +                     return ret;
>> +             prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
>> +             return 0;
>> +     }
>>
>>       obj->offloaded = true;
>
> In cls_bpf we do store information about whether program is offloaded or
> not already (see the @offloaded member).  Could we simplify the code
> thanks to this?

yeah, I felt like I don't fully understand the role of the offloaded
member. As I wrote, this patch is compile tested only, I will be happy
if you can test it post here a better version, I don't think we need
to add/change the flags semantics, see next

> I'm obviously all for reporting whether tc objects are offloaded or not
> but let me ask perhaps the silly question of why reuse the SKIP_HW flag?
> We don't have to worry about flag bits running out, could it be clearer
> to users to report whether object is present in HW using a new flag?  Or
> even two flags for present/non-present so user doesn't have to ponder
> what no flag means (old kernel or not offloaded?). I don't really mind
> either way I'm just wondering what the motivation was and maybe how
> others feel.

yeah, the flags are a bit confusing to some people, but it's all about
polarity..

when the flags were introduced few of us where in favor of "positive"
polarity, that is with possibly three values: "sw only" "hw only" and
"both" but that JJJ (Jiri/John/Jamal) consensus was to pick a
"negative" polarity of "skip sw" "skip hw" and "default" which means
the filter is in SW and possibly in HW. I think we can live with that
semantics and this small series just helps for the default case, allow
user-space to know if the filter was offloaded using the existing
fields.

I am not in favor of making this more complex...

thanks for the feedback and review

Or.

Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

[Jakub Kicinski]

On Fri, 10 Feb 2017 18:33:13 +0200, Or Gerlitz wrote:
> On Fri, Feb 10, 2017 at 3:22 AM, Jakub Kicinski <kubakici@wp.pl> wrote:
> > On Thu,  9 Feb 2017 16:18:08 +0200, Or Gerlitz wrote:  
> >> Currently there is no way of querying whether a filter is
> >> offloaded to HW or not when using both policy (no flag).
> >>
> >> Reuse the skip flags to show the insertion status by setting
> >> the skip_hw flag in case the filter wasn't offloaded.
> >>
> >> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> >> ---
> >>  net/sched/cls_bpf.c | 17 +++++++++++++----
> >>  1 file changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> >> index d9c9701..91ba90d 100644
> >> --- a/net/sched/cls_bpf.c
> >> +++ b/net/sched/cls_bpf.c
> >> @@ -185,14 +185,23 @@ static int cls_bpf_offload(struct tcf_proto *tp, struct cls_bpf_prog *prog,
> >>                       return -EINVAL;
> >>               }
> >>       } else {
> >> -             if (!tc_should_offload(dev, tp, prog->gen_flags))
> >> -                     return skip_sw ? -EINVAL : 0;
> >> +             if (!tc_should_offload(dev, tp, prog->gen_flags)) {
> >> +                     if (tc_skip_sw(prog->gen_flags))
> >> +                             return -EINVAL;
> >> +                     prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
> >> +                     return 0;
> >> +             }
> >>               cmd = TC_CLSBPF_ADD;
> >>       }
> >>
> >>       ret = cls_bpf_offload_cmd(tp, obj, cmd);
> >> -     if (ret)
> >> -             return skip_sw ? ret : 0;
> >> +
> >> +     if (ret) {
> >> +             if (skip_sw)
> >> +                     return ret;
> >> +             prog->gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
> >> +             return 0;
> >> +     }
> >>
> >>       obj->offloaded = true;  
> >
> > In cls_bpf we do store information about whether program is offloaded or
> > not already (see the @offloaded member).  Could we simplify the code
> > thanks to this?  
> 
> yeah, I felt like I don't fully understand the role of the offloaded
> member. As I wrote, this patch is compile tested only, I will be happy
> if you can test it post here a better version, I don't think we need
> to add/change the flags semantics, see next

The @offloaded member just tells us whether the program is offloaded.
We need it because unlike u32 and flower (I think) we have explicit
ADD and REPLACE.  Other filters just always do REPLACE.  I assume the
driver keeps track if there is already an associated rule in that case?

> > I'm obviously all for reporting whether tc objects are offloaded or not
> > but let me ask perhaps the silly question of why reuse the SKIP_HW flag?
> > We don't have to worry about flag bits running out, could it be clearer
> > to users to report whether object is present in HW using a new flag?  Or
> > even two flags for present/non-present so user doesn't have to ponder
> > what no flag means (old kernel or not offloaded?). I don't really mind
> > either way I'm just wondering what the motivation was and maybe how
> > others feel.  
> 
> yeah, the flags are a bit confusing to some people, but it's all about
> polarity..
> 
> when the flags were introduced few of us where in favor of "positive"
> polarity, that is with possibly three values: "sw only" "hw only" and
> "both" but that JJJ (Jiri/John/Jamal) consensus was to pick a
> "negative" polarity of "skip sw" "skip hw" and "default" which means
> the filter is in SW and possibly in HW. I think we can live with that
> semantics and this small series just helps for the default case, allow
> user-space to know if the filter was offloaded using the existing
> fields.
> 
> I am not in favor of making this more complex...

I'm 100% with you.  Restating my proposal was to leave the SKIP_* flags
with all their existing semantics and complexity and for reporting to
user space whether something got offloaded add new ones.  My opinion
is that it would make things simpler, but I'm happy with your version
if none else thinks this way.

To spell it out with this patchset we would get the following semantics
of flags in dumps:
 - no flags - offloaded || old kernel;
 - skip_hw - not offloaded (either on user request || no flag
   was set but offload could not happen);
 - skip_sw - offload only on explicit request.

What we could do if we want to add flags would be:
 - no flags - old kernel;
 - no_offload - not offloaded;
 - skip_hw | no_offload - not offloaded on explicit user request;
 - offload - offloaded opportunistically;
 - skip_sw | offload - offloaded on explicit request. 

Because of dealing with distributor's kernels and various other
backports checking if kernel is old is a pain in practice :|

> thanks for the feedback and review

I will try to test and provide a simplified version if possible soon
(sorry for taking long, I got relocated and I'm still sorting out my
setup).

Re: [PATCH net-next 0/4] net/sched: Use TC skip flags to reflect HW offload status

[David Miller]

From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Thu,  9 Feb 2017 16:18:04 +0200

> Currently there is no way of querying whether a filter is
> offloaded to HW or not when using both policy (no flag).
> 
> Reuse the skip flags to show the insertion status by setting
> the skip_hw flag in case the filter wasn't offloaded.
> 
> The bpf patch is compile tested only, Daniel/Jakub, will 
> appreciate your review/ack.
 ...

I'm learning towards suggesting that you use new flags, this way it
will be unambiguous whether we are running an old kernel.

If you just use the skip flag, it's impossible to tell the difference.

Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

[Jakub Kicinski]

On Thu,  9 Feb 2017 16:18:08 +0200, Or Gerlitz wrote:
> Currently there is no way of querying whether a filter is
> offloaded to HW or not when using both policy (no flag).
> 
> Reuse the skip flags to show the insertion status by setting
> the skip_hw flag in case the filter wasn't offloaded.
> 
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>

FWIW I tested this one and it works.  I also tested this version which
would take advantage of @offloaded:

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index d9c97018317d..51d464f991ff 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -568,8 +568,8 @@ static int cls_bpf_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 			struct sk_buff *skb, struct tcmsg *tm)
 {
 	struct cls_bpf_prog *prog = (struct cls_bpf_prog *) fh;
+	u32 gen_flags, bpf_flags = 0;
 	struct nlattr *nest;
-	u32 bpf_flags = 0;
 	int ret;
 
 	if (prog == NULL)
@@ -601,8 +601,11 @@ static int cls_bpf_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		bpf_flags |= TCA_BPF_FLAG_ACT_DIRECT;
 	if (bpf_flags && nla_put_u32(skb, TCA_BPF_FLAGS, bpf_flags))
 		goto nla_put_failure;
-	if (prog->gen_flags &&
-	    nla_put_u32(skb, TCA_BPF_FLAGS_GEN, prog->gen_flags))
+
+	gen_flags = prog->gen_flags;
+	if (!prog->offloaded)
+		gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
+	if (gen_flags && nla_put_u32(skb, TCA_BPF_FLAGS_GEN, gen_flags))
 		goto nla_put_failure;
 
 	nla_nest_end(skb, nest);

Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

[Or Gerlitz]

On Fri, Feb 10, 2017 at 7:42 PM, Jakub Kicinski <kubakici@wp.pl> wrote:
> On Fri, 10 Feb 2017 18:33:13 +0200, Or Gerlitz wrote:
>> On Fri, Feb 10, 2017 at 3:22 AM, Jakub Kicinski <kubakici@wp.pl> wrote:

>> > In cls_bpf we do store information about whether program is offloaded or
>> > not already (see the @offloaded member).  Could we simplify the code
>> > thanks to this?

>> yeah, I felt like I don't fully understand the role of the offloaded
>> member. As I wrote, this patch is compile tested only, I will be happy
>> if you can test it post here a better version, I don't think we need
>> to add/change the flags semantics, see next

> The @offloaded member just tells us whether the program is offloaded.
> We need it because unlike u32 and flower (I think) we have explicit
> ADD and REPLACE.  Other filters just always do REPLACE.  I assume the
> driver keeps track if there is already an associated rule in that case?

In flower you always add new filter through the replace
(TC_CLSFLOWER_REPLACE) call

Re: [PATCH net-next 0/4] net/sched: Use TC skip flags to reflect HW offload status

[Or Gerlitz]

On Fri, Feb 10, 2017 at 9:36 PM, David Miller <davem@davemloft.net> wrote:
> From: Or Gerlitz <ogerlitz@mellanox.com>
> Date: Thu,  9 Feb 2017 16:18:04 +0200
>
>> Currently there is no way of querying whether a filter is
>> offloaded to HW or not when using both policy (no flag).
>>
>> Reuse the skip flags to show the insertion status by setting
>> the skip_hw flag in case the filter wasn't offloaded.
>>
>> The bpf patch is compile tested only, Daniel/Jakub, will
>> appreciate your review/ack.
>  ...
>
> I'm learning towards suggesting that you use new flags, this way it
> will be unambiguous whether we are running an old kernel.

> If you just use the skip flag, it's impossible to tell the difference.


Dave,

Re the old kernel argument, these patches are small and pointish,
would it make sense
to you to consider them as fixes and push them back to the relevant
stable kernels?

Or.

Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

[Or Gerlitz]

On Fri, Feb 10, 2017 at 7:42 PM, Jakub Kicinski <kubakici@wp.pl> wrote:
> On Fri, 10 Feb 2017 18:33:13 +0200, Or Gerlitz wrote:
>> On Fri, Feb 10, 2017 at 3:22 AM, Jakub Kicinski <kubakici@wp.pl> wrote:
>> > On Thu,  9 Feb 2017 16:18:08 +0200, Or Gerlitz wrote:
>> >> Currently there is no way of querying whether a filter is
>> >> offloaded to HW or not when using both policy (no flag).

>> >> Reuse the skip flags to show the insertion status by setting
>> >> the skip_hw flag in case the filter wasn't offloaded.

>> > I'm obviously all for reporting whether tc objects are offloaded or not
>> > but let me ask perhaps the silly question of why reuse the SKIP_HW flag?
>> > We don't have to worry about flag bits running out, could it be clearer
>> > to users to report whether object is present in HW using a new flag?  Or
>> > even two flags for present/non-present so user doesn't have to ponder
>> > what no flag means (old kernel or not offloaded?). I don't really mind
>> > either way I'm just wondering what the motivation was and maybe how
>> > others feel.

>> yeah, the flags are a bit confusing to some people, but it's all about
>> polarity..

>> when the flags were introduced few of us where in favor of "positive"
>> polarity, that is with possibly three values: "sw only" "hw only" and
>> "both" but that JJJ (Jiri/John/Jamal) consensus was to pick a
>> "negative" polarity of "skip sw" "skip hw" and "default" which means
>> the filter is in SW and possibly in HW. I think we can live with that
>> semantics and this small series just helps for the default case, allow
>> user-space to know if the filter was offloaded using the existing
>> fields.

>> I am not in favor of making this more complex...

> I'm 100% with you.  Restating my proposal was to leave the SKIP_* flags
> with all their existing semantics and complexity and for reporting to
> user space whether something got offloaded add new ones.  My opinion
> is that it would make things simpler, but I'm happy with your version
> if none else thinks this way.

> To spell it out with this patchset we would get the following semantics
> of flags in dumps:
>  - no flags - offloaded || old kernel;
>  - skip_hw - not offloaded (either on user request || no flag
>    was set but offload could not happen);
>  - skip_sw - offload only on explicit request.

> What we could do if we want to add flags would be:
>  - no flags - old kernel;
>  - no_offload - not offloaded;
>  - skip_hw | no_offload - not offloaded on explicit user request;
>  - offload - offloaded opportunistically;
>  - skip_sw | offload - offloaded on explicit request.

Jakub,

Re old kernels, I was thinking that we can address that with pushing
this series to
stable kernels, if this direction can fly, we can avoid adding these
two new flags.

Checking that with dave on the cover letter thread, lets see where
this takes us.

Or.

Re: [PATCH net-next 4/4] net/sched: cls_bpf: Use skip flags to reflect HW offload status

[Or Gerlitz]

On Sat, Feb 11, 2017 at 12:19 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:

> FWIW I tested this one and it works.  I also tested this version which
> would take advantage of @offloaded:

I assume that if we go on the existing suggestion, the below is what
you prefer,
so I will pick it up, let know if you think otherwise.


> diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
> index d9c97018317d..51d464f991ff 100644
> --- a/net/sched/cls_bpf.c
> +++ b/net/sched/cls_bpf.c
> @@ -568,8 +568,8 @@ static int cls_bpf_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
>                         struct sk_buff *skb, struct tcmsg *tm)
>  {
>         struct cls_bpf_prog *prog = (struct cls_bpf_prog *) fh;
> +       u32 gen_flags, bpf_flags = 0;
>         struct nlattr *nest;
> -       u32 bpf_flags = 0;
>         int ret;
>
>         if (prog == NULL)
> @@ -601,8 +601,11 @@ static int cls_bpf_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
>                 bpf_flags |= TCA_BPF_FLAG_ACT_DIRECT;
>         if (bpf_flags && nla_put_u32(skb, TCA_BPF_FLAGS, bpf_flags))
>                 goto nla_put_failure;
> -       if (prog->gen_flags &&
> -           nla_put_u32(skb, TCA_BPF_FLAGS_GEN, prog->gen_flags))
> +
> +       gen_flags = prog->gen_flags;
> +       if (!prog->offloaded)
> +               gen_flags |= TCA_CLS_FLAGS_SKIP_HW;
> +       if (gen_flags && nla_put_u32(skb, TCA_BPF_FLAGS_GEN, gen_flags))
>                 goto nla_put_failure;
>
>         nla_nest_end(skb, nest);

Re: [PATCH net-next 0/4] net/sched: Use TC skip flags to reflect HW offload status

[David Miller]

From: Or Gerlitz <gerlitz.or@gmail.com>
Date: Sun, 12 Feb 2017 11:54:25 +0200

> Re the old kernel argument, these patches are small and pointish,
> would it make sense to you to consider them as fixes and push them
> back to the relevant stable kernels?

Sorry, it doesn't work that way.

Re: [PATCH net-next 0/4] net/sched: Use TC skip flags to reflect HW offload status

[Or Gerlitz]

On Fri, Feb 10, 2017 at 9:36 PM, David Miller <davem@davemloft.net> wrote:
> From: Or Gerlitz <ogerlitz@mellanox.com>
> Date: Thu,  9 Feb 2017 16:18:04 +0200
>
>> Currently there is no way of querying whether a filter is
>> offloaded to HW or not when using both policy (no flag).
>>
>> Reuse the skip flags to show the insertion status by setting
>> the skip_hw flag in case the filter wasn't offloaded.
>>
>> The bpf patch is compile tested only, Daniel/Jakub, will
>> appreciate your review/ack.

> I'm learning towards suggesting that you use new flags, this way it
> will be unambiguous whether we are running an old kernel.

> If you just use the skip flag, it's impossible to tell the difference.

Dave, all

SB Jakub detailed proposal, will be good to know if you're happy with
that so I can have backwind for implementing it... basically I was
thinking to add into the UAPI @
 http://lxr.free-electrons.com/source/include/uapi/linux/pkt_cls.h#L157

which has the following flags

/* tca flags definitions */
#define TCA_CLS_FLAGS_SKIP_HW   (1 << 0)
#define TCA_CLS_FLAGS_SKIP_SW   (1 << 1)

#define TCA_CLS_FLAGS_OFFLOADED  (1 << 2)
#define TCA_CLS_FLAGS_NOT_OFFLOADED (1 << 3)

we need two new flags and not only one for user space to
be able and identify if they run over older kernel

And set these flags in each of the offloading classifiers

This is along the proposal of Jakub below

thoughts?

Or.

On Fri, Feb 10, 2017 at 7:42 PM, Jakub Kicinski <kubakici@wp.pl> wrote:

> Restating my proposal was to leave the SKIP_* flags
> with all their existing semantics and complexity and for reporting to
> user space whether something got offloaded add new ones.  My opinion
> is that it would make things simpler, but I'm happy with your version
> if none else thinks this way.
>
> To spell it out with this patchset we would get the following semantics
> of flags in dumps:
>  - no flags - offloaded || old kernel;
>  - skip_hw - not offloaded (either on user request || no flag
>    was set but offload could not happen);
>  - skip_sw - offload only on explicit request.
>
> What we could do if we want to add flags would be:
>  - no flags - old kernel;
>  - no_offload - not offloaded;
>  - skip_hw | no_offload - not offloaded on explicit user request;
>  - offload - offloaded opportunistically;
>  - skip_sw | offload - offloaded on explicit request.
>
> Because of dealing with distributor's kernels and various other
> backports checking if kernel is old is a pain in practice :|