On Tue, Jan 14, 2014 at 09:32:02AM -0800, Joe Stringer wrote:
> On 13 January 2014 14:55, Ben Pfaff <[email protected]> wrote:
> > I have two concerns about nl_dump_next().
> >
> > The first is that any nonzero status always overwrites the existing
> > status. This means that EOF in one thread overwrites EPROTO (or some
> > other error) from another thread, effectively dropping the error. I
> > can't think, off-hand, of a good way to avoid this without two
> > variables, so maybe we should use two variables.
> >
>
> Right, this was somewhat intentional (although quite possibly flawed). In
> cases of socket errors, it was expected that these would prevent the final
> nl_msg from being received, so EOF wouldn't overwrite. In the case of
> EPROTO, this suggests that a malformed nl_msg is observed in one thread,
> and a separate thread has successfully received the final,
> correctly-formatted nl_msgs, and processed through to the end of them. I
> didn't previously expect that this would occur.
I think that you're probably right in the former case.
In the latter case, it's a (probably unlikely) race, and it does not
appear that it is too hard to avoid, so maybe we should avoid it.
> By "use two variables", do you mean one for storing the EOF status (error
> denoting success), and one for all other errors (failures)?
Yes, or we can divide a single variable up into fields.
> The second is that ignoring EAGAIN in the loop makes it possible for
> > threads (other than the one thread that receives NLMSG_DONE) to spin
> > in essentially a no-op loop waiting for the thread that receives
> > NLMSG_DONE to set dump->status. I think we can avoid that by treating
> > EAGAIN as a reason to return false; I'm pretty sure that the kernel
> > netlink code will never return EAGAIN while a dump is in progress
> >
>
> That does sound a bit tidier. There's a few complications with the
> nl_dump_recv() call: If the socket returns EINTR, nl_dump_recv()
> reinterprets this as EAGAIN. It will also return EAGAIN if it receives an
> unexpected nl_msg (wrong seq). As I understand, we should actually retry if
> we encounter these cases. However, if we receive an EAGAIN from the socket
> then this is a case to fail out and return. Is that in line with what
> you're thinking?
Good points.
Here's a concept that you could work from. It reflects what I have in
mind, and it compiles, but I have not tested it at all. Feel free to
start from it, if you like.
diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
index f072488..3d71c2f 100644
--- a/lib/netlink-socket.c
+++ b/lib/netlink-socket.c
@@ -690,45 +690,19 @@ nl_sock_drain(struct nl_sock *sock)
void
nl_dump_start(struct nl_dump *dump, int protocol, const struct ofpbuf *request)
{
- dump->status = nl_pool_alloc(protocol, &dump->sock);
- if (dump->status) {
+ int status = nl_pool_alloc(protocol, &dump->sock);
+
+ if (status) {
return;
}
nl_msg_nlmsghdr(request)->nlmsg_flags |= NLM_F_DUMP | NLM_F_ACK;
- dump->status = nl_sock_send__(dump->sock, request,
- nl_sock_allocate_seq(dump->sock, 1), true);
+ status = nl_sock_send__(dump->sock, request,
+ nl_sock_allocate_seq(dump->sock, 1), true);
+ atomic_init(&dump->status, status);
dump->seq = nl_msg_nlmsghdr(request)->nlmsg_seq;
}
-/* Helper function for nl_dump_next(). */
-static int
-nl_dump_recv(struct nl_dump *dump, struct ofpbuf *buffer)
-{
- struct nlmsghdr *nlmsghdr;
- int retval;
-
- retval = nl_sock_recv__(dump->sock, buffer, true);
- if (retval) {
- return retval == EINTR ? EAGAIN : retval;
- }
-
- nlmsghdr = nl_msg_nlmsghdr(buffer);
- if (dump->seq != nlmsghdr->nlmsg_seq) {
- VLOG_DBG_RL(&rl, "ignoring seq %#"PRIx32" != expected %#"PRIx32,
- nlmsghdr->nlmsg_seq, dump->seq);
- return EAGAIN;
- }
-
- if (nl_msg_nlmsgerr(buffer, &retval)) {
- VLOG_INFO_RL(&rl, "netlink dump request error (%s)",
- ovs_strerror(retval));
- return retval && retval != EAGAIN ? retval : EPROTO;
- }
-
- return 0;
-}
-
/* Attempts to retrieve another reply from 'dump' into 'buffer'. 'dump' must
* have been initialized with nl_dump_start(), and 'buffer' must have been
* initialized with enough space to receive a netlink reply.
@@ -742,40 +716,75 @@ nl_dump_recv(struct nl_dump *dump, struct ofpbuf *buffer)
* to 0. Failure might indicate an actual error or merely the end of replies.
* An error status for the entire dump operation is provided when it is
* completed by calling nl_dump_done().
+ *
+ * Multiple threads may call this function, passing the same nl_dump, however
+ * each must provide an independent buffer. This function may cache multiple
+ * flows in the buffer, and these will be processed before more flows are
+ * fetched. When this function returns false, other threads may continue to
+ * process flows in their buffers, but they will not fetch more flows.
*/
bool
nl_dump_next(struct nl_dump *dump, struct ofpbuf *reply, struct ofpbuf *buffer)
{
struct nlmsghdr *nlmsghdr;
+ int error;
reply->data = NULL;
reply->size = 0;
- if (dump->status) {
- return false;
- }
+ /* If 'buffer' is empty, fetch another batch of nlmsgs. */
while (!buffer->size) {
- int retval = nl_dump_recv(dump, buffer);
- if (retval) {
+ struct nlmsghdr *nlmsghdr;
+ unsigned int status;
+ int retval;
+
+ atomic_read(&dump->status, &status);
+ if (status) {
+ return false;
+ }
+
+ retval = nl_sock_recv__(dump->sock, buffer, false);
+ if (retval == EINTR) {
ofpbuf_clear(buffer);
- if (retval != EAGAIN) {
- dump->status = retval;
- return false;
- }
+ continue;
+ }
+
+ nlmsghdr = nl_msg_nlmsghdr(buffer);
+ if (dump->seq != nlmsghdr->nlmsg_seq) {
+ VLOG_DBG_RL(&rl, "ignoring seq %#"PRIx32" != expected %#"PRIx32,
+ nlmsghdr->nlmsg_seq, dump->seq);
+ ofpbuf_clear(buffer);
+ continue;
+ }
+
+ if (nl_msg_nlmsgerr(buffer, &retval)) {
+ VLOG_INFO_RL(&rl, "netlink dump request error (%s)",
+ ovs_strerror(retval));
+ error = retval && retval != EAGAIN ? retval : EPROTO;
+ ofpbuf_clear(buffer);
+ goto exit;
}
}
+ /* Fetch the next nlmsg in the current batch. */
nlmsghdr = nl_msg_next(buffer, reply);
if (!nlmsghdr) {
VLOG_WARN_RL(&rl, "netlink dump reply contains message fragment");
- dump->status = EPROTO;
- return false;
+ error = EPROTO;
} else if (nlmsghdr->nlmsg_type == NLMSG_DONE) {
- dump->status = EOF;
- return false;
+ error = EOF;
+ } else {
+ error = 0;
}
- return true;
+exit:
+ if (error == EOF) {
+ unsigned int old;
+ atomic_or(&dump->status, 1, &old);
+ } else if (error) {
+ atomic_store(&dump->status, error << 1);
+ }
+ return !error;
}
/* Completes Netlink dump operation 'dump', which must have been initialized
@@ -784,22 +793,27 @@ nl_dump_next(struct nl_dump *dump, struct ofpbuf *reply,
struct ofpbuf *buffer)
int
nl_dump_done(struct nl_dump *dump)
{
- struct ofpbuf buf;
+ unsigned int status;
/* Drain any remaining messages that the client didn't read. Otherwise the
* kernel will continue to queue them up and waste buffer space.
*
* XXX We could just destroy and discard the socket in this case. */
- ofpbuf_init(&buf, 4096);
- while (!dump->status) {
- struct ofpbuf reply;
- if (!nl_dump_next(dump, &reply, &buf)) {
- ovs_assert(dump->status);
+ atomic_read(&dump->status, &status);
+ if (!status) {
+ struct ofpbuf reply, buf;
+
+ ofpbuf_init(&buf, 4096);
+ while (nl_dump_next(dump, &reply, &buf)) {
+ /* Nothing to do. */
}
+ atomic_read(&dump->status, &status);
+ ovs_assert(status);
+ ofpbuf_uninit(&buf);
}
- ofpbuf_uninit(&buf);
+ atomic_destroy(&dump->status);
nl_pool_release(dump->sock);
- return dump->status == EOF ? 0 : dump->status;
+ return status >> 1;
}
/* Causes poll_block() to wake up when any of the specified 'events' (which is
diff --git a/lib/netlink-socket.h b/lib/netlink-socket.h
index 4e1e588..233a826 100644
--- a/lib/netlink-socket.h
+++ b/lib/netlink-socket.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -35,13 +35,23 @@
* Thread-safety
* =============
*
- * Only a single thread may use a given nl_sock or nl_dump at one time.
+ * Most of the netlink functions are not fully thread-safe: Only a single
+ * thread may use a given nl_sock or nl_dump at one time. The exceptions are:
+ *
+ * - nl_sock_recv() is conditionally thread-safe: it may be called from
+ * different threads with the same nl_sock, but each caller must provide
+ * an independent receive buffer.
+ *
+ * - nl_dump_next() is conditionally thread-safe: it may be called from
+ * different threads with the same nl_dump, but each caller must provide
+ * independent buffers.
*/
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
#include "ofpbuf.h"
+#include "ovs-atomic.h"
struct nl_sock;
@@ -99,7 +109,8 @@ void nl_transact_multiple(int protocol, struct
nl_transaction **, size_t n);
struct nl_dump {
struct nl_sock *sock; /* Socket being dumped. */
uint32_t seq; /* Expected nlmsg_seq for replies. */
- int status; /* 0=OK, EOF=done, or positive errno value. */
+ atomic_uint status; /* Low bit set if we read last message.
+ * Other bits hold an errno (0 for success). */
};
void nl_dump_start(struct nl_dump *, int protocol,
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
