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.

Sign up for GitHub

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

Added Recirculation functionality in PNA_NIC #1273

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added Recirculation functionality in PNA_NIC #1273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions targets/pna_nic/pna_nic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,24 @@ PnaNic::main_thread() {

Deparser *deparser = this->get_deparser("main_deparser");
deparser->deparse(packet.get());

// recirculation
if ((port_t) packet->get_egress_port() == PNA_PORT_RECIRCULATE) {
BMLOG_DEBUG_PKT(*packet, "Recirculating packet");

phv->reset();
phv->reset_header_stacks();
phv->reset_metadata();

phv->get_field("pna_main_parser_input_metadata.input_port")
.set(input_port);
phv->get_field("pna_main_parser_input_metadata.recirculated")
.set(1);

input_buffer.push_front(std::move(packet));
Copy link
Member

Choose a reason for hiding this comment

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

what's the expected value of pna_main_parser_input_metadata.input_port for recirculated packets?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it should be unchanged from the original. Unfortunately some things like this are not yet spelled out in as precise a form in the PNA architecture as they are in the PSA architecture, but I believe "input_port is unchanged after recirculate operation" is a reasonable starting place (and maybe finishing place, too).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. This is not what the current PR does, but it can be done easily. See PSA implementation:

behavioral-model/targets/psa_switch/psa_switch.cpp

Lines 618 to 619 in 28b736c

phv->get_field("psa_ingress_parser_input_metadata.ingress_port")
.set(PSA_PORT_RECIRCULATE);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Will update the line.

Just a doubt: Why are we setting ingress_port field to PSA_PORT_RECIRCULATE ? The P4 programs will find whether the packet is recirculated on not using recirculated_flag field.

Copy link
Member

Choose a reason for hiding this comment

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

According to @jafingerhut, the port should be set to the original ingress port, not PSA_PORT_RECIRCULATE
But we should still set the port to something. With your current implementation, the port will always be set to 0. At least that's what will happen I think based on the reset_metadata call (I didn't test).

continue;
}

output_buffer.push_front(std::move(packet));
}
}
Expand Down
1 change: 1 addition & 0 deletions targets/pna_nic/pna_nic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ class PnaNic : public Switch {

private:
static packet_id_t packet_id;
static constexpr port_t PNA_PORT_RECIRCULATE = 0xfffffffa;
Copy link
Member

Choose a reason for hiding this comment

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

There is no mention of a recirculation port in the PNA spec. @jafingerhut is this correct?
All I know is that this is the SDN port value defined by the P4Runtime spec as the recirculation port.
I guess targets are free to "choose" (or define) the value used for the recirculation port.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, there is a recirculate() operation in PNA, and if that is done, then the packet should recirculate when it completes the current execution of the main control. There is NO recirculation port like there is in PSA or v1model architectures.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: As an implementation technique, it is probably a reasonable idea to have a C++ boolean or 1-bit flag somewhere associated with the packet in the PNA implementation that is initialized to 0/false whenever a packet begins execution of the main control, and it is set to 1/true when the recirculate() extern function is called.

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find a recirculate() extern function in pna.p4? Is there a reference to it somehere.
If PNA specifies such an extern function, then this is what this PR should introduce (not a special port value). Implementation of the extern function can use a special port value if appropriate (the boolean flag approach you suggested may be a bit more difficult within the constraints of bmv2).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I didn't find any recirculate() extern function in pna.p4. So, I followed the PSA implementation.

If introducing a new recirculate() function is better approach, we can implement that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @antoninbas, Is the current approach Ok? Or should I change it recirculate() extern approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

One approach is to wait until there is an official PNA definition for how recirculation is done, and behaves, and implement other features that are already well-defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok


enum PktInstanceType {
FROM_NET_PORT,
Expand Down