sliver-openvswitch.git
12 years agoconfigure: Run sparse automatically if C=1 specified on "make" command.
Ben Pfaff [Fri, 6 May 2011 20:00:49 +0000 (13:00 -0700)]
configure: Run sparse automatically if C=1 specified on "make" command.

The C=1 convention matches the kernel's convention, so running "make C=1"
will now get sparse results for both userspace and kernel compiles.

12 years agoMake the source tree sparse clean.
Ben Pfaff [Fri, 6 May 2011 19:59:51 +0000 (12:59 -0700)]
Make the source tree sparse clean.

With this commit, the tree compiles clean with sparse commit 87f4a7fda3d
"Teach 'already_tokenized()' to use the stream name hash table" with patch
"evaluate: Allow sizeof(_Bool) to succeed" available at
http://permalink.gmane.org/gmane.comp.parsers.sparse/2461 applied, as long
as the "include/sparse" directory is included for use by sparse (only),
e.g.:
     make CC="CHECK='sparse -I../include/sparse' cgcc"

12 years agotest-sha1: Suppress sparse warning.
Ben Pfaff [Fri, 6 May 2011 19:39:27 +0000 (12:39 -0700)]
test-sha1: Suppress sparse warning.

Otherwise sparse warns: warning: memset with byte count of 1000000

12 years agoFix up usage of flow_wildcards_t.
Ben Pfaff [Fri, 6 May 2011 19:38:20 +0000 (12:38 -0700)]
Fix up usage of flow_wildcards_t.

The flow_wildcards_t type is defined as a distinct type from sparse's
perspective (with __attribute__((bitwise))) so that we don't accidentally
mix it with only-partially-compatible OFPFW_* flags.  But we were weren't
using it quite right in a few plces.  This fixes it up.

12 years agocsum: Annotate byte order to match actual usage.
Ben Pfaff [Fri, 6 May 2011 19:34:46 +0000 (12:34 -0700)]
csum: Annotate byte order to match actual usage.

The IP checksum algorithm yields identical results regardless of whether
arithmetic little-endian or big-endian, but in practice OVS only passes in
big-endian data, so it seems reasonable to annotate these functions that
way.

12 years agoutil: Suppress build assertions when building with sparse.
Ben Pfaff [Fri, 6 May 2011 18:43:04 +0000 (11:43 -0700)]
util: Suppress build assertions when building with sparse.

sparse simply doesn't like our build assertions on packed structures.
It seems that its ideas about struct packing are different from GCC's:

../lib/cfm.h:50:1: error: invalid bitfield width, -1.
../lib/packets.h:206:1: error: invalid bitfield width, -1.
../lib/packets.h:213:1: error: invalid bitfield width, -1.
../lib/packets.h:367:1: error: invalid bitfield width, -1.

sparse isn't generating code so we don't really care how it lays out
structures.  We might as well just skip the assertions, as done here.

12 years agoSuppress sparse warnings for global variables initialized in headers.
Ben Pfaff [Fri, 6 May 2011 18:38:19 +0000 (11:38 -0700)]
Suppress sparse warnings for global variables initialized in headers.

sparse warns if a non-static variable with external linkage has an
initializer at first declaration, because it suspects that it should be
static instead.  Generally it's correct, but not in these cases, so add
a redundant declaration to suppress the warning.

The suppress warnings look like:
../ofproto/connmgr.c:40:1: warning: symbol 'VLM_connmgr' was not declared. Should it be static?
../ofproto/collectors.c:31:1: warning: symbol 'vlog_module_ptr_collectors' was not declared. Should it be static?
../ofproto/connmgr.c:43:1: warning: symbol 'counter_ofconn_stuck' was not declared. Should it be static?

12 years agocompiler: Suppress sparse complaints about function attributes.
Ben Pfaff [Fri, 6 May 2011 18:34:59 +0000 (11:34 -0700)]
compiler: Suppress sparse complaints about function attributes.

GCC allows __attribute__s to be included in function prototypes and
then omitted later on the function definition, but sparse complains about
this.  Furthermore, sparse doesn't like the placement of the __attribute__s
that we tend to use in OVS.

I don't see any value in "fixing" these to suit sparse so it seems better
to just omit them.

12 years agoFix incorrect byte order annotations.
Ben Pfaff [Fri, 6 May 2011 18:27:05 +0000 (11:27 -0700)]
Fix incorrect byte order annotations.

These are not actual bugs, just deceptive use of the wrong function or
type.

Found by sparse.

12 years agoopenflow: Change types from uint<N>_t to ovs_be<N>.
Ben Pfaff [Wed, 4 May 2011 21:03:43 +0000 (14:03 -0700)]
openflow: Change types from uint<N>_t to ovs_be<N>.

I've been reluctant in the past to make wholesale changes to openflow.h
because it would be a divergence from upstream that would make comparisons
and merges more difficult.  But, in practice, no one does such comparisons
and no merges happen (because OpenFlow 1.0 is not changing).  I'd still be
inclined to resist, except that in this series I'm adding actual checking
for byte order conventions (as opposed to just documentation).

12 years agoConvert remaining network-byte-order "uint<N>_t"s into "ovs_be<N>"s.
Ben Pfaff [Tue, 29 Mar 2011 21:42:20 +0000 (14:42 -0700)]
Convert remaining network-byte-order "uint<N>_t"s into "ovs_be<N>"s.

I looked at almost every uint<N>_t in the tree to determine whether it was
really in network byte order, and converted the ones that were.

The only remaining ones, modulo my mistakes, are in openflow.h.  I'm not
sure whether we should convert those, because there might be some value
in remaining close to upstream for this header.

12 years agoofproto: Maintain ofp_phy_port for each ofport in network byte order.
Ben Pfaff [Tue, 29 Mar 2011 21:11:39 +0000 (14:11 -0700)]
ofproto: Maintain ofp_phy_port for each ofport in network byte order.

It's rather confusing to have an instance of a whole structure in an
unexpected byte order.  This commit gets rid of that oddity.

12 years agoConsistently write null pointer constants as NULL instead of 0.
Ben Pfaff [Wed, 4 May 2011 20:49:42 +0000 (13:49 -0700)]
Consistently write null pointer constants as NULL instead of 0.

Found with sparse.

12 years agoofproto: Drop duplicate "const" in parameter declaration.
Ben Pfaff [Wed, 4 May 2011 21:45:31 +0000 (14:45 -0700)]
ofproto: Drop duplicate "const" in parameter declaration.

Found by sparse.

12 years agoAdd missing "static" keywords.
Ben Pfaff [Wed, 4 May 2011 20:58:10 +0000 (13:58 -0700)]
Add missing "static" keywords.

Found by sparse.

12 years agoRemove unnecessary #include directives.
Ben Pfaff [Fri, 6 May 2011 18:40:26 +0000 (11:40 -0700)]
Remove unnecessary #include directives.

12 years agoofproto: Fix ofproto_send_packet() treatment of vlan_tci parameter.
Ben Pfaff [Wed, 4 May 2011 22:44:51 +0000 (15:44 -0700)]
ofproto: Fix ofproto_send_packet() treatment of vlan_tci parameter.

ofproto_send_packet() seems very confused about whether vlan_tci is in
host or network byte order.  But the callers always pass 0 anyway, so we
might as well remove it.

12 years agostream-ssl: Fix call to accept().
Ben Pfaff [Wed, 4 May 2011 22:46:27 +0000 (15:46 -0700)]
stream-ssl: Fix call to accept().

GCC and glibc conspire to allow struct sockaddr_in * to be passed in
place of struct sockaddr *, but that's non-standard and we're better
off not taking advantage of it.

Found by sparse.

13 years agotests: Check ovs-openflowd log output instead of ignoring it.
Ben Pfaff [Mon, 2 May 2011 19:52:56 +0000 (12:52 -0700)]
tests: Check ovs-openflowd log output instead of ignoring it.

ovs-openflowd outputs a number of log messages that we don't want to
suppress.  We do want to know if it logs anything that we don't expect.
So this commit starts checking the log output, discarding any normal,
expected messages.

Reviewed-by: Simon Horman <horms@verge.net.au>
13 years agonetdev-linux: Open AF_PACKET socket only when it is needed.
Ben Pfaff [Mon, 2 May 2011 16:53:31 +0000 (09:53 -0700)]
netdev-linux: Open AF_PACKET socket only when it is needed.

Only a privileged process can open a raw AF_PACKET socket, so netdev-linux
will fail to initialize if run as non-root and you get a cascade of error
messages, like this:

netdev_linux|ERR|failed to create packet socket: Operation not permitted
netdev|ERR|failed to initialize system network device class: Operation not permitted
netdev|ERR|failed to initialize internal network device class: Operation not permitted
netdev|ERR|failed to initialize tap network device class: Operation not permitted

But in fact the AF_PACKET socket is not needed for most operations (only
for sending packets) and it is never needed for testing with the "dummy"
datapath and network device, so we can avoid logging all of these errors
by opening the packet socket only on demand, as this commit does.

Reviewed-by: Simon Horman <horms@verge.net.au>
13 years agonetdev-linux: Only call set_nonblocking() if socket creation succeeds.
Ben Pfaff [Fri, 29 Apr 2011 22:53:36 +0000 (15:53 -0700)]
netdev-linux: Only call set_nonblocking() if socket creation succeeds.

Reviewed-by: Simon Horman <horms@verge.net.au>
13 years agoofp-util: Revise OpenFlow 1.0 ofp_match normalization.
Ben Pfaff [Mon, 2 May 2011 18:04:33 +0000 (11:04 -0700)]
ofp-util: Revise OpenFlow 1.0 ofp_match normalization.

For a long time, Open vSwitch has "normalized" OpenFlow 1.0 flows in a
funny way: it tries to change fields that are wildcarded into fields
that are exact-match.  For example, the normalize_match() function
knows that if dl_type is wildcarded, then all of the L3 and L4 fields
will always be extracted as 0, so it sets those fields to exact-match
and their values to 0.

The reason for this was originally that exact-match flows were much
cheaper for Open vSwitch to implement, because they could be implemented
with a hash table, whereas other kinds of flows had to be implemented
with an expensive linear search.  But these days Open vSwitch has a
smarter classifier in which wildcarded flows have minimal cost.  Also,
it is no longer possible for OpenFlow 1.0 to specify truly exact-match
flows, because Open vSwitch supports fields for which OpenFlow 1.0
cannot specify values and therefore will always be wildcarded.

Now, it no longer makes sense to do this transformation, so this commit
removes it.  Presumably, this will be less surprising for users.

Reviewed-by: Simon Horman <horms@verge.net.au>
13 years agoofp-util: Simplify OpenFlow 1.0 ofp_match normalization.
Ben Pfaff [Mon, 2 May 2011 18:46:17 +0000 (11:46 -0700)]
ofp-util: Simplify OpenFlow 1.0 ofp_match normalization.

The normalize_match() function does more work than really needed.  It goes
to some trouble to zero out fields that are wildcarded.  This is not
necessary, because cls_rule_from_match() will take care of it later.

Also make normalize_match() private to ofp-util.c, since it has no other
users now and I don't expect more later.

Reviewed-by: Simon Horman <horms@verge.net.au>
13 years agoofp-util: Don't warn for different forms of nw_{src,dst} wildcards.
Ben Pfaff [Fri, 29 Apr 2011 22:47:26 +0000 (15:47 -0700)]
ofp-util: Don't warn for different forms of nw_{src,dst} wildcards.

OpenFlow 1.0 uses a 6-bit field to express the number of wildcarded bits
in the nw_src and nw_dst field.  Any value 32 or greater in these fields
(binary 1xxxxx) means that all of the bits are wildcarded.  That means
that there are 32 different ways to express a wildcarded nw_src or nw_dst.
At least two of those seem sensible (100000 and 111111) so we shouldn't
warn about one of them.

This fixes the problem by ORing with 100000 instead of 111111, so that any
already-correct wildcarded mask won't be affected.

This fix allows us to update some tests.

Reviewed-by: Simon Horman <horms@verge.net.au>
13 years agolockfile: Don't warn if successful lock takes a little time.
Ben Pfaff [Fri, 13 May 2011 21:43:44 +0000 (14:43 -0700)]
lockfile: Don't warn if successful lock takes a little time.

This code issues a warning if obtaining a lock takes even 1 millisecond.
That's far too aggressive.  There's no need to warn if we have to wait
a few milliseconds.  This function already warns elsewhere if locking takes
more than 1 second, which is much more reasonable.

This change allows us to test ovsdb-server stderr output more carefully.
Before now, the tests had to ignore what ovsdb-server writes to stderr
because sometimes it would log a warning that locking took 1 ms (or so).

Reviewed-by: Simon Horman <horms@verge.net.au>
13 years agotests: Check test output more carefully.
Ben Pfaff [Fri, 29 Apr 2011 17:44:58 +0000 (10:44 -0700)]
tests: Check test output more carefully.

It's better to check output than to ignore it, because ignoring
output can fail to detect real bugs later if the output changes.

Reviewed-by: Simon Horman <horms@verge.net.au>
13 years agopoll-loop: Make wakeup logging more portable and easier to understand.
Ben Pfaff [Fri, 13 May 2011 20:06:49 +0000 (13:06 -0700)]
poll-loop: Make wakeup logging more portable and easier to understand.

Until now, when the poll_loop module's log level was turned up to "debug",
it would log a backtrace of the call stack for the event that caused poll()
to wake up in poll_block().  This was pretty useful from time to time to
find out why ovs-vswitchd was using more CPU than expected, because we
could find out what was causing it to wake up.

But there were some issues.  One is simply that the backtrace was printed
as a series of hexadecimal numbers, so GDB or another debugger was needed
to translate it into human-readable format.  Compiler optimizations meant
that even the human-readable backtrace wasn't, in my experience, as helpful
as it could have been.  And, of course, one needed to have the binary to
interpret the backtrace.  When the backtrace couldn't be interpreted or
wasn't meaningful, there was essentially nothing to fall back on.

This commit changes the way that "debug" logging for poll_block() wakeups
works.  Instead of logging a backtrace, it logs the source code file name
and line number of the call to a poll_loop function, using __FILE__ and
__LINE__.  This is by itself much more meaningful than a sequence of
hexadecimal numbers, since no additional interpretation is necessary.  It
can be useful even if the Open vSwitch version is only approximately known.

In addition to the file and line, this commit adds, for wakeups caused by
file descriptors, information about the file descriptor itself: what kind
of file it is (regular file, directory, socket, etc.), the name of the file
(on Linux only), and the local and remote endpoints for socket file
descriptors.

Here are a few examples of the new output format:

932-ms timeout at ../ofproto/in-band.c:507
[POLLIN] on fd 20 (192.168.0.20:35388<->192.168.0.3:6633) at ../lib/stream-fd.c:149
[POLLIN] on fd 7 (FIFO pipe:[48049]) at ../lib/fatal-signal.c:168

13 years agobacktrace: Make backtrace_capture() work on more systems.
Ben Pfaff [Fri, 13 May 2011 18:55:22 +0000 (11:55 -0700)]
backtrace: Make backtrace_capture() work on more systems.

The backtrace_capture() implementation only worked properly with GNU C on
systems that have a simple stack frame with a frame pointer.  Notably,
the x86-64 ABI by default has no frame pointer, so this failed on x86-64.

However, glibc has a function named backtrace() that does what we want.
This commit tests for this function and uses it when it is present, fixing
x86-64 backtraces.

13 years agocfm: Clarify cfm_create() documentation.
Ethan Jackson [Fri, 13 May 2011 19:52:00 +0000 (12:52 -0700)]
cfm: Clarify cfm_create() documentation.

Reported-by: Ben Pfaff <blp@nicira.com>
13 years agocfm: Always log on CCM reception.
Ethan Jackson [Thu, 12 May 2011 20:48:23 +0000 (13:48 -0700)]
cfm: Always log on CCM reception.

This commit causes the CFM library to log at debug level when valid
CCMs are received.

13 years agocfm: Replace recv_time with a flag.
Ethan Jackson [Thu, 12 May 2011 01:13:35 +0000 (18:13 -0700)]
cfm: Replace recv_time with a flag.

This makes the code more obviously correct in my opinion.

This patch also removes timer_enabled_at() along with its only
user.

13 years agocfm: No longer keep track of bad CCMs.
Ethan Jackson [Thu, 12 May 2011 00:50:16 +0000 (17:50 -0700)]
cfm: No longer keep track of bad CCMs.

According to the 802.1ag specification, reception of a CCM from an
unexpected source should trigger a fault. This patch causes the CFM
module to simply warn instead.  There are several reasons for this
change outlined below.

  - Faults can cause controllers to make potentially expensive
    changes to the network topology.
  - Faults can be maliciously triggered by crafting invalid CCMs.
  - With this patch, cfm->fault and rmp->fault are only updated in
    cfm_run() making the code easier to debug and reason about.

13 years agocfm: No longer trigger fault upon unexpected ccm_interval.
Ethan Jackson [Thu, 12 May 2011 00:55:41 +0000 (17:55 -0700)]
cfm: No longer trigger fault upon unexpected ccm_interval.

According to the 802.1ag specification, when a CCM is received
which advertises a misconfigured transmission interval, a fault
should be triggered.  This patch goes against the spec by simply
warning when this happens.  This is done for several reasons.

  - Faults can cause controllers to make potentially expensive
    changes in the network topology.
  - Faults can be maliciously triggered by crafting invalid CCMs.
  - Reducing the number of places in the code where rmp->fault and
    cfm->fault are changed makes the code easier to debug and
    reason about.

13 years agobridge: Keep default Ethernet address stable between runs.
Ben Pfaff [Tue, 10 May 2011 18:38:24 +0000 (11:38 -0700)]
bridge: Keep default Ethernet address stable between runs.

In some circumstances the bridge can't find a stable physical Ethernet
address to use, so until now it has just picked a random Ethernet address.
In these circumstances, therefore, the bridge Ethernet address would change
from one ovs-vswitchd run to another.  But OVS does have a stable
identifier for a bridge: its UUID.  This commit changes to use that as the
default bridge Ethernet address.

The datapath ID is sometimes derived from the bridge Ethernet address, so
this change also makes the bridge Ethernet address more stable.

CC: Natasha Gude <natasha@nicira.com>
Bug #5594.

13 years agodatapath: Pull data into linear area only on demand.
Jesse Gross [Tue, 10 May 2011 18:48:36 +0000 (11:48 -0700)]
datapath: Pull data into linear area only on demand.

We currently always pull 64 bytes of data (if it exists) into the
skb linear data area when parsing flows.  The theory behind this
is that the data should always be there and it's enough to parse
common flows.  However, this causes a number of problems in
different situations.  The first is that it is not enough to handle
IPv6 so we must pull additional data anyways.  However, the main
problem is that GRO typically allocates a new skb and puts just the
headers in there.  For a typical TCP/IPv4 packet there are 54 bytes
of headers, which means that we must possibly reallocate and copy
on every packet.  In addition, GRO creates frag_lists with this
specific geometry in order to allow later segmentation if the packet
is forwarded to a device that does not support frag_lists.  When
we pull additional data it changes the geometry and causes later
problems for the device.  This patch instead incrementally pulls
data, which avoids these problems.

Signed-off-by: Jesse Gross <jesse@nicira.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
13 years agoxenserver: Fix bugs related to using xe-switch-network-backend in spec file.
Justin Pettit [Tue, 10 May 2011 06:30:07 +0000 (23:30 -0700)]
xenserver: Fix bugs related to using xe-switch-network-backend in spec file.

Commit daf2ebb (xenserver: Use xe-switch-network-stack in RPM spec
file.) changed the spec file to use xe-switch-network-backend instead of
directly modifying "/etc/xensource/network.conf".  It incorrectly
assumed that the command was in the search path.  It also didn't take
into account that the command will remove the "openvswitch" service with
chkconfig.  This commit fixes those errors.

Signed-off-by: Justin Pettit <jpettit@nicira.com>
13 years agostream-ssl: Improve messages when configuring SSL if it is unsupported.
Ben Pfaff [Tue, 10 May 2011 16:17:37 +0000 (09:17 -0700)]
stream-ssl: Improve messages when configuring SSL if it is unsupported.

Previously, if --private-key or another option that requires SSL support
was used, but OVS was built without OpenSSL support, then OVS would fail
with an error message that the specified option was not supported.  This
confused users because it made them think that the option had been removed:
    http://openvswitch.org/pipermail/discuss/2011-April/005034.html

This commit improves the error message: OVS will now report that it was
built without SSL support.  This should be make the problem clear to users.

Reported-by: Aaron Rosen <arosen@clemson.edu>
Feature #5325.

13 years agoINSTALL.XenServer: Document Open vSwitch boot process on XenServer.
Ben Pfaff [Tue, 10 May 2011 16:15:44 +0000 (09:15 -0700)]
INSTALL.XenServer: Document Open vSwitch boot process on XenServer.

Inspired by a conversation with David Erickson <derickso@stanford.edu>.

13 years agoovs-vsctl: Issue warning for likely erroneous "get" commands.
Ben Pfaff [Mon, 9 May 2011 17:29:51 +0000 (10:29 -0700)]
ovs-vsctl: Issue warning for likely erroneous "get" commands.

Suggested-by: Reid Price <reid@nicira.com>
Feature #5527.

13 years agobridge: Don't configure QoS without Queues.
Ethan Jackson [Sat, 7 May 2011 00:02:02 +0000 (17:02 -0700)]
bridge: Don't configure QoS without Queues.

It doesn't make sense to create a QoS object without any queues.
Before this patch, OVS would configure the QoS object and as a
result drop all traffic going through the affected interface.  With
this patch, OVS will simply clear QoS configuration on the
interface.

Bug #5583.

13 years agoofproto: Resubmit statistics improperly account during failover.
Ethan Jackson [Mon, 2 May 2011 20:15:59 +0000 (13:15 -0700)]
ofproto: Resubmit statistics improperly account during failover.

In some cases, when a facet's actions change because it resubmits
into a different rule, it will account all packets it as accrued
in the datapath to the new rule.  Due to the algorithm we are
using, it is acceptable for a facet to miscount at most 1 second
worth of packets in this manner.  This patch implements the proper
behavior.

Generally speaking, when a facet is facet_put__() into the
datapath, the kernel returns the old flow's statistics so they may
be accounted for in user space.  These statistics are generally
pushed down to the relevant facet's resubmit children.  Before this
patch, facet_put__() did not compensate for the fact that many of
the statistics in the datapath may have been already pushed.
Thus the entire packet count stored in the datapath would be pushed
to its children instead of simply the packets which have accrued
since the last accounting.  This patch fixes the behavior by
subtracting already accounted for packets from the statistics
returned by the datapath.

13 years agolacp: New "lacp-heartbeat" mode.
Ethan Jackson [Thu, 5 May 2011 23:52:56 +0000 (16:52 -0700)]
lacp: New "lacp-heartbeat" mode.

This commit creates a new heartbeat mode for LACP.  This mode
treats LACP as a protocol simply for monitoring link status.  It
strips out most of the sanity checks built into the protocol.
Addition of this mode makes "lacp-force-aggregatable" and
"lacp-strict" options obsolete so they are removed.

13 years agobond: Create new "bond-stable-id".
Ethan Jackson [Thu, 5 May 2011 23:01:11 +0000 (16:01 -0700)]
bond: Create new "bond-stable-id".

Stable bonding mode needs an ID to guarantee consistent slave
selection decisions across ovs-vswitchd instances.  Before this
patch, we used the lacp-port-id for this purpose.  However, LACP
places restrictions on how lacp-port-ids can be allocated which may
be inconvenient.  This patch creates a special purpose
bond-stable-id other_config setting which allows users to tweak
this value directly.

13 years agobond: Convert stb_id to 32bit parameter.
Ethan Jackson [Thu, 5 May 2011 21:27:38 +0000 (14:27 -0700)]
bond: Convert stb_id to 32bit parameter.

The 16 bits currently in use is artificially restrictive.

13 years agoxenserver: Better document scriplet action in RPM spec file.
Justin Pettit [Wed, 4 May 2011 06:16:46 +0000 (23:16 -0700)]
xenserver: Better document scriplet action in RPM spec file.

13 years agoxenserver: Use xe-switch-network-stack in RPM spec file.
Justin Pettit [Wed, 27 Apr 2011 02:58:19 +0000 (19:58 -0700)]
xenserver: Use xe-switch-network-stack in RPM spec file.

The proper way to switch the networking back-end is to use the
"xe-switch-network-stack" command rather than directly modifying
"/etc/xensource/network.conf".  Use that method in the spec file.

13 years agoofp-util: Fix validation of OFPAT_SET_VLAN_PCP actions.
Ben Pfaff [Wed, 4 May 2011 22:47:27 +0000 (15:47 -0700)]
ofp-util: Fix validation of OFPAT_SET_VLAN_PCP actions.

Found by sparse.

13 years agoDESIGN: Move in-band control design discussion here.
Ben Pfaff [Wed, 4 May 2011 20:46:21 +0000 (13:46 -0700)]
DESIGN: Move in-band control design discussion here.

It seems more likely that interested users and administrators will be able
to find it here.

13 years agoovs-tcpundump: Document that ovs-appctl sends ofproto/trace command.
Ben Pfaff [Wed, 4 May 2011 16:58:30 +0000 (09:58 -0700)]
ovs-tcpundump: Document that ovs-appctl sends ofproto/trace command.

Suggested-by: Reid Price <reid@nicira.com>
Bug #5538.

13 years agoxenserver: Don't remove network.dbcache on uninstall.
Ben Pfaff [Tue, 3 May 2011 18:03:08 +0000 (11:03 -0700)]
xenserver: Don't remove network.dbcache on uninstall.

network.dbcache was introduced by Open vSwitch for its own purposes, but
it has now migrated into the base install of XenServer, which uses it
whether Open vSwitch is installed or not, so we should no longer remove it
on package uninstall.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Reported-by: Bob Ball <bob.ball@citrix.com>
13 years agoovs-brcompatd: Document bug.
Ben Pfaff [Tue, 3 May 2011 17:30:17 +0000 (10:30 -0700)]
ovs-brcompatd: Document bug.

Reported-by: Gregor Schaffrath <grsch@net.t-labs.tu-berlin.de>
13 years agoxenserver: Use .../extra not .../kernel/extra for kernel modules.
Ben Pfaff [Tue, 3 May 2011 17:51:06 +0000 (10:51 -0700)]
xenserver: Use .../extra not .../kernel/extra for kernel modules.

On XenServer, depmod.conf causes modules in /lib/modules/$(uname -r)/extra
to take priority over standard modules.  Unfortunately, we were installing
our modules in /lib/modules/$(uname -r)/kernel/extra, which isn't special.
This commit fixes the problem.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Reported-by: Bob Ball <bob.ball@citrix.com>
13 years agovswitchd: Update schema version number.
Ethan Jackson [Mon, 2 May 2011 23:33:01 +0000 (16:33 -0700)]
vswitchd: Update schema version number.

Quite a few changes to LACP and bonding have gone in recently which
allowed additional other_config parameters on ports and interfaces.
These changes should have updated the vswitch.ovsschema version
number.

Requested-by: Jeremy Stribling <strib@nicira.com>
13 years agoovs-dpctl: Add -s option to print packet and byte counters.
Ben Pfaff [Thu, 28 Apr 2011 20:02:15 +0000 (13:02 -0700)]
ovs-dpctl: Add -s option to print packet and byte counters.

13 years agonetdev-linux: New functions for converting netdev stats formats.
Ben Pfaff [Thu, 28 Apr 2011 18:13:53 +0000 (11:13 -0700)]
netdev-linux: New functions for converting netdev stats formats.

An upcoming commit will introduce another function that needs to convert
between rtnl_link_stats64 and netdev_stats, so it seemed best to just add
functions to do the conversion.

13 years agotunneling: Add DF inherit and default flags to set of public tunnel flags.
Andrew Evans [Sun, 1 May 2011 17:18:45 +0000 (10:18 -0700)]
tunneling: Add DF inherit and default flags to set of public tunnel flags.

Signed-off-by: Andrew Evans <aevans@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
13 years agotunneling: Add df_default and df_inherit tunnel options.
Andrew Evans [Sat, 30 Apr 2011 00:05:58 +0000 (17:05 -0700)]
tunneling: Add df_default and df_inherit tunnel options.

Split existing pmtud tunnel option's functionality into three. Existing pmtud
option still exists, but now governs only whether datapath sends ICMP frag
needed messages. New df_inherit option controls whether DF bit is copied from
packet inner header to outer tunnel header. New df_default option controls
whether DF bit is set if inner packet isn't IP or if df_inherit is disabled.

Suggested-by: Jesse Gross <jesse@nicira.com>
Signed-off-by: Andrew Evans <aevans@nicira.com>
Feature #5456.

13 years agodpif-linux: Recycle leaked ports.
Ethan Jackson [Fri, 29 Apr 2011 20:12:19 +0000 (13:12 -0700)]
dpif-linux: Recycle leaked ports.

When ports are deleted from the datapath they need to be added to
an LRU list maintained in dpif-linux so they may be reallocated.
When using vswitchd to delete the ports this happens automatically.
However, if a port is deleted directly from the datapath it is
never reclaimed by dpif-linux.  If this happens often, eventually
no ports will be available for allocation and dpif-linux will fall
back to using the old, kernel implemented, allocation strategy.

This commit fixes the problem by automatically reclaiming ports
missing from the datapath whenever the list of ports in the
datapath is dumped.

Bug #2140.

13 years agodatapath: Remove dead code in queue_control_packets().
Ethan Jackson [Fri, 29 Apr 2011 00:13:50 +0000 (17:13 -0700)]
datapath: Remove dead code in queue_control_packets().

Fixes the following warning:

datapath.c:473:6: warning: variable 'port_no' set but not used
[-Wunused-but-set-variable]

Signed-off-by: Ethan Jackson <ethan@nicira.com>
13 years agodatapath: Drop parameters from execute_actions().
Ben Pfaff [Fri, 29 Apr 2011 17:49:06 +0000 (10:49 -0700)]
datapath: Drop parameters from execute_actions().

It's (almost) always easier to understand a function with fewer parameters,
so this removes the now-redundant sw_flow_key and actions parameters from
execute_actions(), since they can be found through OVS_CB(skb)->flow now.

This also necessarily moves loop detection into execute_actions().
Otherwise, the flow's actions could have changed between the time that the
loop was detected and the time that it was suppressed, which would mean
that the wrong (version of the) flow would get suppressed.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
13 years agodatapath: Make every packet passing through the datapath have an sw_flow.
Ben Pfaff [Thu, 28 Apr 2011 23:54:07 +0000 (16:54 -0700)]
datapath: Make every packet passing through the datapath have an sw_flow.

This way, it's always possible to get a packet's key or hash simply by
looking at its 'flow', without considering whether the packet came from
userspace or from a vport.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
13 years agodatapath: Avoid freeing wild pointer in corner case.
Ben Pfaff [Thu, 28 Apr 2011 23:34:56 +0000 (16:34 -0700)]
datapath: Avoid freeing wild pointer in corner case.

In odp_flow_cmd_new_or_set(), if flow_actions_alloc() fails in the "new
flow" case, then flow_put() will kfree() the new flow's 'sf_acts' pointer,
but nothing has initialized that pointer.  Initialize the pointer to NULL
to avoid the problem.

Found by inspection.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
13 years agodatapath: No need to zero cb anymore in odp_packet_cmd_execute().
Ben Pfaff [Thu, 28 Apr 2011 23:13:39 +0000 (16:13 -0700)]
datapath: No need to zero cb anymore in odp_packet_cmd_execute().

Before commit 3f19d399f "datapath: Fix mysterious GRE-over-IPSEC problems,"
'packet' in opd_packet_cmd_execute() was an skb cloned from one created by
Netlink, so its cb member wasn't necessarily zeroed.  But that commit
changed 'packet' to be freshly allocated with __dev_alloc_skb(), which
means that cb is zeroed, so we don't have to do it again.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
13 years agoovs-monitor-ipsec: Allow IKE fragmentation
Justin Pettit [Wed, 27 Apr 2011 15:46:38 +0000 (08:46 -0700)]
ovs-monitor-ipsec: Allow IKE fragmentation

Some (broken) firewalls do not properly pass UDP fragments, which will
prevent IKE from completing.  This commit enables the racoon option to
allow application-level fragmenting and allow security associations to
be created.

13 years agodatapath: Make git ignore linux-2.6/vlan.c.
Andrew Evans [Thu, 28 Apr 2011 01:58:16 +0000 (18:58 -0700)]
datapath: Make git ignore linux-2.6/vlan.c.

Signed-off-by: Andrew Evans <aevans@nicira.com>
13 years agodatapath: Backport DIV_ROUND_UP.
Jesse Gross [Thu, 14 Apr 2011 20:10:09 +0000 (13:10 -0700)]
datapath: Backport DIV_ROUND_UP.

Older kernels didn't define DIV_ROUND_UP, so this provides a
backported version.

Signed-off-by: Jesse Gross <jesse@nicira.com>
13 years agolacp: Allow configurable aggregation keys.
Ethan Jackson [Tue, 26 Apr 2011 22:39:58 +0000 (15:39 -0700)]
lacp: Allow configurable aggregation keys.

Users will the ability to manually set aggregation keys on a
per-slave basis in order to use some of the more advanced LACP
features.  Most notably, LACP controlled active-backup bonding
requires fine grained aggregation key configuration.

13 years agolacp: New other_config setting "lacp-force-aggregatable".
Ethan Jackson [Tue, 26 Apr 2011 22:05:19 +0000 (15:05 -0700)]
lacp: New other_config setting "lacp-force-aggregatable".

In some extremely advanced situations, one may want to force
non-bondable slaves to advertise themselves as bondable.  This
patch adds that capability.

Also includes some minor code cleanup.

13 years agoofproto: Remove unused variable.
Ethan Jackson [Wed, 27 Apr 2011 00:32:41 +0000 (17:32 -0700)]
ofproto: Remove unused variable.

Fixes the following warning:

ofproto/ofproto.c:4176:30: error: variable 'pin' set but not used
[-Werror=unused-but-set-variable]

13 years agoconnmgr: Remove unused variable.
Ethan Jackson [Wed, 27 Apr 2011 00:28:44 +0000 (17:28 -0700)]
connmgr: Remove unused variable.

Fixes the following warning:

ofproto/connmgr.c:396:11: error: variable 'ss_exists' set but not
used [-Werror=unused-but-set-variable]

13 years agovswitch.xml: s/switchs/switches/g
Andrew Evans [Tue, 26 Apr 2011 20:58:35 +0000 (13:58 -0700)]
vswitch.xml: s/switchs/switches/g

13 years agobond: New bond-hash-basis setting.
Ethan Jackson [Thu, 21 Apr 2011 20:55:45 +0000 (13:55 -0700)]
bond: New bond-hash-basis setting.

13 years agobridge: Create new "null" interface type.
Ethan Jackson [Mon, 25 Apr 2011 23:06:54 +0000 (16:06 -0700)]
bridge: Create new "null" interface type.

Null interfaces are completely ignored by Open vSwitch.

13 years agoRemove support for obsolete "tun_id_from_cookie" extension.
Ben Pfaff [Tue, 26 Apr 2011 16:42:18 +0000 (09:42 -0700)]
Remove support for obsolete "tun_id_from_cookie" extension.

The "tun_id_from_cookie" OpenFlow extension predated NXM and supports only
a fraction of its features.  Nothing (at Nicira, anyway) uses it any
longer.  Support for it had been broken since January and it took until a
few days ago for anyone to complain, so it cannot be too important.  This
commit removes it.

13 years agotests: Add ovs-openflowd to programs that need valgrind wrappers.
Ben Pfaff [Thu, 21 Apr 2011 23:38:05 +0000 (16:38 -0700)]
tests: Add ovs-openflowd to programs that need valgrind wrappers.

The tests run ovs-openflowd so "make check-valgrind" should run it under
valgrind.

13 years agobridge: Remove slaves from the bond before closing their netdevs.
Ben Pfaff [Thu, 21 Apr 2011 23:37:38 +0000 (16:37 -0700)]
bridge: Remove slaves from the bond before closing their netdevs.

A bond slave has a pointer to its iface's netdev, so we don't want it to
keep that pointer after the bridge closes the netdev.

This is becoming a bit of a mess so perhaps we need reference counting for
netdevs (although Jesse didn't like the idea when I proposed it before).

13 years agobond: Be more careful about adding and removing netdevs in the monitor.
Ben Pfaff [Thu, 21 Apr 2011 23:34:51 +0000 (16:34 -0700)]
bond: Be more careful about adding and removing netdevs in the monitor.

The code was careless about updating the netdev_monitor.  Newly added
slaves weren't added to the monitor until the next bond_reconfigure() call,
and netdevs were never removed from the monitor.

13 years agoofproto: Adjust netdev_monitor when switching netdevs.
Ben Pfaff [Thu, 21 Apr 2011 23:25:41 +0000 (16:25 -0700)]
ofproto: Adjust netdev_monitor when switching netdevs.

This fixes a segfault in the "ofproto - mod-port" test.  The segfault
should not occur--there must be a bug in the netdev_monitor or possibly
the netdev_dummy implementation--but the netdev_monitor_remove() and
netdev_monitor_add() calls are definitely wanted here in any case to ensure
that the new netdev, not the old one, is what gets monitored.

13 years agobridge: Tolerate missing Port and Interface records for local port.
Ben Pfaff [Wed, 13 Apr 2011 18:10:44 +0000 (11:10 -0700)]
bridge: Tolerate missing Port and Interface records for local port.

Until now, ovs-vswitchd has been unable to configure IP addresses and
routes for bridges whose Bridge records lack a Port and an Interface
record for the bridge's local port (e.g. OFPP_LOCAL, the port with the
same name as the bridge itself).  When such a bridge was reconfigured,
ovs-vswitchd would output a log message that worried people.

This commit fixes the internal limitation that led to the message being
printed.

Bug #5385.

13 years agoofproto: Rework and fix bugs in port change detection.
Ben Pfaff [Wed, 20 Apr 2011 20:48:11 +0000 (13:48 -0700)]
ofproto: Rework and fix bugs in port change detection.

The OpenFlow port change detection code in update_port() is supposed to
send out an OFPT_PORT_STATUS message whenever an OpenFlow port is added or
removed or changes in some way.  This commit fixes a number of bugs that
have persisted until now.

First, if a port with a given name is removed from the datapath and a new
port with the same name but a different port number is added to the
datapath, then update_port() would report this as a port "modify" change.
Reporting this as a "modify" seems likely to confuse controllers, which
have no reason to realize that the old port was deleted and may not
understand why a port that has not been reported as added would be
modified.  (This scenario is more likely than before, because the Linux
datapath implementation no longer quickly reuses port numbers.  This
problem has actually been reported in testing.)  This commit fixes the
problem by changing update_port() to report a "delete" of the old port
followed by an "add" of the new port.

Second, suppose that a datapath initially has "eth1" on port 1 and "eth2"
on port 2.  Then, "eth1" gets removed and "eth2" is reassigned to port 1.
If update_port() is first passed "eth2", then the old implementation would
have sent out an OpenFlow "modify" notification instead of "delete"
followed by "add", which is the same as the previous scenario.  But as a
further wrinkle, it would have failed to remove "eth1", which meant that we
ended up with two "ofports" with port number 1!  This commit fixes this
problem too.

Reported-by: David Tsai <dtsai@nicira.com>
Bug #5466.
NIC-372.

13 years agoofproto: Consistently use netdev's name instead of ofp_phy_port name.
Ben Pfaff [Wed, 20 Apr 2011 20:03:45 +0000 (13:03 -0700)]
ofproto: Consistently use netdev's name instead of ofp_phy_port name.

There are at least two ways to get an ofport's name: from the netdev using
netdev_get_name() or from the ofp_phy_port's 'name' member.  Some code used
one, some used the other.  This switches all relevant code to use only
netdev_get_name(), because the 'name' member in ofp_phy_port is
fixed-length and thus a long name could be truncated.

This isn't a problem under Linux since the maximum length of a network
device's name under Linux is the same as the field width in ofp_phy_port.

13 years agosocket-util: Use portable solution for setting Unix socket permissions.
Ben Pfaff [Thu, 21 Apr 2011 16:22:39 +0000 (09:22 -0700)]
socket-util: Use portable solution for setting Unix socket permissions.

Requested-by: Jesse Gross <jesse@nicira.com>
13 years agobond: BM_STABLE consistent hashing.
Ethan Jackson [Wed, 20 Apr 2011 22:53:58 +0000 (15:53 -0700)]
bond: BM_STABLE consistent hashing.

This patch converts stable bonds from modulo n based hashing to
Highest Random Weight based hashing.  This hashing strategy only
redistributes 1/n_slaves traffic when a slave is enabled or
disabled.  It also turns out to have a vastly simpler
implementation.

13 years agobond: New flag "bond_revalidate".
Ethan Jackson [Wed, 20 Apr 2011 23:01:28 +0000 (16:01 -0700)]
bond: New flag "bond_revalidate".

Used in future patches.

13 years agoINSTALL.Linux: Mention that SSL options require building with SSL support.
Ben Pfaff [Fri, 8 Apr 2011 19:40:49 +0000 (12:40 -0700)]
INSTALL.Linux: Mention that SSL options require building with SSL support.

Reported-by: Aaron Rosen <arosen@clemson.edu>
13 years agobond: Revalidate no_slaves_tag when revalidating everything.
Ethan Jackson [Wed, 20 Apr 2011 01:02:53 +0000 (18:02 -0700)]
bond: Revalidate no_slaves_tag when revalidating everything.

13 years agobond: Give stable bonds one tag.
Ethan Jackson [Wed, 20 Apr 2011 00:19:25 +0000 (17:19 -0700)]
bond: Give stable bonds one tag.

Stable bonds require all flows to be revalidated when anything
changes.  Instead of giving each slave a tag, and ORing them
together.  This commit creates one tag representing the entire
bond.  This will cause less false positives when deciding which
flows to revalidate.

13 years agobridge: Avoid memory leak from RSPAN mirrors in bridge_destroy().
Ben Pfaff [Mon, 11 Apr 2011 18:22:39 +0000 (11:22 -0700)]
bridge: Avoid memory leak from RSPAN mirrors in bridge_destroy().

Mirrors that output to ports will be destroyed when their output ports are
destroyed, but mirrors that output to VLANs ("RSPAN" mirrors) don't get
automatically destroyed like this and we need to take care of them in a
separate loop.

13 years agobond: bond_stb_enable_slave() never triggered.
Ethan Jackson [Tue, 19 Apr 2011 21:11:23 +0000 (14:11 -0700)]
bond: bond_stb_enable_slave() never triggered.

bond_stb_enable_slave() depended on bond->stb_slaves being
nonnull.  However, bond_stb_enable_slave() is responsible for
initializing this parameter.  Thus none of it's logic ever ran.

13 years agolacp: Implement custom timing mode.
Ethan Jackson [Mon, 18 Apr 2011 19:22:12 +0000 (12:22 -0700)]
lacp: Implement custom timing mode.

With this patch, the LACP module may be manually configured to use
an arbitrary transmission rate set in the database.

13 years agolacp: Remove LACP_[FAST|SLOW]_TIME_RX macros.
Ethan Jackson [Mon, 18 Apr 2011 19:48:59 +0000 (12:48 -0700)]
lacp: Remove LACP_[FAST|SLOW]_TIME_RX macros.

The receive rate for a LACP packets is simply 3 times the
transmission rate.  It doesn't make sense to maintain separate
macros for these values especially since future patches will allow
arbitrary transmission rates.

13 years agolacp: Move LACP packet data to lacp header file.
Ethan Jackson [Mon, 18 Apr 2011 19:33:14 +0000 (12:33 -0700)]
lacp: Move LACP packet data to lacp header file.

13 years agoofp-util: Properly handle "tun_id"s in tun_id_from_cookie flows.
Ben Pfaff [Mon, 18 Apr 2011 17:11:43 +0000 (10:11 -0700)]
ofp-util: Properly handle "tun_id"s in tun_id_from_cookie flows.

Just setting the tun_id field isn't enough--it's also necessary to set
the tun_id_mask.  Otherwise the call to cls_rule_zero_wildcarded_fields()
at the end of ofputil_cls_rule_from_match() will zero out the tun_id again.

This was broken by commit 8368c090cab "Implement arbitrary bitwise masks
for tun_id field" back in January.  (This makes me wonder whether we can
drop support for tun_id_from_cookie now.)

Reported-by: Dan Wendlandt <dan@nicira.com>
13 years agosocket-util: Properly set socket permissions in make_unix_socket().
Ben Pfaff [Mon, 18 Apr 2011 18:24:50 +0000 (11:24 -0700)]
socket-util: Properly set socket permissions in make_unix_socket().

Under Linux, at least, bind and fchmod interact for Unix sockets in a way
that surprised me.  Calling fchmod() on a Unix socket successfully sets the
permissions for the socket's own inode.  But that has no effect on any
inode that has already been created in the file system by bind(), because
that inode is not the same as the one for the Unix socket itself.

However, if you bind() *after* calling fchmod(), then the bind() takes the
permissions for the new inode from the Unix socket inode, which has the
desired effect.

This also adds a more portable fallback for non-Linux systems.

Reported-by: YAMAMOTO Takashi <yamamoto@valinux.co.jp>
13 years agobridge: LACP port ID and system ID in database.
Ethan Jackson [Sat, 16 Apr 2011 00:03:37 +0000 (17:03 -0700)]
bridge: LACP port ID and system ID in database.

Extremely advanced users may want fine grained control over the
LACP port and system IDs of a bond.  This would be extremely
unusual for the average user, so this patch puts the configuration
parameters in other_config of the relevant tables.

13 years agolacp: New "strict" lacp mode.
Ethan Jackson [Mon, 18 Apr 2011 22:13:34 +0000 (15:13 -0700)]
lacp: New "strict" lacp mode.

When LACP negotiations are unsuccessful, OVS falls back to standard
balance-slb bonding.  In some cases, users may want to require
successful LACP negotiations for any slaves to be enabled at all.
This patch implements a new "strict" mode which disables all slaves
when LACP negotiations are unsuccessful.

13 years agolacp: Update attached status more often.
Ethan Jackson [Mon, 18 Apr 2011 22:32:30 +0000 (15:32 -0700)]
lacp: Update attached status more often.

The attached status of slaves should be updated when certain global
configuration settings change, or when a slave is destroyed.

13 years agovlog: Fix VLOG and VLOG_RL macros' treatment of LEVEL argument.
Ben Pfaff [Thu, 7 Apr 2011 21:39:36 +0000 (14:39 -0700)]
vlog: Fix VLOG and VLOG_RL macros' treatment of LEVEL argument.

These macros expanded the LEVEL argument without protecting it with
parentheses, which meant that an argument like 'cond ? VLL_DBG : VLL_WARN'
did not have the desired effect (and caused a GCC warning).

This commit fixes the problem and avoids expanding LEVEL more than once,
too.

13 years agobridge: Fix VLAN selection mirroring logic.
Ben Pfaff [Fri, 8 Apr 2011 19:52:23 +0000 (12:52 -0700)]
bridge: Fix VLAN selection mirroring logic.

The logic here did not make sense.  A packet arriving on a port is mirrored
if the port is a mirroring source port AND (not OR) the packet is in one of
the VLANs that is mirrored.

This test has been here since the mirroring code was introduced.  It seems
to me that it was never correct.

13 years agobridge: Reintroduce log message that was lost (and wrong).
Ben Pfaff [Tue, 29 Mar 2011 18:16:31 +0000 (11:16 -0700)]
bridge: Reintroduce log message that was lost (and wrong).

Setting the 'mac' in the Interface record for a bridge's local port has
always been ineffective, but the log message was suppressed because of a
check at too high of a level.  This commit fixes the problem.  It also
fixes the wording of the log message, which has been obsolete since the
introduction of the database.

Finally, it seems better to check for the local port before checking for a
multicast address, so this reverses the order of the checks.