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

Jump to bottom

Adds L2 mac-learning to L2Forward module #904

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

Adds L2 mac-learning to L2Forward module #904

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
1 change: 1 addition & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,4 @@ Please add your name to the end of this file and include this file to the PR, un
* Eran Gampel
* Tamás Lévai
* Matthew Mussomele
* Tim Rozet
10 changes: 10 additions & 0 deletions core/modules/l2_forward.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,9 @@ CommandResponse L2Forward::Init(const bess::pb::L2ForwardArg &arg) {
int ret = 0;
int size = arg.size();
int bucket = arg.bucket();
if (arg.learn()) {
Copy link
Member

Choose a reason for hiding this comment

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

learn_ = arg.learn()...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix

learn_ = 1;
}

default_gate_ = DROP_GATE;

Expand Down Expand Up @@ -597,6 +600,13 @@ void L2Forward::ProcessBatch(Context *ctx, bess::PacketBatch *batch) {
int ret = l2_find(&l2_table_,
*(snb->head_data<uint64_t *>()) & 0x0000ffffffffffff,
&out_gate);
if (learn_) {
// when learning mode is enabled, add source entry to table
// output gate == input gate mapping is assumed
l2_add_entry(&l2_table_,
Copy link
Member

Choose a reason for hiding this comment

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

One issue here is that this operation is not thread safe. Note that L2Forward module can be used by multiple worker threads; the table must be protected with proper synchronization mechanisms such as locks, RCU, transactional memory, etc. It has been fine without any synchronization, since all operations in ProcessBatch() were read-only, thus thread-safe.

Or we can simply give up concurrency by setting max_allowed_workers_ = 1, but this is not an option for this module...

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 I'll try to figure out the best way to protect the table. If you have a preferred method that is consistently used in this project for other modules let me know. I don't really understand what you mean by "It has been fine without any synchronization, since all operations in ProcessBatch() were read-only, thus thread-safe." - if I have a bunch of workers handling packets for different pmd ports, wouldn't there be a risk that both write new l2 entries at the same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kot-begemot-uk had the idea of using a backup/active table swap which he thinks would be safe and avoids having to use locks:
https://github.com/kot-begemot-uk/bess/blob/master/core/modules/l2_forward.cc#L721

@sangjinhan what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @trozet and @sangjinhan .

The code at https://github.com/kot-begemot-uk/bess/blob/master/core/modules/l2_forward.cc#L721 is still a PoC - I lost some of the error semantics there and it does not cover all the config methods. I would also like to make it fully transactional so if a particular add/delete fails it rolls back to a known state. Presently, the L2Forward module does not do that. If a request was to add 30 macs and it failed half the way through it will leave it in half-the-way through state.

If we use the active/backup method it should not be difficult and the memory cost for a generic compute system is insignificant.

*(snb->head_data<uint64_t *>(6)) & 0x0000ffffffffffff,
ctx->current_igate);
}
if (ret != 0) {
EmitPacket(ctx, snb, default_gate);
} else {
Expand Down
4 changes: 3 additions & 1 deletion core/modules/l2_forward.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,11 @@ struct l2_table {
class L2Forward final : public Module {
public:
static const gate_idx_t kNumOGates = MAX_GATES;
static const gate_idx_t kNumIGates = MAX_GATES;

static const Commands cmds;

L2Forward() : Module(), l2_table_(), default_gate_() {
L2Forward() : Module(), l2_table_(), default_gate_(), learn_() {
max_allowed_workers_ = Worker::kMaxWorkers;
}

Expand All @@ -84,6 +85,7 @@ class L2Forward final : public Module {
private:
struct l2_table l2_table_;
gate_idx_t default_gate_;
int learn_;
Copy link
Member

Choose a reason for hiding this comment

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

bool would be better :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, will fix

};

#endif // BESS_MODULES_L2FORWARD_H_
1 change: 1 addition & 0 deletions protobuf/module_msg.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,7 @@ message IPLookupArg {
message L2ForwardArg {
int64 size = 1; /// Configures the forwarding hash table -- total number of hash table entries.
int64 bucket = 2; /// Configures the forwarding hash table -- total number of slots per hash value.
bool learn = 3; /// Whether or not to activate learning mode.
}

/**
Expand Down