[Patch iproute2] u32: add terminal parameter

[Patch iproute2] u32: add terminal parameter

[Cong Wang]

It is useful to allow user to specify to terminate u32 filter matching
when there is no action. Currently we only terminate it when there is
an action attached.

Cc: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 tc/f_u32.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tc/f_u32.c b/tc/f_u32.c
index f2a862d..a64ad10 100644
--- a/tc/f_u32.c
+++ b/tc/f_u32.c
@@ -1129,7 +1129,14 @@ static int u32_parse_opt(struct filter_util *qu, char *handle,
 			}
 			terminal_ok++;
 			continue;
-
+		} else if (matches(*argv, "terminal") == 0) {
+			argc--;
+			if (argc != 0) {
+				fprintf(stderr, "Illegal \"terminal\"\n");
+				return -1;
+			}
+			terminal_ok++;
+			continue;
 		} else if (matches(*argv, "police") == 0) {
 			NEXT_ARG();
 			if (parse_police(&argc, &argv, TCA_U32_POLICE, n)) {
-- 
1.7.11.7

Re: [Patch iproute2] u32: add terminal parameter

[Stephen Hemminger]

On Fri,  4 Apr 2014 10:17:06 -0700
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> It is useful to allow user to specify to terminate u32 filter matching
> when there is no action. Currently we only terminate it when there is
> an action attached.
> 
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  tc/f_u32.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Iproute2 commands are supposed to be invertible.
If you take something as input, it should be displayed on output.

Re: [Patch iproute2] u32: add terminal parameter

[Cong Wang]

On Fri, Apr 11, 2014 at 2:04 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> Iproute2 commands are supposed to be invertible.
> If you take something as input, it should be displayed on output.

It's already there:

        } else if (sel && sel->flags&TC_U32_TERMINAL) {
                fprintf(f, "terminal flowid ??? ");
        }

Re: [Patch iproute2] u32: add terminal parameter

[Stephen Hemminger]

On Fri, 11 Apr 2014 17:45:53 -0700
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> On Fri, Apr 11, 2014 at 2:04 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > Iproute2 commands are supposed to be invertible.
> > If you take something as input, it should be displayed on output.
> 
> It's already there:
> 
>         } else if (sel && sel->flags&TC_U32_TERMINAL) {
>                 fprintf(f, "terminal flowid ??? ");
>         }

But it printing it like an error, not an known good option.
The ??? implies that somebody did something wrong.

Re: [Patch iproute2] u32: add terminal parameter

[Cong Wang]

On Fri, Apr 11, 2014 at 5:58 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Fri, 11 Apr 2014 17:45:53 -0700
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> It's already there:
>>
>>         } else if (sel && sel->flags&TC_U32_TERMINAL) {
>>                 fprintf(f, "terminal flowid ??? ");
>>         }
>
> But it printing it like an error, not an known good option.
> The ??? implies that somebody did something wrong.

I don't know why ??? is displayed together with terminal, but
nothing seems wrong here, check the output after I add
a mirred action:

filter protocol ip pref 49152 u32
filter protocol ip pref 49152 u32 fh 800: ht divisor 1
filter protocol ip pref 49152 u32 fh 800::800 order 2048 key ht 800
bkt 0 terminal flowid ???
  match 01020304/ffffffff at 16
action order 1: mirred (Egress Mirror to device virbr0) pipe
  index 1 ref 1 bind 1

Re: [Patch iproute2] u32: add terminal parameter

[Jamal Hadi Salim]

Cong,

You MUST specify flowid in your filter construct;->
So I agree with Stephen on the verdict on your patch.
We need a patch to avoid this FAQ problem since it happens
often i.e reject a filter that doesnt have flow/classid
specified. How about you provide that patch instead?

cheers,
jamal

On 04/11/14 21:15, Cong Wang wrote:
> On Fri, Apr 11, 2014 at 5:58 PM, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>> On Fri, 11 Apr 2014 17:45:53 -0700
>> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>> It's already there:
>>>
>>>          } else if (sel && sel->flags&TC_U32_TERMINAL) {
>>>                  fprintf(f, "terminal flowid ??? ");
>>>          }
>>
>> But it printing it like an error, not an known good option.
>> The ??? implies that somebody did something wrong.
>
> I don't know why ??? is displayed together with terminal, but
> nothing seems wrong here, check the output after I add
> a mirred action:
>
> filter protocol ip pref 49152 u32
> filter protocol ip pref 49152 u32 fh 800: ht divisor 1
> filter protocol ip pref 49152 u32 fh 800::800 order 2048 key ht 800
> bkt 0 terminal flowid ???
>    match 01020304/ffffffff at 16
> action order 1: mirred (Egress Mirror to device virbr0) pipe
>    index 1 ref 1 bind 1
> --
> 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: [Patch iproute2] u32: add terminal parameter

[Cong Wang]

On Sat, Apr 12, 2014 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> Cong,
>
> You MUST specify flowid in your filter construct;->

Why? The qdisc my u32 filters attached to is classless (ingress).

Also, why this is relevant to u32 terminal?

> So I agree with Stephen on the verdict on your patch.
> We need a patch to avoid this FAQ problem since it happens
> often i.e reject a filter that doesnt have flow/classid
> specified. How about you provide that patch instead?
>

Even if you were right, it is too late to change, we obviously don't
want to break existing scripts that work fine. I am pretty sure
my script runs well without flowid.

Re: [Patch iproute2] u32: add terminal parameter

[Jamal Hadi Salim]

On 04/14/14 17:18, Cong Wang wrote:
> On Sat, Apr 12, 2014 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> Why? The qdisc my u32 filters attached to is classless (ingress).

flowid is a filter id. I am almost certain it was a mistake on your
part to leave it out. This is why i called it an FAQ and suggested
you provide a patch to block people from leaving it out.

> Also, why this is relevant to u32 terminal?

Because if you defined a flowid you are defining it as a terminal
u32 node. You wouldnt have to complain about "???" or produce a patch
to solve a non-existent problem.
You want to add a new syntax and didnt even bother to look at the code?

>> So I agree with Stephen on the verdict on your patch.
>> We need a patch to avoid this FAQ problem since it happens
>> often i.e reject a filter that doesnt have flow/classid
>> specified. How about you provide that patch instead?
>>
>
> Even if you were right, it is too late to change, we obviously don't
> want to break existing scripts that work fine. I am pretty sure
> my script runs well without flowid.
>

I dont think it can be guaranteed to work at all if you dont specify
a flowid. You may have gotten lucky.

cheers,
jamal

Re: [Patch iproute2] u32: add terminal parameter

[Cong Wang]

On Mon, Apr 14, 2014 at 4:14 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 04/14/14 17:18, Cong Wang wrote:
>>
>> On Sat, Apr 12, 2014 at 4:43 AM, Jamal Hadi Salim <jhs@mojatatu.com>
>> wrote:
>
>
>> Why? The qdisc my u32 filters attached to is classless (ingress).
>
>
> flowid is a filter id. I am almost certain it was a mistake on your
> part to leave it out. This is why i called it an FAQ and suggested
> you provide a patch to block people from leaving it out.
>
>

So essentially why u32 filters can't just do matching without bothering
classid especially when they are attached to an ingress qdisc? I still
fail to see a reason why I have to care about classid here.

Re: [Patch iproute2] u32: add terminal parameter

[Jamal Hadi Salim]

On 04/14/14 21:57, Cong Wang wrote:

> So essentially why u32 filters can't just do matching without bothering

u32 is a special animal. You can actually have intermediate nodes
which are _not_ terminal. i.e you can build a tree of hashes which
interconnect other hashes eventually leading to terminal nodes.

> classid especially when they are attached to an ingress qdisc? I still
> fail to see a reason why I have to care about classid here.

It is more of a historical artifact with "benefits".
Historical in that egress qdiscs with classes existed before ingress;
so it makes sense to keep the syntax people are already familiar with.
Benefits because infact with the right approach the flowid even at
ingress can be used as input(look at the route classifier for example
and association with route accounting of the source).


cheers,
jamal

Re: [Patch iproute2] u32: add terminal parameter

[Cong Wang]

On Tue, Apr 15, 2014 at 6:00 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 04/14/14 21:57, Cong Wang wrote:
>
>> So essentially why u32 filters can't just do matching without bothering
>
>
> u32 is a special animal. You can actually have intermediate nodes
> which are _not_ terminal. i.e you can build a tree of hashes which
> interconnect other hashes eventually leading to terminal nodes.
>

I knew, I thought at least classid is an alternative for action, that is,
one doing classify and one doing some other action.

>
>> classid especially when they are attached to an ingress qdisc? I still
>> fail to see a reason why I have to care about classid here.
>
>
> It is more of a historical artifact with "benefits".
> Historical in that egress qdiscs with classes existed before ingress;
> so it makes sense to keep the syntax people are already familiar with.
> Benefits because infact with the right approach the flowid even at
> ingress can be used as input(look at the route classifier for example
> and association with route accounting of the source).
>

I see. The next question would be what is a reasonable classid
value for a classless qdisc? Or can we set it to some value to express
"we don't care"?

Thanks!

Re: [Patch iproute2] u32: add terminal parameter

[Jamal Hadi Salim]

On 04/15/14 16:17, Cong Wang wrote:

>
> I knew, I thought at least classid is an alternative for action, that is,
> one doing classify and one doing some other action.
>

Indeed you could look at setting the flowid as "default action".

>
> I see. The next question would be what is a reasonable classid
> value for a classless qdisc? Or can we set it to some value to express
> "we don't care"?
>

Mostly it is dont care (and no new syntax is introduced); however,
like i pointed out with the route classifier there could be correlation
with other things  downstream.

cheers,
jamal