Skip to content
Closed
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
2 changes: 2 additions & 0 deletions include/bm/bm_sim/P4Objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,8 @@ class P4Objects {
void enable_arith(header_id_t header_id, int field_offset);
void enable_arith(header_id_t header_id);

bool is_selector_fanout(
const Json::Value &cfg_next_nodes) const;
std::unique_ptr<Calculation> process_cfg_selector(
const Json::Value &cfg_selector) const;
};
Expand Down
4 changes: 4 additions & 0 deletions include/bm/bm_sim/action_profile.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ class ActionProfile : public NamedP4Object {
void serialize(std::ostream *out) const;
void deserialize(std::istream *in, const P4Objects &objs);

void set_selector_fanout();

private:
using ReadLock = boost::shared_lock<boost::shared_mutex>;
using WriteLock = boost::unique_lock<boost::shared_mutex>;
Expand Down Expand Up @@ -261,6 +263,7 @@ class ActionProfile : public NamedP4Object {

private:
mbr_hdl_t choose_from_group(grp_hdl_t grp, const Packet &pkt) const;
std::vector<ActionProfile::mbr_hdl_t> get_all_mbrs_from_group(grp_hdl_t grp) const;

MatchErrorCode get_member_(mbr_hdl_t handle, Member *member) const;

Expand Down Expand Up @@ -307,6 +310,7 @@ class ActionProfile : public NamedP4Object {
std::shared_ptr<GroupSelectionIface> grp_selector_{nullptr};
GroupSelectionIface *grp_selector{&grp_mgr};
std::unique_ptr<Calculation> hash{nullptr};
bool selector_fanout_enabled{false};
};

} // namespace bm
Expand Down
2 changes: 2 additions & 0 deletions include/bm/bm_sim/match_tables.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include "lookup_structures.h"
#include "action_entry.h"
#include "action_profile.h"
#include "replicated_pkt_vec.h"

namespace bm {

Expand All @@ -52,6 +53,7 @@ enum class MatchTableType {
class MatchTableAbstract : public NamedP4Object {
public:
friend class handle_iterator;
friend class ReplicatedPktVec;

using counter_value_t = Counter::counter_value_t;

Expand Down
13 changes: 13 additions & 0 deletions include/bm/bm_sim/packet.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include <atomic>
#include <algorithm> // for std::min
#include <limits>
#include <optional>

#include <cassert>

Expand All @@ -40,6 +41,7 @@
#include "parser_error.h"
#include "phv_source.h"
#include "phv_forward.h"
#include "control_flow.h"

namespace bm {

Expand Down Expand Up @@ -315,6 +317,16 @@ class Packet final {
//! @copydoc clone_choose_context
std::unique_ptr<Packet> clone_choose_context_ptr(cxt_id_t new_cxt) const;

// Hao: pkt permutation stuff:
bool has_continue_node() const;
//! Get the continue node, if it exists
const ControlFlowNode *get_continue_node() const;
//! Set the continue node, which is used to continue the packet processing
void set_continue_node(const ControlFlowNode *node);
//! Reset the continue node
void reset_continue_node();


//! Deleted copy constructor
Packet(const Packet &other) = delete;
//! Deleted copy assignment operator
Expand Down Expand Up @@ -385,6 +397,7 @@ class Packet final {

bool checksum_error{false};

std::optional<const ControlFlowNode *> continue_node{std::nullopt};
private:
static CopyIdGenerator *copy_id_gen;
};
Expand Down
2 changes: 2 additions & 0 deletions include/bm/bm_sim/pipeline.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class Pipeline : public NamedP4Object {
//! step (table lookup or condition evaluation), according to the P4 control
//! flow graph.
void apply(Packet *pkt);
// Start from continue_node instead of first_node
void apply_continued(Packet *pkt);

//! Deleted copy constructor
Pipeline(const Pipeline &other) = delete;
Expand Down
52 changes: 52 additions & 0 deletions include/bm/bm_sim/replicated_pkt_vec.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/* Copyright 2013-present Contributors to the P4 Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#ifndef BM_BM_SIM_REPLICATED_PKT_VEC_H_
#define BM_BM_SIM_REPLICATED_PKT_VEC_H_

#include "logger.h"
#include "packet.h"
#include "match_tables.h"
#include <vector>

namespace bm {
class MatchTableAbstract;
// TODO(Hao): should this be per ingress thread or just global?
class ReplicatedPktVec {
public:
ReplicatedPktVec(const ReplicatedPktVec&) = delete;
ReplicatedPktVec& operator=(const ReplicatedPktVec&) = delete;

static ReplicatedPktVec& instance() {
static ReplicatedPktVec instance_;
return instance_;
}

void set_next_nodes(const MatchTableAbstract *match_table, bool hit);
// TODO(Hao): deduplicate packets replciated

public:
// Replicated pkts will first be added to replicated_pkts
// in order to get the corresponding next table node.
// After the cont_node is set, the pkt will be added to replicated_pkts
std::vector<Packet *> replicated_pkts;
std::vector<std::pair<Packet *, p4object_id_t>> replicated_pkts_w_act_id;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't these be protected by a mutex?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

currently it is only used in ingress, which is single threaded, so it is safe. But added the mutex for egress later.


private:
ReplicatedPktVec() = default;
};

} // namespace bm

#endif // BM_BM_SIM_REPLICATED_PKT_VEC_H_
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT: missing white space at end of file

1 change: 1 addition & 0 deletions src/bm_sim/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ add_library(bmsim OBJECT
phv_source.cpp
pipeline.cpp
port_monitor.cpp
replicated_pkt_vec.cpp
simple_pre.cpp
simple_pre_lag.cpp
source_info.cpp
Expand Down
1 change: 1 addition & 0 deletions src/bm_sim/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ pcap_file.cpp \
pipeline.cpp \
periodic_task.cpp \
port_monitor.cpp \
replicated_pkt_vec.cpp \
phv.cpp \
phv_source.cpp \
stateful.cpp \
Expand Down
23 changes: 21 additions & 2 deletions src/bm_sim/P4Objects.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <bm/bm_sim/P4Objects.h>
#include <bm/bm_sim/phv.h>
#include <bm/bm_sim/actions.h>
#include <bm/bm_sim/logger.h>

#include <iostream>
#include <istream>
Expand Down Expand Up @@ -1655,8 +1656,20 @@ P4Objects::init_pipelines(const Json::Value &cfg_root,
std::unique_ptr<ActionProfile> action_profile(
new ActionProfile(act_prof_name, act_prof_id, with_selection));
if (with_selection) {
auto calc = process_cfg_selector(cfg_act_prof["selector"]);
action_profile->set_hash(std::move(calc));
// TODO(Hao): clean debug logs
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove debug logs

BMLOG_DEBUG(
"Action profile '{}' with id {} has a selector",
act_prof_name, act_prof_id);

if(is_selector_fanout(cfg_act_prof["selector"])){
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Formatting: add space after "if" and before the ending "{"

BMLOG_DEBUG(
"Action profile '{}' with id {} enabled selector fanout",
act_prof_name, act_prof_id);
action_profile->set_selector_fanout();
}else{
auto calc = process_cfg_selector(cfg_act_prof["selector"]);
action_profile->set_hash(std::move(calc));
}
Comment on lines +1660 to +1673
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if this should really be a command-line option for a target, instead of a special "algorithm" in the JSON. It feels like running in this "mode" should not require updating the JSON. Unless maybe the goal is to target individual selectors, and not all of them?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think ideally it would target individual selectors, though for our purposes I think it would be fine to only allow all-or-nothing for now if easier. That said, we felt the 'mode' was the best choice mostly because the other mode options are kinda non-sense in the presence of creating all outputs. E.g. what does it mean to hash a packet to a member when it should go to all of them? Or round-robin? So it seemed like it was just really a different selection scheme which is "Pick all".

}
add_action_profile(act_prof_name, std::move(action_profile));
}
Expand Down Expand Up @@ -2499,6 +2512,12 @@ P4Objects::check_hash(const std::string &name) const {
return nullptr;
}

// TODO(Hao): messy, so i think use pragma is better
bool P4Objects::is_selector_fanout(const Json::Value &cfg_selector) const {
return cfg_selector.isMember("algo") && // not to hardcode names, fix later
cfg_selector["algo"].asString() == "selector_fanout";
}

std::unique_ptr<Calculation>
P4Objects::process_cfg_selector(const Json::Value &cfg_selector) const {
const auto selector_algo = cfg_selector["algo"].asString();
Expand Down
46 changes: 45 additions & 1 deletion src/bm_sim/action_profile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <bm/bm_sim/action_profile.h>
#include <bm/bm_sim/logger.h>
#include <bm/bm_sim/packet.h>
#include <bm/bm_sim/replicated_pkt_vec.h>

#include <iostream>
#include <memory>
Expand Down Expand Up @@ -126,6 +127,8 @@ ActionProfile::mbr_hdl_t
ActionProfile::GroupMgr::get_from_hash(grp_hdl_t grp, hash_t h) const {
const auto &group_info = groups.at(grp);
auto s = group_info.size();
BMLOG_DEBUG("Hao: Choosing member from group {} with hash {} (size {})",
grp, h, s);
return group_info.get_nth(h % s);
}

Expand Down Expand Up @@ -175,9 +178,36 @@ ActionProfile::lookup(const Packet &pkt, const IndirectIndex &index) const {
} else {
grp_hdl_t grp = index.get_grp();
assert(is_valid_grp(grp));
//Hao: critical part for fanout all possible members
std::vector<mbr_hdl_t> mbrs;
mbr = choose_from_group(grp, pkt);
// TODO(antonin): change to trace?
BMLOG_DEBUG_PKT(pkt, "Choosing member {} from group {}", mbr, grp);
BMLOG_DEBUG_PKT(pkt, "Chose member {} from group {}", mbr, grp);

if(selector_fanout_enabled){
BMLOG_DEBUG_PKT(pkt, "Selector fanout enabled, choosing member from group {} WIP", grp);
mbrs = get_all_mbrs_from_group(grp);
for(auto m:mbrs){
if(m == mbr){
continue;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I feel like there is no strong reason to treat one member differently in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It makes the output easier to reason about. If the selected member is also replicated, 2 identical sets of redundant pkts would be derived (one from the original, one from the replicated): it could be confusing as there would be more unexpected pkts.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but why call choose_from_group at all in this case? you don't even have an hash algorithm for fanout selectors.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because if selector_fanout_enabled is false, then the mbr got from choose_from_group will use whatever hash is specified in the P4 program.

}
// TODO(Hao): apply_action in match_tables has a full procedure,
// need to make sure that directly applying the func does not
// cause any issues
//Things to consider:
// 1. set the entry index
// 2. set meters
// 3. set counters
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't believe there is a good solution for this. You are bound to end up with discrepancies between counters at different stages of the pipeline. I think both of these solutions could be considered valid:

  1. apply meters / counters a single time
  2. apply meters / counters as many times as there are copies

Ultimately you are generating a lot of packet copies from a single packet, so it could create issues with meters.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My understanding (perhaps incorrect) is that the primary intent of these changes is for testing, not running a switch "in production". It seems that applying meters/counters/register-updates as many times as there are copies is the only straighforward thing to implement. Anything else seems to create a rabbit-hole of subtle issues.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jafingerhut 's understanding is correct. I would leave it as is, the side effects should be considered by the user.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It seems that applying meters/counters/register-updates as many times as there are copies is the only straighforward thing to implement

the counters are bound to be incorrect anyway, given that generated copies are injected in the middle of the pipeline. The counters at the beginning of the pipeline will show values < to counters later in the pipeline, even if they are meant to be applied for every packet. That's all I was pointing out here.

// 4. incorporate with the debugger?
const ActionEntry &action = action_entries[m];
Packet* rep_pkt = pkt.clone_with_phv_ptr().release();
action.action_fn(rep_pkt);
BMLOG_DEBUG_PKT(*rep_pkt, "Action {} applied to replicated packet",
action);
ReplicatedPktVec::instance().replicated_pkts_w_act_id.emplace_back(rep_pkt,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please use public methods, there is no need to expose the implementation internals of ReplicatedPktVec

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

action.action_fn.get_action_id());
}
}
}
assert(is_valid_mbr(mbr));

Expand Down Expand Up @@ -617,4 +647,18 @@ ActionProfile::choose_from_group(grp_hdl_t grp, const Packet &pkt) const {
return grp_selector->get_from_hash(grp, h);
}

std::vector<ActionProfile::mbr_hdl_t>
ActionProfile::get_all_mbrs_from_group(grp_hdl_t grp) const {
std::vector<ActionProfile::mbr_hdl_t> mbrs;
const GroupInfo& grp_info = grp_mgr.at(grp);
for(auto mbr = grp_info.begin(); mbr != grp_info.end(); ++mbr) {
mbrs.push_back(*mbr);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

isn't there a more elegant way of doing this, e.g., using std::vector::insert?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed

return mbrs;
}

void ActionProfile::set_selector_fanout(){
selector_fanout_enabled = true;
}

} // namespace bm
2 changes: 1 addition & 1 deletion src/bm_sim/match_tables.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ MatchTableAbstract::apply_action(Packet *pkt) {
Debugger::FIELD_ACTION, action_entry.action_fn.get_action_id());

BMLOG_DEBUG_PKT(*pkt, "Action entry is {}", action_entry);

action_entry.action_fn(pkt);

return next_node;
Expand Down Expand Up @@ -723,6 +722,7 @@ MatchTableIndirect::lookup(const Packet &pkt,
*next_node = (*hit) ?
get_next_node(entry.action_fn.get_action_id()) :
get_next_node_default(entry.action_fn.get_action_id());
ReplicatedPktVec::instance().set_next_nodes(this,*hit);
return entry;
}

Expand Down
14 changes: 14 additions & 0 deletions src/bm_sim/packet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,20 @@ Packet::clone_no_phv_ptr() const {
return clone_choose_context_ptr(cxt_id);
}

bool Packet::has_continue_node() const {
return continue_node.has_value();
}
const ControlFlowNode *Packet::get_continue_node() const {
return continue_node.value_or(nullptr);
}
void Packet::set_continue_node(const ControlFlowNode *node) {
continue_node = node;
}

void Packet::reset_continue_node() {
continue_node.reset();
}

/* Cannot get away with defaults here, we need to swap the phvs, otherwise we
could "leak" the old phv (i.e. not put it back into the pool) */

Expand Down
35 changes: 35 additions & 0 deletions src/bm_sim/pipeline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,15 @@ Pipeline::apply(Packet *pkt) {
BMLOG_DEBUG_PKT(*pkt, "Pipeline '{}': start", get_name());
const ControlFlowNode *node = first_node;
while (node) {
BMLOG_DEBUG_PKT(*pkt, "Pipeline '{}': after applying node '{}'",
get_name(), node->get_name());
if (pkt->is_marked_for_exit()) {
BMLOG_DEBUG_PKT(*pkt, "Packet is marked for exit, interrupting pipeline");
break;
}
node = (*node)(pkt);
BMLOG_DEBUG_PKT(*pkt, "Pipeline '{}': before applying node '{}'",
get_name(), node ? node->get_name() : "None");
}
BMELOG(pipeline_done, *pkt, *this);
DEBUGGER_NOTIFY_CTR(
Expand All @@ -50,4 +54,35 @@ Pipeline::apply(Packet *pkt) {
BMLOG_DEBUG_PKT(*pkt, "Pipeline '{}': end", get_name());
}

// Hao: placeholder for path permutation, just seems easier to have another apply here
// Should snapshot some metadata/reg states before calling this
void Pipeline::apply_continued(Packet *pkt) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it could be more readable / intuitive if this method was called apply_from, and took the "start" node as a parameter

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

renamed to apply_from_next_node. The node is carried in Packet as then it does not need to keep track of the mapping between pkt and the node.

const ControlFlowNode *cont_node = pkt->get_continue_node();
// pipeline_start is used for monitoring and debugging purposes, we can have ours as well
// but should change this anyways to differentiate between the two applies
BMELOG(pipeline_start, *pkt, *this);
// TODO(antonin)
// this is temporary while we experiment with the debugger
DEBUGGER_NOTIFY_CTR(
Debugger::PacketId::make(pkt->get_packet_id(), pkt->get_copy_id()),
DBG_CTR_CONTROL | get_id());
BMLOG_DEBUG_PKT(*pkt, "Pipeline '{}': continue from node '{}'",
get_name(), cont_node? cont_node->get_name() : "None");
const ControlFlowNode *node = cont_node;
while (node) {
if (pkt->is_marked_for_exit()) {
BMLOG_DEBUG_PKT(*pkt, "Packet is marked for exit, interrupting pipeline");
break;
}
node = (*node)(pkt);
}
// same
BMELOG(pipeline_done, *pkt, *this);
DEBUGGER_NOTIFY_CTR(
Debugger::PacketId::make(pkt->get_packet_id(), pkt->get_copy_id()),
DBG_CTR_EXIT(DBG_CTR_CONTROL) | get_id());
pkt->reset_continue_node();
BMLOG_DEBUG_PKT(*pkt, "Pipeline '{}': continue end", get_name());
}

} // namespace bm
Loading
Loading