Merge max-next-v2016.07-1 into next

CONTRIBUTING.md

@@ -1,18 +1,30 @@
-Whenever you create a Pull Request (PR), a maintainer will self-assign
+# Hints for Contributors
+
+* Whenever you create a Pull Request (PR), a maintainer will self-assign
 themselves as the *upstream*. The upstream decides if your PR is accepted and
 might require you to amend additional changes before merging. You can expect
 the upstream to communicate clearly if there are any issues preventing your PR
 from being merged, and how they can be fixed. Once merged, the upstream will
-add the `merged` label to your PR.
+add the `merged` label to your PR. More on [our Git workflow](https://github.com/snabbco/snabb/blob/master/src/doc/git-workflow.md).
+
+* Target your PR against the branch you would like it to be merged into. Refer
+to the [list of subsystem branches](https://github.com/snabbco/snabb/blob/master/src/doc/branches.md).
+When in doubt use `master`.
 
-You are also welcome to submit PRs you would like to receive feedback on, but
+* Feel free to @ping the assignee if you feel like your PR has been overlooked,
+and are waiting for a response.
+
+* You are also welcome to submit PRs you would like to receive feedback on, but
 which are not ready to be merged: include the labels `[wip]` in the title of
 PRs that require further work, and `[sketch]` for PRs that are not meant to be
 merged at all.
 
-Our [Documentation Guide](https://github.com/SnabbCo/snabbswitch/blob/master/src/doc/documentation-guide.md)
+* Please make sure your editor is configured to not emit tabs and use three
+spaces for indentation.
+
+* Our [Documentation Guide](https://github.com/SnabbCo/snabbswitch/blob/master/src/doc/documentation-guide.md)
 gives pointers on how to contribute to the project's documentation.
 
-If you wish to record a copyright notice with your contribution then you can
+* If you wish to record a copyright notice with your contribution then you can
 optionally do this in the file `src/COPYRIGHT`; copyright notices in other
 files will be rejected.

src/README.md

@@ -99,6 +99,18 @@ Tables of named input and output links.  These tables are initialized by
 the engine for use in processing and are *read-only*.
 
 
+— Field **myapp.appname**
+
+Name of the app. *Read-only*.
+
+
+— Method **myapp:link**
+
+*Optional*. Called any time the app’s links may have been changed (including on
+start-up). Guaranteed to be called before `pull` and `push` are called with new
+links.
+
+
 — Method **myapp:pull**
 
 *Optional*. Pull packets into the network.
@@ -376,10 +388,13 @@ can be accessed directly by network cards. The important
 characteristic of DMA memory is being located in contiguous physical
 memory at a stable address.
 
-— Function **memory.dma_alloc** *bytes*
+— Function **memory.dma_alloc** *bytes*, *[alignment]*
 
 Returns a pointer to *bytes* of new DMA memory.
 
+Optionally a specific *alignment* requirement can be provided (in
+bytes). The default alignment is 128.
+
 — Function **memory.virtual_to_physical** *pointer*
 
 Returns the physical address (`uint64_t`) the DMA memory at *pointer*.

src/apps/bridge/base.lua

@@ -26,12 +26,6 @@
 -- can access the configuration via self._conf.config.  If config is
 -- not set, it is initialiezed to an empty table.
 --
--- Note that it is necessary to call the method post_config() after
--- the app has been configured with engine.configure() to complete the
--- initialization.  This step will add the ringbuffers associated with
--- the ports to an internal data structure to save a lookup in the
--- input and output tables during packet processing.
---
 -- To make processing in the fast path easier, each port and group is
 -- assigned a unique integer greater than zero to serve as a "handle".
 -- The group handle 0 is assigned to all free ports.
@@ -149,7 +143,7 @@ end
 -- accessible via the keys l_in and l_out, respectively.  This helps
 -- to speed up packet forwarding by eliminating a lookup in the input
 -- and output tables.
-function bridge:post_config ()
+function bridge:link ()
    assert(self.input and self.output)
    for _, port in ipairs(self._ports) do
       port.l_in = self.input[port.name]

src/apps/vhost/vhost_user.lua

@@ -176,13 +176,49 @@ function VhostUser:set_features (msg)
    self.dev:set_features(features)
 end
 
+-- Feature cache: A kludge to be compatible with a "QEMU reconnect" patch.
+--
+-- QEMU upstream (circa 2015) does not support the vhost-user device
+-- (Snabb) reconnecting to QEMU. That is unfortunate because being
+-- able to reconnect after a restart of either the Snabb process or
+-- simply a vhost-user app is very practical.
+--
+-- Reconnect support can however be enabled in QEMU with a small patch
+-- [1]. Caveat: Feature negotiation does not work reliably on the new
+-- connections and may provide an invalid feature list. Workaround:
+-- Cache the most recently negotiated features for each vhost-user
+-- socket and reuse those when available.
+--
+-- This is far from perfect but it is better than nothing.
+-- Reconnecting to QEMU VMs is very practical and enables faster
+-- development, restart of the Snabb process for recovery or upgrade,
+-- and stop/start of vhost-user app instances e.g. due to
+-- configuration changes.
+--
+-- QEMU upstream seem to be determined to solve the reconnect problem
+-- by requiring changes to the guest drivers so that the device could
+-- request a reset. However, this has the undesirable properties that
+-- it will not be transparent to the guest and nor will it work on
+-- existing guest drivers.
+--
+-- And so for now we have this cache for people who want to patch
+-- reconnect support into their QEMU...
+--
+-- 1: QEMU patch:
+--   https://github.com/SnabbCo/qemu/commit/f393aea2301734647fdf470724433f44702e3fb9.patch
+
+-- Consider using virtio-net feature cache to override negotiated features.
 function VhostUser:update_features (features)
    local stat = syscall.stat(self.socket_path)
    local mtime = ("%d.%d"):format(tonumber(stat.st_mtime),
                                   tonumber(stat.st_mtime_nsec))
    local cachepath = "/tmp/vhost_features_"..string.gsub(self.socket_path, "/", "__")
    local f = io.open(cachepath, 'r')
-   if f then
+   -- Use cached features when:
+   --   Negotiating features for the first time for this app instance
+   --   Cache is populated
+   --   QEMU vhost-user socket file has same timestamp as cache
+   if not self.have_negotiated_features and f then
       local file_features, file_mtime = f:read('*a'):match("features:(.*) mtime:(.*)\n")
       f:close()
       if file_mtime == mtime then
@@ -193,6 +229,12 @@ function VhostUser:update_features (features)
          print(("vhost_user: Skipped old feature cache in %s"):format(cachepath))
       end
    end
+   -- Features are now negotiated for this app instance. If they are
+   -- negotiated again it will presumably be due to guest driver
+   -- restart and in that case we should trust the new features rather
+   -- than overriding them with the cache.
+   self.have_negotiated_features = true
+   -- Cache features after they are negotiated
    f = io.open(cachepath, 'w')
    if f then
       print(("vhost_user: Caching features (0x%s) in %s"):format(

src/core/app.lua

@@ -152,8 +152,8 @@ end
 -- Update the active app network by applying the necessary actions.
 function apply_config_actions (actions, conf)
    -- The purpose of this function is to populate these tables:
-   local new_app_table,  new_app_array  = {}, {}, {}
-   local new_link_table, new_link_array = {}, {}, {}
+   local new_app_table,  new_app_array  = {}, {}
+   local new_link_table, new_link_array = {}, {}
    -- Temporary name->index table for use in link renumbering
    local app_name_to_index = {}
    -- Table of functions that execute config actions
@@ -229,9 +229,13 @@ function apply_config_actions (actions, conf)
    for linkspec, r in pairs(link_table) do
       if not new_link_table[linkspec] then link.free(r, linkspec) end
    end
-   -- commit changes
+   -- Commit changes.
    app_table, link_table = new_app_table, new_link_table
    app_array, link_array = new_app_array, new_link_array
+   -- Trigger link event for each app.
+   for _, app in ipairs(app_array) do
+      if app.link then app:link() end
+   end
 end
 
 -- Call this to "run snabb switch".

src/core/memory.lua

@@ -23,13 +23,20 @@ chunks = {}
 
 -- Allocate DMA-friendly memory.
 -- Return virtual memory pointer, physical address, and actual size.
-function dma_alloc (bytes)
+function dma_alloc (bytes,  align)
+   align = align or 128
    assert(bytes <= huge_page_size)
-   bytes = lib.align(bytes, 128)
-   if #chunks == 0 or bytes + chunks[#chunks].used > chunks[#chunks].size then
+   -- Get current chunk of memory to allocate from
+   if #chunks == 0 then allocate_next_chunk() end
+   local chunk = chunks[#chunks]
+   -- Skip allocation forward pointer to suit alignment
+   chunk.used = lib.align(chunk.used, align)
+   -- Need a new chunk to service this allocation?
+   if chunk.used + bytes > chunk.size then
       allocate_next_chunk()
+      chunk = chunks[#chunks]
    end
-   local chunk = chunks[#chunks]
+   -- Slice out the memory we need
    local where = chunk.used
    chunk.used = chunk.used + bytes
    return chunk.pointer + where, chunk.physical + where, bytes

src/lib/hardware/pci.lua

@@ -202,6 +202,7 @@ function selftest ()
    assert(qualified("0000:01:00.0") == "0000:01:00.0", "qualified 1")
    assert(qualified(     "01:00.0") == "0000:01:00.0", "qualified 2")
    assert(qualified(     "0a:00.0") == "0000:0a:00.0", "qualified 3")
+   assert(qualified(     "0A:00.0") == "0000:0A:00.0", "qualified 4")
    assert(canonical("0000:01:00.0") ==      "01:00.0", "canonical 1")
    assert(canonical(     "01:00.0") ==      "01:00.0", "canonical 2")
    scan_devices()

src/lib/virtio/net_device.lua

@@ -306,7 +306,16 @@ function VirtioNetDevice:tx_buffer_add_mrg_rxbuf(tx_p, addr, len)
       self.tx.finished = true
    end
 
-   return to_copy
+   -- XXX The "adjust" is needed to counter-balance an adjustment made
+   -- in virtq_device. If we don't make this adjustment then we break
+   -- chaining together multiple buffers in that we report the size of
+   -- each buffer (except for the first) to be 12 bytes more than it
+   -- really is. This causes the VM to see an inflated ethernet packet
+   -- size which may or may not be noticed by an application.
+   --
+   -- This formulation is not optimal and it would be nice to make
+   -- this code more transparent. -luke
+   return to_copy - adjust
 end
 
 function VirtioNetDevice:tx_packet_end_mrg_rxbuf(header_id, total_size, tx_p)