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

Add extern function recirculate() #114

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jfingerh
Copy link
Contributor

No description provided.

// recirculate() - Cause the packet to be recirculated when it
// finishes completing the main control.
//
// Invoking recirculate() is supported only within the main control.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this would be better comment:

Invoking recirculate() is supported directly or indirectly called from the main control.

I know this is how it is written in other locations but being more explict of what it means be invoked from the main control is always better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to explore following types of recirculations -

  1. parser -> control -> deparser -- recirc1 --> parser ..... (as shown in pna arch diagram) - User metadata is NOT preserved. Any data needed is added as recirc-header, which will be extracted by parser.
  2. parser -> control -> -- recirc2 --> control .... -> deparser - User metadata is preserved.

recirc1 and recirc2 (to be named appropriately) are both invoked in action routines and take effect at the end of the pipeline.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the 2nd form of doing recirc, maybe I suggest, the name redocontrol or something similar to that.
With that being said my hardware can support the 2nd form though I am not so sure the 2nd form is useful; can you think of an usecase for each form?
I can think of removing the xvlan and doing some ACLs and then recirculating it and redoing the parsing to get a new set of headers. That would be the first form.
But I can't think of use case for the 2nd form though unless we have the original parser also get the inner headers. I am not 100% sure if that will always work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use cases of 2nd form I can think of -

  • implementing longer (stages) pipeline on a target that has smaller number of stages. Each pass launches different set of tables to complete the processing.
  • perform hash-chaining on hash collision
  • Performance benefit in avoiding parser/deparser processing times if packet is not changed between passes.

@jafingerhut
Copy link
Collaborator

Explicitly document what is preserved with packets when they are recirculated.

One thought is that all user-defined metadata is preserved, but this is not universally implementable by all vendors wanting to support PNA.

One mechanism that seems universally implementable is to explicitly add one or more headers to the packet that must be preserved during recirculation.

Another potential mechanism would be that there are one or more arguments to the recirculate operation that are specified by the P4 developer, and this causes those fields to be preserved by the device. For one example of this approach, see the v1model and PSA architectures (they are not the same as each other in the P4 source code mechanisms, but both require the P4 developer to be explicit in what they want to save vs. not).

Another idea would be to have different operations for preserving vs. not preserving user-defined metadata, or some way to specify exactly which subset of user-defined metadata is saved, e.g. copying it into a header or struct explicitly in the user's P4 code. (perhaps calling one resubmit vs. recirculate).

AI Andy: Present v1model and PSA mechanisms (maybe also TNA?) for preserving metadata at next PNA meeting (probably 2nd week Feb 2023).

@thantry
Copy link

thantry commented Jan 30, 2023

It would be good to also specify precisely what intrinsic metadata changes, and if that is agreed upon behavior between all architectures

@thomascalvert-xlnx
Copy link
Member

If PNA supports mutable table entries that is also a mechanism for storing state in-between passes.

Various implementations will have different cost functions for preserving context in: metadata vs packet headers vs tables.

@jfingerh
Copy link
Contributor Author

There was a discussion about how to control which subset of metadata should be preserved with the packet after a recirculate operation, with notes here: https://docs.google.com/document/d/1vX5GStrE01Pbj6d-liuuHF-4sYXjc601n5zJ4FHQXpM/edit#bookmark=id.uqs2nsxo6mx0

Below is another variation that is somewhat of a combination of some other ideas discussed before, plus one new thing not mentioned during the 2023-Feb-13 meeting.

Proposal:

  • There is a user_metadata parameter that is direction 'inout' to the main parser, main control, and main deparser. ALL of its contents are preserved during recirculation.
  • If there is a significant amount of user metadata that someone DOES NOT want to preserve during recirculation, they can declare it as one or more local variables in the main parser, or the main control. This is very natural in the definition of the P4 language to have a "lifetime" that is only the processing of a packet during the execution of that parser or control, and cannot "leave" the parser/control in which it is declared, unless you explicitly copy it into other places that are outputs of the parser/control.
  • If someone wants highly variable quantities of data preserved with a recirculated packet, e.g. 16 bytes for all packets, but an extra 64 bytes for 10% of packets, declare the 16 bytes as user_metadata, and put the additional 64 bytes into 1 or more headers that are added to only those recirculated packets that you want to save it for. Packets that do not need it, will not have to incur its cost.

Thoughts?

@jafingerhut
Copy link
Collaborator

Mario: If you want a significant amount of user metadata that is assigned in the main parser, and read and/or written in the main control or main deparser, but NOT recirculated, the approach described in the comment above would not handle that well.

Andy: We could consider applying a small tweak to the previous comment where there are two user_metadata parameters: one that is explicitly preserved across recirculation, the other defined NEVER to be preserved across recirculation.

Thomas: The approach of adding headers for preserving recirculated metadata could interact badly with hardware accelerators, e.g. IPsec encrypt/decrypt on the recircucation path, unless they were configured somehow to ignore the user-defined custom headers.

@jafingerhut
Copy link
Collaborator

Some notes from 2023-Mar-20 discussion of this topic:

Thomas: What about recirculate(single_bit_W_value); extern call, where single_bit_W_value is the ONLY value promised to be preserved with the recirculated packet, and that value is copied into some intrinsic metadata input field that we define, e.g. recirc_data. This new input value would be an input to the MainParser and the MainControl.
The new recirculation_data field would be in structs named pna_main_parser_input_metadata_t and pna_main_input_metadata_t
Alan: Why not a user-defined struct type rather than a single field?
Thomas: The main reason is to lower the bar to something we think everyone can support for sure.
Why have this feature at all, vs. not? The main reason to consider it is to enable a way to preserve at least a LITTLE bit of data with the packet, WITHOUT requiring the P4 developer from adding headers before recirculating.

@jafingerhut
Copy link
Collaborator

2023-Apr-03 discussion: Sounds reasonable to update this PR to add one bit type to recirculate() extern function as its only parameter, and that value becomes the value of a new recirculate_data field in the parser input and main control input on the next pass.

Should we do this, too? Also have a 0-argument variant of recirculate in case a user has no such data they care to include with the packet, in case a target can optimize in this case.

@jafingerhut
Copy link
Collaborator

@jfingerh Reminder from Andy to himself to make progress on this issue.

@jafingerhut
Copy link
Collaborator

With commit 4 of this PR, I believe I have addressed all of the earlier comments above, except for this proposed idea:

  • pbhide proposed the idea of a variant of the recirculate operation that causes the packet NOT to execute the deparser, or the parser, but to effectively to "goto label_at_beginning_of_main_control"

I would recommend that such a different operation be part of a separate PR from this one, if someone wants to champion that proposal.

// `recirc_data` has the same value as the field with the same
// name in struct `pna_main_parser_input_metadata_t` did when this
// packet was parsed.
bit<32> recirc_data;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propose changing bit<32> to type parameter T, like so:

struct pna_main_input_metdata_t<T> {
      // other fields here
      T recirc_data;
}

also hopefully the package definitions below can be used to force T to be the same between parser and main control.

`recirculate` is the same as calling `recirculate` then `drop_packet`,
or different. Alternately, we can explicitly say "every PNA target
should define and document their behavior on arbitrary combinations of
these operations, if they wish". P4 developers would not find that as
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Andy: Add a note about one possibility: last-one-wins among drop_packet send_to_port recirculate, but mirror_packet always takes effect on top of the last of those 3 ops.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants