We currently drop RTNL before adding a new datapath to sysfs but
then access the dp data structures. This moves the call to
dp_sysfs_add_dp() before we drop the locks to prevent a potential
race. All other calls to sysfs functions already hold RTNL.
Found with lockdep.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Mark functions and global variables used only in a single file as
static.
Found with sparse.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Checksum offloading has changed quite a bit across different kernel
and Xen versions. Since it is part of the skb data structure it is
unfortunately difficult to separate out into compatibility code.
This consolidates all of the checksum code in one place which makes
it easier read and remove as we prepare for upstreaming. On newer
kernels it also puts everything in inline functions, eliminating the
need to run through the compat code or make extra function calls.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
These steps are sequentially in lockstep, so we might as well combine them.
This expands the region over which the vport_lock is held. I didn't
carefully verify that this was OK.
This also eliminates the synchronize_rcu() call from destruction of tunnel
vports, since they didn't appear to me to need it.
It should be possible to eliminate the synchronize_rcu() from the netdev,
patch, and internal_dev vports, but this commit does not do that.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
After the previous commit, which changed the datapath to always create and
attach a vport at the same time, and to always detach and delete a vport
at the same time, there is no longer any real distinction between a dp_port
and a vport. This commit, therefore, merges the two together to simplify
code. It might even improve performance, although I have not checked.
I wasn't sure at first whether the merged structure should be "struct
dp_port" or "struct vport". I went with the latter since the "v" prefix
sounds cool.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
For some time now, Open vSwitch datapaths have internally made a
distinction between adding a vport and attaching it to a datapath. Adding
a vport just means to create it, as an entity detached from any datapath.
Attaching it gives it a port number and a datapath. Similarly, a vport
could be detached and deleted separately.
After some study, I think I understand why this distinction exists. It is
because ovs-vswitchd tries to open all the datapath ports before it tries
to create them. However, changing it to create them before it tries to
open them is not difficult, so this commit does this.
The bulk of this commit, however, changes the datapath interface to one
that always creates a vport and attaches it to a datapath in a single step,
and similarly detaches a vport and deletes it in a single step.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
Upcoming commits will keep needing to pass more information to the vport
'create' member function. It's annoying to have to modify a dozen pieces
of code every time just to do this, so this commit encapsulates all of the
parameters in a new struct and passes that instead.
Acked-by: Jesse Gross <jesse@nicira.com>
Variables which are changed only infrequently should be annotated
with __read_mostly, which will group them together in a special
linker section. This prevents them from sharing cache lines with
data on the hot path.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Our normal loop detection requires disabling preemption while
packet processing takes place. On RT kernels this isn't acceptable
and interacts badly with spinlocks, so we can't use it. This
takes advantage of some extra space that is added to struct
task_struct on RT kernels (and the knowledge that we will always
have a valid task_struct) to store the loop counter for a given
thread. Since we can't make these assumptions on non-RT kernels,
we continue to use the previous method of loop detection there.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
There's no need to have a mask in this action, because both parts of the
TCI are part of the flow structure.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
Commit a01ef04ce "datapath: Drop padding from struct odp_flow_key" removed
the "reserved" member from struct odp_flow_key but overlooked uses of that
member from 64-bit compatibility code. This commit fixes up the problem.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
This commit fixes a problem where datapath flow used stats can report
incorrent value, thus may result in flow expirations being incorrect.
This happens when a kernel jiffies rollover occurs between the last time
a flow is hit and the flow stats is queried.
Acked-by: Jesse Gross <jesse@nicira.com>
Signed-off-by: Hao Zheng <hzheng@nicira.com>
These functions run 99% of the time in atomic context and the benefit of
passing along the 'gfp' argument for the other 1% doesn't seem to outweigh
the cost.
Suggested-by: Stephen Hemminger <shemminger@vyatta.com>
Acked-by: Jesse Gross <jesse@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Breaking this out as a separate commit should make it easier to see what
needs to change later, if we need to reintroduce padding at some point.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
This allows eliminating padding from odp_flow_key, although actually doing
that is postponed until the next commit.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
The "port group" concept seems like a good one, but it has not been
used very much in userspace so far, so before we commit ourselves to
a frozen API that we must maintain forever, remove it. We can always
add it back in later as a new kind of vport.
Signed-off-by: Ben Pfaff <blp@nicira.com>
An upcoming commit will use some workqueue functions that weren't
available on earlier kernels, so this backports those functions.
The backporting uses timers instead of delayed work queues because
the earlier versions of work queues have some unsafe corner cases.
In addition, this removes some unused work queue backporting code
that is no longer used because it is potentially unsafe.
Note that this commit changes the behavior of work queues: normally
they run in process context but the backported version runs in
softirq context.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Reviewed-by: Ben Pfaff <blp@nicira.com>
is_frag is only used for communication between two functions, which
means that it doesn't really need to be in the SKB CB. This wouldn't
necessarily be a problem except that there are also a number of other
paths that lead to this being uninitialized. This isn't a problem
now but uninitialized memory seems dangerous and there isn't much
upside.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Reviewed-by: Ben Pfaff <blp@nicira.com>
An upcoming commit will add support for supplying cached flows for
packets entering the datapath. This adds the code in the datapath
itself to recognize these cached flows and use them instead of
extracting the flow fields and doing a lookup.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Reviewed-by: Ben Pfaff <blp@nicira.com>
Currently flows are only used within the confines of one
rcu_read_lock()/rcu_read_unlock() session. However, with the
addition of header caching we will need to hold references to flows
for longer periods of time. This adds support for that by adding
refcounts to flows. RCU is still used for normal packet handling
to avoid a performance impact from constantly updating the refcount.
However, instead of directly freeing the flow after a grace period
we simply decrement the refcount.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Reviewed-by: Ben Pfaff <blp@nicira.com>
As the process to allocate a flow becomes more involved it becomes
more cumbersome for the code to be mixed in with the general
datapath so split it out into a new function.
Signed-off-by: Jesse Gross <jesse@nicira.com>
Reviewed-by: Ben Pfaff <blp@nicira.com>
Nothing uses it anymore and it causes problems when backported on
some distributions. Kernels we support have net_random(), which
is the same thing so there is no reason to have an entire copy of
the random number generator in our source tree.
Reported-by: Alexey I. Froloff <raorn@altlinux.org>
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
flow_actions_alloc() returns an error code in the form of a pointer
but we checked that the pointer was not NULL, which is always true.
This caused oopses on allocation errors when we would write into
an invalid pointer.
NIC-234
Signed-off-by: Jesse Gross <jesse@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Signed-off-by: Joe Perches <joe@perches.com>
Acked-by: Simon Horman <horms@verge.net.au>
[Jesse: Added missing pr_fmt in vport-gre.c and dp_sysfs_dp.c]
Signed-off-by: Jesse Gross <jesse@nicira.com>
Until now flow_extract() has simply returned a bogus flow when memory
allocation errors occurred. This fixes the problem by propagating the
error to the caller.
Signed-off-by: Ben Pfaff <blp@nicira.com>
flow_extract() can fail due to memory allocation errors in pskb_may_pull().
Currently it doesn't return those properly, instead just reporting a bogus
flow to the caller. But its return value is currently in use for reporting
whether the packet was an IPv4 fragment. This commit switches to reporting
that in the skb itself so that the return value can be reused to report
errors.
Signed-off-by: Ben Pfaff <blp@nicira.com>
We enforce mutual exclusion when updating statistics by disabling
bottom halves and only writing to per-CPU state. However, reading
requires looking at the statistics for foreign CPUs, which could be
in the process of updating them since there isn't a lock. This means
we could get garbage values for 64-bit values on 32-bit machines or
byte counts that don't correspond to packet counts, etc.
This commit introduces a sequence lock for statistics values to avoid
this problem. Getting a write lock is very cheap - it only requires
incrementing a counter plus a memory barrier (which is compiled away
on x86) to acquire or release the lock and will never block. On
read we spin until the sequence number hasn't changed in the middle
of the operation, indicating that the we have a consistent set of
values.
Signed-off-by: Jesse Gross <jesse@nicira.com>
In-kernel loops need to be suppressed; otherwise, they cause high CPU
consumption, even to the point that the machine becomes unusable. Ideally
these flows should never be added to the Open vSwitch flow table, but it
is fairly easy for a buggy controller to create them given the menagerie
of tunnels, patches, etc. that OVS makes available.
Commit ecbb6953b "datapath: Add loop checking" did the initial work
toward suppressing loops, by dropping packets that recursed more than 5
times. This at least prevented the kernel stack from overflowing and
thereby OOPSing the machine. But even with this commit, it is still
possible to waste a lot of CPU time due to loops. The problem is not
limited to 5 recursive calls per packet: any packet can be sent to
multiple destinations, which in turn can themselves be sent to multiple
destinations, and so on. We have actually seen in practice a case where
each packet was, apparently, sent to at least 2 destinations per hop, so
that each packet actually consumed CPU time for 2**5 == 32 packets,
possibly more.
This commit takes loop suppression a step further, by clearing the actions
of flows that are implicated in loops. Thus, after the first packet in
such a flow, later packets for either the "root" flow or for flows that
it ends up looping through are simply discarded, saving a huge amount of
CPU time.
This version of the commit just clears the actions from the flows that a
part of the loop. Probably, there should be some additional action to tell
ovs-vswitchd that a loop has been detected, so that it can in turn inform
the controller one way or another.
My test case was this:
ovs-controller -H --max-idle=permanent punix:/tmp/controller
ovs-vsctl -- \
set-controller br0 unix:/tmp/controller -- \
add-port br0 patch00 -- \
add-port br0 patch01 -- \
add-port br0 patch10 -- \
add-port br0 patch11 -- \
add-port br0 patch20 -- \
add-port br0 patch21 -- \
add-port br0 patch30 -- \
add-port br0 patch31 -- \
set Interface patch00 type=patch options:peer=patch01 -- \
set Interface patch01 type=patch options:peer=patch00 -- \
set Interface patch10 type=patch options:peer=patch11 -- \
set Interface patch11 type=patch options:peer=patch10 -- \
set Interface patch20 type=patch options:peer=patch21 -- \
set Interface patch21 type=patch options:peer=patch20 -- \
set Interface patch30 type=patch options:peer=patch31 -- \
set Interface patch31 type=patch options:peer=patch30
followed by sending a single "ping" packet from an attached Ethernet
port into the bridge. After this, without this commit the vswitch
userspace and kernel consume 50-75% of the machine's CPU (in my KVM
test setup on a single physical host); with this commit, some CPU is
consumed initially but it converges on 0% quickly.
A more challenging test sends a series of packets in multiple flows;
I used "hping3" with its default options. Without this commit, the
vswitch consumes 100% of the machine's CPU, most of which is in the
kernel. With this commit, the vswitch consumes "only" 33-50% CPU,
most of which is in userspace, so the machine is more responsive.
A refinement on this commit would be to pass the loop counter down to
userspace as part of the odp_msg struct and then back up as part of
the ODP_EXECUTE command arguments. This would, presumably, reduce
the CPU requirements, since it would allow loop detection to happen
earlier, during initial setup of flows, instead of just on the second
and subsequent packets of flows.
Originally, the datapath didn't care about IP TOS at all. Then, to support
NetFlow, we made it keep track of the last-seen IP TOS value on a per-flow
basis. Then, to support OpenFlow 1.0, we added a nw_tos field to
odp_flow_key. We don't need both methods, so this commit drops the
NetFlow-specific tracking.
This introduces a small kernel ABI break: upgrading the kernel module
without upgrading the OVS userspace will mean that NetFlow records will
all show an IP TOS value of 0. I don't consider that to be a serious
problem.
When we detached a vport we would assign NULL to dp_port->vport
before calling synchronize_rcu(). However, since vports have a
longer lifetime than dp_ports there were no checks before
dereferencing dp_port->vport. This changes the behavior to
match the assumption by not assigning NULL during detach. This
avoids a potential NULL pointer dereference in do_output() among
other places.
On vport ingress we already check for shared SKBs but then later
warn in several other places. In a similar vein, we check every
packet to see if it is LRO but only certain vports can produce
these packets. Remove and consolidate checks to the places where
they are needed.
Rather than actually query the time every time a packet comes through,
just store the current jiffies and convert it to actual time when
requested. GRE is the primary beneficiary of this because the traffic
travels through the datapath twice. This change reduces CPU utilization
3-4% with GRE.
In some places we would put the return type on the same line as
the rest of the function definition and other places we wouldn't.
Reformat everything to match kernel style.
The offsets for checksum offsets should always be positive so make
that explicit by using unsigned ints. This helps bug checks that
test if the offsets are greater than their upper limits.
Our code that handles checksumming does essentially the same thing
as skb_checksum_help() except it folds the process into copying to
userspace. This makes the two functions more closely resemble
each other in structure, including adding a couple of BUG() checks.
This should have no functional change but makes comparision easier
when debugging.
linux/highmem.h should be included rather than asm/highmem.h,
otherwise openvswitch_mod cannot resolve kmap and kunmap on some arch.
Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>
If we are only copying part of a packet to userspace don't bother
computing the checksum on that part since it is meaningless.
Instead, fall back to the old method of checksumming before
copying to ensure the correct result.
This was supposed to be part of the previous commit but was left
off.
Currently we compute the checksums on packets being sent to
userspace first and then copy them to a userspace buffer. However,
these two operations can be combined for a significant savings
because the packet data only has to be loaded once. This also
allows GSO packets to save an extra copy.
This will likely have an impact on NIC-121 because it eliminates
the code path that triggers the issue. However, it is not a fix
for the root cause.
Since GSO computes checksums as it does segmentation, we need to
setup the checksum pointers before calling skb_gso_segment(). Failing
to do so can potentially result in warnings, incorrect checksums,
crashes, or redundant checksum computation. In general we don't
hit this case because the code path is run during the first packet
in a flow, which is generally not a large GSO packet.
This was found during the investigation of NIC-121 but has no impact
on it because vswitch_skb_checksum_setup() is a no-op on 2.6.27-based
XenServer kernels.
Adds a method to set a group of stats to be added to the values
gathered normally. This is needed for the fake bond device to
show the stats of its underlying slaves. Also enables devices
that use the generic stats layer to define a get_stats() function
to provide additional error counts.
The vport library can be accessed from both userspace and the
kernel using different sets of functions. These functions were
named similarly, so add _user to the userspace variants to
distinguish them.
The final argument to do_get_port_group() is supposed to be a user pointer
to the number of ports, to be updated with put_user(), but it was actually
a kernel pointer, so "ovs-dpctl dump-groups" and anything else that used
this ioctl would always fail with -EFAULT. This commit fixes it.
Bug introduced in commit 44e05eca "datapath: Prepare to support 32-bit
compatibility ioctls" for normal ioctls and for compat ioctls at their
introduction in commit 3fbd517acf"datapath: Add 32-bit compatibility
ioctls."
Many of the vport operations require that either RTNL lock or
rcu_read_lock be held. However, operations from userspace often
hold a different lock so grab rcu_read_lock as well.
We currently document that BHs need to be disabled when handling
received packets. However, this isn't actually generally the
case (usually preemption is disabled but not BHs). Only one place
actually relies on BHs being disabled so fix that and update the
documentation of our expectations.
Places that update per-cpu stats without locking need to have bottom
halves disabled. Otherwise we can be running in process context and
in the middle of an operation and be interrupted by a softirq.
We are never called in hardirq context - only process or softirq.
Therefore it is not necessary to disable interrupts with
spin_lock_irqsave(), so use spin_lock_bh() everywhere.
When a 32-bit userspace program runs on a 64-bit kernel, data structures
that contain members whose sizes or alignments change from 32- to 64-bit
must be translated when they are passed to ioctls. This commit adds such
support for openvswitch_mod.
We should really reconsider some parts of the Open vSwitch ioctl interface
to avoid needing as much translation as we do.
Lightly tested with 32-bit userspace on sparc64.
