Add units for meter rate to documentation of simple_switch #753

hesingh · 2019-04-11T13:55:13Z

Also, a cosmetic change was made to relevant code to move mutiple lines of code on single line to another line.

codecov-io · 2019-04-11T14:31:41Z

Codecov Report

Merging #753 into master will decrease coverage by 0.15%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #753      +/-   ##
==========================================
- Coverage   74.48%   74.33%   -0.16%     
==========================================
  Files         115      115              
  Lines        9988    10136     +148     
==========================================
+ Hits         7440     7535      +95     
- Misses       2548     2601      +53

Impacted Files	Coverage Δ
include/bm/bm_sim/actions.h	`24.39% <0%> (-4.6%)`	⬇️
src/bm_sim/pcap_file.cpp	`80.23% <0%> (-1.81%)`	⬇️
src/bm_sim/dev_mgr_packet_in.cpp	`80% <0%> (-1.52%)`	⬇️
src/bm_sim/periodic_task.cpp	`90.54% <0%> (-1.25%)`	⬇️
src/bm_sim/P4Objects.cpp	`77.77% <0%> (-1.1%)`	⬇️
src/bm_sim/switch.cpp	`72.91% <0%> (-0.42%)`	⬇️
src/bm_sim/target_parser.cpp	`74.64% <0%> (-0.36%)`	⬇️
src/bm_sim/dev_mgr.cpp	`37.7% <0%> (-0.32%)`	⬇️
include/bm/bm_sim/match_units.h	`86.36% <0%> (-0.31%)`	⬇️
src/bm_sim/packet.cpp	`90.35% <0%> (-0.25%)`	⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99d8311...e870599. Read the comment docs.

hesingh · 2019-04-11T14:53:39Z

Please review this PR - thanks.

jafingerhut · 2019-04-11T16:05:18Z

docs/simple_switch.md

+
+### BMv2 `meter rate units` implementation notes
+
+The user enters the meter rate and burtsize in


It sounds reasonable that the meter rate is in units of bytes per microsec, but the units of "burst size" should be some kind of unit like bytes. It has nothing to do with time. I do not know if simple_switch uses bytes or some other unit like kilobytes.

From the names of variables and some of the math done to convert units, it appears like perhaps the burst sizes are specified in multiples of 1000 bits (Kbits would be confusing to many people, I think, as it would often be interpreted as multiples of 1024 bits).

Also, these are likely the units used when a meter has meter type equal to MeterType.bytes in v1model.

Does BMv2 support different behavior for meters that have a type equal to MeterType.packets ?

If so, what are the units of the meter rates and burst sizes for such meters?

It sounds reasonable that the meter rate is in units of bytes per microsec, but the units of "burst size" should be some kind of unit like bytes. It has nothing to do with time. I do not know if simple_switch uses bytes or some other unit like kilobytes.

Sorry, I missed the burst_size in bytes. I will change the doc.

From the names of variables and some of the math done to convert units, it appears like perhaps the burst sizes are specified in multiples of 1000 bits (Kbits would be confusing to many people, I think, as it would often be interpreted as multiples of 1024 bits).

Makes sense to use number of kilo octets.

Also, these are likely the units used when a meter has meter type equal to MeterType.bytes in v1model.

Does BMv2 support different behavior for meters that have a type equal to MeterType.packets ?

If so, what are the units of the meter rates and burst sizes for such meters?

pps and packets.

hesingh · 2019-04-11T17:49:01Z

The internal storage for meter rate by simple_switch is using kbps. Thus, doesn't it make sense to change the documentation to let the user specify rate in kbps? The code takes the rate in kbps and converts to bytes/microsecond.

antoninbas · 2019-04-11T20:00:22Z

pdfixed/include/bm/pdfixed/pd_common.h

@@ -73,7 +73,7 @@ typedef struct p4_pd_packets_meter_spec {

 typedef struct p4_pd_bytes_meter_spec {
  uint32_t cir_kbps;
-  uint32_t cburst_kbits;


we are not changing the interface, unless there is a bug that needs to be fixed

There is no bug in the code. However, one reviewer has already made a comment saying "burst sizes are specified in multiples of 1000 bits (Kbits would be confusing to many people, I think, as it would often be interpreted as multiples of 1024 bits)." To address the comment, I decided to use kilo octets for burst_size. On second thought, I think "kilo" is still confusing because folks will ask if it is 1024 or 1000?

Why not use octets for burst_size?

A size in several specifications uses octets. See RFC 8200 for IPv6 Payload length which uses octets.

If the code comments and conversions are correct, it is not kilo octets. It is 10^3 bits. Or you could say it is in multiples of 125 bytes/octets.

i know the code conversion is correct. But I changed the code to use kilo octets. But kilo is ambiguous. So, we could use 10^3 octets.

Or 10^3 bytes would work too. Then the original code conversion remains unchanged.

hesingh · 2019-04-11T22:50:17Z

Please see if latest changes work for reviewers - thanks.

antoninbas

I don't see the value of this PR. The simple_switch.md documentation is supposed to help people write P4 programs for v1model.p4 and potentially write control-plane applications for them. But there are many tools to set meter rates & burst sizes, and the change does not provide any context.

antoninbas · 2019-04-11T23:45:22Z

docs/simple_switch.md

@@ -685,3 +685,12 @@ you have two actions for table `t` where one has the action
 `m.read(result_field2)`, or if both of those calls are in the same
 action.  All calls to the `read()` method for `m` must have the same
 result parameter where the result is written.
+
+### BMv2 `meter rate units` implementation notes


what is this documentation for? The runtime CLI? The Thrift service? The PD API?

simple_switch_CLI

that's not accurate then
IIRC, the burst size is in "units" (packets or bytes) and the rate is in "units per microsecond" (https://github.com/p4lang/behavioral-model/blob/master/thrift_src/standard.thrift#L88)

Wow, even I quoted the same line of code in the Slack channel today. But, seeing the code in pd_helpers.cpp, I saw the conversion from kbps and kbits. This cpp code also happens to be the only code in the behavioral-model repo converting rate and burst_size and thus I assumed the code is used with simpe_switch_CLI as well.

that's not accurate then
IIRC, the burst size is in "units" (packets or bytes) and the rate is in "units per microsecond" (https://github.com/p4lang/behavioral-model/blob/master/thrift_src/standard.thrift#L88)

There is some confusion. Based on your reply for meter type as packet, its "units in microsecond" and this is the relevant code in pd_helpers.cpp. I see the division by 10^6 for microseconds.

info_rate = static_cast<double>(meter_spec->cir_pps) / 1000000.;

Isn't the user input captured by simple_switch_CLI in meter_spec->cir_pps and thus the user inputs in pps units? Or is the code in pd_helpers.cpp not used at all by simple_switch_CLI?

The PD layer is not used at all by the CLI. If you look at the CLI code, you will see that it calls the Thrift service directly.
It is fine to have this information in simple_switch.md BTW, but it would be far more useful to improve the runtime_CLI help message for the meter_set_rates CLI command, so that users can get this information directly from the CLI a they are using it.

Ok, I will edit the simple_switch.md doc for CLI rates and burst_size. Even, I was thinking of adding the help message for the meter_set_rates. I will add that. Thanks!

antoninbas · 2019-04-11T23:45:47Z

pdfixed/src/pd_helpers.cpp

@@ -33,13 +33,16 @@ pd_bytes_meter_spec_to_rates(p4_pd_bytes_meter_spec_t *meter_spec) {

  // bytes per microsecond
  info_rate = static_cast<double>(meter_spec->cir_kbps) / 8000.;
+  // 10^6 bytes


that's incorrect
it's converting it to bytes, not Mbytes

Oops, you are correct.

hesingh · 2019-04-11T23:57:40Z

I don't see the value of this PR. The simple_switch.md documentation is supposed to help people write P4 programs for v1model.p4 and potentially write control-plane applications for them. But there are many tools to set meter rates & burst sizes, and the change does not provide any context.

In the P4 Slack general channel today, two users asked the units for meter rate and burst_size because they weren't sure. So, it was asked to document, at least, what the simple_switch_CLI uses. The code changes are cosmetic. I updated the PR.

hesingh · 2019-04-12T19:33:09Z

Unit test

RuntimeCmd: help meter_set_rates
Configure rates for a meter: meter_set_rates <name> <index> <rate_1>:<burst_1> <rate_2>:<burst_2> ...
Rate uses units/microsecond and burst uses units where units is bytes or packets
RuntimeCmd:

hesingh · 2019-04-12T20:22:34Z

All checks have passed. Please review final diffs.

hesingh · 2019-04-16T22:07:09Z

@antoninbas I restored the pdfixed file to its original form. Now the diffs have only two files changed. Please review - thanks.

hesingh · 2019-04-17T13:52:04Z

Any other reviewers to help review and check in this PR? @antoninbas seems busy. The PR has only changed one .md file for documentation and added help for one CLI in Python code.

antoninbas · 2019-04-17T17:44:03Z

docs/simple_switch.md

@@ -685,3 +685,12 @@ you have two actions for table `t` where one has the action
 `m.read(result_field2)`, or if both of those calls are in the same
 action.  All calls to the `read()` method for `m` must have the same
 result parameter where the result is written.
+
+### BMv2 `meter rate and burst_size units` implementation notes.


Why is this between backticks?
why do you think this qualifies as "implementation notes"?

I removed the backticks since the exact names of the CLI args are not used. Also removed "implemented notes"

antoninbas · 2019-04-17T17:44:54Z

docs/simple_switch.md

@@ -685,3 +685,12 @@ you have two actions for table `t` where one has the action
 `m.read(result_field2)`, or if both of those calls are in the same
 action.  All calls to the `read()` method for `m` must have the same
 result parameter where the result is written.
+
+### BMv2 `meter rate and burst_size units` implementation notes.
+### Units help configuring meter with simple_switch_CLI


This is not specific to simple_switch so I don't understand why we are putting this in this document.
Why not start a separate runtime_CLI documentation if this is your intent?

On 04/10, in the P4 Slack channel, a note said to add the info to simple_switch.md. The note is copied below.

"If you learn the answer, it would be good to document the answer in this file: https://github.com/p4lang/behavioral-model/blob/master/docs/simple_switch.md"

After all, p4runtime_CLI does not run unless simple_switch is running first.

Anyway, your point makes sense. Let me add a new file and move the documentation there. thanks.

After all, p4runtime_CLI does not run unless simple_switch is running first.

That is not accurate. p4runtime_CLI works with any bmv2 target. It does not have to be simple_switch.

I see - thanks. Sorry, I haven't used other bmv2 targets and have used p4runtime_CLI only with simple_switch. If I do not launch the simple_switch, I see this error.

hemant@ubuntu:~/behavioral-model/tools$ ./runtime_CLI.py Could not connect to thrift client on port 9090 Make sure the switch is running and that you have the right port hemant@ubuntu:~/behavioral-model/tools$

hesingh · 2019-04-18T23:05:19Z

I don't understand the codecov/project failure. I think, it doesn't look like a blocking issue to merge the changes.

antoninbas · 2019-04-19T01:59:47Z

docs/runtime_CLI.md

+The user enters the meter rate in bytes/microsecond and burst_size in
+bytes.
+
+If the meter type is packets, the rate is enterted in packets/microsecond and


typo s/enterted/entered

antoninbas · 2019-04-19T02:01:43Z

docs/runtime_CLI.md

@@ -0,0 +1,7 @@
+### BMv2 meter rate and burst_size units


I think if we are going to start a CLI documentation we should do it right and organize it by command. For the meter_set_rates command, more information may be useful, and in particular a pointer to RFC 2698.

antoninbas · 2019-04-19T02:02:10Z

docs/runtime_CLI.md

+bytes.
+
+If the meter type is packets, the rate is enterted in packets/microsecond and
+burst_size is number of packets.


is the number of packets

hesingh · 2019-04-23T22:29:35Z

@antoninbas Please review the last changes and if they look good, please merge this code - thanks.

antoninbas

Have you taken a look at the rendered file on Github?

antoninbas · 2019-04-24T16:25:20Z

docs/runtime_CLI.md

+### meter_set_rates
+Meter behavior is specified in RFC 2698.
+
+The user enters the meter rate in bytes/microsecond and burst_size in


why not give the command syntax here as well?

hesingh · 2019-04-24T17:12:48Z

Have you taken a look at the rendered file on Github?

Wow, did not. Good catch! I checked in code to fix rendering. Syntax of CLI was added by a prior checkin. Code is ready for review.

hesingh · 2019-04-24T19:00:50Z

@antoninbas Travis passed with latest changes(add syntax and fix rendering) and only codecov has failed.

hesingh added 2 commits April 11, 2019 09:53

Add units for meter rate

37a407d

Edited text because meter rate is entered by CLI

84aa5eb

Forgot to change two more lines

3bf1a4e

jafingerhut reviewed Apr 11, 2019

View reviewed changes

hesingh added 2 commits April 11, 2019 13:32

Fix burst_size doc and code

a906886

fix typo

69c20d3

Change rate doc

cc3f725

antoninbas reviewed Apr 11, 2019

View reviewed changes

Revet octets change; update doc

96076bc

antoninbas requested changes Apr 11, 2019

View reviewed changes

Fix/add comments, change doc

a7dbf6e

hesingh added 2 commits April 11, 2019 20:07

Add context to doc

7b4f242

Edited doc; reverted pdchanges, added help to cli;

252cbd1

Merge branch 'master' into master

56f8dca

Restore pdfixed code

0367e3d

antoninbas reviewed Apr 17, 2019

View reviewed changes

Address review comments

048b2ef

antoninbas reviewed Apr 19, 2019

View reviewed changes

Address review comments

06967ec

antoninbas reviewed Apr 24, 2019

View reviewed changes

hesingh added 2 commits April 24, 2019 13:07

Add CLI syntax and test one change to fix rendering on github

8a114ea

Try to fix rendering on github

e870599

antoninbas approved these changes Apr 25, 2019

View reviewed changes

antoninbas merged commit 9020b72 into p4lang:master Apr 25, 2019


		### BMv2 `meter rate units` implementation notes

		The user enters the meter rate and burtsize in

Add units for meter rate to documentation of simple_switch #753

Add units for meter rate to documentation of simple_switch #753

Conversation

hesingh commented Apr 11, 2019

codecov-io commented Apr 11, 2019 • edited Loading

Codecov Report

hesingh commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hesingh commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hesingh commented Apr 11, 2019

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hesingh commented Apr 11, 2019

hesingh commented Apr 12, 2019

hesingh commented Apr 12, 2019

hesingh commented Apr 16, 2019

hesingh commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hesingh commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hesingh commented Apr 23, 2019

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hesingh commented Apr 24, 2019

hesingh commented Apr 24, 2019

codecov-io commented Apr 11, 2019 •

edited

Loading