Jesse Gross [Thu, 16 Jun 2011 22:32:26 +0000 (15:32 -0700)]
datapath: Use consume_skb() on non-errors.
It's possible to trace kfree_skb() call sites to find out where
packets are getting dropped. Situations where kfree_skb() does
not actually indicate an error adds additional noise, so use
consume_skb() instead to avoid tracing non-errors.
Suggested-by: Ben Pfaff <blp@nicira.com> Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross [Fri, 27 May 2011 22:53:49 +0000 (15:53 -0700)]
datapath: Further mirror checksum offloading state on old kernels.
Older kernels (those before 2.6.22) rely on implicit assumptions
to determine checksum offloading status. These assumptions tend
to break down when doing switching because it sits in the middle
of the transmit and receive path. Newer kernels deal with this
problem by adding more explicit information about how to checksum.
This replicates that behavior by mirroring the state from newer
kernels in private OVS storage on the kernels that lack it. On
ingress and egress we then map that state onto the appropriate
location for the given kernel and can consistently manipulate it
within OVS. Some of this was already done for the checksum type
but this makes it more robust and expands it to the checksum start
and offset as well.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross [Wed, 8 Jun 2011 00:11:02 +0000 (17:11 -0700)]
datapath: Drop set_skb_csum_bits().
Various older kernels have had different bugs with copying checksum
state when a complete copy of a packet is made. However, it is not
actually necessary to make these copies and all occurrences have
now been removed. Therefore, we can also remove the workarounds to
deal with these bugs.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross [Wed, 8 Jun 2011 00:09:35 +0000 (17:09 -0700)]
tunneling: Avoid extra copying if expanding headroom.
Currently if we need additional headroom before encapsulating a
packet a clone is made before expanding headroom or if we are
just trying to make the headroom writable then we copy both
the struct sk_buff and the paged data. Both of these are unnecessary
and we end up freeing the original copy. We can remove these copies
and simplify the code by just expanding the linear data area.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross [Tue, 7 Jun 2011 02:17:25 +0000 (19:17 -0700)]
datapath: Simplify make_writable().
The current implementation of make_writable() is both overly complex
and unnecessarily aggressive about copying data. We can improve
performance by only making a copy of the data if someone else holds
a reference to the portion of the data that we want to modify. This
means that if a clone is held by the TCP stack for retransmission then
we do not need to make a copy if we are changing the IP header because
it will get regenerated on retransmit anyways. Even when it is necessary
to copy we avoid duplicating struct sk_buff.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross [Mon, 6 Jun 2011 23:11:47 +0000 (16:11 -0700)]
datapath: Use strip_vlan() for modify_vlan_tci().
The sematics for setting a vlan tag are to modify the existing tag
if one exists. This can be expressed as removing the existing tag
first and then adding a new one. This simplifies the code by not
requiring two copies of the logic that manipulates non-accelerated
vlans and should not make a performance difference because the vlan
tag is contained in a single cache line.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Ben Pfaff [Wed, 15 Jun 2011 23:39:49 +0000 (16:39 -0700)]
xenserver: Don't delete pidfile in stop_daemon.
Deleting the pidfile introduces a minor race: if some other process is
starting a daemon at the same time that the init script is stopping the
same daemon, then this could delete the new daemon's pidfile.
This is really a theoretical risk, because no one should be starting and
stopping a single daemon at the same time.
Ben Pfaff [Fri, 10 Jun 2011 20:10:28 +0000 (13:10 -0700)]
xenserver: Don't require confirmation to restart vswitch.
Early development versions of ovs-vswitchd didn't always restart
successfully, so we required confirmation if "restart" was invoked
interactively. Recent versions do just fine, so drop the confirmation
prompt.
Justin Pettit [Wed, 15 Jun 2011 18:28:51 +0000 (11:28 -0700)]
ofproto: Rename ofproto-sflow.[ch] to ofproto-dpif-sflow.[ch].
The name ofproto-sflow.[ch] implies that this code would work with any
ofproto implementation, but in fact it is specific to the ofproto_dpif
implementation. Rename it to avoid confusion.
Ben Pfaff [Tue, 14 Jun 2011 17:36:56 +0000 (10:36 -0700)]
xenserver: Run "depmod" after old modules are removed by "rpm -U".
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets says that
package upgrade does the following steps:
1. %pretrans of new package
2. %pre of new package
3. (package install)
4. %post of new package
5. %triggerin of other packages (set off by installing new package)
6. %triggerin of new package (if any are true)
7. %triggerun of old package (if it's set off by uninstalling the old
package)
8. %triggerun of other packages (set off by uninstalling old package)
9. %preun of old package
10. (removal of old package)
11. %postun of old package
12. %triggerpostun of old package (if it's set off by uninstalling the
old package)
13. %triggerpostun of other packages (if they're setu off by
uninstalling the old package)
14. %posttrans of new package
We're getting in trouble because the %post runs in step 4 before the
old files are removed in step 10, so depmod is finding the old modules.
This commit switches to running depmod in step 14 instead, after the
old files are removed.
Bug #5916. Reported-by: Jesse Gross <jesse@nicira.com> Reported-by: Henrik Amren <henrik@nicira.com> Signed-off-by: Ben Pfaff <blp@nicira.com>
Ben Pfaff [Tue, 14 Jun 2011 22:00:50 +0000 (15:00 -0700)]
ovsdb-idl: Optimize out transactions that are complete no-ops.
Commit 1cc618c3252 "ovsdb-idl: Fix atomicity of writes that don't change a
column's value" fixed transactions that write the existing value to
some columns, ensuring that those columns still got written to the database
to avoid making the transaction nonatomic in the presence of writes that do
modify part of the database.
However, that commit was too conservative: we can still optimize out a
database transaction that writes *only* existing values to the database,
because if we drop such a transaction then the resulting database is still
one that could result from executing transactions in a serial order. This
commit implements that optimization.
As an example of what this commit does, before this commit, an "ovs-vsctl
set" command that specified the existing value for a column would do a
round-trip to the database to write that existing value. After this
commit, that round-trip would not occur.
Simon Horman [Wed, 15 Jun 2011 01:59:24 +0000 (10:59 +0900)]
Debian: ${source:Version} dependency on python-openvswitch
As reported by lintian:
E: openvswitch source: not-binnmuable-any-depends-all openvswitch-ipsec -> python-openvswitch
N:
N: The package is not safely binNMUable because an arch:any package depends
N: on an arch:all package with a (= ${Source-Version}) or (=
N: ${binary:Version}) relationship. Please use (= ${source:Version})
N: instead.
N:
N: Severity: important, Certainty: certain
Simon Horman [Wed, 15 Jun 2011 01:59:22 +0000 (10:59 +0900)]
Debian: Add ${misc:Depends} dependency to python-openvswitch
As reported by lintian -i:
W: openvswitch source: debhelper-but-no-misc-depends python-openvswitch
N:
N: The source package uses debhelper, but it does not include
N: ${misc:Depends} in the given binary package's debian/control entry. Any
N: debhelper command may add dependencies to ${misc:Depends} that are
N: required for the work that it does, so recommended best practice is to
N: always add ${misc:Depends} to the dependencies of each binary package if
N: debhelper is in use.
N:
N: Refer to the debhelper(7) manual page for details.
N:
N: Severity: normal, Certainty: possible
Ben Pfaff [Wed, 8 Jun 2011 20:35:00 +0000 (13:35 -0700)]
ofp-parse: Ignore extra fields produced by dump-flows.
It's reasonable to want to dump a flow table with "dump-flows" and then
later use that output as input to other commands that read flows, but until
now that hasn't been directly possible because "dump-flows" adds extra
fields that other commands reject on input. This changes the flow parser
to just ignore those extra fields.
Ben Pfaff [Wed, 8 Jun 2011 19:36:53 +0000 (12:36 -0700)]
ofp-print: Change "table_id" to "table" in flow_mod and flow_stats.
Flow dumps printed the OpenFlow table ID under the name "table_id", but
the flow parser only accepted "table". This makes them consistent by
changing the output. (Another alternative would be to change the accepted
input name.)
Ben Pfaff [Wed, 1 Jun 2011 00:09:43 +0000 (17:09 -0700)]
ofproto: Add simple tests for dumping aggregates and for OpenFlow 1.0.
We had no tests that exercised OFPST_AGGREGATE or NXST_AGGREGATE. At one
point in development I screwed up aggregate stats badly enough that they
caused an immediate and reproducible segfault, which this simple test would
have caught.
Also, it's best to test everything in both NXM and OpenFlow 1.0 flow
formats, since they have slightly different code.
Some switching hardware takes a very long time to update its forwarding
rules, up to hundreds of milliseconds. It is undesirable for Open
vSwitch to block waiting this long for individual OpenFlow flow table
modification commands to complete. This commit enables ofproto to queue
up any number of independent flow table operations with asynchronous
completion.
I tested earlier versions of this commit using the "ofproto/clog" and
"ofproto/unclog" commands that it implements in the software switch
implementation. I have not tested the current version very much at all.
Ben Pfaff [Fri, 27 May 2011 21:13:46 +0000 (14:13 -0700)]
ofproto: Collect all rules for an OpenFlow request before acting on any.
An upcoming commit adds support for asynchronous flow table modification.
In an attempt to ensure that the software and hardware flow tables are
properly in sync, that commit limits any given rule to a single outstanding
operation at a time. It does so by figuring out all of the rules that an
OpenFlow request will affect before modifying any of them, and then
deferring the request if it will affect any rules that have ongoing
operations. This commit is a step in that direction.
Ben Pfaff [Tue, 24 May 2011 20:30:04 +0000 (13:30 -0700)]
nicira-ext: Separate nx_aggregate_stats_reply from ofp_aggregate_stats_reply.
An upcoming patch will make stats messages much more like other messages,
in that their structures will include all of the headers. This means that
struct ofp_aggregate_stats_reply will no longer be appropriate as a
member of struct nx_aggregate_stats_reply, because it will then include
those additional header members.
Also, struct nx_aggregate_stats_reply doesn't need to use the special
ovs_32aligned_be64 type, since its 64-bit members are correctly aligned,
which is another reasonable reason to keep it separate.
Ben Pfaff [Thu, 26 May 2011 16:30:25 +0000 (09:30 -0700)]
openflow: Merge ofp_stats_request and ofp_stats_reply.
These structures for OpenFlow stats requests and replies have identical
memebers, but until now they have been separate structures. Since in some
cases we actually want to treat both of them the same way, this has led
to various kinds of awkwardness. This commit merges them into a new
"struct ofp_stats_msg" and fixes up the users.
Ethan Jackson [Mon, 13 Jun 2011 20:45:05 +0000 (13:45 -0700)]
lacp: Loosen lacp_slave_is_current().
The lacp_slave_is_current() function is used to indicate to a
controller failover status of the LACP module. However, the result
of this function is more strict than the failover logic. Thus, the
function will generally return false quite a bit before a failover
actually happens. This patch loosens lacp_slave_is_current() so
that it changes in-line with the failover logic.
Justin Pettit [Tue, 14 Jun 2011 02:26:47 +0000 (19:26 -0700)]
netdev: Add methods to do netdev-specific argument comparisons.
When doing a netdev_open(), a check is first done to make sure the
arguments are equivalent for any open devices with the same name. In
most cases, a simple shash comparison is sufficient. However, IPsec
key configuration is handled by an external program, so it is not pushed
down into the kernel module. Thus, when the "unparse_config" method is
called on an existing IPsec-based vport, a simple comparison with the
returned data will not match the original configuration. This commit
adds code to allow netdev-specific argument comparisons and has
"ipsec_gre" make use of them.
Jesse Gross [Mon, 13 Jun 2011 18:36:19 +0000 (11:36 -0700)]
datapath: Check for supported kernel versions.
Most of the time kernels older or newer than the ones we support
simply fail to compile. However, sometimes they appear to succeed
but then cause problems later on. This explicitly checks for
supported versions at compile time.
Ben Pfaff [Thu, 9 Jun 2011 22:18:21 +0000 (15:18 -0700)]
nicira-ext: Renumber NXT_FLOW_MOD_TABLE_ID.
We don't know of anyone using this command in production yet, so it seems
reasonable to give it a new number instead of coping with it having a
conflicting number.
Justin Pettit [Thu, 9 Jun 2011 22:43:18 +0000 (15:43 -0700)]
Remove NXAST_DROP_SPOOFED_ARP action.
The NXAST_DROP_SPOOFED_ARP action has been deprecated in favor of
defining flows using the NXM_NX_ARP_SHA flow match for a while. This
commit removes it.
Ben Pfaff [Wed, 8 Jun 2011 21:03:47 +0000 (14:03 -0700)]
dpif: Improve logging of upcalls.
The kernel now provides the entire flow key for a packet sent up to
userspace, but dpif_recv() would only log the in_port. This change makes
userspace log the entire flow key.
This would have made a bug that I recently looked at a bit easier to
investigate.
Jesse Gross [Wed, 8 Jun 2011 19:28:57 +0000 (12:28 -0700)]
datapath: Remove redundant nw_ prefix from fields in flow key.
The fields of the kernel flow key are now grouped by protocol rather
than using generic names. The containing structures describe the
category, so it is no longer necessary to use prefixes. Most of
these prefixes have been removed but nw_proto and nw_tos have
retained them. This renames the fields for consistency and brevity.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Jesse Gross [Wed, 8 Jun 2011 19:23:05 +0000 (12:23 -0700)]
datapath: IP fragments should include L4 header in flow length.
If we can't parse a header because it is invalid or not present due to
fragmentation, we still need to include the length of that header when
comparing the flow key. The value of the field will be zero to
indicate that header was not present, rather than effectively
wildcarding the value. However, this was not done with fragments on
flow extract but is effectively done on flow setup. Since the flow
length also changes the hash, it caused all fragments to miss the
hash table and be sent to useerspace.
Reported-by: Ben Pfaff <blp@nicira.com> Signed-off-by: Jesse Gross <jesse@nicira.com> Tested-by: Ben Pfaff <blp@nicira.com>
Sajjad Lateef [Wed, 1 Jun 2011 21:15:51 +0000 (14:15 -0700)]
xenserver: Collect xenserver configured timezone in bugtool report
xenserver logs can be in any timezone, as configured by user
during install time. Also, the timezone data is not currently
recorded in the bugtool report. Therefore, it is currently not
possible to easily correlate logs collected on xenserver with
logs from other devices on the network.
With this change, system-configuration capability of the bugtool plugin
has been added. Also, added an extension to this capability to collect
the time information in RFC3339 format, which includes the system timezone.
Ben Pfaff [Wed, 8 Jun 2011 16:15:38 +0000 (09:15 -0700)]
ovs-brcompatd: Run ovs-vsctl instead of accessing database directly.
ovs-vsctl is carefully written to avoid races in database access. It is
much simpler to just call it than to reimplement its capabilities.
This eliminates the requirement that bridges managed by ovs-brcompatd have
no ports at ovs-brcompatd startup time. It also eliminates races between
competing brctl and ovs-vsctl processes.
Ben Pfaff [Wed, 8 Jun 2011 00:08:48 +0000 (17:08 -0700)]
ovs-brcompatd: Remove --prune-timeout option.
This option had an effect when it was initially implemented, but later it
made essentially a no-op without properly removing it or updating the
documentation. This removes it in favor of keeping the default and only
sensible behavior.
Ben Pfaff [Fri, 3 Jun 2011 17:20:17 +0000 (10:20 -0700)]
ovs-brcompatd: Use rtnetlink_link_notifier instead of open-coding it.
ovs-brcompatd has always had its own code to listen on an RTNL socket, but
I don't see any reason for it. This commit rips it out in favor of
rtnetlink_link_notifier.
This change looks fairly big but a lot of it boils down to changing the
indentation level of rtnl_recv_update().
If there is a string in ip addr show output, that "coincide with the
name of the device or ... prefixed with the device name followed by
colon", consider this is an address label string.
Signed-off-by: Alexey I. Froloff <raorn@altlinux.org> Signed-off-by: Ben Pfaff <blp@nicira.com>
Ethan Jackson [Sat, 4 Jun 2011 00:15:12 +0000 (17:15 -0700)]
dpif-linux: Avoid duplicate code in dpif_linux_vport_send().
dpif_linux_vport_send() had duplicated most of the code in
dpif_linux_execute() in order to execute output actions in the
kernel. This forces developers to remember to change both
functions whenever the kernel interface changes. In particular,
commit 80e5eed9 "datapath: Get packet metadata from userspace in
odp_packet_cmd_execute()." broke netdev_linux_vport_send(). This
commit reorganizes the code and fixes the regression.
Ben Pfaff [Mon, 6 Jun 2011 16:21:45 +0000 (09:21 -0700)]
vswitchd: Add unique indexes for some columns.
This is mainly intended to make life easier for database clients that don't
already have code to avoid creating records with duplicate names in various
OVS database tables. With this commit, the database server itself rejects
attempts to add Port or Interface records with duplicate names or
Controller or Manager records with duplicate targets.
Ben Pfaff [Wed, 1 Jun 2011 23:17:00 +0000 (16:17 -0700)]
ovsdb: Move ovsdb_table_put_row() into test program.
This function is not useful inside ovsdb itself but only in the
"test-ovsdb" test program. To avoid the temptation to use it incorrectly
inside ovsdb, this commit moves it into the test program.
Ben Pfaff [Wed, 1 Jun 2011 23:14:46 +0000 (16:14 -0700)]
ovsdb: Make ovsdb_column_set_from_json() take table schema instead of table.
This function took a struct ovsdb_table but only used the 'schema' member.
An upcoming patch needs to parse a column set when only the schema is
available, so to prepare for that this patch changes
ovsdb_column_set_from_json() to only take the schema that it really needs.
Jesse Gross [Thu, 2 Jun 2011 22:50:47 +0000 (15:50 -0700)]
tunneling: Warn if CAPWAP is not available on this kernel.
For most of our kernel module we support back to 2.6.18 but we only
work with 2.6.26 or newer for CAPWAP. On later kernels support for
CAPWAP silently disappears. In these situations, emit a warning so
that people aren't suprised when their tunnels fail.
Signed-off-by: Jesse Gross <jesse@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Ethan Jackson [Fri, 27 May 2011 17:48:58 +0000 (10:48 -0700)]
ofproto: Optimize datapath actions.
The translation from ofproto to datapath actions has historically
been fairly naive. Since redundant ODP actions result in more code
execution in the fast path, this patch changes xlate_actions() to
emit fewer ODP actions for a given flow.
This change is not simply a theoretical optimization. It actually
reduces the number of ODP actions for real flow tables which
Nicira's controllers use in practice. Furthermore, removing
unnecessary modification actions after the last output action has
the added benefit of saving a whole skb_clone() in the fast path.
Ben Pfaff [Wed, 1 Jun 2011 20:39:51 +0000 (13:39 -0700)]
datapath: Get packet metadata from userspace in odp_packet_cmd_execute().
Until now, the tun_id and in_port have been lost when a packet is sent from
the kernel to userspace and then back to the kernel. I didn't think that
this was a problem, but recent behavior made me look closer and see that
it makes a difference if sFlow is turned on or if an
ODP_ATTR_ACTION_CONTROLLER action is present. We could possibly kluge
around those, but for future-proofing it seems better to pass the packet
metadata from userspace to the kernel. That is what this commit does.
This commit introduces a user-kernel protocol break. We could avoid that,
if it is desirable, by making ODP_PACKET_ATTR_KEY optional for
ODP_PACKET_CMD_EXECUTE commands.
Signed-off-by: Ben Pfaff <blp@nicira.com> Acked-by: Jesse Gross <jesse@nicira.com>
Ben Pfaff [Wed, 1 Jun 2011 17:53:53 +0000 (10:53 -0700)]
learning-switch: Don't limit message queued by --with-flows.
queue_tx() intentionally limits the number of outstanding OpenFlow messages
queued to the switch. This was unintentionally being applied to the
messages queued to the switch at startup by ovs-ofctl's --with-flows
command. This patch should fix the problem, by calling rconn_send()
directly instead of through queue_tx().
Ahmed reported that with this patch there was still a problem when 30,000
flows were specified in the file.