diff mbox series

[iproute2-next,v2,2/2] tc: implement support for terse dump

Message ID 20200930165924.16404-3-vladbu@nvidia.com
State Superseded
Headers show
Series Implement filter terse dump mode support | expand

Commit Message

Vlad Buslov Sept. 30, 2020, 4:59 p.m. UTC 
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 -br filter show dev ens1f0 ingress

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>
---

Notes:
    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(-)

Comments

David Ahern Oct. 8, 2020, 1:40 a.m. UTC | #1 
On 9/30/20 9:59 AM, 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 -br filter show dev ens1f0 ingress
> 
> 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.
> 

I really want to get agreement from other heavy tc users about what the
right information is for a brief mode.
Jamal Hadi Salim Oct. 8, 2020, 12:58 p.m. UTC | #2 
On 2020-10-07 9:40 p.m., David Ahern wrote:
> On 9/30/20 9:59 AM, 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 -br filter show dev ens1f0 ingress
>>
>> 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.
>>
> 
> I really want to get agreement from other heavy tc users about what the
> right information is for a brief mode.

Vlad, would have been helpful in your commit log to show both
terse vs no terse (or at least the terse output). Cant tell short
of patching and testing. Having said that:
The differentiation via TCA_DUMP_FLAGS_TERSE in the request
is in my opinion sufficient to accept the patch.
Also, assuming you have tested with outstanding tc tests for the
first patch i think it looks reasoan

cheers,
jamal
Vlad Buslov Oct. 8, 2020, 3:34 p.m. UTC | #3 
On Thu 08 Oct 2020 at 15:58, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-07 9:40 p.m., David Ahern wrote:
>> On 9/30/20 9:59 AM, 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 -br filter show dev ens1f0 ingress
>>>
>>> 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.
>>>
>>
>> I really want to get agreement from other heavy tc users about what the
>> right information is for a brief mode.
>
> Vlad, would have been helpful in your commit log to show both
> terse vs no terse (or at least the terse output). Cant tell short
> of patching and testing. Having said that:
> The differentiation via TCA_DUMP_FLAGS_TERSE in the request
> is in my opinion sufficient to accept the patch.
> Also, assuming you have tested with outstanding tc tests for the
> first patch i think it looks reasoan

Hi Jamal,

The existing terse dump tdc tests will have to be changed according with
new iproute2 tc syntax for brief(terse) output.

Regards,
Vlad
Jamal Hadi Salim Oct. 9, 2020, 12:03 p.m. UTC | #4 
On 2020-10-08 11:34 a.m., Vlad Buslov wrote:
> 
> On Thu 08 Oct 2020 at 15:58, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> 
> Hi Jamal,
> 
> The existing terse dump tdc tests will have to be changed according with
> new iproute2 tc syntax for brief(terse) output.
>

Which test(s)?
I see for example d45e mentioning terse but i dont see corresponding
code in the iproute2 tree i just pulled.

I feel like changing the tests this early may not be a big issue
if they havent propagated in the wild.

cheers,
jamal
Vlad Buslov Oct. 9, 2020, 12:15 p.m. UTC | #5 
On Fri 09 Oct 2020 at 15:03, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-08 11:34 a.m., Vlad Buslov wrote:
>>
>> On Thu 08 Oct 2020 at 15:58, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>>
>> Hi Jamal,
>>
>> The existing terse dump tdc tests will have to be changed according with
>> new iproute2 tc syntax for brief(terse) output.
>>
>
> Which test(s)?
> I see for example d45e mentioning terse but i dont see corresponding
> code in the iproute2 tree i just pulled.

Yes. The tests d45e and 7c65 were added as a part of kernel series, but
corresponding iproute2 patches were never merged. Tests expect original
"terse flag" syntax of V1 iproute2 series and will have to be changed to
use -brief option instead.

>
> I feel like changing the tests this early may not be a big issue
> if they havent propagated in the wild.
>
> cheers,
> jamal
Jamal Hadi Salim Oct. 9, 2020, 12:45 p.m. UTC | #6 
On 2020-10-09 8:15 a.m., Vlad Buslov wrote:
> 
> On Fri 09 Oct 2020 at 15:03, Jamal Hadi Salim <jhs@mojatatu.com> wrote:

>> Which test(s)?
>> I see for example d45e mentioning terse but i dont see corresponding
>> code in the iproute2 tree i just pulled.
> 
> Yes. The tests d45e and 7c65 were added as a part of kernel series, but
> corresponding iproute2 patches were never merged. Tests expect original
> "terse flag" syntax of V1 iproute2 series and will have to be changed to
> use -brief option instead.

Then i dont see a problem in changing the tests.
If you are going to send a v3 please include my acked-by.
Would have been nice to see what terse output would have looked like.

cheers,
jamal
Vlad Buslov Oct. 9, 2020, 4:38 p.m. UTC | #7 
On Fri 09 Oct 2020 at 15:45, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-09 8:15 a.m., Vlad Buslov wrote:
>>
>> On Fri 09 Oct 2020 at 15:03, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
>>> Which test(s)?
>>> I see for example d45e mentioning terse but i dont see corresponding
>>> code in the iproute2 tree i just pulled.
>>
>> Yes. The tests d45e and 7c65 were added as a part of kernel series, but
>> corresponding iproute2 patches were never merged. Tests expect original
>> "terse flag" syntax of V1 iproute2 series and will have to be changed to
>> use -brief option instead.
>
> Then i dont see a problem in changing the tests.
> If you are going to send a v3 please include my acked-by.
> Would have been nice to see what terse output would have looked like.
>
> cheers,
> jamal

Sure. Just waiting for everyone to voice their opinion regarding the
output format before proceeding with any changes.
David Ahern Oct. 13, 2020, 2:05 a.m. UTC | #8 
On 10/9/20 9:38 AM, Vlad Buslov wrote:
> 
> On Fri 09 Oct 2020 at 15:45, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>> On 2020-10-09 8:15 a.m., Vlad Buslov wrote:
>>>
>>> On Fri 09 Oct 2020 at 15:03, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>>>> Which test(s)?
>>>> I see for example d45e mentioning terse but i dont see corresponding
>>>> code in the iproute2 tree i just pulled.
>>>
>>> Yes. The tests d45e and 7c65 were added as a part of kernel series, but
>>> corresponding iproute2 patches were never merged. Tests expect original
>>> "terse flag" syntax of V1 iproute2 series and will have to be changed to
>>> use -brief option instead.
>>
>> Then i dont see a problem in changing the tests.
>> If you are going to send a v3 please include my acked-by.
>> Would have been nice to see what terse output would have looked like.
>>
>> cheers,
>> jamal
> 
> Sure. Just waiting for everyone to voice their opinion regarding the
> output format before proceeding with any changes.
> 

go ahead and send a v3 with example output. cc Jamal and Cong
diff mbox series

Patch

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;