| Message ID | 20210607064408.1668142-1-roid@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next,v4,1/1] police: Add support for json output | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 6/7/21 12:44 AM, Roi Dayan wrote: > Change to use the print wrappers instead of fprintf(). > > This is example output of the options part before this commit: > > "options": { > "handle": 1, > "in_hw": true, > "actions": [ { > "order": 1 police 0x2 , > "control_action": { > "type": "drop" > }, > "control_action": { > "type": "continue" > }overhead 0b linklayer unspec > ref 1 bind 1 > , > "used_hw_stats": [ "delayed" ] > } ] > } > > This is the output of the same dump with this commit: > > "options": { > "handle": 1, > "in_hw": true, > "actions": [ { > "order": 1, > "kind": "police", > "index": 2, > "control_action": { > "type": "drop" > }, > "control_action": { > "type": "continue" > }, > "overhead": 0, > "linklayer": "unspec", > "ref": 1, > "bind": 1, > "used_hw_stats": [ "delayed" ] > } ] > } > > Signed-off-by: Roi Dayan <roid@nvidia.com> > Reviewed-by: Paul Blakey <paulb@nvidia.com> > --- > > Notes: > v2 > - fix json output to match correctly the other actions > i.e. output the action name in key 'kind' and unsigned for the index > > v3 > - print errors to stderr. > - return -1 on null key. > > v4 > - removed left over debug that was forgotten. sorry for that. > > tc/m_police.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > applied to iproute2-next
On Mon, Jun 07, 2021 at 09:44:08AM +0300, Roi Dayan wrote: > Change to use the print wrappers instead of fprintf(). > > This is example output of the options part before this commit: > > "options": { > "handle": 1, > "in_hw": true, > "actions": [ { > "order": 1 police 0x2 , > "control_action": { > "type": "drop" > }, > "control_action": { > "type": "continue" > }overhead 0b linklayer unspec > ref 1 bind 1 > , > "used_hw_stats": [ "delayed" ] > } ] > } > > This is the output of the same dump with this commit: > > "options": { > "handle": 1, > "in_hw": true, > "actions": [ { > "order": 1, > "kind": "police", > "index": 2, > "control_action": { > "type": "drop" > }, > "control_action": { > "type": "continue" > }, > "overhead": 0, > "linklayer": "unspec", > "ref": 1, > "bind": 1, > "used_hw_stats": [ "delayed" ] > } ] > } > > Signed-off-by: Roi Dayan <roid@nvidia.com> > Reviewed-by: Paul Blakey <paulb@nvidia.com> > --- [...] > > @@ -300,13 +301,13 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg) > RTA_PAYLOAD(tb[TCA_POLICE_RATE64]) >= sizeof(rate64)) > rate64 = rta_getattr_u64(tb[TCA_POLICE_RATE64]); > > - fprintf(f, " police 0x%x ", p->index); > + print_uint(PRINT_ANY, "index", "\t index %u ", p->index); Hi everyone, This update break all policy checking in kernel tc selftest actions/police.json. As the new output would like total acts 1 action order 0: police index 1 rate 1Kbit burst 10Kb mtu 2Kb action reclassify overhead 0 ref 1 bind 0 And the current test checks like "matchPattern": "action order [0-9]*: police 0x1 rate 1Kbit burst 10Kb" I plan to update the kselftest to mach the new output. But I have a question. Why need we add a "\t" before index output? Is it needed or could be removed? Thanks Hangbin
On Mon, 7 Jun 2021 09:44:08 +0300 Roi Dayan <roid@nvidia.com> wrote: > - fprintf(f, " police 0x%x ", p->index); > + print_uint(PRINT_ANY, "index", "\t index %u ", p->index); Why did output format have to change here? Why not: print_hex(PRINT_ANY, "police", " police %#x", p->index);
On Mon, 7 Jun 2021 09:44:08 +0300 Roi Dayan <roid@nvidia.com> wrote: > - fprintf(f, "\n\tref %d bind %d", p->refcnt, p->bindcnt); > + print_string(PRINT_ANY, "linklayer", "linklayer %s ", > + sprint_linklayer(linklayer, b2)); > + print_int(PRINT_ANY, "ref", "ref %d ", p->refcnt); > + print_int(PRINT_ANY, "bind", "bind %d ", p->bindcnt); > if (show_stats) { This should add newline like original by using print_nl()
On 7/5/21 10:28 AM, Stephen Hemminger wrote: > On Mon, 7 Jun 2021 09:44:08 +0300 > Roi Dayan <roid@nvidia.com> wrote: > >> - fprintf(f, " police 0x%x ", p->index); >> + print_uint(PRINT_ANY, "index", "\t index %u ", p->index); > > Why did output format have to change here? Why not: > > print_hex(PRINT_ANY, "police", " police %#x", p->index); > it should not have. I caught it in the first version in a review comment; missed it in v4 that was applied.
On Mon, Jul 05, 2021 at 06:41:37PM +0800, Hangbin Liu wrote: > On Mon, Jun 07, 2021 at 09:44:08AM +0300, Roi Dayan wrote: > > Change to use the print wrappers instead of fprintf(). > > > > Signed-off-by: Roi Dayan <roid@nvidia.com> > > Reviewed-by: Paul Blakey <paulb@nvidia.com> > > --- hello Hangbin, [...] > > > > @@ -300,13 +301,13 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg) > > RTA_PAYLOAD(tb[TCA_POLICE_RATE64]) >= sizeof(rate64)) > > rate64 = rta_getattr_u64(tb[TCA_POLICE_RATE64]); > > > > - fprintf(f, " police 0x%x ", p->index); > > + print_uint(PRINT_ANY, "index", "\t index %u ", p->index); > > Hi everyone, > > This update break all policy checking in kernel tc selftest actions/police.json. > As the new output would like thanks for catching this! > total acts 1 > > action order 0: police index 1 rate 1Kbit burst 10Kb mtu 2Kb action reclassify overhead 0 ref 1 bind 0 > > > And the current test checks like > > "matchPattern": "action order [0-9]*: police 0x1 rate 1Kbit burst 10Kb" > > I plan to update the kselftest to mach the new output. my 2 cents: what about using PRINT_FP / PRINT_JSON, so we fix the JSON output only to show "index", and preserve the human-readable printout iproute and kselftests? besides avoiding failures because of mismatching kselftests / iproute, this would preserve functionality of scripts that configure / dump the "police" action. WDYT? thanks,
On 2021-07-05 7:30 PM, David Ahern wrote: > On 7/5/21 10:28 AM, Stephen Hemminger wrote: >> On Mon, 7 Jun 2021 09:44:08 +0300 >> Roi Dayan <roid@nvidia.com> wrote: >> >>> - fprintf(f, " police 0x%x ", p->index); >>> + print_uint(PRINT_ANY, "index", "\t index %u ", p->index); >> >> Why did output format have to change here? Why not: >> >> print_hex(PRINT_ANY, "police", " police %#x", p->index); >> > > it should not have. I caught it in the first version in a review > comment; missed it in v4 that was applied. > Hi, I replied to your review in v0 that I wanted to match all the other actions output which output as unsigned. Since I didn't get another reply I thought its ok to continue and sent another version which other changes that were required.
On 2021-07-06 11:30 AM, Roi Dayan wrote: > > > On 2021-07-05 7:30 PM, David Ahern wrote: >> On 7/5/21 10:28 AM, Stephen Hemminger wrote: >>> On Mon, 7 Jun 2021 09:44:08 +0300 >>> Roi Dayan <roid@nvidia.com> wrote: >>> >>>> - fprintf(f, " police 0x%x ", p->index); >>>> + print_uint(PRINT_ANY, "index", "\t index %u ", p->index); >>> >>> Why did output format have to change here? Why not: >>> >>> print_hex(PRINT_ANY, "police", " police %#x", p->index); >>> >> >> it should not have. I caught it in the first version in a review >> comment; missed it in v4 that was applied. >> > > Hi, > > I replied to your review in v0 that I wanted to match all the other > actions output which output as unsigned. > Since I didn't get another reply I thought its ok to continue and sent > another version which other changes that were required. > > beside the unsigned the json output in all other actions use "index" as far as I notice. the action name, e.g. "police", being printed under "kind" also to match the other actions. So I wanted the json output for police to match with same keys as the other actions. at least the keys "kind" and "index" which all have and not each action use different key.
On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote: > my 2 cents: > > what about using PRINT_FP / PRINT_JSON, so we fix the JSON output only to show "index", and > preserve the human-readable printout iproute and kselftests? besides avoiding failures because > of mismatching kselftests / iproute, this would preserve functionality of scripts that > configure / dump the "police" action. WDYT? +1
On 2021-07-07 9:53 AM, Hangbin Liu wrote: > On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote: >> my 2 cents: >> >> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output only to show "index", and >> preserve the human-readable printout iproute and kselftests? besides avoiding failures because >> of mismatching kselftests / iproute, this would preserve functionality of scripts that >> configure / dump the "police" action. WDYT? > > +1 > why not fix the kselftest to look for the correct output? all actions output unsigned as the index. though I did find an issue with the fp output that you pasted that I missed. action order 0: police index 1 rate 1Kbit burst 10Kb mtu 2Kb action reclassify overhead 0 ref 1 bind 0 You asked about the \t before index and actually there is a missing print_nl() call before printing index and the rest as in the other actions. then the match should be something like this "matchPattern": "action order [0-9]*: police.*index 0x1 rate 1Kbit burst 10Kb"
On 2021-07-08 9:57 AM, Roi Dayan wrote: > > > On 2021-07-07 9:53 AM, Hangbin Liu wrote: >> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote: >>> my 2 cents: >>> >>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output >>> only to show "index", and >>> preserve the human-readable printout iproute and kselftests? besides >>> avoiding failures because >>> of mismatching kselftests / iproute, this would preserve >>> functionality of scripts that >>> configure / dump the "police" action. WDYT? >> >> +1 >> > > > why not fix the kselftest to look for the correct output? > > all actions output unsigned as the index. > though I did find an issue with the fp output that you pasted > that I missed. > > > action order 0: police index 1 rate 1Kbit burst 10Kb mtu 2Kb > action reclassify overhead 0 ref 1 bind 0 > > > You asked about the \t before index and actually there is a missing > print_nl() call before printing index and the rest as in the other > actions. > > then the match should be something like this > > "matchPattern": "action order [0-9]*: police.*index 0x1 rate 1Kbit > burst 10Kb" actually its "matchPattern": "action order [0-9]*: police.*index 1 rate 1Kbit burst 10Kb" also i found some selftest fail for "overhead" matching as i used print_uint() instead of print_size(). let me send a fix for this with fix for the kselftest of police.json i think its better to have the consistent output of all actions instead of police using hex for the instance number.
On 7/8/21 12:57 AM, Roi Dayan wrote: > > > On 2021-07-07 9:53 AM, Hangbin Liu wrote: >> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote: >>> my 2 cents: >>> >>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output >>> only to show "index", and >>> preserve the human-readable printout iproute and kselftests? besides >>> avoiding failures because >>> of mismatching kselftests / iproute, this would preserve >>> functionality of scripts that >>> configure / dump the "police" action. WDYT? >> >> +1 >> > > > why not fix the kselftest to look for the correct output? That is but 1 user. The general rule is that you do not change the output like you did.
On 2021-07-08 5:46 PM, David Ahern wrote: > On 7/8/21 12:57 AM, Roi Dayan wrote: >> >> >> On 2021-07-07 9:53 AM, Hangbin Liu wrote: >>> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote: >>>> my 2 cents: >>>> >>>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output >>>> only to show "index", and >>>> preserve the human-readable printout iproute and kselftests? besides >>>> avoiding failures because >>>> of mismatching kselftests / iproute, this would preserve >>>> functionality of scripts that >>>> configure / dump the "police" action. WDYT? >>> >>> +1 >>> >> >> >> why not fix the kselftest to look for the correct output? > > That is but 1 user. The general rule is that you do not change the > output like you did. > but the output was "broken" and not consistent with all actions. we are not fixing this kind of thing? so to continue with the suggestion to use print_fp and keep police action output broken and print_json for the json output? just to be sure before submitting change back to old output for fp. ... action order 1: police 0x1 rate 1Mbit burst 20Kb mtu 2Kb action reclassify overhead 0b - print_string(PRINT_ANY, "kind", "%s", "police"); + print_string(PRINT_JSON, "kind", "%s", "police"); - print_uint(PRINT_ANY, "index", "\tindex %u ", p->index); + print_hex(PRINT_FP, NULL, " police 0x%x ", p->index); + print_uint(PRINT_JSON, "index", NULL, p->index);
On 7/11/21 4:24 AM, Roi Dayan wrote: > > > On 2021-07-08 5:46 PM, David Ahern wrote: >> On 7/8/21 12:57 AM, Roi Dayan wrote: >>> >>> >>> On 2021-07-07 9:53 AM, Hangbin Liu wrote: >>>> On Tue, Jul 06, 2021 at 10:27:34AM +0200, Davide Caratti wrote: >>>>> my 2 cents: >>>>> >>>>> what about using PRINT_FP / PRINT_JSON, so we fix the JSON output >>>>> only to show "index", and >>>>> preserve the human-readable printout iproute and kselftests? besides >>>>> avoiding failures because >>>>> of mismatching kselftests / iproute, this would preserve >>>>> functionality of scripts that >>>>> configure / dump the "police" action. WDYT? >>>> >>>> +1 >>>> >>> >>> >>> why not fix the kselftest to look for the correct output? >> >> That is but 1 user. The general rule is that you do not change the >> output like you did. >> > > but the output was "broken" and not consistent with all actions. > we are not fixing this kind of thing? It has been in hex since 2004, and you can not decide in 2021 that it is 'broken' and change it. > so to continue with the suggestion to use print_fp and keep police > action output broken and print_json for the json output? > just to be sure before submitting change back to old output for fp. > > > ... > action order 1: police 0x1 rate 1Mbit burst 20Kb mtu 2Kb action > reclassify overhead 0b > > > > > - print_string(PRINT_ANY, "kind", "%s", "police"); > + print_string(PRINT_JSON, "kind", "%s", "police"); > > - print_uint(PRINT_ANY, "index", "\tindex %u ", p->index); > + print_hex(PRINT_FP, NULL, " police 0x%x ", p->index); > + print_uint(PRINT_JSON, "index", NULL, p->index); > > Jamal: opinions?
On 2021-07-11 12:00 p.m., David Ahern wrote: >> ... >> action order 1: police 0x1 rate 1Mbit burst 20Kb mtu 2Kb action >> reclassify overhead 0b >> >> >> >> >> - print_string(PRINT_ANY, "kind", "%s", "police"); >> + print_string(PRINT_JSON, "kind", "%s", "police"); >> >> - print_uint(PRINT_ANY, "index", "\tindex %u ", p->index); >> + print_hex(PRINT_FP, NULL, " police 0x%x ", p->index); >> + print_uint(PRINT_JSON, "index", NULL, p->index); >> >> > > Jamal: opinions? Looks good to me. Roi please run the kernel tests to make sure nothing breaks. cheers, jamal
On 2021-07-12 2:02 PM, Jamal Hadi Salim wrote: > On 2021-07-11 12:00 p.m., David Ahern wrote: > >>> ... >>> action order 1: police 0x1 rate 1Mbit burst 20Kb mtu 2Kb >>> action >>> reclassify overhead 0b >>> >>> >>> >>> >>> - print_string(PRINT_ANY, "kind", "%s", "police"); >>> + print_string(PRINT_JSON, "kind", "%s", "police"); >>> >>> - print_uint(PRINT_ANY, "index", "\tindex %u ", p->index); >>> + print_hex(PRINT_FP, NULL, " police 0x%x ", p->index); >>> + print_uint(PRINT_JSON, "index", NULL, p->index); >>> >>> >> >> Jamal: opinions? > > Looks good to me. Roi please run the kernel tests to make sure > nothing breaks. > > cheers, > jamal > ok I sent patch titled "police: Fix normal output back to what it was" I ran the tdc tests and passed.
diff --git a/tc/m_police.c b/tc/m_police.c index 9ef0e40b861b..2594c08979e0 100644 --- a/tc/m_police.c +++ b/tc/m_police.c @@ -278,18 +278,19 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg) __u64 rate64, prate64; __u64 pps64, ppsburst64; + print_string(PRINT_ANY, "kind", "%s", "police"); if (arg == NULL) return 0; parse_rtattr_nested(tb, TCA_POLICE_MAX, arg); if (tb[TCA_POLICE_TBF] == NULL) { - fprintf(f, "[NULL police tbf]"); - return 0; + fprintf(stderr, "[NULL police tbf]"); + return -1; } #ifndef STOOPID_8BYTE if (RTA_PAYLOAD(tb[TCA_POLICE_TBF]) < sizeof(*p)) { - fprintf(f, "[truncated police tbf]"); + fprintf(stderr, "[truncated police tbf]"); return -1; } #endif @@ -300,13 +301,13 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg) RTA_PAYLOAD(tb[TCA_POLICE_RATE64]) >= sizeof(rate64)) rate64 = rta_getattr_u64(tb[TCA_POLICE_RATE64]); - fprintf(f, " police 0x%x ", p->index); + print_uint(PRINT_ANY, "index", "\t index %u ", p->index); tc_print_rate(PRINT_FP, NULL, "rate %s ", rate64); buffer = tc_calc_xmitsize(rate64, p->burst); print_size(PRINT_FP, NULL, "burst %s ", buffer); print_size(PRINT_FP, NULL, "mtu %s ", p->mtu); if (show_raw) - fprintf(f, "[%08x] ", p->burst); + print_hex(PRINT_FP, NULL, "[%08x] ", p->burst); prate64 = p->peakrate.rate; if (tb[TCA_POLICE_PEAKRATE64] && @@ -327,8 +328,8 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg) pps64 = rta_getattr_u64(tb[TCA_POLICE_PKTRATE64]); ppsburst64 = rta_getattr_u64(tb[TCA_POLICE_PKTBURST64]); ppsburst64 = tc_calc_xmitsize(pps64, ppsburst64); - fprintf(f, "pkts_rate %llu ", pps64); - fprintf(f, "pkts_burst %llu ", ppsburst64); + print_u64(PRINT_ANY, "pkts_rate", "pkts_rate %llu ", pps64); + print_u64(PRINT_ANY, "pkts_burst", "pkts_burst %llu ", ppsburst64); } print_action_control(f, "action ", p->action, ""); @@ -337,14 +338,17 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg) __u32 action = rta_getattr_u32(tb[TCA_POLICE_RESULT]); print_action_control(f, "/", action, " "); - } else - fprintf(f, " "); + } else { + print_string(PRINT_FP, NULL, " ", NULL); + } - fprintf(f, "overhead %ub ", p->rate.overhead); + print_uint(PRINT_ANY, "overhead", "overhead %u ", p->rate.overhead); linklayer = (p->rate.linklayer & TC_LINKLAYER_MASK); if (linklayer > TC_LINKLAYER_ETHERNET || show_details) - fprintf(f, "linklayer %s ", sprint_linklayer(linklayer, b2)); - fprintf(f, "\n\tref %d bind %d", p->refcnt, p->bindcnt); + print_string(PRINT_ANY, "linklayer", "linklayer %s ", + sprint_linklayer(linklayer, b2)); + print_int(PRINT_ANY, "ref", "ref %d ", p->refcnt); + print_int(PRINT_ANY, "bind", "bind %d ", p->bindcnt); if (show_stats) { if (tb[TCA_POLICE_TM]) { struct tcf_t *tm = RTA_DATA(tb[TCA_POLICE_TM]); @@ -352,7 +356,7 @@ static int print_police(struct action_util *a, FILE *f, struct rtattr *arg) print_tm(f, tm); } } - fprintf(f, "\n"); + print_nl(); return 0;
Change to use the print wrappers instead of fprintf(). This is example output of the options part before this commit: "options": { "handle": 1, "in_hw": true, "actions": [ { "order": 1 police 0x2 , "control_action": { "type": "drop" }, "control_action": { "type": "continue" }overhead 0b linklayer unspec ref 1 bind 1 , "used_hw_stats": [ "delayed" ] } ] } This is the output of the same dump with this commit: "options": { "handle": 1, "in_hw": true, "actions": [ { "order": 1, "kind": "police", "index": 2, "control_action": { "type": "drop" }, "control_action": { "type": "continue" }, "overhead": 0, "linklayer": "unspec", "ref": 1, "bind": 1, "used_hw_stats": [ "delayed" ] } ] } Signed-off-by: Roi Dayan <roid@nvidia.com> Reviewed-by: Paul Blakey <paulb@nvidia.com> --- Notes: v2 - fix json output to match correctly the other actions i.e. output the action name in key 'kind' and unsigned for the index v3 - print errors to stderr. - return -1 on null key. v4 - removed left over debug that was forgotten. sorry for that. tc/m_police.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-)