tc rsvp filter show broke

tc rsvp filter show broke

[John Fastabend]

Hi Jamal,

Here is another strange 'tc' result that I think is a
bug. If it was intended I can't see why. Show filter
commands on rsvp classifiers only list the first entry?


# for i in {10..20}; do
tc filter add dev p3p2 protocol ip parent 8001: prio $i \
	rsvp session 12.0.0.$i ipproto udp classid 1:$i;
done

# tc filter show dev p3p2
filter parent 8001: protocol ip pref 10 rsvp
# tc filter show dev p3p2  prio 11
filter parent 8001: protocol ip rsvp
# tc filter show dev p3p2  prio 22

Although the entries must still be around because listing
the prio explicitly show the entries. Also the case where
the prio doesn't exist returns without any errors. A better
response might be 'EINVAL' or something.

This is on 3.16 so nothing to do with my changes. I'm
not sure I'll get a chance until Monday to look at it
so figured I'll document it here in case someone else has
a moment.

Thanks,
John


-- 
John Fastabend         Intel Corporation

Re: tc rsvp filter show broke

[Jamal Hadi Salim]

On 09/26/14 13:41, John Fastabend wrote:
> Hi Jamal,
>
> Here is another strange 'tc' result that I think is a
> bug. If it was intended I can't see why. Show filter
> commands on rsvp classifiers only list the first entry?
>

Hrm  - poking inside there and this seems to be one of those
classifiers that hasnt been getting a lot of love. Doesnt
support actions well for example.

It seems to work for me:
----------
/sbin/tc -V
tc utility, iproute2-ss140610

uname -a
Linux jhs-1 3.16.0-rc2+ #4 SMP Fri Aug 29 11:13:53 EDT 2014 x86_64 
x86_64 x86_64 GNU/Linux

sudo /sbin/tc qdisc add dev eth0 root handle 1:0 prio
sudo /sbin/tc filter add dev eth0 pref 10 proto ip parent 1:0 rsvp 
session 10.0.0.1 ipproto icmp classid 1:1  police rate 1kbit burst 90k 
conform-exceed drop/ok
/sbin/tc -s filter show dev eth0 parent 1:0
filter protocol ip pref 10 rsvp
filter protocol ip pref 10 rsvp fh 0x0001100a flowid 1:1 session 
10.0.0.1 ipproto icmp
  Sent 0 bytes 0 pkts (dropped 0, overlimits 0)

Now lets send a fast ping:
sudo ping -f 10.0.0.1 -c 10000
.......
10000 packets transmitted, 386 received, 96% packet loss, time 115495ms
rtt min/avg/max/mdev = 0.300/0.499/1.262/0.164 ms, ipg/ewma 11.550/0.463 ms

/sbin/tc -s filter show dev eth0 parent 1:0
filter protocol ip pref 10 rsvp
filter protocol ip pref 10 rsvp fh 0x0001100a flowid 1:1 session 
10.0.0.1 ipproto icmp
  Sent 980000 bytes 10000 pkts (dropped 9614, overlimits 9614)

cheers,
jamal

Re: tc rsvp filter show broke

[John Fastabend]

On 09/26/2014 03:48 PM, Jamal Hadi Salim wrote:
> On 09/26/14 13:41, John Fastabend wrote:
>> Hi Jamal,
>>
>> Here is another strange 'tc' result that I think is a
>> bug. If it was intended I can't see why. Show filter
>> commands on rsvp classifiers only list the first entry?
>>
> 
> Hrm  - poking inside there and this seems to be one of those
> classifiers that hasnt been getting a lot of love. Doesnt
> support actions well for example.
> 
> It seems to work for me:

This works for me as well try adding two or more policers and
check if they all get printed.

> ----------
> /sbin/tc -V
> tc utility, iproute2-ss140610
> 
> uname -a
> Linux jhs-1 3.16.0-rc2+ #4 SMP Fri Aug 29 11:13:53 EDT 2014 x86_64 x86_64 x86_64 GNU/Linux
> 
> sudo /sbin/tc qdisc add dev eth0 root handle 1:0 prio
> sudo /sbin/tc filter add dev eth0 pref 10 proto ip parent 1:0 rsvp session 10.0.0.1 ipproto icmp classid 1:1  police rate 1kbit burst 90k conform-exceed drop/ok
> /sbin/tc -s filter show dev eth0 parent 1:0
> filter protocol ip pref 10 rsvp
> filter protocol ip pref 10 rsvp fh 0x0001100a flowid 1:1 session 10.0.0.1 ipproto icmp
>  Sent 0 bytes 0 pkts (dropped 0, overlimits 0)
> 
> Now lets send a fast ping:
> sudo ping -f 10.0.0.1 -c 10000
> .......
> 10000 packets transmitted, 386 received, 96% packet loss, time 115495ms
> rtt min/avg/max/mdev = 0.300/0.499/1.262/0.164 ms, ipg/ewma 11.550/0.463 ms
> 
> /sbin/tc -s filter show dev eth0 parent 1:0
> filter protocol ip pref 10 rsvp
> filter protocol ip pref 10 rsvp fh 0x0001100a flowid 1:1 session 10.0.0.1 ipproto icmp
>  Sent 980000 bytes 10000 pkts (dropped 9614, overlimits 9614)
> 
> cheers,
> jamal
> 
> 
> 
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: tc rsvp filter show broke

[Jamal Hadi Salim]

On 09/26/14 19:25, John Fastabend wrote:
> On 09/26/2014 03:48 PM, Jamal Hadi Salim wrote:

>> Hrm  - poking inside there and this seems to be one of those
>> classifiers that hasnt been getting a lot of love. Doesnt
>> support actions well for example.
>>
>> It seems to work for me:
>
> This works for me as well try adding two or more policers and
> check if they all get printed.
>

Multiple actions wont work for this classifier. I have time,
will send patch.

cheers,
jamal

Re: tc rsvp filter show broke

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

On Sun, Sep 28, 2014 at 8:05 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 09/26/14 19:25, John Fastabend wrote:
>>
>> On 09/26/2014 03:48 PM, Jamal Hadi Salim wrote:
>
>
>>> Hrm  - poking inside there and this seems to be one of those
>>> classifiers that hasnt been getting a lot of love. Doesnt
>>> support actions well for example.
>>>
>>> It seems to work for me:
>>
>>
>> This works for me as well try adding two or more policers and
>> check if they all get printed.
>>
>
> Multiple actions wont work for this classifier. I have time,
> will send patch.
>

Hmm, looks like there is a bug in police dump. Please
try the attached patch.

Thanks.

[-- Attachment #2: cls-fix.diff --]
[-- Type: text/plain, Size: 949 bytes --]

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index c28b0d3..1218900 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -576,16 +576,18 @@ int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts)
 			if (tcf_action_dump(skb, &exts->actions, 0, 0) < 0)
 				goto nla_put_failure;
 			nla_nest_end(skb, nest);
-		} else if (exts->police) {
-			struct tc_action *act = tcf_exts_first_act(exts);
-			nest = nla_nest_start(skb, exts->police);
-			if (nest == NULL || !act)
-				goto nla_put_failure;
-			if (tcf_action_dump_old(skb, act, 0, 0) < 0)
-				goto nla_put_failure;
-			nla_nest_end(skb, nest);
 		}
 	}
+
+	if (exts->police) {
+		struct tc_action *act = tcf_exts_first_act(exts);
+		nest = nla_nest_start(skb, exts->police);
+		if (nest == NULL || !act)
+			goto nla_put_failure;
+		if (tcf_action_dump_old(skb, act, 0, 0) < 0)
+			goto nla_put_failure;
+		nla_nest_end(skb, nest);
+	}
 	return 0;
 
 nla_put_failure:

Re: tc rsvp filter show broke

[John Fastabend]

On 09/28/2014 10:15 AM, Cong Wang wrote:
> On Sun, Sep 28, 2014 at 8:05 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 09/26/14 19:25, John Fastabend wrote:
>>>
>>> On 09/26/2014 03:48 PM, Jamal Hadi Salim wrote:
>>
>>
>>>> Hrm  - poking inside there and this seems to be one of those
>>>> classifiers that hasnt been getting a lot of love. Doesnt
>>>> support actions well for example.
>>>>
>>>> It seems to work for me:
>>>
>>>
>>> This works for me as well try adding two or more policers and
>>> check if they all get printed.
>>>
>>
>> Multiple actions wont work for this classifier. I have time,
>> will send patch.
>>
>
> Hmm, looks like there is a bug in police dump. Please
> try the attached patch.
>
> Thanks.
>


I don't think we need this change, (or perhaps it needs to be a bit
more complete if something is missing) take a look a
tcf_exts_validate(), notice the policer is added to act->list so the
if block in dump() work out,


  int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct 
nlattr **tb,
                    struct nlattr *rate_tlv, struct tcf_exts *exts, bool 
ovr)
  {
  #ifdef CONFIG_NET_CLS_ACT
          {
                  struct tc_action *act;

                  INIT_LIST_HEAD(&exts->actions);
                  if (exts->police && tb[exts->police]) {
                          act = tcf_action_init_1(net, tb[exts->police], 
rate_tlv,
                                                  "police", ovr,
                                                  TCA_ACT_BIND);
                          if (IS_ERR(act))
                                  return PTR_ERR(act);

                          act->type = exts->type = TCA_OLD_COMPAT;
                          list_add(&act->list, &exts->actions);
			^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

                  } else if (exts->action && tb[exts->action]) {
                          int err;
                          err = tcf_action_init(net, tb[exts->action], 
rate_tlv,
                                                NULL, ovr,
                                                TCA_ACT_BIND, 
&exts->actions);
                          if (err)
                                  return err;
                  }
          }

[...]



-- 
John Fastabend         Intel Corporation

Re: tc rsvp filter show broke

[Jamal Hadi Salim]

On 09/28/14 13:50, John Fastabend wrote:
> On 09/28/2014 10:15 AM, Cong Wang wrote:

>                           act->type = exts->type = TCA_OLD_COMPAT;
>                           list_add(&act->list, &exts->actions);
>              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>

Right.

Note, this exts->xx thing we should eventually kill. It was intended to
allow for old binary compatibility (for about 10 years now).
In the earlier days, policer was a speacial action and one could invoke
it directly from "tc filter ....."
But over time, the more sane way is:
"tc filter .. action police ..."
We have killed ability to compile in the kernel policer to support to
allow old tools to continue to use it at least 5 years ago.
tc still has support but I doubt if anyone uses it. Otherwise there
would have been a lot of fallout from
commit 4bfb21ca2043dc5251102e4debcc0ae27f7304e7 in iproute2.

So if there are any takers for patches, i think the first thing is
to kill every call in iproute2 which calls police directly.
And if nobody whines, then get rid of most of exts->XXX (at minimal
there would be no need for exts->police)

cheers,
jamal

Re: tc rsvp filter show broke

[Cong Wang]

On Sun, Sep 28, 2014 at 10:50 AM, John Fastabend
<john.fastabend@gmail.com> wrote:
>
>
> I don't think we need this change, (or perhaps it needs to be a bit
> more complete if something is missing) take a look a
> tcf_exts_validate(), notice the policer is added to act->list so the
> if block in dump() work out,

I thought it's clear that act->list is correct here. :)

I was thinking the dump logic was wrong, but you are right that
it should match the logic in validate. Actually the bug is we forgot
to copy exts->type, I am going to submit a patch below:

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 77147c8..aad6a67 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -549,6 +549,7 @@ void tcf_exts_change(struct tcf_proto *tp, struct
tcf_exts *dst,
  tcf_tree_lock(tp);
  list_splice_init(&dst->actions, &tmp);
  list_splice(&src->actions, &dst->actions);
+ dst->type = src->type;
  tcf_tree_unlock(tp);
  tcf_action_destroy(&tmp, TCA_ACT_UNBIND);
 #endif

Re: tc rsvp filter show broke

[John Fastabend]

On 09/28/2014 06:40 PM, Cong Wang wrote:
> On Sun, Sep 28, 2014 at 10:50 AM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>>
>>
>> I don't think we need this change, (or perhaps it needs to be a bit
>> more complete if something is missing) take a look a
>> tcf_exts_validate(), notice the policer is added to act->list so the
>> if block in dump() work out,
>
> I thought it's clear that act->list is correct here. :)
>
> I was thinking the dump logic was wrong, but you are right that
> it should match the logic in validate. Actually the bug is we forgot
> to copy exts->type, I am going to submit a patch below:
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 77147c8..aad6a67 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -549,6 +549,7 @@ void tcf_exts_change(struct tcf_proto *tp, struct
> tcf_exts *dst,
>    tcf_tree_lock(tp);
>    list_splice_init(&dst->actions, &tmp);
>    list_splice(&src->actions, &dst->actions);
> + dst->type = src->type;
>    tcf_tree_unlock(tp);
>    tcf_action_destroy(&tmp, TCA_ACT_UNBIND);
>   #endif
>

Yep it does seem to be missing...

Although with the latest code can we just drop tcf_exts_change()
and call tcf_exts_validate() directly. I'll check in the morning
but a quick glance makes me think it should be OK and simplifies
things.

.John

-- 
John Fastabend         Intel Corporation