The nl_sock_transact_multiple function enters in an infinite loop,
when invalid error, EINVAL, is returned by nl_sock_transact_multiple__.
EINVAL is the error returned by the latter function when a driver
request fails.
Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
Reported-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Reported-at: https://github.com/openvswitch/ovs-issues/issues/57
Acked-by: Eitan Eliahu <eliahue@vmware.com>
Acked-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Added a new IOCTL in order to retrieve the PID from the kernel datapath.
The new method uses a direct and cleaner way, as opposed to the old way
of using a Netlink transaction, avoiding the unnecessary overhead.
Signed-off-by: Sorin Vinturis <svinturis@cloudbasesolutions.com>
Reported-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Reported-at: https://github.com/openvswitch/ovs-issues/issues/31
Acked-by: Nithin Raju <nithin@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Tested-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
ofpbuf was complicated due to its wide usage across all
layers of OVS, Now we have introduced independent dp_packet
which can be used for datapath packet, we can simplify ofpbuf.
Following patch removes DPDK mbuf and access API of ofpbuf
members.
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
Acked-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
A new function vlog_insert_module() is introduced to avoid using
list_insert() from the vlog.h header.
Signed-off-by: Thomas Graf <tgraf@noironetworks.com>
Acked-by: Ben Pfaff <blp@nicira.com>
The pid must be set in the NL header as the driver checks it against the pid in
the instance paired with the socket.
Signed-off-by: Eitan Eliahu <eliahue@vmware.com>
Acked-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
In this patch, we add support in userspace for packet subscribe API
similar to the join/leave MC group API that is used for port events.
The kernel code has already been commited.
Signed-off-by: Nithin Raju <nithin@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
We have not yet tested the wakup via pending IRP functionality on
Windows yet. Hence we use poll_immediate_wake().
Signed-off-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Eitan Eliahu <eliahue@vmware.com>
In nl_sock_recv__() on Windows, we realloc a new ofpbuf to copy received
data if the caller specified buffer is small. While we do so, we need
reset some of the other stack variables to point to the new ofpbuf.
Other fixes are around using 'error' rather than 'errno'.
Signed-off-by: Nithin Raju <nithin@vmware.com>
Acked-by: Ankur Sharma <ankursharma@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
We need to pass down the output buffer so that the kernel can return
transaction status - error or otherwise.
Also, we were processing the output buffer only when when
'txn->reply != NULL' ie when the caller specified an ofpbuf for the
reply. In this patch, the code has been updated to process the reply
unconditionally, but making sure to copy the reply to the 'txn->reply'
only when it is not NULL. The reason for the unconditional processing is
we can pass up transactional errors in 'txn->error'. Otherwise, it
results in an endless loop of calling nl_transact().
Signed-off-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ankur Sharma <ankursharma@vmware.com>
In this patch, we add support for family ID lookup of
OVS_WIN_NETDEV_FAMILY.
Signed-off-by: Nithin Raju <nithin@vmware.com>
Acked-by: Ankur Sharma <ankursharma@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Tested-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
User mode sends down three distinct Read ioctl commands for Events, Packet
Reads and Dumps. In case the Packet Read socket can not be distinguished a
Set function will be provided.
Signed-off-by: Eitan Eliahu <eliahue@vmware.com>
Acked-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
In this patch, we add support for nl_transact() on Windows using
the OVS_IOCTL_TRANSACT ioctl that sends down the request and gets
the reply in the same call to the kernel.
This is obviously a digression from the way it is implemented in
Linux where all the sends are done at once using sendmsg() and
replies are received one at a time.
Initial implementation was in the Linux way using multiple writes
followed by reads, but decided against it since it is not efficient
and also it complicates the state machine in the kernel.
The Windows implementation has equivalent code for handling corner
cases and error coditions similar to Linux. Some of it is not
applicable yet. Eg. the Windows kernel does not embed an error
in the netlink message itself. There's userspace code nevertheless
for this.
Signed-off-by: Nithin Raju <nithin@vmware.com>
Acked-by: Samuel Ghinet <sghinet@cloudbasesolutions.com>
Acked-by: Eitan Eliahu <eliahue@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Use a specific out of band device control to subscribe/unsubscribe a socket
to the driver event queue for notification.
Signed-off-by: Eitan Eliahu <eliahue@vmware.com>
Acked-by: Nithin Raju <nithin@vmware.com>
Acked-by: Saurabh Shah <ssaurabh@vmware.com>
Acked-by: Ankur Sharma <ankursharma@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
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>
'sock' is not initialized and hence should not be un-initialized
as well in the failure path.
Reported-by: Gurucharan Shetty <shettyg@nicira.com>
Signed-off-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Gurucharan Shetty <gshetty@nicira.com>
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>
The Windows datapath supports a READ/WRITE ioctl instead of ReadFile/WriteFile.
In this change, we update the following:
- WriteFile() in nl_sock_send__() to use DeviceIoControl(OVS_IOCTL_WRITE)
- ReadFile() in nl_sock_recv__() to use DeviceIoControl(OVS_IOCTL_READ)
The WriteFile() call in nl_sock_transact_multiple__() has not been touched
since it is not needed yet.
Main motive for this change is to be able to unblock the DP Dump workflow.
Signed-off-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Eitan Eliahu <eliahue@vmware.com>
Acked-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
A typo crept in while respinning get_sock_pid_from_kernel() in the previous
patch. Fixing it now. Also, get_sock_pid_from_kernel() doesn't need an OUT
argument. Fixing that too.
Signed-off-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
While we work out whether nl_sock_join_mcgroup() will be the mechanism
to support VPORT events, it is easy to add support for
nl_lookup_genl_mcgroup() and make progress on the other commands.
In this patch, we implement support for nl_lookup_genl_mcgroup() only
for the VPORT family though, which is all what dpif-linux.c needs.
Validation:
- A ported dpif-linux.c with epoll code commented out went so far as
to call dp_enumerate! DP Dump commands can be implemented next.
Signed-off-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
In this patch, we add support for querying the genl family id for any
family supported by the OVS kernel datapath. On platforms that support
netlink natively, the operating system assigns a family ID, and the
OS netlink infrastructure supports querying the family ID by name.
In case of Windows, since OVS datpath provides the netlink support,
it is not necessary to make a call into the kernel. Returning a
family ID that is consistent between the userspace and kernel
is sufficient. Once there is code to support netlink message parsing
as well as constructing netlink messages, we can make a call into
the kernel, but that in itself may not buy anything more than this
approach.
This patch is a precursor to make progress of the other commands.
The next hurdle is to support nl_lookup_genl_mcgroup().
Signed-off-by: Nithin Raju <nithin@vmware.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
To verify if the netlink support in the kernel works, I updated
the netlink-socket.c code to get the PID for a given device
descriptor.
In the existing code, userspace sets the PID, which will not be
unique across different processes. So, it is better for the
kernel to generate the PID and give it back to userspace.
dpif-linux.c was ported to Windows (similar to Alin's change in
the cloudbase repo) and was able to exercise the code changes
in netlink-socket.c to read the PID. dpif-linux.c changes are
not being checked in.
Signed-off-by: Nithin Raju <nithin@vmware.com>
Acked-by: Alin Serdean <aserdean@cloudbasesolutions.com>
Acked-by: Ankur Sharma <ankursharma@vmware.com>
Acked-by: Saurabh Shah <ssaurabh@vmware.com>
Reported-at: https://github.com/openvswitch/ovs-issues/issues/18
Signed-off-by: Ben Pfaff <blp@nicira.com>
Add two functions set_sock_pid_in_kernel and portid_next. This will allow
the channel identification for the kernel extension to send back messages.
Replace send with WriteFile equivalent and ignore nl_sock_drain for the moment
under MSVC.
Replace sendmsg and recvmsg with ReadFile and WriteFile equivalents.
On MSVC put in handle instead of fd(sock->fd becomes sock->handle).
Creation of the netlink socket will be replaced by CreateFile equivalent.
Add MAX_STACK_LENGTH for MSVC. This will be our maximum size for on-stack
copy buffer.
Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com>
Signed-off-by: Ben Pfaff <blp@nicira.com>
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>
