Adds L2 mac-learning to L2Forward module #904
Conversation
New learn mode for the L2Forward module allows MAC address learning. The incoming packets are added to the L2 FIB with the source MAC address and incoming gate. Therefore multiple input gates are now allowed for this module, and should be used when using learn mode. The output gates used should mirror the same ID used for input gates for learn to work correctly. For example: tcam::L2Forward(learn=True) inaf = PortInc(port=p0) in86 = PortInc(port=p1) inaf -> 1:tcam in86 -> 2:tcam outaf = PortOut(port=p0) out86 = PortOut(port=p1) tcam:2 -> out86 tcam:1 -> outaf
Codecov Report
@@ Coverage Diff @@
## master #904 +/- ##
==========================================
+ Coverage 55.70% 55.78% +0.08%
==========================================
Files 9 9
Lines 1149 1149
==========================================
+ Hits 640 641 +1
+ Misses 509 508 -1
Continue to review full report at Codecov.
|
| @@ -84,6 +85,7 @@ class L2Forward final : public Module { | |||
| private: | |||
| struct l2_table l2_table_; | |||
| gate_idx_t default_gate_; | |||
| int learn_; | |||
| @@ -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()) { | |||
| if (learn_) { | ||
| // when learning mode is enabled, add source entry to table | ||
| // output gate == input gate mapping is assumed | ||
| l2_add_entry(&l2_table_, |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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.
|
Is this branch going to be merged or it is abandoned? |
|
I can have a look if it is still applicable and update/rebase it. I have not looked at this for a while. If memory serves me right the discussion on this and other transactional changes uncovered a couple of possible issues. |
|
That would be great. I was think about implementing learning capability for L2Forward before I stumbled upon this pull request. I would be happy to help. About the discussion relating to the transactions and thread safety, I guess using the suggested active/backup table would help with transactional part of the problem, but, the thread safety is still an issue unless there would be a separate table for each concurrent transaction and they merged to the active table. Which is a complicated solution. I guess locking mechanism is inevitable in this situation. |
|
I have looked through my notes and what I worked on at the time and the executive summary is "no, you cannot and this is a design limitation". As @sangjinhan noted, the changes to the forwarding table should be thread safe. While this is possible to be done using my transactional changes PR, it cannot be done out of a dataplane call sequence. The swap between active and backup table in that PR must be done from the control plane. That PR is also stale and I have to look at it, if it is still relevant. The right BESS-specific approach here would be to use the default rule feature in the L2Forward module and punt the packet which has no forwarding entry to userspace using the Unix Port module. That has vector IO now and can support substantial data rates (in the GB/s range). You then make the learning decision and you program it into the L2Forward module. If we want to have a learning module that is entirely in-dataplane, that will require it to be separate from the L2Forwarding module and have some facility in BESS to run module tasks periodically. Example:
Basically - a standard fast/slow path design. That will require periodic tasks for modules, something it did not have last time I looked at it. |
New learn mode for the L2Forward module allows MAC address learning. The
incoming packets are added to the L2 FIB with the source MAC address and
incoming gate. Therefore multiple input gates are now allowed for this
module, and should be used when using learn mode. The output gates used
should mirror the same ID used for input gates for learn to work
correctly. For example:
tcam::L2Forward(learn=True)
inaf = PortInc(port=p0)
in86 = PortInc(port=p1)
inaf -> 1:tcam
in86 -> 2:tcam
outaf = PortOut(port=p0)
out86 = PortOut(port=p1)
tcam:2 -> out86
tcam:1 -> outaf