Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buffer Occupancy #55

Merged
merged 20 commits into from Dec 8, 2018

Conversation

@rsivakolundu
Copy link
Contributor

commented Oct 2, 2018

Added instruction bit for buffer occupancy

rsivakolundu added 10 commits Jun 21, 2018
different number of INT hops.

* The fields in the INT metadata header are interpreted the following way:
- Ver (4b): INT metadata header version. Should be 1 for this version.

This comment has been minimized.

Copy link
@mhira1

mhira1 Oct 3, 2018

Contributor

Version must be 2 in this paragraph

* Buffer occupancy
- The build-up of traffic in the shared buffer (in bytes, cells, or packets) that the
INT packet observes in the device while being forwarded.

This comment has been minimized.

Copy link
@mhira1

mhira1 Oct 3, 2018

Contributor

Perhaps remove the word "shared". Reporting both queue occupancy and buffer occupancy makes sense only if the buffer is shared, but technically the buffer could be a per-queue buffer, just that in this case, both values reported will be the same. INT source does not know whether downstream switches have shared buffers or not. It can set both bits. Switches that have per-queue buffers (unlikely, switches typically use shared buffers) will just report the same value for both instructions.

This comment has been minimized.

Copy link
@mhira1

mhira1 Oct 3, 2018

Contributor

May be word this differently, to say this instruction is to get buffer occupancy. Use-case is when buffer is shared.

This comment has been minimized.

Copy link
@rsivakolundu

rsivakolundu Oct 3, 2018

Author Contributor

Removed the reference to shared.

This comment has been minimized.

Copy link
@mickeyspiegel

mickeyspiegel Oct 4, 2018

Contributor

Minor editorial rewording:
The use case is when the buffer is shared between multiple queues.

This comment has been minimized.

Copy link
@rsivakolundu

rsivakolundu Oct 4, 2018

Author Contributor

Added the minor editorial change suggested.

@mhira1
Copy link
Contributor

left a comment

Minor change needed.

switch(es) set the M bit based on knowledge of the network topology
and "Switch ID, Ingress port ID, Egress port ID" tuples in the INT
metadata stack.
- R (9b): Reserved bits.

This comment has been minimized.

Copy link
@mhira1

mhira1 Oct 3, 2018

Contributor

Number of reserved bits is still 10 correct? I think this text with 9 bits is coming from your other patch in which you are using one of the reserved bits for "Sink" bit. There is no change in number of reserved bits between v1 and v2 in this patch.

@mhira1 mhira1 requested review from mickeyspiegel, mhira1 and jklr Oct 3, 2018

@@ -910,6 +915,170 @@ from (shim header length \* 4).
For INT over Geneve it is 8 bytes subtracted from (length in Geneve tunnel
option header \* 4).


INT Metadata Header and Metadata Stack (Version = 2):

This comment has been minimized.

Copy link
@jklr

jklr Oct 3, 2018

Contributor

@mhira1 I was meaning to propose make a 1.x version cut before merging these changes, and move INT.mdk to version2 draft. Then we don't need to list both v1 and v2 in the same doc. How do you think?

This comment has been minimized.

Copy link
@mhira1

mhira1 Oct 3, 2018

Contributor

Makes sense. Let's remove this second paragraph on v2 header then.

@rsivakolundu

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2018

@jklr

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2018

@rsivakolundu hold on, pls. This buffer occupancy change is NOT backward compatible, right? Hence it should be added only to v2. Let's discuss version maintenance policy tomorrow and then make further changes on the PRs. I don't want you to waste time during PTO.

* Buffer occupancy
- The build-up of traffic in the shared buffer (in bytes, cells, or packets) that the
INT packet observes in the device while being forwarded.

This comment has been minimized.

Copy link
@mickeyspiegel

mickeyspiegel Oct 4, 2018

Contributor

Minor editorial rewording:
The use case is when the buffer is shared between multiple queues.

@@ -1279,6 +1289,7 @@ struct headers {
int_egress_tstamp_t int_egress_tstamp;
int_level2_port_ids_t int_level2_port_ids;
int_egress_port_tx_util_t int_egress_port_tx_util;
int_b_occupancy_t int_b_occupancy;

This comment has been minimized.

Copy link
@mickeyspiegel

mickeyspiegel Oct 4, 2018

Contributor

nit: fix indent

This comment has been minimized.

Copy link
@rsivakolundu

rsivakolundu Oct 4, 2018

Author Contributor

Done.

@@ -1610,6 +1621,11 @@ control EgressDeparserImpl(packet_out packet,
ck.add({hdr.int_egress_port_tx_util.egress_port_tx_util});
}

if (hdr.int_b_occupancy.isValid()) {

This comment has been minimized.

Copy link
@mickeyspiegel

mickeyspiegel Oct 4, 2018

Contributor

nit: fix indent

This comment has been minimized.

Copy link
@rsivakolundu

rsivakolundu Dec 6, 2018

Author Contributor

Fixed

@@ -1656,6 +1672,7 @@ control EgressDeparserImpl(packet_out packet,
packet.emit(hdr.int_egress_tstamp);
packet.emit(hdr.int_level2_port_ids);
packet.emit(hdr.int_egress_port_tx_util);
packet.emit(hdr.int_b_occupancy);

This comment has been minimized.

Copy link
@mickeyspiegel

mickeyspiegel Oct 4, 2018

Contributor

nit: fix indent

This comment has been minimized.

Copy link
@rsivakolundu

rsivakolundu Oct 4, 2018

Author Contributor

Done.

@rsivakolundu rsivakolundu changed the title Shared buffer Buffer Occupancy Oct 4, 2018

rsivakolundu added 3 commits Oct 4, 2018
Update INT.mdk
Fixed minor editorial comment from Mickey
@mhira1
Copy link
Contributor

left a comment

Ramesh, I looked through the edits again, marked some typos. Also, at the previous meeting, we decided to include both buffer ID and buffer occupancy into the 32-bit space. Please make that change as well.

telemetry/specs/INT.mdk Show resolved Hide resolved
- The build-up of traffic in the buffer (in bytes, cells, or packets) that the
INT packet observes in the device while being forwarded. Use case is when buffer is
shared between multiple queue.
* Instantaneous buffer occupance

This comment has been minimized.

Copy link
@mhira1

mhira1 Oct 16, 2018

Contributor

typo: occupance -> occupancy

This comment has been minimized.

Copy link
@rsivakolundu

rsivakolundu Dec 6, 2018

Author Contributor

Fixed

telemetry/specs/INT.mdk Show resolved Hide resolved
Update INT.mdk
Added some text to queue occupancy and buffer occupancy description about how the YANG model shall define the format and units of these fields to enable switch implementation specific details.
@jklr
jklr approved these changes Dec 4, 2018
Copy link
Contributor

left a comment

@rsivakolundu thanks for the PR. I suggested several editorial changes. Please check the last two nit comments from Mukesh.

telemetry/specs/INT.mdk Outdated Show resolved Hide resolved
telemetry/specs/INT.mdk Outdated Show resolved Hide resolved
telemetry/specs/INT.mdk Outdated Show resolved Hide resolved
@@ -1219,6 +1229,10 @@ header int_egress_port_tx_util_t {
bit<32> egress_port_tx_util;
}

header int_b_occupancy_t {
bit<32> q_occupancy;

This comment has been minimized.

Copy link
@jklr

jklr Dec 4, 2018

Contributor

Please make it consistent with the (example) layout provided in line 866, which has 8b buffer id.

This comment has been minimized.

Copy link
@rsivakolundu

rsivakolundu Dec 4, 2018

Author Contributor

Let us discuss the semantics of Queue and Buffer occupancy. I will make it consistent after the discussion.

This comment has been minimized.

Copy link
@rsivakolundu

rsivakolundu Dec 6, 2018

Author Contributor

Added Buffer ID (8 bits) and Buffer Occupancy (24 bits)

telemetry/specs/INT.mdk Show resolved Hide resolved
telemetry/specs/INT.mdk Outdated Show resolved Hide resolved
Update INT.mdk
Addressed some of the comments and changes proposed. Semantics of the queue and buffer occupancy still need to be addressed.
rsivakolundu added 2 commits Dec 6, 2018
Update INT.mdk
Changes to address all review comments.
Update INT.mdk
Corrected a line spacing issue
@jklr

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

@rsivakolundu your latest commit merged domain specific extension to this branch/PR!

@rsivakolundu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

@jklr

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

@rsivakolundu hmm I think there is a github bug. The bottom most commit in this PR's commit tab showed domain specific changes 2 hours ago. I double checked at that time.
It's not like that any more.

@jk

This comment has been minimized.

Copy link

commented Dec 7, 2018

@jklr jklr merged commit 1ce57dd into p4lang:master Dec 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.