diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 77914f29b2..bd4a20d245 100644 --- a/CONTRIBUTING.md +++ b/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. diff --git a/src/README.md b/src/README.md index 911ebf501f..7ad5b88068 100644 --- a/src/README.md +++ b/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*. diff --git a/src/apps/bridge/base.lua b/src/apps/bridge/base.lua index be320c8a2d..284cef64c7 100644 --- a/src/apps/bridge/base.lua +++ b/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] diff --git a/src/apps/vhost/vhost_user.lua b/src/apps/vhost/vhost_user.lua index 7890b495f5..752dfb5142 100644 --- a/src/apps/vhost/vhost_user.lua +++ b/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( diff --git a/src/core/app.lua b/src/core/app.lua index c4fb94836c..619e4fd78e 100644 --- a/src/core/app.lua +++ b/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". diff --git a/src/core/memory.lua b/src/core/memory.lua index 046eaa3091..ecfba556e1 100644 --- a/src/core/memory.lua +++ b/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 diff --git a/src/lib/hardware/pci.lua b/src/lib/hardware/pci.lua index 9f3ec2a6aa..de34a4d5d7 100644 --- a/src/lib/hardware/pci.lua +++ b/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() diff --git a/src/lib/virtio/net_device.lua b/src/lib/virtio/net_device.lua index 9c93e66fbb..be06069833 100644 --- a/src/lib/virtio/net_device.lua +++ b/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)