Based on a conversation with the VMware Hyper-V team earlier today.
This commit also changes a couple of functions that were only used with
netlink-socket.c into static functions. I couldn't think of a reason for
code outside that file to use them.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Commit 93295354 (netlink-socket: Simplify multithreaded dumping
to match Linux reality.) makes the call to recvmsg() block if no
messages are available. This can cause revalidator threads hanging
for long time or even deadlock when main thread tries to stop the
revalidator threads.
This commit fixes the issue by enabling the MSG_DONTWAIT flag in
the call to recvmsg().
Signed-off-by: Alex Wang <alexw@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
Commit 0791315e4d (netlink-socket: Work around kernel Netlink dump thread
races.) introduced a simple workaround for Linux kernel races in Netlink
dumps. However, the code remained more complicated than needed. This
commit simplifies it.
The main reason for complication in the code was 'status_seq' in nl_dump.
This member was there to allow a thread to wait for some other thread to
refill the socket buffer with another dump message (although we did not
understand the reason at the time it was introduced). Now that we know
that Netlink dumps properly need to be serialized to work in existing
Linux kernels, there's no additional value in having 'status_seq',
because serialized recvmsg() calls always refill the socket buffer
properly.
This commit updates nl_msg_next() to clear its buffer argument on error.
This is a more convenient interface for the new version of the Netlink
dump code. nl_msg_next() doesn't have any other callers.
Signed-off-by: Ben Pfaff <blp@nicira.com>
If nl_pool_alloc() failed, then 'dump' was not initialized at all and
further use of the dump would access uninitialized data, probably causing
a crash.
Found by inspection.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Joe Stringer <joestringer@nicira.com>
The Linux kernel Netlink implementation has two races that cause problems
for processes that attempt to dump a table in a multithreaded manner.
The first race is in the structure of the kernel netlink_recv() function.
This function pulls a message from the socket queue and, if there is none,
reports EAGAIN:
skb = skb_recv_datagram(sk, flags, noblock, &err);
if (skb == NULL)
goto out;
Only if a message is successfully read from the socket queue does the
function, toward the end, try to queue up a new message to be dumped:
if (nlk->cb && atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2) {
ret = netlink_dump(sk);
if (ret) {
sk->sk_err = ret;
sk->sk_error_report(sk);
}
}
This means that if thread A reads a message from a dump, then thread B
attempts to read one before A queues up the next, B will get EAGAIN. This
means that, following EAGAIN, B needs to wait until A returns to userspace
before it tries to read the socket again. nl_dump_next() already does
this, using 'dump->status_seq' (although the need for it has never been
explained clearly, to my knowledge).
The second race is more serious. Suppose thread X and thread Y both
simultaneously attempt to queue up a new message to be dumped, using the
call to netlink_dump() quoted above. netlink_dump() begins with:
mutex_lock(nlk->cb_mutex);
cb = nlk->cb;
if (cb == NULL) {
err = -EINVAL;
goto errout_skb;
}
Suppose that X gets cb_mutex first and finds that the dump is complete. It
will therefore, toward the end of netlink_dump(), clear nlk->cb to NULL to
indicate that no dump is in progress and release the mutex:
nlk->cb = NULL;
mutex_unlock(nlk->cb_mutex);
When Y grabs cb_mutex afterward, it will see that nlk->cb is NULL and
return -EINVAL as quoted above. netlink_recv() stuffs -EINVAL in sk_err,
but that error is not reported immediately; instead, it is saved for the
next read from the socket. Since Open vSwitch maintains a pool of Netlink
sockets, that next failure can crop up pretty much anywhere. One of the
worst places for it to crop up is in the execution of a later transaction
(e.g. in nl_sock_transact_multiple__()), because userspace treats Netlink
transactions as idempotent and will re-execute them when socket errors
occur. For a transaction that sends a packet, this causes packet
duplication, which we actually observed in practice. (ENOBUFS should
actually cause transactions to be re-executed in many cases, but EINVAL
should not; this is a separate bug in the userspace netlink code.)
VMware-BZ: #1283188
Reported-and-tested-by: Alex Wang <alexw@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Alex Wang <alexw@nicira.com>
Commit 8f20fd98db (netlink-socket: Work around upstream kernel Netlink
bug.) got the sign of the error code wrong, so that it reported e.g. -22
for EINVAL to nl_sock_recv__()'s caller, instead of 22.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Alex Wang <alexw@nicira.com>
The upstream kernel net/netlink/af_netlink.c netlink_recvmsg() contains the
following code to refill the Netlink socket buffer with more dump skbs
while a dump is in progress:
if (nlk->cb && atomic_read(&sk->sk_rmem_alloc) <= sk->sk_rcvbuf / 2) {
ret = netlink_dump(sk);
if (ret) {
sk->sk_err = ret;
sk->sk_error_report(sk);
}
}
The netlink_dump() function that this calls returns a negative number on
error, the convention used throughout the kernel, and thus sk->sk_err
receives a negative value on error.
However, sk->sk_err is supposed to contain either 0 or a positive errno
value, as one can see from a quick "grep" through net for 'sk_err =', e.g.:
ipv4/tcp.c:2067: sk->sk_err = ECONNRESET;
ipv4/tcp.c:2069: sk->sk_err = ECONNRESET;
ipv4/tcp_input.c:4106: sk->sk_err = ECONNREFUSED;
ipv4/tcp_input.c:4109: sk->sk_err = EPIPE;
ipv4/tcp_input.c:4114: sk->sk_err = ECONNRESET;
netlink/af_netlink.c:741: sk->sk_err = ENOBUFS;
netlink/af_netlink.c:1796: sk->sk_err = ENOBUFS;
packet/af_packet.c:2476: sk->sk_err = ENETDOWN;
unix/af_unix.c:341: other->sk_err = ECONNRESET;
unix/af_unix.c:407: skpair->sk_err = ECONNRESET;
The result is that the next attempt to receive from the socket will return
the error to userspace with the wrong sign.
(The root of the error in this case is that multiple threads are attempting
to read a single flow dump from a shared fd. That should work, but the
kernel has an internal race that can result in one or more of those threads
hitting the EINVAL case at the start of netlink_dump(). The EINVAL is
harmless in this case and userspace should be able to ignore it, but
reporting the EINVAL as if it were a 22-byte message received in userspace
throws a real wrench in the works.)
This bug makes me think that there are probably not many programs doing
multithreaded Netlink dumps. Maybe it is good that we are considering
other approaches.
VMware-BZ: #1255704
Reported-by: Mihir Gangar <gangarm@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Alex Wang <alexw@nicira.com>
I think these were leftovers from the removal of %z for MSVC that happened
some time ago.
VMware-BZ: 1265762
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Pritesh Kothari <pritesh.kothari@cisco.com>
None of the atomic implementations need a destroy function anymore, so it's
"more standard" and more convenient for users to get rid of them.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Andy Zhou <azhou@nicira.com>
This patch modifies 'struct nl_dump' and nl_dump_next() to allow
multiple threads to share the same nl_dump. These changes are targeted
around synchronizing dump status between multiple callers, and
allowing callers to fully process their existing buffers before
determining whether to stop fetching flows.
The 'status' field of 'struct nl_dump' becomes atomic, so that multiple
threads may check and/or update it to communicate when there is an error
or the netlink dump is finished. The low bit holds whether the final
message was seen, while the higher bits hold an errno value.
nl_dump_next() will now read all messages from the given buffer before
checking the shared error status and attempting to fetch more. Multiple
threads may call this with the same nl_dump, but must provide
independent buffers. As previously, the final dump status can be
determined by calling nl_dump_done() from a single thread.
Signed-off-by: Joe Stringer <joestringer@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
This patch makes all of the users of 'struct nl_dump' allocate their own
buffers to pass down to nl_dump_next(). This paves the way for allowing
multithreaded flow dumping.
Signed-off-by: Joe Stringer <joestringer@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
An upcoming patch will introduce another, completely unrelated seq to
'struct nl_dump'. Giving this one a better name should reduce confusion.
Signed-off-by: Joe Stringer <joestringer@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
The function comment still referred to a 'msg' variable, which has been
renamed.
Signed-off-by: Joe Stringer <joestringer@nicira.com>
[blp@nicira.com did further proofreading]
Signed-off-by: Ben Pfaff <blp@nicira.com>
The MSVC C library printf() implementation does not support the 'z', 't',
'j', or 'hh' format specifiers. This commit changes the Open vSwitch code
to avoid those format specifiers, switching to standard macros from
<inttypes.h> where available and inventing new macros resembling them
where necessary. It also updates CodingStyle to specify the macros' use
and adds a Makefile rule to report violations.
Signed-off-by: Alin Serdean <aserdean@cloudbasesolutions.com>
Co-authored-by: Ben Pfaff <blp@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Found by Clang.
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>
We've seen a number of deadlocks in the tree since thread safety was
introduced. So far, all of these are self-deadlocks, that is, a single
thread acquiring a lock and then attempting to re-acquire the same lock
recursively. When this has happened, the process simply hung, and it was
somewhat difficult to find the cause.
POSIX "error-checking" mutexes check for this specific problem (and
others). This commit switches from other types of mutexes to
error-checking mutexes everywhere that we can, that is, everywhere that
we're not using recursive mutexes. This ought to help find problems more
quickly in the future.
There might be performance advantages to other kinds of mutexes in some
cases. However, the existing mutex type choices were just guesses, so I'd
rather go for easy detection of errors until we know that other mutex
types actually perform better in specific cases. Also, I did a quick
microbenchmark of glibc mutex types on my host and found that the
error checking mutexes weren't any slower than the other types, at least
when the mutex is uncontended.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>
This commit adds annotations for thread safety check. And the
check can be conducted by using -Wthread-safety flag in clang.
Co-authored-by: Alex Wang <alexw@nicira.com>
Signed-off-by: Alex Wang <alexw@nicira.com>
Signed-off-by: Ethan Jackson <ethan@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
This disentangles "struct nl_dump" from "struct nl_sock", clearing the way
to make the use of either one thread-safe in an obviously correct manner.
Signed-off-by: Ben Pfaff <blp@nicira.com>
The "stress" library was introduced years ago. We intended at the time to
start using it to provoke errors in testing, to make sure that Open vSwitch
was resilient against those errors. The intention was good, but there were
few actual implementations of stress options, and the testing never
materialized.
Rather than adapt the stress library for thread safety, this seems like a
good opportunity to remove it, so this commit does so.
Signed-off-by: Ben Pfaff <blp@nicira.com>
This was the only obvious use of bare malloc() in the tree, other
than in the implementation of wrapper functions.
Signed-off-by: Ben Pfaff <blp@nicira.com>
This patch fixes tests when they are run with "fakeroot debian/rules binary"
command.
The problem was that under fakeroot setsockopt() call could still return
EPERM and lead to a warning message being logged.
Signed-off-by: Ansis Atteka <aatteka@nicira.com>
Some Open vSwitch utilities can do useful work when they are not run as
root. Without this commit, these utilities will log a warning on failure
to use the SO_RCVBUFFORCE socket option if they open any Netlink sockets.
This will always happen, it does not report anything unexpected or
fixable as non-root, and sometimes it makes users wonder if something is
wrong, so there is no benefit to logging it. This commit drops it in that
case.
Signed-off-by: Ben Pfaff <blp@nicira.com>
This is a straight search-and-replace, except that I also removed #include
<assert.h> from each file where there were no assert calls left.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>
Casts are sometimes necessary. One common reason that they are necessary
is for discarding a "const" qualifier. However, this can impede
maintenance: if the type of the expression being cast changes, then the
presence of the cast can hide a necessary change in the code that does the
cast. Using CONST_CAST, instead of a bare cast, makes these changes
visible.
Inspired by my own work elsewhere:
http://git.savannah.gnu.org/cgit/pspp.git/tree/src/libpspp/cast.h#n80
Signed-off-by: Ben Pfaff <blp@nicira.com>
Commit 7d7447 (netlink: Postpone choosing sequence numbers until send
time.) broke ovs-brcompatd because it prevented userspace replies to
kernel requests from using the correct sequence numbers. This commit fixes
it.
Atzm Watanabe found the root cause and provided an alternative patch to
avoid the problem.
Reported-by: André Ruß <andre.russ@hybris.com>
Reported-by: Atzm Watanabe <atzm@stratosphere.co.jp>
Tested-by: Atzm Watanabe <atzm@stratosphere.co.jp>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Replaced all instances of Nicira Networks(, Inc) to Nicira, Inc.
Feature #10593
Signed-off-by: Raju Subramanian <rsubramanian@nicira.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Typically an nl_sock client can stack-allocate the buffer for receiving
a Netlink message, which provides a performance boost.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Choosing sequence numbers at time of creating a packet means that
nl_sock_transact_multiple() has to search for the sequence number
of a reply, because the sequence numbers of the requests aren't
necessarily sequential. This commit makes it possible to avoid
the search, by deferring choice of sequence numbers until the
time that we send the packets. It doesn't actually modify
nl_sock_transact_multiple(), which will happen in a later commit.
Previously, I was concerned about a theoretical race condition
described in a comment in the old versino of this code:
This implementation uses sequence numbers that are unique
process-wide, to avoid a hypothetical race: send request, close
socket, open new socket that reuses the old socket's PID value,
send request on new socket, receive reply from kernel to old
socket but with same PID and sequence number. (This race could be
avoided other ways, e.g. by preventing PIDs from being quickly
reused).
However, I no longer believe that this can be a real problem,
because Netlink operates synchronously. The reply to a request
will always arrive before the socket can be closed and a new
socket opened with the old socket's PID.
Signed-off-by: Ben Pfaff <blp@nicira.com>
Open vSwitch userspace can set up flows at a high rate, but it is somewhat
"bursty" in opportunities to set up flows, by which I mean that OVS sets up
a batch of flows, then goes off and does some other work for a while, then
sets up another batch of flows, and so on. The result is that, if a large
number of packets that need flow setups come in all at once, then some of
them can overflow the relatively small kernel-to-user buffers.
This commit increases the kernel-to-user buffers from the default of
approximately 120 kB each to 1 MB each. In one somewhat synthetic test
case that I ran based on an "hping3" that generated a load of about 20,000
new flows per second (including both requests and replies), this reduced
the packets dropped at the kernel-to-user interface from about 30% to none.
I expect that it will similarly improve packet loss in workloads where
flow arrival is not easily predictable.
(This has little effect on workloads generated by "ovs-benchmark rate"
because that benchmark is effectively "self-clocking", that is, a new flow
is triggered only by a reply to a request made earlier, which means that
the number of buffered packets at any given has a known, constant upper
limit.)
Bug #10210.
Signed-off-by: Ben Pfaff <blp@nicira.com>
The Netlink code in the Linux kernel has been willing to choose unique
Netlink pids for userspace sockets since at least 2.4.36 and probably
earlier. There's no value in choosing them ourselves.
This simplifies the code and eliminates the possibility of exhausting our
supply of Netlink PIDs.
Using poll() internally in dpif_linux_recv(), instead of relying
on the results of the main loop poll() call, brings netperf CRR
performance back within 1% of par versus the code base before the
poll_fd_woke() optimizations were introduced. It also increases
the ovs-benchmark results by about 5% versus that baseline, too.
My theory is that this is because the main loop takes long enough
that a significant number of packets can arrive during the main
loop itself, so this reduces the time before OVS gets to those
packets.
This reverts commit 1e276d1a10539a8cd97d2ad63c073a9a43f0f1ef.
The poll_fd_woke() and nl_sock_woke() function added in that commit are
no longer used, so there is no reason to keep them in the tree.
Each time we run through the poll loop, we check all file descriptors
that we were waiting on to see if there is data available. However,
this requires a system call and poll already provides information on
which FDs caused the wakeup so it is inefficient as the number of
active FDs grows. This provides a way to check whether a given FD
has data.
In the future, the kernel will use unicast messages instead of
multicast to send upcalls. As a result, we need to be able to
tell it where to direct the traffic. This adds a function to expose
the Netlink pid of a socket so it can be included in messages to the
kernel.
A Netlink socket that receives asynchronous notifications (e.g. from a
multicast group) cannot be used for transactions or dumps, because those
operations would discard asynchronous messages that arrive while waiting
for replies.
This commit documents this issue in a comment on nl_sock_join_mcgroup().
It also removes an internal attempt to avoid mixing multicast reception
with other operations. The attempt was incomplete, because it only
handled dumps even though ordinary transactions are also problematic. It
seems better to remove it than to fix it because, first, all of the
existing users in OVS already separate multicast reception from other
operations and, second, an upcoming commit will start using unicast
Netlink for asynchronous notifications, which has the same issues but
doesn't use nl_sock_join_mcgroup().
The nl_lookup_genl_mcgroup() function can fail on older kernels
which do not support the required netlink interface. Before this
patch, dpif-linux would refuse to create a datapath when this
happened. With this patch, it attempts to use a workaround. If
the workaround fails it simply disables the affected features
without completely disabling the dpif.
Commit e408762f "netlink-socket: New function nl_lookup_genl_mcgroup()"
modified do_lookup_genl_family() to return the Netlink attributes to the
caller, but it still freed the Netlink message itself, which meant that
the attributes pointed into freed memory. This commit fixes the problem.
This commit is not a minimal fix. It refactors do_lookup_genl_family(),
changing the return value from "negative errno value or positive genl
family id" to the more common "zero or positive errno value".
Found by valgrind.
Until now, each attempt to receive a message from a Netlink socket has
taken at least two system calls, one to check the size of the message to
be received and a second one to delete the message from the socket buffer.
This commit switches to a new strategy that requires only one system call
per message received.
In my testing this increases the maximum flow setups per second by a little
over 10%.
