initial import of packetblaster lwaftr

[mwiget]

This is a first working prototype of a lwaftr specific IPv4 and IPv6 traffic generator with the following features, for which I'm seeking feedback from the community in general and @kbara, @dpino and @wingo in particular.

• Refactored packetblaster synth and replay into their own directories.
  Send IPv4 and IPv6 traffic concurrently to fully test and report on a LWAFTR "on a stick design"
• Traffic sent either on a PCI port or Linux tap interface
• Simulates any number of B4 clients
• Rate limit generated traffic in MPPS (fractions are fine)
• Report separately and combined on incoming IPv4 and IPv6 traffic in MPPS, Gbps and packet loss
• Report on incoming packet loss by calculating the delta of the embedded packet counter, separately for IPv4 and IPv6 (allowing for separate sender and receiver applications).

See https://github.com/mwiget/snabbswitch/tree/packetblaster-lw4o6/src/program/packetblaster/lwaftr for a detailed description.

Some topics I'm not sure about yet:

• Would it make sense to integrate this tighter with lwaftr, e.g. by importing lwutil.lua or leverage the protocol libraries for udp and datagram?
• Performance seems suboptimal. It max's out at roughly 5 MPPS on receiving. Maybe its just a matter of triggering jit.flush() or there are some low hanging fruits to optimise.

[dpino]

It seems there's no support of VLAN tags. I would like to have it as it's something we use often.

Since you're defining ethernet_header, one way of implementing support of VLAN tags is to change the definition of the ethernet header if VLAN tags are used. In that case, ether_header_ptr_type will point to the definition of a 802.1Q header. See https://github.com/Igalia/snabbswitch/blob/lwaftr_starfruit/src/apps/lwaftr/generator.lua#L103

Same goes for the hardcoded offsets. In case of supporting VLAN tags, offsets should change (14 turns into 16, 34 (20 + 14) turns into 36, etc). As for me I prefer to use constants instead of hardcoded values since it makes easier to understand the meaning of a value (ipv4_header_size + ethernet_header_size instead of 34).

[mwiget]

Many thanks for your detailed feedback. I'll go thru all of them and make the required changes. On VLAN support I'm wondering if I could simply leverage VMDq instead of handling VLANs in software. One drawback is of course the lack of VLAN support on tap interface. On the other hand, my intention with the tap interface was simply a method to make the packets visible via tcpdump if needed.

[mwiget]

Hmm. VMDq is probably not a good idea, because it requires to register with a mac address unless a VLAN is given. Just thinking out loud here: maybe VMDq can still be used with VLAN tag, but must be disabled for untagged traffic. Setting MAC address is an issue for the single stick design, which can have two different destination MAC's for v4 and v6 traffic returned from LWAFTR.

[mwiget]

Went with VMDq set to true when VLAN is given. Its an efficient way to add VLAN support without changing the source code: VLAN handling is done by the Intel NIC. That obviously doesn't work for tap interfaces and I don't have a warning about it just yet.

[dpino]

Looking good. Supporting VLAN tags is the main concern to me.

[dpino]

@mwiget thanks for tackling the issues. If VLAN tagging can be managed by VMDq it works for me too.

[dpino]

Wrong error message (copy-paste error I believe).

[mwiget]

argh and thanks ;-)

[mwiget]

I think I fixed all open issues so far, removing the [wip]. Happy to make more changes of course.

[dpino]

As for me, no more changes required. LGTM.

[eugeneia]

@mwiget I am not a big fan of the code duplication here. Could the shared code below be put into a shared function?

[mwiget]

@eugeneia: Hmm, that was a result of moving synth and replay into their own directories, so they are in line with the added lwaftr. Would it make more sense to leave the original packetblaster.lua as is and simply enhance its logic to also allow commands taken from subdirectories? Kind of having synth and replay be handled within packetblaster.lua (as before this pull) and only add lwaftr/lwaftr.lua?
Or do you have something different in mind?

[eugeneia]

@mwiget With the risk of being super annoying: the code before and after uses of config_loadgen is identical in both cases as far as I can tell. How do you feel about moving that code into config_loadgen and rename it to run_loadgen?

E.g. instead of

   local patterns = args
   local nics = packetblaster.config_loadgen(c, patterns)
   assert(nics > 0, "<PCI> matches no suitable devices.")
   engine.busywait = true
   intel10g.num_descriptors = 32*1024
   engine.configure(c)
   local t = timer.new("report", packetblaster.report, 1e9, 'repeating')
   timer.activate(t)
   if duration then engine.main({duration=duration})
   else             engine.main() end

we would have

packetblaster.run_loadgen(c, args, duration)

[mwiget]

@eugeneia Ah, now I get it. Did as suggested and it does indeed look cleaner. Made a few more commits to address VLAN support without VMDq and creation of a pcap file plus selftest the pcap file.

[plajjan]

@mwiget What is WIP about this one? I'm looking at making additions/modifications to packetblaster and your code seems like a better foundation than what we currently have on master.

[mwiget]

@plajjan I wanted to add some more test cases and I have a change on reporting packet loss in % instead of absolute numbers. So nothing major. I should take the WIP away again.

[plajjan]

Would it make sense to integrate this tighter with lwaftr, e.g. by importing lwutil.lua or leverage the protocol libraries for udp and datagram?

@mwiget I think not. It's kind of nice with a second implementation from a testing perspective. One could argue this is just for performance testing and that such validation could be performed by another test app but.. why? Better that that the test apps share code but we have to make sure we validate the program itself and if we share code with the lwAFTR how can we do that?

I noticed you have 2 space indent. I think Snabb typically uses 3 space indent, no? (Do we have a style guide?)

[kbara]

It does indeed usually use a 3 space indent.

[dpino]

@plajjan That's correct, the convention is to use 3-spaces indention. With regard to a style guideline I remember reading the codebase follows the Lua Style Guideline http://lua-users.org/wiki/LuaStyleGuide. I remember reading also the rule of thumb is to write code in the same style as the code that it has been written so far. I think those notes were on the "size budget" page in the Wiki, but I cannot find it anymore cc @lukego

[mwiget]

@plajjan, @kbara and @dpino: didn't even notice and my .vimrc did technically make the mistake ;-), but I fixed it now.