[PATCH iproute2-next v3 0/2] Implement filter terse dump mode support

[PATCH iproute2-next v3 0/2] Implement filter terse dump mode support

[Vlad Buslov]

Implement support for terse dump mode which provides only essential
classifier/action info (handle, stats, cookie, etc.). Use new
TCA_DUMP_FLAGS_TERSE flag to prevent copying of unnecessary data from
kernel.

Vlad Buslov (2):
  tc: skip actions that don't have options attribute when printing
  tc: implement support for terse dump

 man/man8/tc.8     | 6 ++++++
 tc/m_bpf.c        | 2 +-
 tc/m_connmark.c   | 2 +-
 tc/m_csum.c       | 2 +-
 tc/m_ct.c         | 2 +-
 tc/m_ctinfo.c     | 2 +-
 tc/m_gact.c       | 2 +-
 tc/m_ife.c        | 2 +-
 tc/m_ipt.c        | 2 +-
 tc/m_mirred.c     | 2 +-
 tc/m_mpls.c       | 2 +-
 tc/m_nat.c        | 2 +-
 tc/m_pedit.c      | 2 +-
 tc/m_sample.c     | 2 +-
 tc/m_simple.c     | 2 +-
 tc/m_skbedit.c    | 2 +-
 tc/m_skbmod.c     | 2 +-
 tc/m_tunnel_key.c | 2 +-
 tc/m_vlan.c       | 2 +-
 tc/m_xt.c         | 2 +-
 tc/m_xt_old.c     | 2 +-
 tc/tc.c           | 6 +++++-
 tc/tc_filter.c    | 9 +++++++++
 23 files changed, 40 insertions(+), 21 deletions(-)

-- 
2.21.0

[PATCH iproute2-next v3 1/2] tc: skip actions that don't have options attribute when printing

[Vlad Buslov]

From: Vlad Buslov <vladbu@mellanox.com>

Modify implementations that return error from action_until->print_aopt()
callback to silently skip actions that don't have their corresponding
TCA_ACT_OPTIONS attribute set (some actions already behave like this). This
is necessary to support terse dump mode in following patch in the series.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 tc/m_bpf.c        | 2 +-
 tc/m_connmark.c   | 2 +-
 tc/m_csum.c       | 2 +-
 tc/m_ct.c         | 2 +-
 tc/m_ctinfo.c     | 2 +-
 tc/m_gact.c       | 2 +-
 tc/m_ife.c        | 2 +-
 tc/m_ipt.c        | 2 +-
 tc/m_mirred.c     | 2 +-
 tc/m_mpls.c       | 2 +-
 tc/m_nat.c        | 2 +-
 tc/m_pedit.c      | 2 +-
 tc/m_sample.c     | 2 +-
 tc/m_simple.c     | 2 +-
 tc/m_skbedit.c    | 2 +-
 tc/m_skbmod.c     | 2 +-
 tc/m_tunnel_key.c | 2 +-
 tc/m_vlan.c       | 2 +-
 tc/m_xt.c         | 2 +-
 tc/m_xt_old.c     | 2 +-
 20 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/tc/m_bpf.c b/tc/m_bpf.c
index e8d704b557f9..6b231cb3d7b0 100644
--- a/tc/m_bpf.c
+++ b/tc/m_bpf.c
@@ -162,7 +162,7 @@ static int bpf_print_opt(struct action_util *au, FILE *f, struct rtattr *arg)
 	int d_ok = 0;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_ACT_BPF_MAX, arg);
 
diff --git a/tc/m_connmark.c b/tc/m_connmark.c
index 4b2dc4e25ef8..38546c75b30c 100644
--- a/tc/m_connmark.c
+++ b/tc/m_connmark.c
@@ -111,7 +111,7 @@ static int print_connmark(struct action_util *au, FILE *f, struct rtattr *arg)
 	struct tc_connmark *ci;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_CONNMARK_MAX, arg);
 	if (tb[TCA_CONNMARK_PARMS] == NULL) {
diff --git a/tc/m_csum.c b/tc/m_csum.c
index afbee9c8de0f..347e5e90e967 100644
--- a/tc/m_csum.c
+++ b/tc/m_csum.c
@@ -167,7 +167,7 @@ print_csum(struct action_util *au, FILE *f, struct rtattr *arg)
 	int uflag_count = 0;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_CSUM_MAX, arg);
 
diff --git a/tc/m_ct.c b/tc/m_ct.c
index 70d186e859f4..20cc9c8a3102 100644
--- a/tc/m_ct.c
+++ b/tc/m_ct.c
@@ -444,7 +444,7 @@ static int print_ct(struct action_util *au, FILE *f, struct rtattr *arg)
 	int ct_action = 0;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_CT_MAX, arg);
 	if (tb[TCA_CT_PARMS] == NULL) {
diff --git a/tc/m_ctinfo.c b/tc/m_ctinfo.c
index e5c1b43642a7..5475fe4d43da 100644
--- a/tc/m_ctinfo.c
+++ b/tc/m_ctinfo.c
@@ -189,7 +189,7 @@ static int print_ctinfo(struct action_util *au, FILE *f, struct rtattr *arg)
 	struct tc_ctinfo *ci;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_CTINFO_MAX, arg);
 	if (!tb[TCA_CTINFO_ACT]) {
diff --git a/tc/m_gact.c b/tc/m_gact.c
index 33f326f823d1..2722e9b805f7 100644
--- a/tc/m_gact.c
+++ b/tc/m_gact.c
@@ -172,7 +172,7 @@ print_gact(struct action_util *au, FILE *f, struct rtattr *arg)
 	struct rtattr *tb[TCA_GACT_MAX + 1];
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_GACT_MAX, arg);
 
diff --git a/tc/m_ife.c b/tc/m_ife.c
index 6a85e087ede1..2491ddb861c2 100644
--- a/tc/m_ife.c
+++ b/tc/m_ife.c
@@ -228,7 +228,7 @@ static int print_ife(struct action_util *au, FILE *f, struct rtattr *arg)
 	SPRINT_BUF(b2);
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_IFE_MAX, arg);
 
diff --git a/tc/m_ipt.c b/tc/m_ipt.c
index cc95eab7fefb..046b310e64ab 100644
--- a/tc/m_ipt.c
+++ b/tc/m_ipt.c
@@ -433,7 +433,7 @@ print_ipt(struct action_util *au, FILE * f, struct rtattr *arg)
 	__u32 hook;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	lib_dir = getenv("IPTABLES_LIB_DIR");
 	if (!lib_dir)
diff --git a/tc/m_mirred.c b/tc/m_mirred.c
index d2bdf4074a73..7c6351865788 100644
--- a/tc/m_mirred.c
+++ b/tc/m_mirred.c
@@ -282,7 +282,7 @@ print_mirred(struct action_util *au, FILE *f, struct rtattr *arg)
 	const char *dev;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_MIRRED_MAX, arg);
 
diff --git a/tc/m_mpls.c b/tc/m_mpls.c
index 3d5d9b25fbf6..1a3bfdda104b 100644
--- a/tc/m_mpls.c
+++ b/tc/m_mpls.c
@@ -198,7 +198,7 @@ static int print_mpls(struct action_util *au, FILE *f, struct rtattr *arg)
 	__u32 val;
 
 	if (!arg)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_MPLS_MAX, arg);
 
diff --git a/tc/m_nat.c b/tc/m_nat.c
index 56e8f47cdefd..63de71014efd 100644
--- a/tc/m_nat.c
+++ b/tc/m_nat.c
@@ -147,7 +147,7 @@ print_nat(struct action_util *au, FILE * f, struct rtattr *arg)
 	int len;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_NAT_MAX, arg);
 
diff --git a/tc/m_pedit.c b/tc/m_pedit.c
index 51dcf10930e8..ec71fcf9922e 100644
--- a/tc/m_pedit.c
+++ b/tc/m_pedit.c
@@ -746,7 +746,7 @@ static int print_pedit(struct action_util *au, FILE *f, struct rtattr *arg)
 	int err;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_PEDIT_MAX, arg);
 
diff --git a/tc/m_sample.c b/tc/m_sample.c
index 4a30513a6247..e2467a93444a 100644
--- a/tc/m_sample.c
+++ b/tc/m_sample.c
@@ -144,7 +144,7 @@ static int print_sample(struct action_util *au, FILE *f, struct rtattr *arg)
 	struct tc_sample *p;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_SAMPLE_MAX, arg);
 
diff --git a/tc/m_simple.c b/tc/m_simple.c
index 70897d6b7c13..bc86be27cbcc 100644
--- a/tc/m_simple.c
+++ b/tc/m_simple.c
@@ -166,7 +166,7 @@ static int print_simple(struct action_util *au, FILE *f, struct rtattr *arg)
 	char *simpdata;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_DEF_MAX, arg);
 
diff --git a/tc/m_skbedit.c b/tc/m_skbedit.c
index 9afe2f0c049d..37625c5ab067 100644
--- a/tc/m_skbedit.c
+++ b/tc/m_skbedit.c
@@ -199,7 +199,7 @@ static int print_skbedit(struct action_util *au, FILE *f, struct rtattr *arg)
 	struct tc_skbedit *p;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_SKBEDIT_MAX, arg);
 
diff --git a/tc/m_skbmod.c b/tc/m_skbmod.c
index d38a5c1921e7..e13d3f16bfcb 100644
--- a/tc/m_skbmod.c
+++ b/tc/m_skbmod.c
@@ -169,7 +169,7 @@ static int print_skbmod(struct action_util *au, FILE *f, struct rtattr *arg)
 	SPRINT_BUF(b2);
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_SKBMOD_MAX, arg);
 
diff --git a/tc/m_tunnel_key.c b/tc/m_tunnel_key.c
index bfec90724d72..f700f6d86c82 100644
--- a/tc/m_tunnel_key.c
+++ b/tc/m_tunnel_key.c
@@ -671,7 +671,7 @@ static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
 	struct tc_tunnel_key *parm;
 
 	if (!arg)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_TUNNEL_KEY_MAX, arg);
 
diff --git a/tc/m_vlan.c b/tc/m_vlan.c
index 1096ba0fbf12..afc9b475ae0a 100644
--- a/tc/m_vlan.c
+++ b/tc/m_vlan.c
@@ -183,7 +183,7 @@ static int print_vlan(struct action_util *au, FILE *f, struct rtattr *arg)
 	struct tc_vlan *parm;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	parse_rtattr_nested(tb, TCA_VLAN_MAX, arg);
 
diff --git a/tc/m_xt.c b/tc/m_xt.c
index 487ba25ad391..deaf96a26f75 100644
--- a/tc/m_xt.c
+++ b/tc/m_xt.c
@@ -320,7 +320,7 @@ print_ipt(struct action_util *au, FILE *f, struct rtattr *arg)
 	__u32 hook;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	/* copy tcipt_globals because .opts will be modified by iptables */
 	struct xtables_globals tmp_tcipt_globals = tcipt_globals;
diff --git a/tc/m_xt_old.c b/tc/m_xt_old.c
index 6a4509a9982f..db014898590d 100644
--- a/tc/m_xt_old.c
+++ b/tc/m_xt_old.c
@@ -358,7 +358,7 @@ print_ipt(struct action_util *au, FILE * f, struct rtattr *arg)
 	__u32 hook;
 
 	if (arg == NULL)
-		return -1;
+		return 0;
 
 	set_lib_dir();
 
-- 
2.21.0

[PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

From: Vlad Buslov <vladbu@mellanox.com>

Implement support for classifier/action terse dump using new TCA_DUMP_FLAGS
tlv with only available flag value TCA_DUMP_FLAGS_TERSE. Set the flag when
user requested it with following example CLI (-br for 'brief'):

$ tc -s -brief filter show dev ens1f0 ingress
filter protocol all pref 49151 flower chain 0
filter protocol all pref 49151 flower chain 0 handle 0x1
  not_in_hw
        action order 1:         Action statistics:
        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
        backlog 0b 0p requeues 0

filter protocol all pref 49152 flower chain 0
filter protocol all pref 49152 flower chain 0 handle 0x1
  not_in_hw
        action order 1:         Action statistics:
        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
        backlog 0b 0p requeues 0

In terse mode dump only outputs essential data needed to identify the
filter and action (handle, cookie, etc.) and stats, if requested by the
user. The intention is to significantly improve rule dump rate by omitting
all static data that do not change after rule is created.

Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---

Notes:
    Changes V2 -> V3:
    
    - Add brief dump output example to commit message.
    
    Changes V1 -> V2:
    
    - Invoke terse dump with tc command '-brief' option instead of
    filter-specific 'terse' flag.
    
    - Extend tc man and usage string with new option.

 man/man8/tc.8  | 6 ++++++
 tc/tc.c        | 6 +++++-
 tc/tc_filter.c | 9 +++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/man/man8/tc.8 b/man/man8/tc.8
index 7e9019f561ea..e8622053df65 100644
--- a/man/man8/tc.8
+++ b/man/man8/tc.8
@@ -854,6 +854,12 @@ option for creating
 alias.
 .RE
 
+.TP
+.BR "\-br" , " \-brief"
+Print only essential data needed to identify the filter and action (handle,
+cookie, etc.) and stats. This option is currently only supported by
+.BR "tc filter show " command.
+
 .SH "EXAMPLES"
 .PP
 tc -g class show dev eth0
diff --git a/tc/tc.c b/tc/tc.c
index 5d57054b45fb..bdd5d4faf886 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -44,6 +44,7 @@ bool use_names;
 int json;
 int color;
 int oneline;
+int brief;
 
 static char *conf_file;
 
@@ -202,7 +203,8 @@ static void usage(void)
 		"       OPTIONS := { -V[ersion] | -s[tatistics] | -d[etails] | -r[aw] |\n"
 		"		    -o[neline] | -j[son] | -p[retty] | -c[olor]\n"
 		"		    -b[atch] [filename] | -n[etns] name | -N[umeric] |\n"
-		"		     -nm | -nam[es] | { -cf | -conf } path }\n");
+		"		     -nm | -nam[es] | { -cf | -conf } path\n"
+		"		     -br[ief] }\n");
 }
 
 static int do_cmd(int argc, char **argv)
@@ -336,6 +338,8 @@ int main(int argc, char **argv)
 			++json;
 		} else if (matches(argv[1], "-oneline") == 0) {
 			++oneline;
+		}else if (matches(argv[1], "-brief") == 0) {
+			++brief;
 		} else {
 			fprintf(stderr,
 				"Option \"%s\" is unknown, try \"tc -help\".\n",
diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index c591a19f3123..71be2e8119c9 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -721,6 +721,15 @@ static int tc_filter_list(int cmd, int argc, char **argv)
 	if (filter_chain_index_set)
 		addattr32(&req.n, sizeof(req), TCA_CHAIN, chain_index);
 
+	if (brief) {
+		struct nla_bitfield32 flags = {
+			.value = TCA_DUMP_FLAGS_TERSE,
+			.selector = TCA_DUMP_FLAGS_TERSE
+		};
+
+		addattr_l(&req.n, MAX_MSG, TCA_DUMP_FLAGS, &flags, sizeof(flags));
+	}
+
 	if (rtnl_dump_request_n(&rth, &req.n) < 0) {
 		perror("Cannot send dump request");
 		return 1;
-- 
2.21.0

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-16 10:42 a.m., Vlad Buslov wrote:
> From: Vlad Buslov <vladbu@mellanox.com>
> 
> Implement support for classifier/action terse dump using new TCA_DUMP_FLAGS
> tlv with only available flag value TCA_DUMP_FLAGS_TERSE. Set the flag when
> user requested it with following example CLI (-br for 'brief'):
> 
> $ tc -s -brief filter show dev ens1f0 ingress
> filter protocol all pref 49151 flower chain 0
> filter protocol all pref 49151 flower chain 0 handle 0x1
>    not_in_hw
>          action order 1:         Action statistics:
>          Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>          backlog 0b 0p requeues 0
> 
> filter protocol all pref 49152 flower chain 0
> filter protocol all pref 49152 flower chain 0 handle 0x1
>    not_in_hw
>          action order 1:         Action statistics:
>          Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>          backlog 0b 0p requeues 0
> 

Should the action name at least show up?


cheers,
jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Fri 16 Oct 2020 at 19:07, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-16 10:42 a.m., Vlad Buslov wrote:
>> From: Vlad Buslov <vladbu@mellanox.com>
>>
>> Implement support for classifier/action terse dump using new TCA_DUMP_FLAGS
>> tlv with only available flag value TCA_DUMP_FLAGS_TERSE. Set the flag when
>> user requested it with following example CLI (-br for 'brief'):
>>
>> $ tc -s -brief filter show dev ens1f0 ingress
>> filter protocol all pref 49151 flower chain 0
>> filter protocol all pref 49151 flower chain 0 handle 0x1
>>    not_in_hw
>>          action order 1:         Action statistics:
>>          Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>>          backlog 0b 0p requeues 0
>>
>> filter protocol all pref 49152 flower chain 0
>> filter protocol all pref 49152 flower chain 0 handle 0x1
>>    not_in_hw
>>          action order 1:         Action statistics:
>>          Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>>          backlog 0b 0p requeues 0
>>
>
> Should the action name at least show up?
>
>
> cheers,
> jamal

All action print callbacks have arg==NULL check and return at the
beginning. To print action type we need either to have dedicated
'brief_dump' callback instead of reusing print_aop() or extend/refactor
print_aop() implementation for all actions to always print the type
before checking the arg. What do you suggest?

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-16 12:42 p.m., Vlad Buslov wrote:

> 
> All action print callbacks have arg==NULL check and return at the
> beginning. To print action type we need either to have dedicated
> 'brief_dump' callback instead of reusing print_aop() or extend/refactor
> print_aop() implementation for all actions to always print the type
> before checking the arg. What do you suggest?
> 

Either one sounds appealing - the refactoring feels simpler
as opposed to a->terse_print().

BTW: the action index, unless i missed something, is not transported
from the kernel for terse option. It is an important parameter
when actions are shared by filters (since they will have the same
index).
Am i missing something?

cheers,
jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Sat 17 Oct 2020 at 14:20, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-16 12:42 p.m., Vlad Buslov wrote:
>
>>
>> All action print callbacks have arg==NULL check and return at the
>> beginning. To print action type we need either to have dedicated
>> 'brief_dump' callback instead of reusing print_aop() or extend/refactor
>> print_aop() implementation for all actions to always print the type
>> before checking the arg. What do you suggest?
>>
>
> Either one sounds appealing - the refactoring feels simpler
> as opposed to a->terse_print().

With such refactoring we action type will be printed before some basic
validation which can lead to outputting the type together with error
message. Consider tunnel key action output callback as example:

static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
{
	struct rtattr *tb[TCA_TUNNEL_KEY_MAX + 1];
	struct tc_tunnel_key *parm;

	if (!arg)
		return 0;

	parse_rtattr_nested(tb, TCA_TUNNEL_KEY_MAX, arg);

	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
		fprintf(stderr, "Missing tunnel_key parameters\n");
		return -1;
	}
	parm = RTA_DATA(tb[TCA_TUNNEL_KEY_PARMS]);

	print_string(PRINT_ANY, "kind", "%s ", "tunnel_key");

If print "kind" call is moved before checking the arg it will always be
printed, even when immediately followed by "Missing tunnel_key
parameters\n" string. Is this a concern?

>
> BTW: the action index, unless i missed something, is not transported
> from the kernel for terse option. It is an important parameter
> when actions are shared by filters (since they will have the same
> index).
> Am i missing something?

Yes, tc_action_ops->dump(), which outputs action index among other data,
is not called at all by terse dump.

>
> cheers,
> jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-18 8:16 a.m., Vlad Buslov wrote:
> On Sat 17 Oct 2020 at 14:20, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2020-10-16 12:42 p.m., Vlad Buslov wrote:
>>

>> Either one sounds appealing - the refactoring feels simpler
>> as opposed to a->terse_print().
> 
> With such refactoring we action type will be printed before some basic
> validation which can lead to outputting the type together with error
> message. Consider tunnel key action output callback as example:
> 
> static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
> {
> 	struct rtattr *tb[TCA_TUNNEL_KEY_MAX + 1];
> 	struct tc_tunnel_key *parm;
> 
> 	if (!arg)
> 		return 0;
> 
> 	parse_rtattr_nested(tb, TCA_TUNNEL_KEY_MAX, arg);
> 
> 	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
> 		fprintf(stderr, "Missing tunnel_key parameters\n");
> 		return -1;
> 	}
> 	parm = RTA_DATA(tb[TCA_TUNNEL_KEY_PARMS]);
> 
> 	print_string(PRINT_ANY, "kind", "%s ", "tunnel_key");
> 
> If print "kind" call is moved before checking the arg it will always be
> printed, even when immediately followed by "Missing tunnel_key
> parameters\n" string. Is this a concern?
> 

That could be a good thing, no? you get to see the action name with the
error. Its really not a big deal if you decide to do a->terse_print()
instead.

>>
>> BTW: the action index, unless i missed something, is not transported
>> from the kernel for terse option. It is an important parameter
>> when actions are shared by filters (since they will have the same
>> index).
>> Am i missing something?
> 
> Yes, tc_action_ops->dump(), which outputs action index among other data,
> is not called at all by terse dump.

I am suggesting it is an important detail that is currently missing.
Alternatively since you have the cookies in there - it is feasible that
someone who creates the action could "encode" the index in the cookie.
But that makes it a "proprietary" choice of whoever is creating
the filter/action.

cheers,
jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Mon 19 Oct 2020 at 16:48, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-18 8:16 a.m., Vlad Buslov wrote:
>> On Sat 17 Oct 2020 at 14:20, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2020-10-16 12:42 p.m., Vlad Buslov wrote:
>>>
>
>>> Either one sounds appealing - the refactoring feels simpler
>>> as opposed to a->terse_print().
>>
>> With such refactoring we action type will be printed before some basic
>> validation which can lead to outputting the type together with error
>> message. Consider tunnel key action output callback as example:
>>
>> static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
>> {
>> 	struct rtattr *tb[TCA_TUNNEL_KEY_MAX + 1];
>> 	struct tc_tunnel_key *parm;
>>
>> 	if (!arg)
>> 		return 0;
>>
>> 	parse_rtattr_nested(tb, TCA_TUNNEL_KEY_MAX, arg);
>>
>> 	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
>> 		fprintf(stderr, "Missing tunnel_key parameters\n");
>> 		return -1;
>> 	}
>> 	parm = RTA_DATA(tb[TCA_TUNNEL_KEY_PARMS]);
>>
>> 	print_string(PRINT_ANY, "kind", "%s ", "tunnel_key");
>>
>> If print "kind" call is moved before checking the arg it will always be
>> printed, even when immediately followed by "Missing tunnel_key
>> parameters\n" string. Is this a concern?
>>
>
> That could be a good thing, no? you get to see the action name with the
> error. Its really not a big deal if you decide to do a->terse_print()
> instead.

Maybe. Just saying that this change would also change user-visible
iproute2 behavior.

>
>>>
>>> BTW: the action index, unless i missed something, is not transported
>>> from the kernel for terse option. It is an important parameter
>>> when actions are shared by filters (since they will have the same
>>> index).
>>> Am i missing something?
>>
>> Yes, tc_action_ops->dump(), which outputs action index among other data,
>> is not called at all by terse dump.
>
> I am suggesting it is an important detail that is currently missing.
> Alternatively since you have the cookies in there - it is feasible that
> someone who creates the action could "encode" the index in the cookie.
> But that makes it a "proprietary" choice of whoever is creating
> the filter/action.

It is not a trivial change. To get this data we need to call
tc_action_ops->dump() which puts bunch of other unrelated info in
TCA_OPTIONS nested attr. This hurts both dump size and runtime
performance. Even if we add another argument to dump "terse dump, print
only index", index is still part of larger options structure which
includes at least following fields:

#define tc_gen \
	__u32                 index; \
	__u32                 capab; \
	int                   action; \
	int                   refcnt; \
	int                   bindcnt

This wouldn't be much of a terse dump anymore. What prevents user that
needs all action info from calling regular dump? It is not like terse
dump substitutes it or somehow makes it harder to use.

>
> cheers,
> jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-19 11:18 a.m., Vlad Buslov wrote:
> 
> On Mon 19 Oct 2020 at 16:48, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2020-10-18 8:16 a.m., Vlad Buslov wrote:

[..]

>> That could be a good thing, no? you get to see the action name with the
>> error. Its really not a big deal if you decide to do a->terse_print()
>> instead.
> 
> Maybe. Just saying that this change would also change user-visible
> iproute2 behavior.
> 

You are right(for the non-terse output). tbh, not sure if it is a big
deal given it happens only for the error case (where scripts look
for exit codes typically); having said that:
a ->terse_print() would be ok

> It is not a trivial change. To get this data we need to call
> tc_action_ops->dump() which puts bunch of other unrelated info in
> TCA_OPTIONS nested attr. This hurts both dump size and runtime
> performance. Even if we add another argument to dump "terse dump, print
> only index", index is still part of larger options structure which
> includes at least following fields:
> 
> #define tc_gen \
> 	__u32                 index; \
> 	__u32                 capab; \
> 	int                   action; \
> 	int                   refcnt; \
> 	int                   bindcnt
>


index is the _only_ important field for analytics purposes in that list.
i.e if i know the index i can correlate stats with one or more
filters (whether shared or not).
My worry is you have a very specific use case for your hardware or
maybe it is ovs - where counters are uniquely tied to filters and
there is no sharing. And possibly maybe only one counter can be tied
to a filter (was not sure if you could handle more than one action
in the terse from looking at the code).
Our assumptions so far had no such constraints.
Maybe a new TERSE_OPTIONS TLV, and then add an extra flag
to indicate interest in the tlv? Peharps store the stats in it as well.

> This wouldn't be much of a terse dump anymore. What prevents user that
> needs all action info from calling regular dump? It is not like terse
> dump substitutes it or somehow makes it harder to use.

Both scaling and correctness are important. You have the cookie
in the terse dump, thats a lot of data.
In our case we totally bypass filters to reduce the amount of data
crossing to user space (tc action ls). Theres still a lot of data
crossing which we could trim with a terse dump. All we are interested
in are stats. Another alternative is perhaps to introduce the index for
the direct dump.

cheers,
jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Tue 20 Oct 2020 at 15:29, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-19 11:18 a.m., Vlad Buslov wrote:
>>
>> On Mon 19 Oct 2020 at 16:48, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2020-10-18 8:16 a.m., Vlad Buslov wrote:
>
> [..]
>
>>> That could be a good thing, no? you get to see the action name with the
>>> error. Its really not a big deal if you decide to do a->terse_print()
>>> instead.
>>
>> Maybe. Just saying that this change would also change user-visible
>> iproute2 behavior.
>>
>
> You are right(for the non-terse output). tbh, not sure if it is a big
> deal given it happens only for the error case (where scripts look
> for exit codes typically); having said that:
> a ->terse_print() would be ok
>
>> It is not a trivial change. To get this data we need to call
>> tc_action_ops->dump() which puts bunch of other unrelated info in
>> TCA_OPTIONS nested attr. This hurts both dump size and runtime
>> performance. Even if we add another argument to dump "terse dump, print
>> only index", index is still part of larger options structure which
>> includes at least following fields:
>>
>> #define tc_gen \
>> 	__u32                 index; \
>> 	__u32                 capab; \
>> 	int                   action; \
>> 	int                   refcnt; \
>> 	int                   bindcnt
>>
>
>
> index is the _only_ important field for analytics purposes in that list.
> i.e if i know the index i can correlate stats with one or more
> filters (whether shared or not).
> My worry is you have a very specific use case for your hardware or
> maybe it is ovs - where counters are uniquely tied to filters and
> there is no sharing. And possibly maybe only one counter can be tied
> to a filter (was not sure if you could handle more than one action
> in the terse from looking at the code).

OVS uses cookie to uniquely identify the flow and it does support
multiple actions per flow.

> Our assumptions so far had no such constraints.
> Maybe a new TERSE_OPTIONS TLV, and then add an extra flag
> to indicate interest in the tlv? Peharps store the stats in it as well.

Maybe, but wouldn't that require making it a new dump mode? Current
terse dump is already in released kernel and this seems like a
backward-incompatible change.

>
>> This wouldn't be much of a terse dump anymore. What prevents user that
>> needs all action info from calling regular dump? It is not like terse
>> dump substitutes it or somehow makes it harder to use.
>
> Both scaling and correctness are important. You have the cookie
> in the terse dump, thats a lot of data.

Cookie only consumes space in resulting netlink packet if used set the
cookie during action init. Otherwise, the cookie attribute is omitted.

> In our case we totally bypass filters to reduce the amount of data
> crossing to user space (tc action ls). Theres still a lot of data
> crossing which we could trim with a terse dump. All we are interested
> in are stats. Another alternative is perhaps to introduce the index for
> the direct dump.

What is the direct dump?

>
> cheers,
> jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-21 4:19 a.m., Vlad Buslov wrote:
> 
> On Tue 20 Oct 2020 at 15:29, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2020-10-19 11:18 a.m., Vlad Buslov wrote:
>> My worry is you have a very specific use case for your hardware or
>> maybe it is ovs - where counters are uniquely tied to filters and
>> there is no sharing. And possibly maybe only one counter can be tied
>> to a filter (was not sure if you could handle more than one action
>> in the terse from looking at the code).
> 
> OVS uses cookie to uniquely identify the flow and it does support
> multiple actions per flow.


ok, so they use it like a flowid/classid to identify the flow.
In our use case the cookie stores all kinds of other state that
the controller can avoid to lookup after a query.
index otoh is universal i.e two different users can intepret it
per action tying it specific stats.
IOW: I dont think it replaces the index.
Do they set cookies on all actions in a flow?


>> Our assumptions so far had no such constraints.
>> Maybe a new TERSE_OPTIONS TLV, and then add an extra flag
>> to indicate interest in the tlv? Peharps store the stats in it as well.
> 
> Maybe, but wouldn't that require making it a new dump mode? Current
> terse dump is already in released kernel and this seems like a
> backward-incompatible change.
> 

I meant you would set a new flag(in addition to TERSE) in a request to
the kernel to ask for the index to be made available on the response.
Response comes back in a TLV with just index in it for now.

>>
>>> This wouldn't be much of a terse dump anymore. What prevents user that
>>> needs all action info from calling regular dump? It is not like terse
>>> dump substitutes it or somehow makes it harder to use.
>>
>> Both scaling and correctness are important. You have the cookie
>> in the terse dump, thats a lot of data.
> 
> Cookie only consumes space in resulting netlink packet if used set the
> cookie during action init. Otherwise, the cookie attribute is omitted.

True, but: I am wondering why it is even considered in when terseness
was a requirement (and index was left out).

>> In our case we totally bypass filters to reduce the amount of data
>> crossing to user space (tc action ls). Theres still a lot of data
>> crossing which we could trim with a terse dump. All we are interested
>> in are stats. Another alternative is perhaps to introduce the index for
>> the direct dump.
> 
> What is the direct dump?

tc action ls ...
Like i said in our use cases to get the stats we just dumped the actions
we wanted. It is a lot less data than having the filter + actions.
And with your idea of terseness we can trim down further how much
data by removing all the action attributes coming back if we set TERSE
flag in such a request. But the index has to be there to make sense.

cheers,
jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Thu 22 Oct 2020 at 17:05, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-21 4:19 a.m., Vlad Buslov wrote:
>>
>> On Tue 20 Oct 2020 at 15:29, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2020-10-19 11:18 a.m., Vlad Buslov wrote:
>>> My worry is you have a very specific use case for your hardware or
>>> maybe it is ovs - where counters are uniquely tied to filters and
>>> there is no sharing. And possibly maybe only one counter can be tied
>>> to a filter (was not sure if you could handle more than one action
>>> in the terse from looking at the code).
>>
>> OVS uses cookie to uniquely identify the flow and it does support
>> multiple actions per flow.
>
>
> ok, so they use it like a flowid/classid to identify the flow.
> In our use case the cookie stores all kinds of other state that
> the controller can avoid to lookup after a query.
> index otoh is universal i.e two different users can intepret it
> per action tying it specific stats.
> IOW: I dont think it replaces the index.
> Do they set cookies on all actions in a flow?

AFAIK on only one action per flow.

>
>
>>> Our assumptions so far had no such constraints.
>>> Maybe a new TERSE_OPTIONS TLV, and then add an extra flag
>>> to indicate interest in the tlv? Peharps store the stats in it as well.
>>
>> Maybe, but wouldn't that require making it a new dump mode? Current
>> terse dump is already in released kernel and this seems like a
>> backward-incompatible change.
>>
>
> I meant you would set a new flag(in addition to TERSE) in a request to
> the kernel to ask for the index to be made available on the response.
> Response comes back in a TLV with just index in it for now.

Makes sense.

>
>>>
>>>> This wouldn't be much of a terse dump anymore. What prevents user that
>>>> needs all action info from calling regular dump? It is not like terse
>>>> dump substitutes it or somehow makes it harder to use.
>>>
>>> Both scaling and correctness are important. You have the cookie
>>> in the terse dump, thats a lot of data.
>>
>> Cookie only consumes space in resulting netlink packet if used set the
>> cookie during action init. Otherwise, the cookie attribute is omitted.
>
> True, but: I am wondering why it is even considered in when terseness
> was a requirement (and index was left out).

There was several reasons for me to include it:

- As I wrote in previous email its TLV is only included in dump if user
  set the cookie. Users who don't use cookies don't lose any performance
  of terse dump.

- Including it didn't require any changes to tc_action_ops->dump() (like
  passing 'terse' flag or introducing dedicated terse_dump() callback)
  because it is processed in tcf_action_dump_1().

- OVS was the main use-case for us because it relies on filter dump for
  flow revalidation and uses cookie to identify the flows.

>
>>> In our case we totally bypass filters to reduce the amount of data
>>> crossing to user space (tc action ls). Theres still a lot of data
>>> crossing which we could trim with a terse dump. All we are interested
>>> in are stats. Another alternative is perhaps to introduce the index for
>>> the direct dump.
>>
>> What is the direct dump?
>
> tc action ls ...
> Like i said in our use cases to get the stats we just dumped the actions
> we wanted. It is a lot less data than having the filter + actions.
> And with your idea of terseness we can trim down further how much
> data by removing all the action attributes coming back if we set TERSE
> flag in such a request. But the index has to be there to make sense.

Yes, that makes sense. I guess introducing something like 'tc action -br
ls ..' mode implemented by means of existing terse flag + new 'also
output action index' flag would achieve that goal.

>
> cheers,
> jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-23 8:48 a.m., Vlad Buslov wrote:
> On Thu 22 Oct 2020 at 17:05, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2020-10-21 4:19 a.m., Vlad Buslov wrote:
>>>
>>> On Tue 20 Oct 2020 at 15:29, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>> On 2020-10-19 11:18 a.m., Vlad Buslov wrote:
>>>> My worry is you have a very specific use case for your hardware or
>>>> maybe it is ovs - where counters are uniquely tied to filters and
>>>> there is no sharing. And possibly maybe only one counter can be tied
>>>> to a filter (was not sure if you could handle more than one action
>>>> in the terse from looking at the code).
>>>
>>> OVS uses cookie to uniquely identify the flow and it does support
>>> multiple actions per flow.
>>
>>
>> ok, so they use it like a flowid/classid to identify the flow.
>> In our use case the cookie stores all kinds of other state that
>> the controller can avoid to lookup after a query.
>> index otoh is universal i.e two different users can intepret it
>> per action tying it specific stats.
>> IOW: I dont think it replaces the index.
>> Do they set cookies on all actions in a flow?
> 
> AFAIK on only one action per flow.
> 

To each their own i guess. Sounds like it is being used as flowid
entity.
We pack a lot of metaencoding into those cookies. And to us
a "service" is essentially a filter match folowed by a graph
of actions (which could cyclic).


>>> Cookie only consumes space in resulting netlink packet if used set the
>>> cookie during action init. Otherwise, the cookie attribute is omitted.
>>
>> True, but: I am wondering why it is even considered in when terseness
>> was a requirement (and index was left out).
> 
> There was several reasons for me to include it:
> 
> - As I wrote in previous email its TLV is only included in dump if user
>    set the cookie. Users who don't use cookies don't lose any performance
>    of terse dump.
> 
> - Including it didn't require any changes to tc_action_ops->dump() (like
>    passing 'terse' flag or introducing dedicated terse_dump() callback)
>    because it is processed in tcf_action_dump_1().
> 
> - OVS was the main use-case for us because it relies on filter dump for
>    flow revalidation and uses cookie to identify the flows.
>

Which is fine - but it is a very ovs specific need.
>>
>>>> In our case we totally bypass filters to reduce the amount of data
>>>> crossing to user space (tc action ls). Theres still a lot of data
>>>> crossing which we could trim with a terse dump. All we are interested
>>>> in are stats. Another alternative is perhaps to introduce the index for
>>>> the direct dump.
>>>
>>> What is the direct dump?
>>
>> tc action ls ...
>> Like i said in our use cases to get the stats we just dumped the actions
>> we wanted. It is a lot less data than having the filter + actions.
>> And with your idea of terseness we can trim down further how much
>> data by removing all the action attributes coming back if we set TERSE
>> flag in such a request. But the index has to be there to make sense.
> 
> Yes, that makes sense. I guess introducing something like 'tc action -br
> ls ..' mode implemented by means of existing terse flag + new 'also
> output action index' flag would achieve that goal.
>

Right. There should be no interest in the cookie here at all. Maybe
it could be optional with a flag indication.
Have time to cook a patch? I'll taste/test it.

cheers,
jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Sat 24 Oct 2020 at 20:40, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-23 8:48 a.m., Vlad Buslov wrote:
>> On Thu 22 Oct 2020 at 17:05, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2020-10-21 4:19 a.m., Vlad Buslov wrote:
>>>>
>>>> On Tue 20 Oct 2020 at 15:29, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>>> On 2020-10-19 11:18 a.m., Vlad Buslov wrote:
>>>>> My worry is you have a very specific use case for your hardware or
>>>>> maybe it is ovs - where counters are uniquely tied to filters and
>>>>> there is no sharing. And possibly maybe only one counter can be tied
>>>>> to a filter (was not sure if you could handle more than one action
>>>>> in the terse from looking at the code).
>>>>
>>>> OVS uses cookie to uniquely identify the flow and it does support
>>>> multiple actions per flow.
>>>
>>>
>>> ok, so they use it like a flowid/classid to identify the flow.
>>> In our use case the cookie stores all kinds of other state that
>>> the controller can avoid to lookup after a query.
>>> index otoh is universal i.e two different users can intepret it
>>> per action tying it specific stats.
>>> IOW: I dont think it replaces the index.
>>> Do they set cookies on all actions in a flow?
>>
>> AFAIK on only one action per flow.
>>
>
> To each their own i guess. Sounds like it is being used as flowid
> entity.
> We pack a lot of metaencoding into those cookies. And to us
> a "service" is essentially a filter match folowed by a graph
> of actions (which could cyclic).
>
>
>>>> Cookie only consumes space in resulting netlink packet if used set the
>>>> cookie during action init. Otherwise, the cookie attribute is omitted.
>>>
>>> True, but: I am wondering why it is even considered in when terseness
>>> was a requirement (and index was left out).
>>
>> There was several reasons for me to include it:
>>
>> - As I wrote in previous email its TLV is only included in dump if user
>>    set the cookie. Users who don't use cookies don't lose any performance
>>    of terse dump.
>>
>> - Including it didn't require any changes to tc_action_ops->dump() (like
>>    passing 'terse' flag or introducing dedicated terse_dump() callback)
>>    because it is processed in tcf_action_dump_1().
>>
>> - OVS was the main use-case for us because it relies on filter dump for
>>    flow revalidation and uses cookie to identify the flows.
>>
>
> Which is fine - but it is a very ovs specific need.
>>>
>>>>> In our case we totally bypass filters to reduce the amount of data
>>>>> crossing to user space (tc action ls). Theres still a lot of data
>>>>> crossing which we could trim with a terse dump. All we are interested
>>>>> in are stats. Another alternative is perhaps to introduce the index for
>>>>> the direct dump.
>>>>
>>>> What is the direct dump?
>>>
>>> tc action ls ...
>>> Like i said in our use cases to get the stats we just dumped the actions
>>> we wanted. It is a lot less data than having the filter + actions.
>>> And with your idea of terseness we can trim down further how much
>>> data by removing all the action attributes coming back if we set TERSE
>>> flag in such a request. But the index has to be there to make sense.
>>
>> Yes, that makes sense. I guess introducing something like 'tc action -br
>> ls ..' mode implemented by means of existing terse flag + new 'also
>> output action index' flag would achieve that goal.
>>
>
> Right. There should be no interest in the cookie here at all. Maybe
> it could be optional with a flag indication.
> Have time to cook a patch? I'll taste/test it.

Patch to make cookie in filter terse dump optional? That would break
existing terse dump users that rely on it (OVS).

>
> cheers,
> jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[David Ahern]

On 10/26/20 5:28 AM, Vlad Buslov wrote:
> Patch to make cookie in filter terse dump optional? That would break
> existing terse dump users that rely on it (OVS).

Maybe I missed something in the discussions. terse dump for tc has not
been committed to the tree yet, so there are no users relying on it.

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Mon 26 Oct 2020 at 16:52, David Ahern <dsahern@gmail.com> wrote:
> On 10/26/20 5:28 AM, Vlad Buslov wrote:
>> Patch to make cookie in filter terse dump optional? That would break
>> existing terse dump users that rely on it (OVS).
>
> Maybe I missed something in the discussions. terse dump for tc has not
> been committed to the tree yet, so there are no users relying on it.

What do you mean? Kernel terse dump flag and implementation have been
committed long time ago and are now present in released kernels.

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[David Ahern]

On 10/26/20 9:06 AM, Vlad Buslov wrote:
> What do you mean? Kernel terse dump flag and implementation have been
> committed long time ago and are now present in released kernels.
> 

ah, kernel side feature, not the iproute2 change.

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-26 7:28 a.m., Vlad Buslov wrote:
> 
> On Sat 24 Oct 2020 at 20:40, Jamal Hadi Salim <jhs@mojatatu.com> wrote:

[..]
>>>
>>> Yes, that makes sense. I guess introducing something like 'tc action -br
>>> ls ..' mode implemented by means of existing terse flag + new 'also
>>> output action index' flag would achieve that goal.
>>>
>>
>> Right. There should be no interest in the cookie here at all. Maybe
>> it could be optional with a flag indication.
>> Have time to cook a patch? I'll taste/test it.
> 
> Patch to make cookie in filter terse dump optional? That would break
> existing terse dump users that rely on it (OVS).

Meant patch for 'tc action -br ls'

Which by default would not include the cookie.

cheers,
jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Mon 26 Oct 2020 at 19:12, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-26 7:28 a.m., Vlad Buslov wrote:
>>
>> On Sat 24 Oct 2020 at 20:40, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> [..]
>>>>
>>>> Yes, that makes sense. I guess introducing something like 'tc action -br
>>>> ls ..' mode implemented by means of existing terse flag + new 'also
>>>> output action index' flag would achieve that goal.
>>>>
>>>
>>> Right. There should be no interest in the cookie here at all. Maybe
>>> it could be optional with a flag indication.
>>> Have time to cook a patch? I'll taste/test it.
>>
>> Patch to make cookie in filter terse dump optional? That would break
>> existing terse dump users that rely on it (OVS).
>
> Meant patch for 'tc action -br ls'
>
> Which by default would not include the cookie.

So action-dump-specific flag that causes act api to output action index
(via new attribute) and skips cookie?

>
> cheers,
> jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-26 1:46 p.m., Vlad Buslov wrote:
> 
> On Mon 26 Oct 2020 at 19:12, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2020-10-26 7:28 a.m., Vlad Buslov wrote:
>>>
>>> On Sat 24 Oct 2020 at 20:40, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>> [..]
>>>>>
>>>>> Yes, that makes sense. I guess introducing something like 'tc action -br
>>>>> ls ..' mode implemented by means of existing terse flag + new 'also
>>>>> output action index' flag would achieve that goal.
>>>>>
>>>>
>>>> Right. There should be no interest in the cookie here at all. Maybe
>>>> it could be optional with a flag indication.
>>>> Have time to cook a patch? I'll taste/test it.
>>>
>>> Patch to make cookie in filter terse dump optional? That would break
>>> existing terse dump users that rely on it (OVS).
>>
>> Meant patch for 'tc action -br ls'
>>
>> Which by default would not include the cookie.
> 
> So action-dump-specific flag that causes act api to output action index
> (via new attribute) and skips cookie?
> 

yeah, something like TCA_ACT_FLAGS_TERSE.

new tcf_action_dump_terse() takes one more field which says to
include or not the cookies since that is shared code and filters
can always include it.
The action index is already present in the passed tc_action
struct just needs a new TLV.


cheers,
jamal

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov]

On Mon 26 Oct 2020 at 20:01, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-26 1:46 p.m., Vlad Buslov wrote:
>>
>> On Mon 26 Oct 2020 at 19:12, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>> On 2020-10-26 7:28 a.m., Vlad Buslov wrote:
>>>>
>>>> On Sat 24 Oct 2020 at 20:40, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>>
>>> [..]
>>>>>>
>>>>>> Yes, that makes sense. I guess introducing something like 'tc action -br
>>>>>> ls ..' mode implemented by means of existing terse flag + new 'also
>>>>>> output action index' flag would achieve that goal.
>>>>>>
>>>>>
>>>>> Right. There should be no interest in the cookie here at all. Maybe
>>>>> it could be optional with a flag indication.
>>>>> Have time to cook a patch? I'll taste/test it.
>>>>
>>>> Patch to make cookie in filter terse dump optional? That would break
>>>> existing terse dump users that rely on it (OVS).
>>>
>>> Meant patch for 'tc action -br ls'
>>>
>>> Which by default would not include the cookie.
>>
>> So action-dump-specific flag that causes act api to output action index
>> (via new attribute) and skips cookie?
>>
>
> yeah, something like TCA_ACT_FLAGS_TERSE.
>
> new tcf_action_dump_terse() takes one more field which says to
> include or not the cookies since that is shared code and filters
> can always include it.
> The action index is already present in the passed tc_action
> struct just needs a new TLV.

Sure, I'll try to find time this week.

Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Jamal Hadi Salim]

On 2020-10-26 2:03 p.m., Vlad Buslov wrote:
> 
> On Mon 26 Oct 2020 at 20:01, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2020-10-26 1:46 p.m., Vlad Buslov wrote:
>>>

>> yeah, something like TCA_ACT_FLAGS_TERSE.
>>
>> new tcf_action_dump_terse() takes one more field which says to
>> include or not the cookies since that is shared code and filters
>> can always include it.
>> The action index is already present in the passed tc_action
>> struct just needs a new TLV.
> 
> Sure, I'll try to find time this week.


Thank you!

cheers,
jamal