p4lang / p4-applications

P4 Applications WG repo
107 stars 45 forks source link

Telemetry report v2.0 #61

Closed rsivakolundu closed 4 years ago

rsivakolundu commented 4 years ago

Added changes for v2.0 report.

rsivakolundu commented 4 years ago

Addressed all the changes requested.

rsivakolundu commented 4 years ago

We have 16 bits for DS Instructions for 4-byte wide DS metadata. Along with baseline Instruction bits, we have an upper limit of 1024 bytes for metadata (baseline and DS) and the packet fragment. DS Instruction bits are totally left up to the network management entity to define the kind of metadata requested.

Do you envision more than 1024 bytes in a report per flow?

-Ramesh

On Mon, Apr 13, 2020 at 9:08 AM Randy Levensalor notifications@github.com wrote:

@RandyLevensalor requested changes on this pull request.

Including an option for both packet fragments and Additional DS Extension Data in the same report will make this a more complete solution.

In particular, I could see have a bit of DS Extension Data in a drop report with a packet fragment. The DS extension can provide information on the system regarding the current roles on the system and the packet fragment which was dropped.

In telemetry/specs/telemetry_report.mdk https://github.com/p4lang/p4-applications/pull/61#discussion_r407555527:

   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
  • | Variable Optional Metadata |
  • +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -`
  • | Node id |
  • +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+<-+
  • |RepType| InType| Report Length | MD Length |D|Q|F|I| Rsvd | |
  • +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ R
  • | RepMdBits | Domain Specific ID | E
  • +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ P
  • | DSMdBits | DSMdstatus | O
  • +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ R
  • | Variable Optional Baseline & DS Metadata | T
  • +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ |
  • | Truncated Packet or Additional DS Extension Data | |

This seems to be overly constrained by not allowing for Truncated Packet or Additional DS Extension Data in the same report.

Allowing for both within the same report will enable the full spectrum of data. Plus it will allow an application that only knows about the packet fragment to read that and skip the DS extension data.

This will require that the "IntType" and Report Length would need to be duplicated for Domain Specific Extension Data and the packet fragment.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#pullrequestreview-392239303, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFUR7ADVDL3566BRHO3RMM2JDANCNFSM4JDZMABQ .

RandyLevensalor commented 4 years ago

I don't envision more than 1024 bytes in an individual flow report.

mickeyspiegel commented 4 years ago

@RandyLevensalor Looking at INT 2.0, I don't see any restriction on the length represented by each DS Instruction bit other than a multiple of 4 bytes. If we relax this similarly in Telemetry Report 2.0, where one DS Instruction bit may represent more than 4 bytes of Variable Optional DS Metadata, would that be sufficient? Could you achieve what you want using DS Instruction bits?

RandyLevensalor commented 4 years ago

@mickeyspiegel That would be fine. Then we could remove 1: Domain Specific Extension Data from the InType, since that could be added to the DS metadata.

rsivakolundu commented 4 years ago

I guess in that case, (MD Length - Baseline Instruction metadata length calculated based upon bitmap) = DS Metadata. The collector needs to be aware of the definition of DS Instruction Bitmap, which is anyways required.

I would like to keep the DS Extension Data encoding in the InType field. Allows for a future extension when Truncated packet is not necessary for the report.

Thanks

-Ramesh

On Mon, Apr 13, 2020 at 11:51 AM Randy Levensalor notifications@github.com wrote:

@mickeyspiegel https://github.com/mickeyspiegel That would be fine. Then we could remove 1: Domain Specific Extension Data from the InType, since that could be added to the DS metadata.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613039289, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFSJMKFRP7GY7KCCQDTRMNNLZANCNFSM4JDZMABQ .

mickeyspiegel commented 4 years ago

@rsivakolundu +1 for keeping DS Extension Data in InType

RandyLevensalor commented 4 years ago

That still limits the options if you need both a variable length (which is why I'm assuming you want to keep the DS Extension Data and not define everything in the bitmap.

That brings us back to having to choose or limit your DS data if you are including that packet fragment.

Note: This is a concern, not a strong objection. It just feels like this is making assumptions about not needing the same level of DS data when the packet fragment is needed.

While this won't break my current use case, the assumption that you will not have the same DS requirements with and without packet fragments.

One assumption that I have is that multiple applications could read that same telemetry report. Where some TR consumers are general-purpose and would ignore DS data. Then DS aware applications would handle the DS fields.

mickeyspiegel commented 4 years ago

@RandyLevensalor Yes the options are a bit limited, but I don't see a particular use case that justifies throwing in yet another type and length. It is a judgement call.

Note that one use case for Additional DS Extension Data is non-packet specific report data, for example control plane information.

RandyLevensalor commented 4 years ago

I'd like to throw out one last suggestion before I drop this discussion around supporting both Domain Specific Extension Data and packet fragments.

To avoid adding extra data to the report header, could we create a new Inner Type for TLV reports.

The TLV would be of the structure of something like: TVL - Type (Domain Specific Extension Data, IPv6, IPv4, Ethernet) Length of the entry. The actual data, either the packet fragment or Domain Specific Extension Data.

Followed by as many of these TLV entire. All TLV entries and heders must be less than 1024 bytes to fit within the current report length.

rsivakolundu commented 4 years ago

Are you proposing something like this?

0 1 2 3

0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|Ver = 2| hw_id | Sequence Number |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| Node ID |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|RepType|In Type| Report Length | MD Length |D|Q|F|I| Rsvd |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| RepMdBits | Domain Specific |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| DSMdBits | DSMdstatus |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable Optional RepMdBits Data

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable Optional DSMdBits Data

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|In Type| TLV Length | Reserved |NxtType|

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable TLV Data

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|In Type| TLV Length | Reserved |NxtType|

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable TLV Data

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

InType (4b): Inner Type – TLV formatted data or Truncated packet or additional Domain Specific Data after Variable Optional Baseline & DS Metadata corresponding to RepMdBits or DS MD Bits.

0 – None

1 – Domain Specific Extension Data

2 - Ethernet

3 – IPv4

4 – IPv6

5 - TLV

6-15 – Reserved.

NxtType (4b): Next Type – TLV formatted data or None after Variable TLV Data

0 – None

1 - TLV

2-15 – Reserved.

Example with DS Extension Data & Truncated Packet.

0 1 2 3

0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|Ver = 2| hw_id | Sequence Number |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| Node ID |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|RepType|0 1 0 1| Report Length | MD Length |D|Q|F|I| Rsvd |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| RepMdBits | Domain Specific |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| DSMdBits | DSMdstatus |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| Variable Optional RepMdBits Data |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable Optional DSMdBits Data

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|0 0 0 1| Length | Reserved |0 0 0 1|

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable TLV Data (Domain Specific Extension Data)

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|0 1 0 0| Length | Reserved |0 0 0 0|

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable TLV Data (Truncated IPv4 Packet)

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

If so, I think it works for any unforeseen use case.

-Ramesh

On Tue, Apr 14, 2020 at 9:55 AM Randy Levensalor notifications@github.com wrote:

I'd like to throw out one last suggestion before I drop this discussion around supporting both Domain Specific Extension Data and packet fragments.

To avoid adding extra data to the report header, could we create a new Inner Type for TLV reports.

The TLV would be of the structure of something like: TVL - Type (Domain Specific Extension Data, IPv6, IPv4, Ethernet) Length of the entry. The actual data, either the packet fragment or Domain Specific Extension Data.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613558763, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFR2NCAZOSHOS6TI733RMSIQPANCNFSM4JDZMABQ .

RandyLevensalor commented 4 years ago

Ramesh,

Yes. That is exactly what I was thinking.

Thanks

mickeyspiegel commented 4 years ago

That proposal is adding another type and length, which is something that I want to avoid. It is also mixing next header and TLV styles in a way that I do not feel brings out the best of either approach.

If necessary, we could revive the TLV format that I had proposed earlier in addition to the format that is currently in this PR. Personally, I don't see a strong enough rationale to add an additional format, but I won't object if others feel differently.

rsivakolundu commented 4 years ago

I am fine with either approaches. Will go along with consensus opinions.

-Ramesh

On Tue, Apr 14, 2020 at 11:22 AM Mickey Spiegel notifications@github.com wrote:

That proposal is adding another type and length, which is something that I want to avoid. It is also mixing next header and TLV styles in a way that I do not feel brings out the best of either approach.

If necessary, we could revive the TLV format that I had proposed earlier in addition to the format that is currently in this PR. Personally, I don't see a strong enough rationale to add an additional format, but I won't object if others feel differently.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613603036, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFQ52I4UTCWAHAAAER3RMSSVRANCNFSM4JDZMABQ .

mickeyspiegel commented 4 years ago

@rsivakolundu The thing that made me uneasy more than anything else in the expanded format is "NxtType". That is what breaks from normal TLV style. I think it is unnecessary. If you drop that and just leave two reserved bytes, then InType 'TLV' means whatever follows MD Length is in TLV style. That includes the possibility of having multiple TLVs as long as they all fit within Report Length.

I can live with the expansion with that change. I would move the values around and make codepoint 1 TLV, with the understanding that in the TLVs the type value must be 2 or greater.

We probably would not support that codepoint initially in our implementation, but I don't see any issue with that.

mickeyspiegel commented 4 years ago

Given that we don't have any flow identity information in the initial part of the report header, do we really need the InType codepoint "None"? Would there ever be a report without any additional information?

rsivakolundu commented 4 years ago

I think so. Flow identification could be a domain specific metadata.

-Ramesh

On Tue, Apr 14, 2020 at 12:03 PM Mickey Spiegel notifications@github.com wrote:

Given that we don't have any flow identity information in the initial part of the report header, do we really need the InType codepoint "None"? Would there ever be a report without any additional information?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613625228, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFXNQNB2E4LFRNNTODDRMSXQLANCNFSM4JDZMABQ .

mickeyspiegel commented 4 years ago

@rsivakolundu Then just add the TLV codepoint as InType 1, shift the other codepoints from 1+ up by one, and define the TLV format as Type (4 bits), Reserved (4 bits), Length (8 bits), Reserved (16 bits), TLV contents (variable multiple of 4 bytes)

rsivakolundu commented 4 years ago

We still need next Type field in the TLV.

InType (4b): Inner Type – TLV formatted data or Truncated packet or additional Domain Specific Data after Variable Optional Baseline & DS Metadata corresponding to RepMdBits or DS MD Bits.

0 – None

1 - TLV

2 – Domain Specific Extension Data

3 - Ethernet

4 – IPv4

5 – IPv6

6-15 – Reserved.

-Ramesh

On Tue, Apr 14, 2020 at 1:06 PM Mickey Spiegel notifications@github.com wrote:

@rsivakolundu https://github.com/rsivakolundu Then just add the TLV codepoint as InType 1, shift the other codepoints from 1+ up by one, and define the TLV format as Type (4 bits), Reserved (4 bits), Length (8 bits), Reserved (16 bits), TLV contents (variable multiple of 4 bytes)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613654368, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFXMZB7IAOROKZVTGPLRMS64RANCNFSM4JDZMABQ .

mickeyspiegel commented 4 years ago

@rsivakolundu I don't understand your comment about "next Type".

The first InType tells you what comes after the Variable Optional MdBits data. When the value of that first InType is TLV, then there are one or more TLVs following the Variable Optional MdBits data, each a multiple of 4 bytes long. Each TLV includes a Type (from InType values 2+), a length, 2 reserved bytes, and a value. To determine whether another TLV is present, you have to evaluate the length of this and preceding TLVs to determine if Report Length has been filled up, as is normal for any TLV encoding scheme.

If you want to call the type in the TLVs something other than InType and copy InType codepoints 2+ in that definition, that might make things clearer.

rsivakolundu commented 4 years ago

Agree with normal TLV encoding scheme. Since we have reserved bits, was trying to simplify the processing at the collector.

Is the following acceptable?

Telemetry Report Header (20+ Octets)

0 1 2 3

0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|Ver = 2| hw_id | Sequence Number |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| Node ID |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|RepType|In Type| Report Length | MD Length |D|Q|F|I| Rsvd |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| RepMdBits | Domain Specific |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| DSMdBits | DSMdstatus |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable Optional RepMdBits Data

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Variable Optional DSMdBits Data

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|TLVType| Rsvd | TLVLength | Reserved |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| Variable TLV Data as identified by TLVType |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

|TLVType| Rsvd | TLVLength | Reserved |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

| Variable TLV Data as identified by TLVType |

+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

TLVType (4b): TLV Type – TLV data or Truncated packet or additional Domain Specific Data after Variable Optional Baseline & DS Metadata corresponding to RepMdBits or DS MD Bits.

0 – Domain Specific Extension Data

1 - Ethernet

2 – IPv4

3 – IPv6

4-15 – Reserved.

Report Length = MD Length + TLVLength + TLVLength in the above example.

-Ramesh

On Tue, Apr 14, 2020 at 1:29 PM Mickey Spiegel notifications@github.com wrote:

@rsivakolundu https://github.com/rsivakolundu I don't understand your comment about "next Type".

The first InType tells you what comes after the Variable Optional MdBits data. When the value of that first InType is TLV, then there are one or more TLVs following the Variable Optional MdBits data, each a multiple of 4 bytes long. Each TLV includes a Type (from InType values 2+), a length, 2 reserved bytes, and a value. To determine whether another TLV is present, you have to evaluate the length of this and preceding TLVs to determine if Report Length has been filled up, as is normal for any TLV encoding scheme.

If you want to call the type in the TLVs something other than InType and copy InType codepoints 2+ in that definition, that might make things clearer.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613665109, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFVUM7ZZRNBHT77DUW3RMTBT3ANCNFSM4JDZMABQ .

mickeyspiegel commented 4 years ago

@rsivakolundu I am fine with the last proposal, except for two points of discussion:

  1. Should the TLVType codepoint values align with the InType codepoint values? I guess I can go either way
  2. In INT the shim header lengths never included the shim header itself (first 4 bytes). This was due to precedents set in other specifications such as VXLAN-GPE. For Telemetry Report, we need to decide whether the first 4 bytes are or are not included in the length. In Report Length, the first 4 bytes are just part of 12 fixed header bytes, so I guess including everything is the way I am leaning, but I am not very sure of myself. In TLVType there is a clear first 4 bytes. Should the first 4 bytes be included in the length?

For the Domain Specific Extensions codepoint, you could use the 2 bytes after type and length rather than making them reserved, but then the encoding would differ between the non-TLV Domain Specific Extensions and the TLV Domain Specific Extensions.

rsivakolundu commented 4 years ago

Great. We are converging.

I prefer the TLVType codepoint values to independent of InType codepoint values. Less confusion and allows for future use cases.

Including the first 4-bytes is more meaningful to me. I didn't like the VXLAN-GPE precedence, but it is what it is at this stage. Nothing forces us to not include the first 4-bytes.

I am leaning towards leaving the Domain Specific Extension to be free form and not bitmapped. I could change those 16 bits to be Domain Specific Extension Template.

Thoughts?

-Ramesh

On Tue, Apr 14, 2020 at 2:52 PM Mickey Spiegel notifications@github.com wrote:

@rsivakolundu https://github.com/rsivakolundu I am fine with the last proposal, except for two points of discussion:

  1. Should the TLVType codepoint values align with the InType codepoint values? I guess I can go either way
  2. In INT the shim header lengths never included the shim header itself (first 4 bytes). This was due to precedents set in other specifications such as VXLAN-GPE. For Telemetry Report, we need to decide whether the first 4 bytes are or are not included in the length. In Report Length, the first 4 bytes are just part of 12 fixed header bytes, so I guess including everything is the way I am leaning, but I am not very sure of myself. In TLVType there is a clear first 4 bytes. Should the first 4 bytes be included in the length?

For the Domain Specific Extensions codepoint, you could use the 2 bytes after type and length rather than making them reserved, but then the encoding would differ between the non-TLV Domain Specific Extensions and the TLV Domain Specific Extensions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613699808, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFX44AZ6JUNTZGQI2CDRMTLKRANCNFSM4JDZMABQ .

mickeyspiegel commented 4 years ago

@rsivakolundu Not sure what you mean by bitmapped. The first 16 bits (type, reserved, length) of each TLV must be the same. After that it can be anything you want. For truncated packet fragments, the next 16 bits should be reserved, but for other type values it should be free form depending on the type.

rsivakolundu commented 4 years ago

Yes. I am stating the same thing. See description below.

TLVType (4b): TLV Type – TLV data or Truncated packet or additional Domain Specific Data after Variable Optional Baseline & DS Metadata corresponding to RepMdBits or DS MD Bits.

0 – Domain Specific Extension Data

1 - Ethernet

2 – IPv4

3 – IPv6

4-15 – Reserved.

Rsvd (4b) – Reserved. Should be set to zero upon transmission and ignored upon reception.

TLVLength (8b) - Indicates the length, in 4-byte word, of Variable TLV Data as identified by TLVType.

TLV Data Template (16b) – TLV Data Template specifies the format of the Variable TLV Data. A non-zero TLV Data Template value specifies the template for TLVType codepoint of Domain Specific Extension Data. The zero TLV Data Template value is reserved for TLVType codepoints for Ethernet, IPv4, IPv6.

Variable TLV Data – Variable length data based upon TLVType.

-Ramesh

On Tue, Apr 14, 2020 at 3:49 PM Mickey Spiegel notifications@github.com wrote:

@rsivakolundu https://github.com/rsivakolundu Not sure what you mean by bitmapped. The first 16 bits (type, reserved, length) of each TLV must be the same. After that it can be anything you want. For truncated packet fragments, the next 16 bits should be reserved, but for other type values it should be free form depending on the type.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613718839, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFWBE45QFE3PCRBNSNDRMTR7NANCNFSM4JDZMABQ .

mickeyspiegel commented 4 years ago

@ramesh I would not even define the next two bytes as a general field. If you do, the last sentence needs to be reworded: "For TLVType codepoints Ethernet, IPv4, and IPv6, the TLV Data Template value should be zero upon transmission and ignored upon reception."

Instead, I would define a format for TLVType codepoints Ethernet, IPv4, and IPv6:

                               |       Reserved (16 bits)       |
|                        Truncated Packet                       |

Somewhere it needs to be stated that each TLV must be a multiple of 4 bytes long. If you stick with your proposed format, that would be in Variable TLV Data.

rsivakolundu commented 4 years ago

TLVType (4b): TLV Type – TLV data or Truncated packet or additional Domain Specific Data after Variable Optional Baseline & DS Metadata corresponding to RepMdBits or DS MD Bits.

0 – Domain Specific Extension Data

1 - Ethernet

2 – IPv4

3 – IPv6

4-15 – Reserved.

Rsvd (4b) – Reserved. Should be set to zero upon transmission and ignored upon reception.

TLVLength (8b) - Indicates the length, in 4-byte word, of Variable TLV Data as identified by TLVType.

TLV Data Template (16b) – TLV Data Template specifies the format of the Variable TLV Data. A non-zero TLV Data Template value specifies the template for TLVType codepoint of Domain Specific Extension Data. For TLVType codepoints Ethernet, IPv4, and IPv6, the TLV Data Template value should be zero upon transmission and ignored upon reception.

Variable TLV Data – Variable length data, based upon TLVType, as 4-byte words.

If we concur, I will add this format to the PR.

Thanks

-Ramesh

On Tue, Apr 14, 2020 at 4:14 PM Mickey Spiegel notifications@github.com wrote:

@Ramesh https://github.com/Ramesh I would not even define the next two bytes as a general field. If you do, the last sentence needs to be reworded: "For TLVType codepoints Ethernet, IPv4, and IPv6, the TLV Data Template value should be zero upon transmission and ignored upon reception."

Instead, I would define a format for TLVType codepoints Ethernet, IPv4, and IPv6:

                           |       Reserved (16 bits)       |

| Truncated Packet |

Somewhere it needs to be stated that each TLV must be a multiple of 4 bytes long. If you stick with your proposed format, that would be in Variable TLV Data.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/p4lang/p4-applications/pull/61#issuecomment-613726361, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZZHFTJ7MQCJUBEJMVMZMTRMTU5XANCNFSM4JDZMABQ .

mickeyspiegel commented 4 years ago

@rsivakolundu Go ahead and add it.

rsivakolundu commented 4 years ago

Updated the PR with InType of TLV.

RandyLevensalor commented 4 years ago

Looks great. Thanks for making these changes!!

mickeyspiegel commented 4 years ago

@jklr Please revise comments or approve