Ethan Jackson [Wed, 17 Sep 2014 20:22:14 +0000 (13:22 -0700)]
ofproto: Warn about excessive rule counts in OpenFlow tables.
Frequently we've run into controller bugs which result in hundreds of
thousands, or even millions of rules being installed in an OpenFlow
table. This isn't something trouble-shooters naturally think of to
check for, so it's nice to have a low rate warning message to hint at
the potential problem.
Signed-off-by: Ethan Jackson <ethan@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
dpif-netdev: Store miniflow length in exact match cache
This optimization is done to avoid calling count_1bits(), which, if
the popcnt istruction is not available might is slow. popcnt may not
be available because:
- We are running on old hardware
- (more likely) We're using a generic build (i.e. packaged OVS from a
distro), not tuned for the specific CPU
Signed-off-by: Daniele Di Proietto <ddiproietto@vmware.com> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
netdev_flow_key is a miniflow with the following constraints:
1) It is used only inside dpif-netdev.c.
2) It always has inline values.
3) It contains only miniflows created by miniflow_extract().
Therefore, by using these new functions instead of the miniflow_*
ones, we get the following (performance related) benefits:
- Because of (1) the functions can be inlined.
- Because of (2) and (3) the netdev_flow_key can be treated as POD.
Specifically, because of (3), we can do comparisons with memcmp,
since if the map is different the miniflow must be different.
Signed-off-by: Daniele Di Proietto <ddiproietto@vmware.com> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
datapath-windows: use the Netlink set API and need new APIs
In this change:
1. we refactor the code that fills up information about the DP into
a seprate function.
2. use the netlink set APIs to fill up the netlink attributes.
3. we define a OVS_DP_STATS to be a typedef of 'struct ovs_dp_stats'
in keeping with the Windows kernel naming conventions.
4. In the absence of netlink set API, I had put in an ASSERT earlier
that the output buffer should be limited to 512 bytes. This is not
true anymore. The netlink set API checks for bounds of the buffer.
Hence removed the ASSERT.
ofproto-dpif-xlate: Suppress some warnings on non-Linux OSes
These warnings were introduced by
commit 7d031d7e511aeea8dd45348922fe8e3bbdd2956e
("ofproto-dpif-xlate: Work around Linux netdev_max_backlog limit.")
and found by --enable-Werror build on NetBSD.
Signed-off-by: YAMAMOTO Takashi <yamamoto@valinux.co.jp> Signed-off-by: Ben Pfaff <blp@nicira.com>
To prevent warnings such as "Not all control paths return a value",
we should define NO_RETURN for MSVC.
Currently for gcc, we add NO_RETURN at the end of function declaration.
But for MSVC, "__declspec(noreturn)" is needed at the beginning of function
declaration. So this commit moves NO_RETURN to the beginning of the function
declaration as it works with gcc and clang too.
Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
MSVC does not support c11 style atomics for the C compiler.
Windows has different InterLocked* functions for different data
sizes. ovs-atomic-msvc.h maps the api in ovs-atomic.h (which is similar
to c11 atomics) to the available atomic functions in Windows. In some
cases, this causes compiler warnings about mismatched data sizes because
the generated code has 'if else' conditions on different data sizes and
proper casting is not possible.
In current OVS code base, we get one compiler warning through ovs-rcu.h
which says "‘void *’ differs in levels of indirection from LONGLONG."
This comes from the following in ovs-atomic-msvc.h for atomic_read64():
*(DST) = InterlockedOr64((int64_t volatile *) (SRC), 0);
when *DST is a void pointer (because InterLockedOr64 returns LONGLONG).
But this code path is only every hit for 64 bit data. So it should be safe to
disable the warning. (Any real bugs in api calls would hopefully be caught
while compiling on Linux using gcc/clang).
Alex Wang [Mon, 15 Sep 2014 20:01:12 +0000 (13:01 -0700)]
netdev-dpdk: Make get_config() report correct queue info.
With the separation of tx queue and rx queue configuration
in netdev-dpdk module, the netdev_dpdk_get_config() can no
longer report 'n_rxq' as tx queue configuration.
This commit fixes the above issue.
Reported-by: Daniele Di Proietto <ddiproietto@vmware.com> Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Daniele Di Proietto <ddiproietto@vmware.com>
Alex Wang [Fri, 5 Sep 2014 21:14:20 +0000 (14:14 -0700)]
dpif-netdev: Create multiple pmd threads by default.
With this commit, ovs by default will create one pmd thread
for each numa node and pin the pmd thread to available cpu
core on the numa node.
NON_PMD_CORE_ID (currently 0) is used to reserve a particular
cpu core for the I/O of all non-pmd threads. No pmd thread
can be pinned to this reserved core.
As side-effects of this commit:
- pmd thread will not be created, if there is no dpdk interface
from the corresponding numa node added to ovs.
- the exact-match cache for non-pmd threads is removed from
'struct dp_netdev'. Instead, all non-pmd threads will use
the exact-match cache defined in the 'struct dp_netdev_pmd_thread'
for NON_PMD_CORE_ID.
- the rx packet processing functions are refactored to use
'struct dp_netdev_pmd_thread' as input.
- the 'netdev_send()' function will be called with the proper
queue id.
- both pmd and non-pmd threads can call the dpif_netdev_execute().
so, use a per-thread key to help recognize the calling thread.
Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Pravin B Shelar <pshelar@nicira.com>
Alex Wang [Fri, 5 Sep 2014 17:56:18 +0000 (10:56 -0700)]
netdev-dpdk: Remove the tx queue spinlock.
The previous commit makes OVS create one tx queue for each
cpu core, each pmd thread will use a separate tx queue.
Also, tx of non-pmd threads on dpdk interface is all through
'NON_PMD_THREAD_TX_QUEUE', protected by the 'nonpmd_mempool_mutex'.
Therefore, the spinlock is no longer needed. And this commit
removes it from 'struct dpdk_tx_queue'.
Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Pravin B Shelar <pshelar@nicira.com>
Alex Wang [Thu, 4 Sep 2014 20:09:22 +0000 (13:09 -0700)]
netdev-dpdk: Add indicator for flushing tx queue.
Previous commit makes OVS create one tx queue for each cpu
core. An upcoming patch will allow multiple pmd threads be
created and pinned to cpu cores. So each pmd thread will use
the tx queue corresponding to its core id.
Moreover, the pmd threads running on different numa node than
the dpdk interface (called non-local pmd thread) will not
handle the rx of the interface. Consequently, there need to
be a way to flush the tx queues of the non-local pmd threads.
To address the queue flushing issue, this commit introduces a
new flag 'flush_tx' in the 'struct dpdk_tx_queue' which is
set if the queue is to be used by a non-local pmd thread.
Then, when enqueueing the tx pkts, if the flag is set, the tx
queue will always be flushed immediately after the enqueue.
Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Pravin B Shelar <pshelar@nicira.com>
Alex Wang [Tue, 17 Jun 2014 17:52:20 +0000 (10:52 -0700)]
dpif-netdev: Create multiple tx/rx queues when adding dpdk interface.
Before this commit, ovs creates one tx and one rx queue for
each dpdk interface and uses only one poll thread for handling
I/O of all dpdk interfaces. An upcoming patch will allow multiple
poll threads be created. As a preparation, this commit changes
the dpif-netdev to create multiple tx/rx queues when the dpdk
interface is added.
Specifically, the number of rx queues will still be one per-dpdk
interface for this commit. But upcoming work will allow user
create multiple rx queues. The number of tx queues will be the
number of cpu cores on the machine. Although not all the tx queues
will be used, each poll thread will have its own queue for
transmission on the dpdk interface.
Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Pravin B Shelar <pshelar@nicira.com>
Alex Wang [Mon, 8 Sep 2014 21:52:54 +0000 (14:52 -0700)]
netdev: Add function for configuring tx and rx queues.
This commit adds a new API to the 'struct netdev_class' which
allows user to configure the number of tx queues and rx queues
of 'netdev'. Upcoming patches will use this function to set
multiple tx/rx queues when adding the netdev to dpif-netdev.
Currently, only netdev-dpdk module implements this function.
Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Pravin B Shelar <pshelar@nicira.com>
ofproto: Do not update stats on fake bond interface.
There are couple of reasons to remove this support:
* This is used in very old OVS use-case. It is much better
to read stats directly from OVS.
* Forthcoming commit will remove support for setting stats
for vport. The stats update depends on stats-set.
Signed-off-by: Pravin B Shelar <pshelar@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Andy Zhou [Wed, 10 Sep 2014 22:36:06 +0000 (15:36 -0700)]
datapath: Improve robustness of this_cpu_ptr definition in compat layer
Current autoconfig detection logic for HAVE_PER_CPU_PTR is not robust.
Depends on linux kernel version, the definition can be in either
linux/percpu.h or asm/percpu.h
Turns out it is simpler and safer to handle missing percpu.h
definitions in linux/percpu.h rather than asm/percpu.h. With this
change, there is no need for the autoconfig detection logic above.
Signed-off-by: Andy Zhou <azhou@nicira.com> Acked-by: Pravin B Shelar <pshelar@nicira.com>
Joe Stringer [Fri, 12 Sep 2014 06:03:56 +0000 (06:03 +0000)]
ofproto: Increase default datapath max_idle time.
The datapath max_idle value determines how long to wait before deleting
an idle datapath flow when operating below the flow_limit. This patch
increases the max_idle to 10 seconds, which allows datapath flows to be
remain cached even if they are used less consistently, and provides a
small improvement in the supported number of flows when operating around
the flow_limit.
Signed-off-by: Joe Stringer <joestringer@nicira.com> Acked-by: Ethan Jackson <ethan@nicira.com>
WANG Cong [Fri, 12 Sep 2014 21:12:24 +0000 (14:12 -0700)]
openvswitch: rename ->sync to ->syncp
Openvswitch defines u64_stats_sync as ->sync rather than ->syncp,
so fails to compile with netdev_alloc_pcpu_stats(). So just rename it to ->syncp.
Reported-by: kbuild test robot <fengguang.wu@intel.com> Fixes: 1c213bd24ad04f4430031 (net: introduce netdev_alloc_pcpu_stats() for drivers) Cc: David S. Miller <davem@davemloft.net> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Reviewed-by: Flavio Leitner <fbl@redhat.com> Signed-off-by: David S. Miller <davem@davemloft.net> Acked-by: Pravin B Shelar <pshelar@nicira.com>
WANG Cong [Fri, 12 Sep 2014 21:05:11 +0000 (14:05 -0700)]
datapath: introduce netdev_alloc_pcpu_stats() for drivers
There are many drivers calling alloc_percpu() to allocate pcpu stats
and then initializing ->syncp. So just introduce a helper function for them.
Cc: David S. Miller <davem@davemloft.net> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> Acked-by: Pravin B Shelar <pshelar@nicira.com>
This patch introduces the use of the macro IS_ERR_OR_NULL in place of
tests for NULL and IS_ERR.
The following Coccinelle semantic patch was used for making the change:
@@
expression e;
@@
- e == NULL || IS_ERR(e)
+ IS_ERR_OR_NULL(e)
|| ...
Signed-off-by: Himangi Saraogi <himangi774@gmail.com> Acked-by: Julia Lawall <julia.lawall@lip6.fr> Acked-by: Pravin B Shelar <pshelar@nicira.com> Signed-off-by: David S. Miller <davem@davemloft.net>
Jean Sacren [Fri, 12 Sep 2014 18:31:27 +0000 (11:31 -0700)]
datapath: fix duplicate #include headers
The #include headers net/genetlink.h and linux/genetlink.h both were
included twice, so delete each of the duplicate.
Signed-off-by: Jean Sacren <sakiwit@gmail.com> Cc: Pravin Shelar <pshelar@nicira.com> Cc: dev@openvswitch.org Signed-off-by: David S. Miller <davem@davemloft.net> Acked-by: Pravin B Shelar <pshelar@nicira.com>
datapath: Replace rcu_dereference() with rcu_access_pointer()
The "rcu_dereference()" call is used directly in a condition.
Since its return value is never dereferenced it is recommended to use
"rcu_access_pointer()" instead of "rcu_dereference()".
Therefore, this patch makes the replacement.
The following Coccinelle semantic patch was used:
@@
@@
Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com> Signed-off-by: David S. Miller <davem@davemloft.net> Acked-by: Pravin B Shelar <pshelar@nicira.com>
Alex Wang [Wed, 3 Sep 2014 21:37:35 +0000 (14:37 -0700)]
netdev: Add n_txq to 'struct netdev'.
This commit adds new variable n_txq to 'struct netdev' for recording
the number of tx queues. Correspondingly, the send_*() functions are
extended to accept queue id as input argument.
All 'netdev-*' implementation will ignore the queue id since having
multiple tx queues is not supported. Upcomping patches will start
using it and create multiple tx queues for dpdk netdev.
Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Pravin B Shelar <pshelar@nicira.com>
Alex Wang [Tue, 9 Sep 2014 18:01:52 +0000 (11:01 -0700)]
ovs-rcu: Make ovsrcu_quiesce() flush the callback event set.
On current master, the per-thread callback event set is flushed
when ovsrcu_quiesce_start() is called or when the callback
event set is full. For threads that only call 'ovsrcu_quiesce()'
to indicate quiescient state, their callback event set will not
be flushed for execution until the set is full. And this could
take a very long time.
Theoretically, this should not be an issue, since rcu postponed
callback events should only free the old version of objects.
However, current ovs does not follow this rule, and some callback
events include other activities like unregistering the netdev
from global name-netdev map. The delay of unregistering the netdev
(by threads that only calls ovsrcu_quiesce()) will prevent the
recreate of same netdev indefinitely.
As a short-term workaround, this commit makes every call to
ovsrcu_quiesce() flush the callback event set. In the long run,
there will be a refactor of the use of ovs-rcu module, in which all
callback events only free the old version of objects.
Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
debian: Don't depened on $RUNLEVEL at startup to create bridges.
Commit b2a0daa5bd (debian: Don't recreate bridges during manual restart.)
added a check on $RUNLEVEL to only create bridges and ports when the
system starts up. This fix does not work with systemd.
This commit uses a different approach to solve the same problem.
Reported-at: https://bugs.debian.org/686518 Reported-by: Philipp S. Schmidt <phils@in-panik.de> Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> Tested-by: Philipp S. Schmidt <phils@in-panik.de>
Avoid uninitialized variable warnings with OBJECT_OFFSETOF() in MSVC.
Implementation of OBJECT_OFFSETOF() for non-GNUC compilers like MSVC
causes "uninitialized variable" warnings. Since OBJECT_OFFSETOF() is
indirectly used through all the *_FOR_EACH() (through ASSIGN_CONTAINER()
and OBJECT_CONTAINING()) macros, the OVS build
on Windows gets littered with "uninitialized variable" warnings.
This patch attempts to workaround the problem.
Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> Acked-by: Saurabh Shah <ssaurabh@vmware.com> Acked-by: Ben Pfaff <blp@nicira.com>
ofproto-dpif: Probe for userdata after backer is fully operational.
When probing for variable length userdata before handler threads are
set, the pid included in the userspace action will be 0, which is
flagged as an error by the linux kernel datapath. As a result the
feature probe will produce an unnecessary log message. By probing for
variable length userdata later the probe works as intended and the
unnecessary log message is avoided.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Thomas Graf [Thu, 11 Sep 2014 19:34:22 +0000 (21:34 +0200)]
travis: Fix DPDK build and treat bad-function-cast warning as non-error
A missing " prevented the DPDK build in the matrix from functioning
so far. This patch enables the DPDK build by properly building DPDK
as a single library and by pointing the OVS build to the corresponding
build directory. Also removes the 'make install' as it is not required
and only slows down the build.
Due to incorrect casts in the DPDK headers, we have to disable
bad-function-cast and cast-align warnings as being treated as errors
for now.
Signed-off-by: Thomas Graf <tgraf@noironetworks.com> Co-authored-by: Daniele Di Proietto <ddiproietto@vmware.com> Signed-off-by: Ben Pfaff <blp@nicira.com>
Thomas Graf [Thu, 11 Sep 2014 19:34:21 +0000 (21:34 +0200)]
build: Respect CFLAGS and LDFLAGS passed to make
configure cannot expect that the user will not pass additional CFLAGS
and LDFLAGS at make time [0]. Use OVS_CFLAGS and OVS_LDFLAGS instead to
collect compiler and linker flags and substitute in Makefile.am.
This allows for:
./configure --with-dpdk=[...]
make CFLAGS=-Wno-error=foo
netlink-socket: Convert from error number to string correctly.
As mentioned in the comment above the function ovs_strerror(), it
should not be used to convert WINAPI error numbers to string.
Use ovs_lasterror_to_string() instead.
Signed-off-by: Gurucharan Shetty <gshetty@nicira.com> Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
definition of __ip_select_ident() changed in newer kernel and
it is backported to stable kernel, Therefore adding configure
check to detect the new function.
Signed-off-by: Pravin B Shelar <pshelar@nicira.com> Acked-by: Andy Zhou <azhou@nicira.com>
Access to RSTP and RSTP port objects is protected by a mutex, so the
refcount unref operation can have relaxed memory order semantics (See
commit 24f8381214966e90819bf4a9ecabf076cbfc1b08).
Ben Pfaff [Tue, 9 Sep 2014 22:06:52 +0000 (15:06 -0700)]
ofproto-dpif-xlate: Work around Linux netdev_max_backlog limit.
Linux has an internal queue that temporarily holds packets transmitted to
certain network devices. If too many packets are transmitted to such
network devices within a single list of actions, then packets tend to get
dropped. Broadcast or flooded or multicast packets on bridges with
thousands of ports are examples of how this can occur.
This commit avoids the problem by implementing a flow in userspace when it
outputs its packet more times than the maximum length of the queue.
The frag member in the Netlink interface is an uint8_t enumeration
type, not a bitfield, so it should always be either fully masked or
not masked at all.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
lib/odp-util: Skip ignored fields when parsing and formatting.
When a whole field of a key value is ignored, skip it when formatting
the key, and allow it to be left out when parsing the key from a
string. However, when the 'verbose' formatting is requested those are
still formatted, as it may help in debugging.
Now the named key fields can also be given in arbitrary order.
Duplicate field values are not checked for, so the last one will
remain in effect.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
lib/odp-execute: Use dpif_packet_set_dp_hash() instead of ->dp_hash
When building with DPDK support, 'struct dpif_packet' won't have 'dp_hash'
member. dpif_packet_set_dp_hash() and dpif_packet_get_dp_hash() should be used.
Furthermore, the masked set action shouldn't read 'md->dp_hash' (which is
shared in a batch), but should use dpif_packet_get_dp_hash() to get each packet
private hash.
This commit fixes the build with DPDK.
Signed-off-by: Daniele Di Proietto <ddiproietto@vmware.com> Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
datapath-windows: refactor code to setup dump start state
Per review comment, in this patch we refactor the code to create a
OvsSetupDumpStart() which can be leveraged by dump functions in the
future. I have not refactored the code that continues the dump
operation primarily since it is not final yet. Once the netlink set
APIs are in place, we can refactor that too.
Signed-off-by: Nithin Raju <nithin@vmware.com> Acked-by: Ankur Sharma <ankursharma@vmware.com> Acked-by: Samuel Ghinet <sghinet@cloudbasesolutions.com> Signed-off-by: Ben Pfaff <blp@nicira.com>
netlink-socket: Add support for async notification on Windows.
We keep an outstanding, out of band, I/O request in the driver at all time.
Once an event generated the driver queues the event message, completes the
pending I/O and unblocks the calling thread through setting the event in the
overlapped structure in the NL socket. The thread will read all all event
messages synchronously through the call of nl_sock_recv()
Signed-off-by: Eitan Eliahu <eliahue@vmware.com> Acked-by: Samuel Ghinet <sghinet@cloudbasesolutions.com> Acked-by: Ankur Sharma <ankursharma@vmware.com> Acked-by: Alin Gabriel Serdean <aserdean at cloudbasesolutions.com> Acked-by: Saurabh Shah <ssaurabh@vmware.com> Signed-off-by: Ben Pfaff <blp@nicira.com>
Current code expects there to be a single thread that is responsible
for creating rstp and creating and deleting rstp_port objects. rstp
objects are also deleted from other threads, as managed by reference
counting.
rstp port objects are not reference counted, which means that
references to rstp ports may only be held while holding the rstp
mutex, or by the thread that creates and deletes them.
This patch adds reference counting to RSTP ports, which allows ports
to be passed from ofproto-dpif to ofproto-dpif-xlate without using the
RSTP port number. This simplifies RSTP port reconfiguration, as the
port need not be resynchronized with xlate if just the port number
changes. This also avoids lookups on the processing of RSTP BPDUs.
This patch also:
1. Exposes the rstp mutex so that related thread safety annotations
can be used also within rstp-state-machines.c.
2. Internal variants of most setter an getter functions are defined,
suffixed with two underscores. These are annotated to be callable
only when the mutex is held.
3. Port setters were only called in a specific pattern. The new external
port setter combines them in a single rspt_port_set() function.
dpif_netdev_execute may be called while doing upcall processing.
Since the context of the input port is not tracked upto this point, we
use the shared dp->emc_cache for packet execution, where the emc_cache
is needed for recirculation.
While recursive mutexes can make thread safety analysis hard, for now
we change emc_mutex to be recursive. Forthcoming new unit tests will
fail with the current non-recursive mutex. Later improvements may
remove the need for this recursion.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Acked-by: Daniele Di Proietto <ddiproietto@vmware.com>
Use the "+-" syntax more uniformly when printing masked flags, and use
the syntax of delimited 1-flags also for formatting fully masked TCP
flags.
The "+-" syntax only deals with masked flags, but if there are many of
those, the printout becomes long and confusing. Typically there are
many flags only when flags are fully masked, but even then most of
them are zeros, so it makes sense to print the flags that are set
(ones) and omit the zero flags.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Some attributes are exact matches even when all bits are not ones.
Make odp_mask_attr_is_exact() to return true if the mask is set for
all the bits we actually care about.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
lib/odp: Masked set action execution and printing.
Add a new action type OVS_ACTION_ATTR_SET_MASKED, and support for
parsing, printing, and committing them.
Masked set actions add a mask, immediately following the netlink
attribute data, within the netlink attribute itself. Thus the key
attribute size for a masked set action is exactly double of the
non-masked set action.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
Alex Wang [Mon, 8 Sep 2014 17:41:36 +0000 (10:41 -0700)]
ofproto-dpif-upcall: Fix a free of uninitialized memory.
On current master, when 'upcall_receive()' returns error, the
ofpbuf 'upcall->put_actions' is uninitialized. In some usecase,
the failure of 'upcall_receive()' will cause uninitialize of
'upcall->put_actions' and free of uninitialized pointer.
This commit fixes the issue by making the caller not conduct
the uninitialize of the 'upcall' when there is error.
Found by inspection.
Signed-off-by: Alex Wang <alexw@nicira.com> Acked-by: Ben Pfaff <blp@nicira.com>
packet execute is setting egress_tun_info in skb->cb, rather
than packet->cb. skb is netlink msg skb. This causes corruption
in netlink skb state stored in skb->cb (NETLINK_CB) which
results in following deadlock in netlink code.
=============================================
[ INFO: possible recursive locking detected ]
3.2.62 #2
---------------------------------------------
handler55/22851 is trying to acquire lock:
(genl_mutex){+.+.+.}, at: [<ffffffff81471ad7>] genl_lock+0x17/0x20
but task is already holding lock:
(genl_mutex){+.+.+.}, at: [<ffffffff81471ad7>] genl_lock+0x17/0x20
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(genl_mutex);
lock(genl_mutex);
*** DEADLOCK ***
May be due to missing lock nesting notation
1 lock held by handler55/22851:
#0: (genl_mutex){+.+.+.}, at: [<ffffffff81471ad7>] genl_lock+0x17/0x20
When there are multiple vlan headers present in a received frame, the
first one is put into vlan_tci and protocol is set to ETH_P_8021Q.
Anything in the skb beyond the VLAN TPID may be still non-linear,
including the inner TCI and ethertype. While ovs_flow_extract takes
care of IP and IPv6 headers, it does nothing with ETH_P_8021Q. Later,
if OVS_ACTION_ATTR_POP_VLAN is executed, __pop_vlan_tci pulls the
next vlan header into vlan_tci.
This leads to two things:
1. Part of the resulting ethernet header is in the non-linear part of
the skb. When eth_type_trans is called later as the result of
OVS_ACTION_ATTR_OUTPUT, kernel BUGs in __skb_pull. Also,
__pop_vlan_tci is in fact accessing random data when it reads
past the TPID.
2. network_header points into the ethernet header instead of behind it.
mac_len is set to a wrong value (10), too.
Reported-by: Yulong Pei <ypei@redhat.com> Signed-off-by: Jiri Benc <jbenc@redhat.com>
I have dropped second change. Since it assumes inner mac header is of
ETH_HLEN len which is not always true. Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
Andy Zhou [Mon, 11 Aug 2014 07:14:05 +0000 (00:14 -0700)]
datapath: Implement recirc action without recursion
Since kernel stack is limited in size, it is not wise to using
recursive function with large stack frames.
This patch provides an alternative implementation of recirc action
without using recursion.
A per CPU fixed sized, 'deferred action FIFO', is used to store either
recirc or sample actions encountered during execution of an action
list. Not executing recirc or sample action in place, but rather execute
them laster as 'deferred actions' avoids recursion.
Deferred actions are only executed after all other actions has been
executed, including the ones triggered by loopback from the kernel
network stack.
The size of the private FIFO, currently set to 20, limits the number
of total 'deferred actions' any one packet can accumulate.
Signed-off-by: Andy Zhou <azhou@nicira.com> Acked-by: Pravin B Shelar <pshelar@nicira.com>