Vpn

[alexandergall]

apps/vpn/vpws.lua provides a L2 point-to-point VPN by encapsulating Ethernet frames in GRE over IPv6. To facilitate integration with a IPv6 transport network, apps/ipv6/ns_responer.lua implements an app that responds to IPv6 neighbor solicitations on the VPN "uplink".

Most of the code, however, deals with parsing and manipulating protocol headers. An OO approach appeared to be well suited for this, since most protocl headers can share a lot of code for basic functionality. The overhead in method calls and object creation may well turn out to be a no-go, but I'd like to see how far I can get with this. To implement inheritance, I have added a small OO framework in class.lua that others might want to use as well.

There aren't any selftests for the VPN code yet, because it's kind of hard to set up a meaningful test environment.

[agladysh]

Doh. Yet another OOP implementation.

Are you really sure this is needed here?

No offense, but to me it is a design smell...

[lukego]

The coding standard encourages experimentation and creative freedom when creating new apps, so adding a new OO framework is totally in the spirit of this.

This is the best pull request yet: adding useful functionality, beautifully documented, carefully structured into logical commits. I look forward to reading in more detail and helping to get it running fast :-)

[agladysh]

Luke, usually, I'm all in favor of experimentation and creative freedom.

However, adding an OO-framework on a global level may have longstanding consequences.

Since the framework is already there, it is too easy to just keep using it, without regard to applicability of OOP for a given task. We all have seen where this road leads to.

Furthermore, since the framework is already there at a global level (not at an application level), this framework will be implicitly made the "official" one, not an experimental one as it probably should be.

It is your call, of course, but — are you sure that you want to go there?

[alexandergall]

Can you please point me to the existing OO framework? I didn't mean to add "yet another one" for the heck of it. Also, please note that this is my first work in Lua, so I appreciate all feedback on style and technique, but please be gentle :)

[lukego]

I'd like to start by understanding the problem that Alex is solving, how he has structured his solution, and how we can help make his project a success. Then we can collectively also work on improving the "eventual consistency" of the code base.

[agladysh]

@alexandergall There is a ton of available OOP-frameworks in Lua — everyone who comes to Lua from OOP background makes it his duty to invent one.

My position — in general, not regarding your code — is that if one wants to do so much OOP in Lua that it needs a framework (even if it is a single class function), then that person must be doing something wrong in a way that is not natural to the language.

For example, I'm troubled by the fact that your framework has isa method. Why would this be ever needed in a language with duck-typing?

(Sorry if I sound acerbic, nothing personal.)

[agladysh]

@lukego Sounds reasonable. This was a knee-jerk reaction on my part to the big red flag of seeing class.lua in a pull request :-) Probably not a best thing to cultivate contributor base :-)

[agladysh]

@alexandergall Just in case, I have not made a single commit in this project myself, so listen to Luke, he is the maintainer. I'm just ranting in background :-)

[lukego]

It's still early in the project's life. Let's make a mess that's worth cleaning up :)

[agladysh]

@lukego Keep in mind, however, that a mess that is created by badly chosen OOP framework often can be cleaned up only by a rewrite from scratch. This is why I'm reacting this way :)

[lukego]

@agladysh I hope we will rewrite every line at least once in the next couple of years. "The best writing is rewriting" :-)

[alexandergall]

The fact that there is a ton of OOP-frameworks appears to me to be a flaw of Lua, really. Frankly, I don't care what we use, I just need simple inheritance. It's already there? Great, tell me how it works and I'll use it.

[lukego]

I love this design idea: to handle neighbor solicitation response as a small discrete app that "does one thing well" instead of adding it as a feature to the IPv6 next-hop-routing app.

[alexandergall]

In this case, the traffic going north->south is passed through untouched. Would it make sense to bypass the app in that direction to avoid the overhead?

[lukego]

I wonder about that too. I suppose it will be worth doing that kind of stuff if the performance difference is significant in practice, once we are done with optimization. I hope we will have the basic app transmit/receive functionality so fast that we don't need to care.

[agladysh]

@alexandergall Regarding that "flaw of Lua" remark — if you're interested in language design, I recommend you to read up a bit on Lua design and philosophy, then we will be able to discuss it further :-)

As for the way to do a "simple inheritance" in this project — this is a question for Luke as a maintainer, since it is largely a matter of taste in Lua.

[lukego]

I suspect there is an easy-to-optimize performance issue here. The syntax {...} is probably allocating a garbage-collectable table and the syntax function()...end is probably allocating a garbage-collectable closure and that's possibly expensive to do inside the main traffic loop.

If the profiler reveals that vows:push() is expensive (or that garbage collection is) then you might want to try creating the {...} table and closure in the initialization function and reuse it in push().

I could also be totally wrong on this guess, not having done any profiling :-). Do you have a reproducible benchmarking setup?

[agladysh]

I'd say that this should be benchmarked before refactoring. If this is compiled away, it should not generate much overhead. (But LuaJIT can be finicky in that matter...)

[alexandergall]

For this case, I think a simple benchmark could be created with Source->vpws->Sink because the actual contents of the packet is irrelevant for encapsulation.

[lukego]

True. And I suppose the decap could be tested with Source->vpws{encap}->vpws{decap}->Sink. Or perhaps even reducing the encap work with something like Source->vpws{encap}->Loop->vpws{decap}->Sink to encapsulate one batch of packets and then replay them without recreating them.

[alexandergall]

Can somebody please enlighten me how to make the profiler produce any output at all? Using -jp stalls the process and nothing works. strace shows

clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f2b01ef29f0) = ? ERESTARTNOINTR (To be restarted)
--- SIGPROF (Profiling timer expired) @ 0 (0)

When I use a bigger sampling interval, e.g. i100, the app runs but I get no output from the profiler.

[alexandergall]

The issue with the hung process is not there on chur (which I'd also like to understand), but the profiler still doesn't produce any output:

gall@chur:~/projects/snabbswitch/src$ sudo ./snabbswitch -jp test.vpws-perf
link report
source.tx->vpn.customer 100,000 packet(s) transmitted
vpn.uplink->sink.rx 100,000 packet(s) transmitted

Must be something silly...

[lukego]

Are you calling os.exit() or otherwise terminating the process before it manages to terminate?
(How about if you use -t before test.vpws-perf? the profiler stuff is in main.lua and related to the command line.)

[alexandergall]

Ok, using require("jit.p").start() in my code works. Just not on the command line. Oh, well.

[lukego]

Could be that you're using an older version of master as the basis and it has bugs?
Soon time to start thinking about creating integration branches as stable points to develop against..

[alexandergall]

No, I have merged with master not long ago (unless my git foo failed me ;) But I'm happy for now with this workaround.

[lukego]

(wow my Macbook is amazingly determined to auto-correct the words encap->encamp and decap->recap! probably time to turn that feature off...)

[alexandergall]

@agladysh: I have no intention to enter that particular rat-hole :) But I'm all ears to learn how to do this with the existing framework.

[agladysh]

@alexandergall Simplest possible inheritance in Lua is implemented by setting __index metamethod to the parent (or rather prototype) object. You can see that, among other things, being done in your class.lua. No frameworks needed to do that, just a setmetatable call.

Everything above that is optional and a matter of taste.

I suspect that Luke at this point is fine with your implementation — he is right about this being early days. But, just for fun, I will look closely at your code tonight and maybe come up with something more to my own liking (FWIW).

[lukego]

I like bitfields in C structs. I think they are currently an "NYI" in LuaJIT though: that the JIT doesn't understand them and as a consequence code that uses them will be interpreted instead of JIT'd. (If that's true you would see an NYI warning in the JIT trace from -jdump.)

We should see about getting support for bitfields in LuaJIT, either by asking Mike Pall or hacking it ourselves (it will be awesome if we can develop LuaJIT-hacking expertise of our own).

[agladysh]

It would be awesome indeed. But we need to coordinate it with Mike, so it would not become a forking experience instead of hacking one — which would be a bit less awesome, I believe :-)

[lukego]

Indeed.
I asked Mike about this when he gave his talk in Zurich: How can Snabb Switch contribute to LuaJIT?
He said he'd appreciate more people understanding the LuaJIT internals and being able to contribute code. He's already pretty satisfied in terms of creating consulting work and getting LuaJIT into widespread use.

[agladysh]

What, Mike gave a talk somewhere? Why did I miss that? :-( Is there a link or something?

[alexandergall]

Ah, good you mention this. I spent some time pondering how to do this right. Bit fields are compiler-dependent by nature, which makes it hard to write portable code. OTOH, we're bound to a specific compiler, so this is probably not much of an issue. Endianness with bit fields is a PITA, though and I believe that there are some limitations on the way those fields can (or rather can't) cross certain byte-boundaries.

So, I decided to manage bit fields with the bit library (which is supported by JIT, I believe), see core.lib.bitfield(). I obviously forgot the stuff in na.lua :) I'd be interested in what you think about this approach.

[lukego]

Yep! Mike stepped briefly out of the shadows last October and gave a surprise three-hour talk about LuaJIT internals at the Zurich IT Geeks meetup group: http://www.meetup.com/zhgeeks/events/141272252/
He mostly walked through code examples and showed how they are compiled and why. There is alas no video available and only scant notes published on the meetup page.
It was a great evening, but don't worry, nobody became an instant expert on LuaJIT internals from being there :-)

[lukego]

Hi again!

I'd like to get this code merged in so that it can be maintained together with the rest of the repository (it's an undue extra maintenance burden on you when it's out-of-tree and the other code is changing).

Could you possibly pull the class/datagram/etc code into the app/ directory? I'd like to make the core/ and lib/ trees small and conservative for now and keep higher-level frameworks inside the apps. This will mean more code in app/ directories in the short term but it means app authors can experiment with any programming style they want without standing on each others' toes.

[lukego]

On reflection I agree that the structure you have now with code in lib/ is better than having everything in app/. I would like to merge this code soon. Does this pull request contain the latest?

[alexandergall]

OK :) the latest commit is alexandergall@0e3116f
It contains some performance improvements and bug fixes. class.lua should probably be moved to some other place, I guess.

[lukego]

Could you perhaps move class.lua somewhere suitable (in lib/ ?) and send a new pull request?

[alexandergall]

Yes, but this will take some time. I just merged with your master and realized that there are substantial changes in the app API and my code no longer runs. This is not the place to discuss this, but I'm hitting a wall right from the start, because config.app() requires the arg to the class' new() method to be a string, but some of my classes expect multiple arguments :(

[lukego]

The easy fix is to pass a string and then parse it as Lua. That's what the ipv6 app does.

The benefit of moving to strings is that it will be possible to load new app networks into a running system with minimal disruption. (The app object won't be recreated if its configuration string is the same.)

[alexandergall]

OK, got it.

[alexandergall]

I'm just about ready to send a new pull request. Regarding class.lua, I propose to put it in lib/lua/class.lua and put the corresponding require in core/main.lua. I thought that maybe strict.lua could be put in lib/lua as well and maybe other similart stuff in the future. What do you think?

[lukego]

Yes, that sounds reasonable.

(I'm also not really sure if the current core/lib/app/design directory layout really makes sense, would be interesting to think about better alternatives.)

[alexandergall]

On Thu, Apr 10, 2014 at 9:19 PM, Javier notifications@github.com wrote:

for lib/lua you mean the system Lua libraries? or a lua/ subdir inside
snabbswitch's src/lib/ dir? If the former, I'd advise against it.
Snabbswitch should be self-contained... we're including LuaJIT itself for a
readon. If the latter, then I don't get why a lua/ subdir. wouldn't it be
just src/lib/class.lua ? after all, almost everything is Lua here, isn't
it?

I mean a directory in Snabb's src/lib/. The idea was to put stuff there
that has something to do with the way we use Lua for this project rather
than something related to a specific feature like what's in src/lib now. I
proposed to move strict.lua to that place as well.

Alex

[lukego]

This assertion is failing in selftest for me. I think the reason is that open() doesn't get called. Adding the open() call to new() looks like it would break RawVhost elsewhere.

[alexandergall]

That was an ill-fated attempt to subclass TapVhost in a simple-minded manner. I don't want to rewrite it and it's probably best to remove the entire RawVhost thing. I never used it anyway (RawSocket, OTOH, is useful for debugging, because you can attach wireshark to such an interface).

[lukego]

I'm ready to merge if you can:

• Tidy up the commit history (git rebase -i).
• Get "make test" passing on chur.
• Leave out the intel10g MTU change from this pull request (save it for after we get in Javier's new code).

[lukego]

Howdy!

To clarify what I meant when requesting to git rebase -i: I'd like the commit list to please reflect the final result and not the intermediate steps. So commits that all update the same bit of code - e.g. first draft, debug, optimize, etc - can be squashed into a single commit for merging.

optional bonus points to split logically separate features into separate commits: socket I/O, vhost changes, etc but if that's too much work then just squash everything into one commit (minus the intel driver changes please because that's going to conflict with Javier's big driver changes that are also coming in now).

[alexandergall]

Ok. Give me a minute to get acquainted with git rebase :)

[lukego]

Here's a possibly handy link: https://help.github.com/articles/interactive-rebase

[alexandergall]

Rebasing into separate commits would be way too complicated due to my rather badly organized commits and the fact that I merged twice with master. Sqashing everything into a single commit is ugly, I agree. I have decided to re-play my stuff onto a fresh branch of the current master in a better organize manner and post a new PR for that and close this PR.

[lukego]

Thanks for making the effort!

By the way: This is the document that made Git 'click' for me, after years of confusion and annoyance: http://ftp.newartisans.com/pub/git.from.bottom.up.pdf