Re: [ovs-dev] [thread-safety 00/11] Thread safety.
On Fri, Jul 26, 2013 at 06:07:01PM -0700, Ethan Jackson wrote: > This series of patches adds thread safety to a bunch of modules used > by ofproto-dpif-xlate. The first patch of the series adds support for > clang's thread safety annotations which as Alex mentioned aren't quite > upstreamed yet. I've made some minor modifications on top of what > he's sent out already. So patch 1 is what Alex sent out, with a few changes folded in? So I should review this version rather than Alex's version? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 00/11] Thread safety.
I think so. I saw new modifications in 01/11 patch. Thanks, Alex Wang, ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 00/11] Thread safety.
On Mon, Jul 29, 2013 at 09:26:56AM -0700, Alex Wang wrote: > I think so. I saw new modifications in 01/11 patch. OK, I'll review Ethan's version. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: fix a bug in SF_FLOW_KEY_PUT macro
On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
> This bug will cause mask values to corrupt the flow key value. So far
> the bug has not showed up because we don't write mask value when
> there is no mask Netlink attributes. However, it needs to be fixed for
> the next and future commits where we will start to set default
> values for key and mask for missing Netlink attributes.
>
> Signed-off-by: Andy Zhou
> ---
> datapath/flow.c |5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 2c11408..ba775f4 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -82,8 +82,9 @@ static void update_range__(struct sw_flow_match *match,
> do { \
> update_range__(match, offsetof(struct sw_flow_key, field), \
> sizeof((match)->key->field), is_mask); \
> - if (is_mask && match->mask != NULL) { \
> - (match)->mask->key.field = value; \
> + if (is_mask) { \
> + if ((match)->mask) \
> + (match)->mask->key.field = value; \
Shouldn't we also do this for the memcpy variant?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] Avoid C preprocessor trick where macro has the same name as a function.
In C, one can do preprocessor tricks by making a macro expansion include
the macro's own name. We actually used this in the tree to automatically
provide function arguments, e.g.:
#define f(x) f(x, __FILE__, __LINE__)
int f(int x, const char *file, int line);
...
f(1);/* Expands to a call like f(1, __FILE__, __LINE__); */
However it's somewhat confusing, so this commit stops using that trick.
Reported-by: Ed Maste
Signed-off-by: Ben Pfaff
---
lib/jsonrpc.c|2 +-
lib/latch.c | 10 +++---
lib/latch.h |4 ++--
lib/ovs-thread.c | 16
lib/ovs-thread.h |8
lib/poll-loop.c | 37 ++---
lib/poll-loop.h | 20 +++-
lib/timer.c | 12
lib/timer.h |6 +++---
9 files changed, 66 insertions(+), 49 deletions(-)
diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
index 56b4cce..469fbae 100644
--- a/lib/jsonrpc.c
+++ b/lib/jsonrpc.c
@@ -350,7 +350,7 @@ void
jsonrpc_recv_wait(struct jsonrpc *rpc)
{
if (rpc->status || rpc->received || !byteq_is_empty(&rpc->input)) {
-(poll_immediate_wake)(rpc->name);
+poll_immediate_wake_at(rpc->name);
} else {
stream_recv_wait(rpc->stream);
}
diff --git a/lib/latch.c b/lib/latch.c
index 9b13006..bf518b9 100644
--- a/lib/latch.c
+++ b/lib/latch.c
@@ -75,9 +75,13 @@ latch_is_set(const struct latch *latch)
return pfd.revents & POLLIN;
}
-/* Causes the next poll_block() to wake up when 'latch' is set. */
+/* Causes the next poll_block() to wake up when 'latch' is set.
+ *
+ * ('where' is used in debug logging. Commonly one would use latch_wait() to
+ * automatically provide the caller's source file and line number for
+ * 'where'.) */
void
-(latch_wait)(const struct latch *latch, const char *where)
+latch_wait_at(const struct latch *latch, const char *where)
{
-(poll_fd_wait)(latch->fds[0], POLLIN, where);
+poll_fd_wait_at(latch->fds[0], POLLIN, where);
}
diff --git a/lib/latch.h b/lib/latch.h
index 08f45b1..0b6e8a3 100644
--- a/lib/latch.h
+++ b/lib/latch.h
@@ -36,7 +36,7 @@ bool latch_poll(struct latch *);
void latch_set(struct latch *);
bool latch_is_set(const struct latch *);
-void latch_wait(const struct latch *, const char *where);
-#define latch_wait(latch) latch_wait(latch, SOURCE_LOCATOR)
+void latch_wait_at(const struct latch *, const char *where);
+#define latch_wait(latch) latch_wait_at(latch, SOURCE_LOCATOR)
#endif /* latch.h */
diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index d08751c..feff102 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -128,9 +128,13 @@ ovsthread_once_done(struct ovsthread_once *once)
}
/* Asserts that the process has not yet created any threads (beyond the initial
- * thread). */
+ * thread).
+ *
+ * ('where' is used in logging. Commonly one would use
+ * assert_single_threaded() to automatically provide the caller's source file
+ * and line number for 'where'.) */
void
-(assert_single_threaded)(const char *where)
+assert_single_threaded_at(const char *where)
{
if (multithreaded) {
VLOG_FATAL("%s: attempted operation not allowed when multithreaded",
@@ -140,9 +144,13 @@ void
/* Forks the current process (checking that this is allowed). Aborts with
* VLOG_FATAL if fork() returns an error, and otherwise returns the value
- * returned by fork(). */
+ * returned by fork().
+ *
+ * ('where' is used in logging. Commonly one would use xfork() to
+ * automatically provide the caller's source file and line number for
+ * 'where'.) */
pid_t
-(xfork)(const char *where)
+xfork_at(const char *where)
{
pid_t pid;
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 9c8023e..afb5ccd 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -368,11 +368,11 @@ ovsthread_once_start(struct ovsthread_once *once)
((ONCE)->done ? false : ({ OVS_ACQUIRE(ONCE); true; }))
#endif
-void assert_single_threaded(const char *where);
-#define assert_single_threaded() assert_single_threaded(SOURCE_LOCATOR)
+void assert_single_threaded_at(const char *where);
+#define assert_single_threaded() assert_single_threaded_at(SOURCE_LOCATOR)
-pid_t xfork(const char *where);
-#define xfork() xfork(SOURCE_LOCATOR)
+pid_t xfork_at(const char *where);
+#define xfork() xfork_at(SOURCE_LOCATOR)
void forbid_forking(const char *reason);
bool may_fork(void);
diff --git a/lib/poll-loop.c b/lib/poll-loop.c
index 5f4b16c..0e11c7e 100644
--- a/lib/poll-loop.c
+++ b/lib/poll-loop.c
@@ -29,11 +29,6 @@
#include "timeval.h"
#include "vlog.h"
-#undef poll_fd_wait
-#undef poll_timer_wait
-#undef poll_timer_wait_until
-#undef poll_immediate_wake
-
VLOG_DEFINE_THIS_MODULE(poll_loop);
COVERAGE_DEFINE(poll_fd_wait);
@@ -73,10 +68,11 @@ static struct poll_waiter *new_waiter(int fd, short int
events,
* is affected. The event will need to be re-registered after poll_block() is
* called if it is to persist.
*
- * Ordinari
Re: [ovs-dev] [PATCH] Avoid C preprocessor trick where macro has the same name as a function.
On Mon, Jul 29, 2013 at 9:57 AM, Ben Pfaff wrote:
> In C, one can do preprocessor tricks by making a macro expansion include
> the macro's own name. We actually used this in the tree to automatically
> provide function arguments, e.g.:
>
> #define f(x) f(x, __FILE__, __LINE__)
> int f(int x, const char *file, int line);
>
> ...
>
> f(1);/* Expands to a call like f(1, __FILE__, __LINE__); */
>
> However it's somewhat confusing, so this commit stops using that trick.
>
> Reported-by: Ed Maste
> Signed-off-by: Ben Pfaff
>
What is the commit on which this patch applies? I can't apply it on top of
master.
Thanks,
Guru
> ---
> lib/jsonrpc.c|2 +-
> lib/latch.c | 10 +++---
> lib/latch.h |4 ++--
> lib/ovs-thread.c | 16
> lib/ovs-thread.h |8
> lib/poll-loop.c | 37 ++---
> lib/poll-loop.h | 20 +++-
> lib/timer.c | 12
> lib/timer.h |6 +++---
> 9 files changed, 66 insertions(+), 49 deletions(-)
>
> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
> index 56b4cce..469fbae 100644
> --- a/lib/jsonrpc.c
> +++ b/lib/jsonrpc.c
> @@ -350,7 +350,7 @@ void
> jsonrpc_recv_wait(struct jsonrpc *rpc)
> {
> if (rpc->status || rpc->received || !byteq_is_empty(&rpc->input)) {
> -(poll_immediate_wake)(rpc->name);
> +poll_immediate_wake_at(rpc->name);
> } else {
> stream_recv_wait(rpc->stream);
> }
> diff --git a/lib/latch.c b/lib/latch.c
> index 9b13006..bf518b9 100644
> --- a/lib/latch.c
> +++ b/lib/latch.c
> @@ -75,9 +75,13 @@ latch_is_set(const struct latch *latch)
> return pfd.revents & POLLIN;
> }
>
> -/* Causes the next poll_block() to wake up when 'latch' is set. */
> +/* Causes the next poll_block() to wake up when 'latch' is set.
> + *
> + * ('where' is used in debug logging. Commonly one would use
> latch_wait() to
> + * automatically provide the caller's source file and line number for
> + * 'where'.) */
> void
> -(latch_wait)(const struct latch *latch, const char *where)
> +latch_wait_at(const struct latch *latch, const char *where)
> {
> -(poll_fd_wait)(latch->fds[0], POLLIN, where);
> +poll_fd_wait_at(latch->fds[0], POLLIN, where);
> }
> diff --git a/lib/latch.h b/lib/latch.h
> index 08f45b1..0b6e8a3 100644
> --- a/lib/latch.h
> +++ b/lib/latch.h
> @@ -36,7 +36,7 @@ bool latch_poll(struct latch *);
> void latch_set(struct latch *);
>
> bool latch_is_set(const struct latch *);
> -void latch_wait(const struct latch *, const char *where);
> -#define latch_wait(latch) latch_wait(latch, SOURCE_LOCATOR)
> +void latch_wait_at(const struct latch *, const char *where);
> +#define latch_wait(latch) latch_wait_at(latch, SOURCE_LOCATOR)
>
> #endif /* latch.h */
> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> index d08751c..feff102 100644
> --- a/lib/ovs-thread.c
> +++ b/lib/ovs-thread.c
> @@ -128,9 +128,13 @@ ovsthread_once_done(struct ovsthread_once *once)
> }
>
> /* Asserts that the process has not yet created any threads (beyond the
> initial
> - * thread). */
> + * thread).
> + *
> + * ('where' is used in logging. Commonly one would use
> + * assert_single_threaded() to automatically provide the caller's source
> file
> + * and line number for 'where'.) */
> void
> -(assert_single_threaded)(const char *where)
> +assert_single_threaded_at(const char *where)
> {
> if (multithreaded) {
> VLOG_FATAL("%s: attempted operation not allowed when
> multithreaded",
> @@ -140,9 +144,13 @@ void
>
> /* Forks the current process (checking that this is allowed). Aborts with
> * VLOG_FATAL if fork() returns an error, and otherwise returns the value
> - * returned by fork(). */
> + * returned by fork().
> + *
> + * ('where' is used in logging. Commonly one would use xfork() to
> + * automatically provide the caller's source file and line number for
> + * 'where'.) */
> pid_t
> -(xfork)(const char *where)
> +xfork_at(const char *where)
> {
> pid_t pid;
>
> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
> index 9c8023e..afb5ccd 100644
> --- a/lib/ovs-thread.h
> +++ b/lib/ovs-thread.h
> @@ -368,11 +368,11 @@ ovsthread_once_start(struct ovsthread_once *once)
> ((ONCE)->done ? false : ({ OVS_ACQUIRE(ONCE); true; }))
> #endif
>
> -void assert_single_threaded(const char *where);
> -#define assert_single_threaded() assert_single_threaded(SOURCE_LOCATOR)
> +void assert_single_threaded_at(const char *where);
> +#define assert_single_threaded() assert_single_threaded_at(SOURCE_LOCATOR)
>
> -pid_t xfork(const char *where);
> -#define xfork() xfork(SOURCE_LOCATOR)
> +pid_t xfork_at(const char *where);
> +#define xfork() xfork_at(SOURCE_LOCATOR)
>
> void forbid_forking(const char *reason);
> bool may_fork(void);
> diff --git a/lib/poll-loop.c b/lib/poll-loop.c
> index 5f4b16c..0e11c7e 100644
> --- a/lib/poll-loop.c
> +++ b/lib/poll-loop.c
> @@ -29,11 +29,6 @@
> #include "timeval.h"
>
Re: [ovs-dev] [PATCH] Avoid C preprocessor trick where macro has the same name as a function.
On Mon, Jul 29, 2013 at 10:24:50AM -0700, Gurucharan Shetty wrote: > On Mon, Jul 29, 2013 at 9:57 AM, Ben Pfaff wrote: > > > In C, one can do preprocessor tricks by making a macro expansion include > > the macro's own name. We actually used this in the tree to automatically > > provide function arguments, e.g.: > > > > #define f(x) f(x, __FILE__, __LINE__) > > int f(int x, const char *file, int line); > > > > ... > > > > f(1);/* Expands to a call like f(1, __FILE__, __LINE__); */ > > > > However it's somewhat confusing, so this commit stops using that trick. > > > > Reported-by: Ed Maste > > Signed-off-by: Ben Pfaff > > > What is the commit on which this patch applies? I can't apply it on top of > master. There's a whole repo here: https://github.com/blp/ovs-reviews/commits/cpp-trick ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling
On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
> diff --git a/datapath/flow.c b/datapath/flow.c
> index ba775f4..5ec1b69 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -224,6 +224,15 @@ static bool ovs_match_validate(const struct
> sw_flow_match *match,
> return false;
> }
>
> + if (match->mask &&
> + !(match->mask->key.eth.tci & htons(VLAN_TAG_PRESENT))) {
> + OVS_NLERR("VLAN present bit can not be
> wildcarded.\n");
> + /* Simply log error until user the space program is
> +* fixed. Then we can switch to return false from
> +* here.
> +*/
> + }
Can we just fix this? We already force the bit on for the value in
userspace, it seems like we could do it for the mask at the same time.
I think we could also simplify it by just removing the !is_mask test
in the check in ovs_key_from_nlattrs().
> @@ -1317,6 +1326,7 @@ static int metadata_from_nlattrs(struct sw_flow_match
> *match, u64 *attrs,
> *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
> } else if (!is_mask) {
> SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS, is_mask);
> + SW_FLOW_KEY_PUT(match, phy.in_port, 0x, !is_mask);
Can you put this in a separate patch? All of these
attribute-not-present corner cases are getting really nasty and I
think that the vlan issues are actually somewhat separate.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
On Fri, Jul 26, 2013 at 06:07:02PM -0700, Ethan Jackson wrote:
> This commit adds annotations for thread safety check. And the
> check can be conducted by using -Wthread-safety flag in clang.
I would think that the commit should also explain where to get clang
with this support, probably by adding it to INSTALL. Also, I'd guess
that it would be worth turning it on by default if it is available, by
adding
OVS_ENABLE_OPTION([-Wthread-safety])
to configure.ac.
Oh, I see Alex sent a separate patch for that. Any reason not to fold
it in?
> +#if __has_feature(c_thread_safety_attributes)
> +/* "clang" annotations for thread safety check.
> + *
> + * OVS_LOCKABLE indicates that the struct contains mutex element
> + * which can be locked by ovs_mutex_lock().
> + *
What does the following sentence mean? I do not understand it. Also,
s/sturct/struct/.
> + * MUTEX below can be more than one OVS_LOCKABLE sturcts.
> + *
> + * In a global variable declaration:
> *
Here, s/must be access/may only be accessed/:
> + * OVS_GUARDED_VAR indicates that the variable must be access while
> + * some MUTEX is acquired.
> *
> + * OVS_GUARDED_BY(...) indicates that the variable must be access while
> + * MUTEX is acquired.
> + *
> + *
> + * In a function prototype:
> + *
> + * OVS_ACQ_RDLOCK(...) and OVS_ACQ_WRLOCK(...) indicate that the
> + * function must be called without MUTEX acquired and that it returns
> + * with MUTEX acquired. OVS_RELEASES(...) indicates the reverse.
OVS_RELEASES applies to both read-locks and write-locks?
I guess that an ordinary mutex is considered a write-lock?
> + * OVS_TRY_RDLOCK(...) and OVS_TRY_WRLOCK(...) indicate that the
> + * function will try to acquire MUTEX. The macro takes one or more
> + * arguments. The first argument is an interger or boolean value
> + * specifying the return value of a successful lock acquisition.
> + * The remaining arguments are MUTEX.
> + *
> + * OVS_REQ_RDLOCK(...) and OVS_REQ_WRLOCK(...) indicate that the
> + * function must be called with MUTEX acquired by the caller and that
> + * the function does not release MUTEX.
> + *
> + * OVS_LOCKS_EXCLUDED(MUTEX) indicates that the function must be called
> + * without MUTEX acquired. */
In lockfile.c, I guess that clang can only handle guarded-by on
pointers? Otherwise I don't see why one would introduce the new
lock_table variable as a level of indirection. Assuming that's true,
can we declare lock_table as a const pointer, e.g.
static struct hmap *const lock_table OVS_GUARDED_BY(lock_table_mutex)
= &lock_table__;
(That is, the pointer is const, not what it points to.)
This commit adds more of the "#define macro(x) macro(x,
SOURCE_LOCATOR)" trick that Ed Maste rightly reported as confusing. I
wrote a patch to get rid of that some time ago, but I guess it didn't
make it to email somehow (it *was* in my reviews repository). I've
re-sent it now
(http://openvswitch.org/pipermail/dev/2013-July/030009.html), and it
would be nice not to introduce more instances in the meantime.
In ovs-thread.c, I see a number of casts from const to nonconst that
don't use CONST_CAST. I suggest that they should.
I'd add semicolons at the ends of these lines unless it makes the
compiler complain:
> +INIT_FUNCTION(mutex)
> +INIT_FUNCTION(rwlock)
The big macros in ovs-thread.c might be slightly more readable if the
names of the arguments were shortened (e.g. LOCK_TYPE => TYPE,
LOCK_FUN => FUN).
I see a little bit of an abstraction mismatch in ovs-thread.h. The
changes to OVS_MUTEX_ADAPTIVE don't make sense, because the new value
isn't useful as an argument to pthread_mutexattr_settype(), as the
previous value was. Also, I'm not sure that it makes sense to
maintain the viewpoint of the big comment, since we're defining a new
type here, not supplementing standard types anymore. Also, the
ordering and arrangement seemed a little odd. I'm appending a
suggested incremental to fold in.
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 54e1791..89790ff 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -22,59 +22,55 @@
#include
#include "ovs-atomic.h"
#include "util.h"
+
+/* Mutex. */
struct OVS_LOCKABLE ovs_mutex {
pthread_mutex_t lock;
const char *where;
};
-struct OVS_LOCKABLE ovs_rwlock {
-pthread_rwlock_t lock;
-const char *where;
-};
-
-#define OVS_RWLOCK_INITIALIZER { PTHREAD_RWLOCK_INITIALIZER, NULL }
-#define OVS_MUTEX_INITIALIZER { PTHREAD_MUTEX_INITIALIZER, NULL }
-
-/* glibc has some non-portable mutex types and initializers:
+/* "struct ovs_mutex" initializers:
*
- *- PTHREAD_MUTEX_ADAPTIVE_NP is a mutex type that works as a spinlock that
- * falls back to a mutex after spinning for some number of iterations.
+ *- OVS_MUTEX_INITIALIZER: common case.
*
- *- PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP is a non-portable initializer
- * for an error-checking mutex.
+ *- OVS_ADAPTIVE_MUTEX_INITIALIZER for a mutex that spins briefly then go
Re: [ovs-dev] [PATCH] Avoid C preprocessor trick where macro has the same name as a function.
On Mon, Jul 29, 2013 at 9:57 AM, Ben Pfaff wrote:
> In C, one can do preprocessor tricks by making a macro expansion include
>
Usually we put a : here. I am not sure whether you expect it on every
patch.
> the macro's own name. We actually used this in the tree to automatically
> provide function arguments, e.g.:
>
> #define f(x) f(x, __FILE__, __LINE__)
> int f(int x, const char *file, int line);
>
> ...
>
> f(1);/* Expands to a call like f(1, __FILE__, __LINE__); */
>
> However it's somewhat confusing, so this commit stops using that trick.
>
> Reported-by: Ed Maste
> Signed-off-by: Ben Pfaff
> ---
> lib/jsonrpc.c|2 +-
> lib/latch.c | 10 +++---
> lib/latch.h |4 ++--
> lib/ovs-thread.c | 16
> lib/ovs-thread.h |8
> lib/poll-loop.c | 37 ++---
> lib/poll-loop.h | 20 +++-
> lib/timer.c | 12
> lib/timer.h |6 +++---
> 9 files changed, 66 insertions(+), 49 deletions(-)
>
> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
> index 56b4cce..469fbae 100644
> --- a/lib/jsonrpc.c
> +++ b/lib/jsonrpc.c
> @@ -350,7 +350,7 @@ void
> jsonrpc_recv_wait(struct jsonrpc *rpc)
> {
> if (rpc->status || rpc->received || !byteq_is_empty(&rpc->input)) {
> -(poll_immediate_wake)(rpc->name);
> +poll_immediate_wake_at(rpc->name);
> } else {
> stream_recv_wait(rpc->stream);
> }
> diff --git a/lib/latch.c b/lib/latch.c
> index 9b13006..bf518b9 100644
> --- a/lib/latch.c
> +++ b/lib/latch.c
> @@ -75,9 +75,13 @@ latch_is_set(const struct latch *latch)
> return pfd.revents & POLLIN;
> }
>
> -/* Causes the next poll_block() to wake up when 'latch' is set. */
> +/* Causes the next poll_block() to wake up when 'latch' is set.
> + *
> + * ('where' is used in debug logging. Commonly one would use
> latch_wait() to
> + * automatically provide the caller's source file and line number for
> + * 'where'.) */
> void
> -(latch_wait)(const struct latch *latch, const char *where)
> +latch_wait_at(const struct latch *latch, const char *where)
> {
> -(poll_fd_wait)(latch->fds[0], POLLIN, where);
> +poll_fd_wait_at(latch->fds[0], POLLIN, where);
> }
> diff --git a/lib/latch.h b/lib/latch.h
> index 08f45b1..0b6e8a3 100644
> --- a/lib/latch.h
> +++ b/lib/latch.h
> @@ -36,7 +36,7 @@ bool latch_poll(struct latch *);
> void latch_set(struct latch *);
>
> bool latch_is_set(const struct latch *);
> -void latch_wait(const struct latch *, const char *where);
> -#define latch_wait(latch) latch_wait(latch, SOURCE_LOCATOR)
> +void latch_wait_at(const struct latch *, const char *where);
> +#define latch_wait(latch) latch_wait_at(latch, SOURCE_LOCATOR)
>
> #endif /* latch.h */
> diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> index d08751c..feff102 100644
> --- a/lib/ovs-thread.c
> +++ b/lib/ovs-thread.c
> @@ -128,9 +128,13 @@ ovsthread_once_done(struct ovsthread_once *once)
> }
>
> /* Asserts that the process has not yet created any threads (beyond the
> initial
> - * thread). */
> + * thread).
> + *
> + * ('where' is used in logging. Commonly one would use
> + * assert_single_threaded() to automatically provide the caller's source
> file
> + * and line number for 'where'.) */
> void
> -(assert_single_threaded)(const char *where)
> +assert_single_threaded_at(const char *where)
> {
> if (multithreaded) {
> VLOG_FATAL("%s: attempted operation not allowed when
> multithreaded",
> @@ -140,9 +144,13 @@ void
>
> /* Forks the current process (checking that this is allowed). Aborts with
> * VLOG_FATAL if fork() returns an error, and otherwise returns the value
> - * returned by fork(). */
> + * returned by fork().
> + *
> + * ('where' is used in logging. Commonly one would use xfork() to
> + * automatically provide the caller's source file and line number for
> + * 'where'.) */
> pid_t
> -(xfork)(const char *where)
> +xfork_at(const char *where)
> {
> pid_t pid;
>
> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
> index 9c8023e..afb5ccd 100644
> --- a/lib/ovs-thread.h
> +++ b/lib/ovs-thread.h
> @@ -368,11 +368,11 @@ ovsthread_once_start(struct ovsthread_once *once)
> ((ONCE)->done ? false : ({ OVS_ACQUIRE(ONCE); true; }))
> #endif
>
> -void assert_single_threaded(const char *where);
> -#define assert_single_threaded() assert_single_threaded(SOURCE_LOCATOR)
> +void assert_single_threaded_at(const char *where);
> +#define assert_single_threaded() assert_single_threaded_at(SOURCE_LOCATOR)
>
> -pid_t xfork(const char *where);
> -#define xfork() xfork(SOURCE_LOCATOR)
> +pid_t xfork_at(const char *where);
> +#define xfork() xfork_at(SOURCE_LOCATOR)
>
> void forbid_forking(const char *reason);
> bool may_fork(void);
> diff --git a/lib/poll-loop.c b/lib/poll-loop.c
> index 5f4b16c..0e11c7e 100644
> --- a/lib/poll-loop.c
> +++ b/lib/poll-loop.c
> @@ -29,11 +29,6 @@
> #include "timeval.h"
> #include "vlog.h"
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
> > +#if __has_feature(c_thread_safety_attributes) > > +/* "clang" annotations for thread safety check. > > + * > > + * OVS_LOCKABLE indicates that the struct contains mutex element > > + * which can be locked by ovs_mutex_lock(). > > + * > > What does the following sentence mean? I do not understand it. Also, > s/sturct/struct/. > Thanks Ben, for pointing my typo out. > > > + * MUTEX below can be more than one OVS_LOCKABLE sturcts. > > + * > This means that we can require a function to hold multiple locks while invoked. e.g. some_function() OVS_REQ_WRLOCK(wrlock_file1, wrlock_file2) Do you think adding the above sentences will make it more comprehensible? > + * In a global variable declaration: > > * > > Here, s/must be access/may only be accessed/: > Thanks for pointing this out. > > + * OVS_GUARDED_VAR indicates that the variable must be access while > > + * some MUTEX is acquired. > > * > > + * OVS_GUARDED_BY(...) indicates that the variable must be access while > > + * MUTEX is acquired. > > + * > > + * > > + * In a function prototype: > > + * > > + * OVS_ACQ_RDLOCK(...) and OVS_ACQ_WRLOCK(...) indicate that the > > + * function must be called without MUTEX acquired and that it returns > > + * with MUTEX acquired. OVS_RELEASES(...) indicates the reverse. > > OVS_RELEASES applies to both read-locks and write-locks? Yes, I guess that an ordinary mutex is considered a write-lock? Yes, I'll wait for updates (as you suggested in comments) and rewrite this part of comments to make it more comprehensible. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
On Mon, Jul 29, 2013 at 10:47:47AM -0700, Ben Pfaff wrote: > On Fri, Jul 26, 2013 at 06:07:02PM -0700, Ethan Jackson wrote: > > This commit adds annotations for thread safety check. And the > > check can be conducted by using -Wthread-safety flag in clang. One additional thought. In reading the annotations in later patches, the need to annotate a mutex as a write-lock causes a little mental dissonance for me. When I see OVS_ACQ_WRLOCK(xyzzy), I think, "Oh, xyzzy must be a read-write lock." Could we allow OVS_ACQUIRES etc. to be a synonym for use with plain mutexes? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 02/11] stp: Make the STP module thread safe.
On Fri, Jul 26, 2013 at 06:07:03PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson In stp_unref(), the list_remove() needs to be guarded by the mutex. Some of the annotations are only on the function prototypes and not the definitions. Although the compiler is OK with this, it would be better for the reader to annotate both places. Some of the functions take the mutex gratuitously, e.g. because they access immutable state. One example is stp_get_name(). I guess that these are not a big deal because this is not going to be performance critical, but at some point we'll hit some performance-critical case where we don't really want to have to take the lock. For cases like that, what's the best way to suppress a warning? Why does stp need a recursive mutex? I don't see any natural recursion here. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling
On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross wrote:
> On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
>> @@ -1317,6 +1326,7 @@ static int metadata_from_nlattrs(struct sw_flow_match
>> *match, u64 *attrs,
>> *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
>> } else if (!is_mask) {
>> SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS, is_mask);
>> + SW_FLOW_KEY_PUT(match, phy.in_port, 0x, !is_mask);
>
> Can you put this in a separate patch? All of these
> attribute-not-present corner cases are getting really nasty and I
> think that the vlan issues are actually somewhat separate.
Actually, I don't think that this part is right anyways. The fact that
someone implicitly used a 'no-port' input port doesn't inherently mean
that they want to match on it - it could just be part of the packet
that's causing the flow to be set up.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling
Should 'no-port' be treated as wildcarded match on in_port?
On Mon, Jul 29, 2013 at 11:31 AM, Jesse Gross wrote:
> On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross wrote:
> > On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
> >> @@ -1317,6 +1326,7 @@ static int metadata_from_nlattrs(struct
> sw_flow_match *match, u64 *attrs,
> >> *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
> >> } else if (!is_mask) {
> >> SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS,
> is_mask);
> >> + SW_FLOW_KEY_PUT(match, phy.in_port, 0x, !is_mask);
> >
> > Can you put this in a separate patch? All of these
> > attribute-not-present corner cases are getting really nasty and I
> > think that the vlan issues are actually somewhat separate.
>
> Actually, I don't think that this part is right anyways. The fact that
> someone implicitly used a 'no-port' input port doesn't inherently mean
> that they want to match on it - it could just be part of the packet
> that's causing the flow to be set up.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Avoid C preprocessor trick where macro has the same name as a function.
On 29 July 2013 12:57, Ben Pfaff wrote: > In C, one can do preprocessor tricks by making a macro expansion include > the macro's own name. We actually used this in the tree to automatically > provide function arguments, e.g.: > > #define f(x) f(x, __FILE__, __LINE__) > int f(int x, const char *file, int line); > > ... > > f(1);/* Expands to a call like f(1, __FILE__, __LINE__); */ > > However it's somewhat confusing, so this commit stops using that trick. > > Reported-by: Ed Maste > Signed-off-by: Ben Pfaff Acked-by: Ed Maste One minor comment: >#define f(x) f(x, __FILE__, __LINE__) >int f(int x, const char *file, int line); Unlike the instances in the source, the example as-is doesn't compile, which I think helps provide justification for the removal of the trick :-) It's missing either an #undef, or the further trick of relying on the preprocessor only expanding function-like macros if the token following is a left parenthesis, resulting in the odd-looking int (f)(int x, const char *file, int line) syntax. X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: fix a bug in SF_FLOW_KEY_PUT macro
Yes indeed. Will add to this patch.
On Mon, Jul 29, 2013 at 9:46 AM, Jesse Gross wrote:
> On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
> > This bug will cause mask values to corrupt the flow key value. So far
> > the bug has not showed up because we don't write mask value when
> > there is no mask Netlink attributes. However, it needs to be fixed for
> > the next and future commits where we will start to set default
> > values for key and mask for missing Netlink attributes.
> >
> > Signed-off-by: Andy Zhou
> > ---
> > datapath/flow.c |5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index 2c11408..ba775f4 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -82,8 +82,9 @@ static void update_range__(struct sw_flow_match *match,
> > do { \
> > update_range__(match, offsetof(struct sw_flow_key,
> field), \
> > sizeof((match)->key->field),
> is_mask); \
> > - if (is_mask && match->mask != NULL) {
> \
> > - (match)->mask->key.field = value;
> \
> > + if (is_mask) {
>\
> > + if ((match)->mask)
>\
> > + (match)->mask->key.field = value;
> \
>
> Shouldn't we also do this for the memcpy variant?
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
On Mon, Jul 29, 2013 at 11:06:55AM -0700, Alex Wang wrote: > > > +#if __has_feature(c_thread_safety_attributes) > > > +/* "clang" annotations for thread safety check. > > > + * > > > + * OVS_LOCKABLE indicates that the struct contains mutex element > > > + * which can be locked by ovs_mutex_lock(). > > > + * > > > > What does the following sentence mean? I do not understand it. Also, > > s/sturct/struct/. > > > > Thanks Ben, for pointing my typo out. > > > > > > > + * MUTEX below can be more than one OVS_LOCKABLE sturcts. > > > + * > > > > This means that we can require a function to hold multiple locks while > invoked. > e.g. some_function() OVS_REQ_WRLOCK(wrlock_file1, wrlock_file2) > > Do you think adding the above sentences will make it more comprehensible? I understand now. Yes, that would help a lot, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling
On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross wrote:
> On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index ba775f4..5ec1b69 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -224,6 +224,15 @@ static bool ovs_match_validate(const struct
> sw_flow_match *match,
> > return false;
> > }
> >
> > + if (match->mask &&
> > + !(match->mask->key.eth.tci & htons(VLAN_TAG_PRESENT))) {
> > + OVS_NLERR("VLAN present bit can not be
> wildcarded.\n");
> > + /* Simply log error until user the space program
> is
> > +* fixed. Then we can switch to return false from
> > +* here.
> > +*/
> > + }
>
> Can we just fix this? We already force the bit on for the value in
> userspace, it seems like we could do it for the mask at the same time.
>
We could, but I was hopping to return an error from here eventually.
>
> I think we could also simplify it by just removing the !is_mask test
> in the check in ovs_key_from_nlattrs().
>
That would cause the mask value to be set twice, which is fine. But the
logic would seem inconsistent with IN_PORT and look odd to me. But I don't
mind changing it if you insist.
> @@ -1317,6 +1326,7 @@ static int metadata_from_nlattrs(struct
sw_flow_match *match, u64 *attrs,
> *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
> } else if (!is_mask) {
> SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS,
is_mask);
> + SW_FLOW_KEY_PUT(match, phy.in_port, 0x, !is_mask);
Can you put this in a separate patch? All of these
> attribute-not-present corner cases are getting really nasty and I
> think that the vlan issues are actually somewhat separate.
>
O.K. I will remove it from this patch. We do need to review the handling
for this and other missing attributes.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling
When we were previously doing just exact match, 'no-port' still
existed and it obviously wasn't a wildcard so I don't think that it
inherently means one now.
When the in port value is omitted, we currently allow you to omit the
mask or have either an all zeros or all ones mask so userspace should
still have control.
On Mon, Jul 29, 2013 at 12:17 PM, Andy Zhou wrote:
> Should 'no-port' be treated as wildcarded match on in_port?
>
>
> On Mon, Jul 29, 2013 at 11:31 AM, Jesse Gross wrote:
>>
>> On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross wrote:
>> > On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
>> >> @@ -1317,6 +1326,7 @@ static int metadata_from_nlattrs(struct
>> >> sw_flow_match *match, u64 *attrs,
>> >> *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
>> >> } else if (!is_mask) {
>> >> SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS,
>> >> is_mask);
>> >> + SW_FLOW_KEY_PUT(match, phy.in_port, 0x, !is_mask);
>> >
>> > Can you put this in a separate patch? All of these
>> > attribute-not-present corner cases are getting really nasty and I
>> > think that the vlan issues are actually somewhat separate.
>>
>> Actually, I don't think that this part is right anyways. The fact that
>> someone implicitly used a 'no-port' input port doesn't inherently mean
>> that they want to match on it - it could just be part of the packet
>> that's causing the flow to be set up.
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [Missing vlan V2 1/2] datapath: fix a bug in SF_FLOW_KEY_PUT macro
This bug will cause mask values to corrupt the flow key value. So far
the bug has not showed up because we don't write mask value when
there is no mask Netlink attributes. However, it needs to be fixed for
the next and future commits where we will start to set default
values for key and mask for missing Netlink attributes.
Signed-off-by: Andy Zhou
---
V1->v2: Apply the same fix for the memcpy version
---
datapath/flow.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/datapath/flow.c b/datapath/flow.c
index 2c11408..7f69538 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -82,8 +82,9 @@ static void update_range__(struct sw_flow_match *match,
do { \
update_range__(match, offsetof(struct sw_flow_key, field), \
sizeof((match)->key->field), is_mask); \
- if (is_mask && match->mask != NULL) { \
- (match)->mask->key.field = value; \
+ if (is_mask) { \
+ if ((match)->mask) \
+ (match)->mask->key.field = value; \
} else {\
(match)->key->field = value;\
} \
@@ -93,8 +94,9 @@ static void update_range__(struct sw_flow_match *match,
do { \
update_range__(match, offsetof(struct sw_flow_key, field), \
len, is_mask); \
- if (is_mask && match->mask != NULL) { \
- memcpy(&(match)->mask->key.field, value_p, len);\
+ if (is_mask) { \
+ if ((match)->mask) \
+ memcpy(&(match)->mask->key.field, value_p,
len);\
} else {\
memcpy(&(match)->key->field, value_p, len); \
} \
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [Missing vlan V2 2/2] datapath: Fix missing VLAN netlink attribute handling
Missing VLAN netlink attribute should be interpreted as exact match
of no VLAN tag, instead of wildcarded match for all VLAN tags.
Bug #18736.
Signed-off-by: Andy Zhou
---
v1->v2
*Remove the fix for in_port.
*Always set VLAN_TAG_PRESENT bit in the mask.
---
datapath/flow.c |9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/datapath/flow.c b/datapath/flow.c
index 7f69538..eed841c 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -225,6 +225,12 @@ static bool ovs_match_validate(const struct sw_flow_match
*match,
return false;
}
+ if (match->mask &&
+ !(match->mask->key.eth.tci & htons(VLAN_TAG_PRESENT))) {
+ OVS_NLERR("VLAN present bit can not be wildcarded.\n");
+ match->mask->key.eth.tci |= htons(VLAN_TAG_PRESENT);
+ }
+
return true;
}
@@ -1373,7 +1379,8 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
*match, u64 attrs,
SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
- }
+ } else if (!is_mask)
+ SW_FLOW_KEY_PUT(match, eth.tci, htons(0x), true);
if (attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
__be16 eth_type;
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling
On Mon, Jul 29, 2013 at 12:44 PM, Andy Zhou wrote:
> On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross wrote:
>>
>> On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
>> > diff --git a/datapath/flow.c b/datapath/flow.c
>> > index ba775f4..5ec1b69 100644
>> > --- a/datapath/flow.c
>> > +++ b/datapath/flow.c
>> > @@ -224,6 +224,15 @@ static bool ovs_match_validate(const struct
>> > sw_flow_match *match,
>> > return false;
>> > }
>> >
>> > + if (match->mask &&
>> > + !(match->mask->key.eth.tci & htons(VLAN_TAG_PRESENT))) {
>> > + OVS_NLERR("VLAN present bit can not be
>> > wildcarded.\n");
>> > + /* Simply log error until user the space program
>> > is
>> > +* fixed. Then we can switch to return false
>> > from
>> > +* here.
>> > +*/
>> > + }
>>
>> Can we just fix this? We already force the bit on for the value in
>> userspace, it seems like we could do it for the mask at the same time.
>
>
> We could, but I was hopping to return an error from here eventually.
I'm not sure that I understand. I was just referring to fixing the
issue that you mentioned in the comment now rather than later.
>>
>>
>> I think we could also simplify it by just removing the !is_mask test
>> in the check in ovs_key_from_nlattrs().
>
> That would cause the mask value to be set twice, which is fine. But the
> logic would seem inconsistent with IN_PORT and look odd to me. But I don't
> mind changing it if you insist.
Why would this cause the mask to get set twice? I think that check is
just protecting the sanity check for the CFI bit.
All of the other checks that are in the validate function look at the
combination of values and mask whereas the ones that can be evaluated
independently are in ovs_key_from_nlattrs(). Since the logic is the
same and needs to be applied to both values and masks I think it fits
pretty much exactly in from_nlattrs.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling
On Mon, Jul 29, 2013 at 1:27 PM, Jesse Gross wrote:
> On Mon, Jul 29, 2013 at 12:44 PM, Andy Zhou wrote:
>> On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross wrote:
>>>
>>> On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
>>> > diff --git a/datapath/flow.c b/datapath/flow.c
>>> > index ba775f4..5ec1b69 100644
>>> > --- a/datapath/flow.c
>>> > +++ b/datapath/flow.c
>>> > @@ -224,6 +224,15 @@ static bool ovs_match_validate(const struct
>>> > sw_flow_match *match,
>>> > return false;
>>> > }
>>> >
>>> > + if (match->mask &&
>>> > + !(match->mask->key.eth.tci & htons(VLAN_TAG_PRESENT))) {
>>> > + OVS_NLERR("VLAN present bit can not be
>>> > wildcarded.\n");
>>> > + /* Simply log error until user the space program
>>> > is
>>> > +* fixed. Then we can switch to return false
>>> > from
>>> > +* here.
>>> > +*/
>>> > + }
>>>
>>> Can we just fix this? We already force the bit on for the value in
>>> userspace, it seems like we could do it for the mask at the same time.
>>
>>
>> We could, but I was hopping to return an error from here eventually.
>
> I'm not sure that I understand. I was just referring to fixing the
> issue that you mentioned in the comment now rather than later.
OK, I understand what you meant now that I see the new version of the
patch. I meant fixing userspace.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [Missing vlan V2 1/2] datapath: fix a bug in SF_FLOW_KEY_PUT macro
On Mon, Jul 29, 2013 at 1:26 PM, Andy Zhou wrote: > This bug will cause mask values to corrupt the flow key value. So far > the bug has not showed up because we don't write mask value when > there is no mask Netlink attributes. However, it needs to be fixed for > the next and future commits where we will start to set default > values for key and mask for missing Netlink attributes. > > Signed-off-by: Andy Zhou Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/2] ovs-dev.py: Rely on configure for warning options.
Both -Wall and -Wextra are handled by autoconf, so there's no longer a
need for ovs-dev.py to pass them through CFLAGS.
Signed-off-by: Ethan Jackson
---
utilities/ovs-dev.py |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utilities/ovs-dev.py b/utilities/ovs-dev.py
index 8c8f5bd..f3a1ba9 100755
--- a/utilities/ovs-dev.py
+++ b/utilities/ovs-dev.py
@@ -26,7 +26,7 @@ OVS_SRC = HOME + "/ovs"
ROOT = HOME + "/root"
PATH = "%(ovs)s/utilities:%(ovs)s/ovsdb:%(ovs)s/vswitchd" % {"ovs": OVS_SRC}
-ENV["CFLAGS"] = "-g -O0 -Wall -Wextra -Wno-deprecated-declarations"
+ENV["CFLAGS"] = "-g -O0"
ENV["PATH"] = PATH + ":" + ENV["PATH"]
options = None
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling
I will send out V3 that returns false, and move the logic back to
ovs_key_from_nlattrs(). Userspace fixes, if any, needs to have its own
patch.
On Mon, Jul 29, 2013 at 1:29 PM, Jesse Gross wrote:
> On Mon, Jul 29, 2013 at 1:27 PM, Jesse Gross wrote:
> > On Mon, Jul 29, 2013 at 12:44 PM, Andy Zhou wrote:
> >> On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross wrote:
> >>>
> >>> On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou wrote:
> >>> > diff --git a/datapath/flow.c b/datapath/flow.c
> >>> > index ba775f4..5ec1b69 100644
> >>> > --- a/datapath/flow.c
> >>> > +++ b/datapath/flow.c
> >>> > @@ -224,6 +224,15 @@ static bool ovs_match_validate(const struct
> >>> > sw_flow_match *match,
> >>> > return false;
> >>> > }
> >>> >
> >>> > + if (match->mask &&
> >>> > + !(match->mask->key.eth.tci &
> htons(VLAN_TAG_PRESENT))) {
> >>> > + OVS_NLERR("VLAN present bit can not be
> >>> > wildcarded.\n");
> >>> > + /* Simply log error until user the space
> program
> >>> > is
> >>> > +* fixed. Then we can switch to return false
> >>> > from
> >>> > +* here.
> >>> > +*/
> >>> > + }
> >>>
> >>> Can we just fix this? We already force the bit on for the value in
> >>> userspace, it seems like we could do it for the mask at the same time.
> >>
> >>
> >> We could, but I was hopping to return an error from here eventually.
> >
> > I'm not sure that I understand. I was just referring to fixing the
> > issue that you mentioned in the comment now rather than later.
>
> OK, I understand what you meant now that I see the new version of the
> patch. I meant fixing userspace.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dev.py: Add support for clang builds.
On Mon, Jul 29, 2013 at 01:46:42PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson Acked-by: Ben Pfaff (I didn't test either of these but they look reasonable to me.) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] ovs-dev.py: Rely on configure for warning options.
On Mon, Jul 29, 2013 at 01:46:41PM -0700, Ethan Jackson wrote: > Both -Wall and -Wextra are handled by autoconf, so there's no longer a > need for ovs-dev.py to pass them through CFLAGS. > > Signed-off-by: Ethan Jackson Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
On Fri, Jul 26, 2013 at 06:07:02PM -0700, Ethan Jackson wrote: > 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 > Signed-off-by: Alex Wang > Signed-off-by: Ethan Jackson Currently, the 'where' members of ovs_mutex and ovs_rwlock are write-only. It would be nice if the pthread failure abort messages mentioned the caller's location and, at least for EDEADLK, the location of the code deadlocked against. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 03/11] cfm: Make the CFM module thread safe.
On Fri, Jul 26, 2013 at 06:07:04PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson Why does cfm_generate_maid() need the mutex? It doesn't seem to use any static data and when it is called the new struct cfm isn't visible outside the running thread. Can all_cfms be a const pointer, e.g: static struct hmap *const all_cfms OVS_GUARDED_BY(mutex) = &all_cfms__; I think there's at least one deadlock. cfm_unixctl_show() takes the lock, calls cfm_print_details(), which calls cfm_get_fault(), which takes the lock. The caller can't use cfm_get_remote_mpids()'s returned data in a race-free way. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [Missing vlan V3 1/2] datapath: fix a bug in SF_FLOW_KEY_PUT macro
This bug will cause mask values to corrupt the flow key value. So far
the bug has not showed up because we don't write mask value when
there is no mask Netlink attributes. However, it needs to be fixed for
the next and future commits where we will start to set default
values for key and mask for missing Netlink attributes.
Signed-off-by: Andy Zhou
---
V1->v2: Apply the same fix for the memcpy version
V2->v3: no change
---
datapath/flow.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/datapath/flow.c b/datapath/flow.c
index 2c11408..7f69538 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -82,8 +82,9 @@ static void update_range__(struct sw_flow_match *match,
do { \
update_range__(match, offsetof(struct sw_flow_key, field), \
sizeof((match)->key->field), is_mask); \
- if (is_mask && match->mask != NULL) { \
- (match)->mask->key.field = value; \
+ if (is_mask) { \
+ if ((match)->mask) \
+ (match)->mask->key.field = value; \
} else {\
(match)->key->field = value;\
} \
@@ -93,8 +94,9 @@ static void update_range__(struct sw_flow_match *match,
do { \
update_range__(match, offsetof(struct sw_flow_key, field), \
len, is_mask); \
- if (is_mask && match->mask != NULL) { \
- memcpy(&(match)->mask->key.field, value_p, len);\
+ if (is_mask) { \
+ if ((match)->mask) \
+ memcpy(&(match)->mask->key.field, value_p,
len);\
} else {\
memcpy(&(match)->key->field, value_p, len); \
} \
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [Missing vlan V3 2/2] datapath: Fix missing VLAN netlink attribute handling
Missing VLAN netlink attribute should be interpreted as exact match
of no VLAN tag, instead of wildcarded match for all VLAN tags.
Bug #18736.
Signed-off-by: Andy Zhou
---
v1->v2
*Remove the fix for in_port.
*Always set VLAN_TAG_PRESENT bit in the mask.
v2->v3
*Move VLAN_TAG_PRESENT bit check back to ovs_key_from_nlattrs.
---
datapath/flow.c | 14 +-
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/datapath/flow.c b/datapath/flow.c
index 7f69538..3acc743 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1365,15 +1365,19 @@ static int ovs_key_from_nlattrs(struct sw_flow_match
*match, u64 attrs,
__be16 tci;
tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
- if (!is_mask)
- if (!(tci & htons(VLAN_TAG_PRESENT))) {
+ if (!(tci & htons(VLAN_TAG_PRESENT))) {
+ if (is_mask)
+ OVS_NLERR("VLAN TCI mask does not have exact
match for VLAN_TAG_PRESENT bit.\n");
+ else
OVS_NLERR("VLAN TCI does not have
VLAN_TAG_PRESENT bit set.\n");
- return -EINVAL;
- }
+
+ return -EINVAL;
+ }
SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
- }
+ } else if (!is_mask)
+ SW_FLOW_KEY_PUT(match, eth.tci, htons(0x), true);
if (attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
__be16 eth_type;
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/2] ovs-dev.py: Add support for clang builds.
Signed-off-by: Ethan Jackson
---
utilities/ovs-dev.py | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/utilities/ovs-dev.py b/utilities/ovs-dev.py
index f3a1ba9..c10ca7d 100755
--- a/utilities/ovs-dev.py
+++ b/utilities/ovs-dev.py
@@ -52,6 +52,9 @@ def uname():
def conf():
tag()
+if options.clang:
+ENV["CC"] = "clang"
+
configure = ["./configure", "--prefix=" + ROOT, "--localstatedir=" + ROOT,
"--with-logdir=%s/log" % ROOT, "--with-rundir=%s/run" % ROOT,
"--with-linux=/lib/modules/%s/build" % uname(),
@@ -75,7 +78,11 @@ def make(args=""):
make = "make -s -j 8 " + args
try:
_sh("cgcc", "--version", capture=True)
-make += " C=1"
+# XXX: For some reason the clang build doesn't place nicely with
+# sparse. At some point this needs to be figured out and this check
+# removed.
+if not options.clang:
+make += " C=1"
except OSError:
pass
_sh(make)
@@ -275,6 +282,8 @@ def main():
help="run ovs-vswitchd under gdb")
group.add_option("--valgrind", dest="valgrind", action="store_true",
help="run ovs-vswitchd under valgrind")
+group.add_option("--clang", dest="clang", action="store_true",
+ help="build ovs-vswitchd with clang")
parser.add_option_group(group)
options, args = parser.parse_args()
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
> OVS_RELEASES applies to both read-locks and write-locks? > > I guess that an ordinary mutex is considered a write-lock? Yep, I suppose we could create a second #define for that. Not sure if it matters or not. > In lockfile.c, I guess that clang can only handle guarded-by on > pointers? Otherwise I don't see why one would introduce the new > lock_table variable as a level of indirection. Assuming that's true, > can we declare lock_table as a const pointer, e.g. > static struct hmap *const lock_table OVS_GUARDED_BY(lock_table_mutex) > = &lock_table__; > (That is, the pointer is const, not what it points to.) Yes, this is fairly annoying. Clang doesn't consider taking the address of a variable as a read. Technically they're right, but that opens the door to all kinds of unsafety. In my experience, this change catches bugs, so I think it's worth the ugliness. X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
On Mon, Jul 29, 2013 at 02:10:40PM -0700, Ethan Jackson wrote: > > In lockfile.c, I guess that clang can only handle guarded-by on > > pointers? Otherwise I don't see why one would introduce the new > > lock_table variable as a level of indirection. Assuming that's true, > > can we declare lock_table as a const pointer, e.g. > > static struct hmap *const lock_table OVS_GUARDED_BY(lock_table_mutex) > > = &lock_table__; > > (That is, the pointer is const, not what it points to.) > > Yes, this is fairly annoying. Clang doesn't consider taking the > address of a variable as a read. Technically they're right, but that > opens the door to all kinds of unsafety. In my experience, this > change catches bugs, so I think it's worth the ugliness. Yeah, I understand the rationale, I just want to know whether we can make the pointer const. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
> Yeah, I understand the rationale, I just want to know whether we can > make the pointer const. I'm fine with it. I used the same trick in some other patches, I'll be sure to update them. Ethan X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dev.py: Add support for clang builds.
I've been using them to dev for some time. Thanks for the reviews, I'll merge soon. Ethan On Mon, Jul 29, 2013 at 1:50 PM, Ben Pfaff wrote: > On Mon, Jul 29, 2013 at 01:46:42PM -0700, Ethan Jackson wrote: >> Signed-off-by: Ethan Jackson > > Acked-by: Ben Pfaff > > (I didn't test either of these but they look reasonable to me.) X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Add vxlan and flow_dissector to gitignore.
Signed-off-by: Ethan Jackson --- datapath/linux/.gitignore |2 ++ 1 file changed, 2 insertions(+) diff --git a/datapath/linux/.gitignore b/datapath/linux/.gitignore index e7ac6c1..8748613 100644 --- a/datapath/linux/.gitignore +++ b/datapath/linux/.gitignore @@ -14,6 +14,7 @@ /exthdrs_core.c /flex_array.c /flow.c +/flow_dissector.c /genetlink-openvswitch.c /genl_exec.c /gre.c @@ -42,4 +43,5 @@ /vport-patch.c /vport-vxlan.c /vport.c +/vxlan.c /workqueue.c -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 02/11] stp: Make the STP module thread safe.
> Some of the functions take the mutex gratuitously, e.g. because they > access immutable state. One example is stp_get_name(). I guess that > these are not a big deal because this is not going to be performance > critical, but at some point we'll hit some performance-critical case > where we don't really want to have to take the lock. For cases like > that, what's the best way to suppress a warning? There are definitely ways around the thread safety analysis. My feeling is that we should be pretty strict about it except in modules where contention is a real problem. > Why does stp need a recursive mutex? I don't see any natural > recursion here. Slightly more convenient, but I think you're right. I'll remove it. Ethan X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [Missing vlan V3 2/2] datapath: Fix missing VLAN netlink attribute handling
On Mon, Jul 29, 2013 at 2:05 PM, Andy Zhou wrote: > Missing VLAN netlink attribute should be interpreted as exact match > of no VLAN tag, instead of wildcarded match for all VLAN tags. > > Bug #18736. > > Signed-off-by: Andy Zhou Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Add vxlan and flow_dissector to gitignore.
On Mon, Jul 29, 2013 at 2:23 PM, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson > --- > datapath/linux/.gitignore |2 ++ > 1 file changed, 2 insertions(+) Acked-by: Jesse Gross X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 04/11] lacp: Make the LACP module thread safe.
On Fri, Jul 26, 2013 at 06:07:05PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson In lacp_unref(), I think we need to move list_remove(&lacp->node); inside the lock/unlock. Otherwise we have a race here where lacp_find() can grab an entry that is being destroyed. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 05/11] bfd: Make the BFD module thread safe.
On Fri, Jul 26, 2013 at 06:07:06PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson I believe that bfd_get_status() has a deadlock because it calls bfd_forwarding() which also takes the lock. Otherwise: Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 06/11] netdev-vport: Make statistics thread safe.
On Fri, Jul 26, 2013 at 06:07:07PM -0700, Ethan Jackson wrote: > Statistics are the only part of netdev-vport which need to be > manipulated by multiple threads. This patch makes them thread safe. > > Signed-off-by: Ethan Jackson I'd rather fold this into my upcoming netdev series if that's OK with you. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Avoid C preprocessor trick where macro has the same name as a function.
On Mon, Jul 29, 2013 at 10:52:52AM -0700, Gurucharan Shetty wrote: > On Mon, Jul 29, 2013 at 9:57 AM, Ben Pfaff wrote: > > > In C, one can do preprocessor tricks by making a macro expansion include > > > Usually we put a : here. I am not sure whether you expect it on every > patch. For commits like this one that affect a treewide convention, I do not normally mention an area, because the commit is not localized to one area. > > @@ -47,17 +47,19 @@ struct poll_waiter; > > * (poll_fd_wait)(fd, events, where); > > > Shouldn't we change the above example to poll_fd_wait_at? The comment did need an update. I changed it to: /* Schedule events to wake up the following poll_block(). * * The poll_loop logs the 'where' argument to each function at "debug" level * when an event causes a wakeup. Each of these ways to schedule an event has * a function and a macro wrapper. The macro version automatically supplies * the source code location of the caller. The function version allows the * caller to supply a location explicitly, which is useful if the caller's own * caller would be more useful in log output. See timer_wait_at() for an * example. */ Thanks for the review. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/2] valgrind: Update glibc timer_create() suppression.
For some reason the current suppression fails to actually suppress the warning. Signed-off-by: Ethan Jackson --- tests/glibc.supp |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/glibc.supp b/tests/glibc.supp index 52d17bc..948ee01 100644 --- a/tests/glibc.supp +++ b/tests/glibc.supp @@ -11,6 +11,7 @@ timer_create Memcheck:Param timer_create(evp) - fun:timer_create + ... + fun:set_up_timer } -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/2] ovs-dev.py: Use custom suppressions when running valgrind.
Signed-off-by: Ethan Jackson --- utilities/ovs-dev.py |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/utilities/ovs-dev.py b/utilities/ovs-dev.py index c10ca7d..21f0fc5 100755 --- a/utilities/ovs-dev.py +++ b/utilities/ovs-dev.py @@ -173,7 +173,9 @@ def run(): if options.gdb: cmd = ["gdb", "--args"] + cmd elif options.valgrind: -cmd = ["valgrind", "--track-origins=yes"] + cmd +cmd = ["valgrind", "--track-origins=yes", + "--suppressions=%s/tests/glibc.supp" % OVS_SRC, + "--suppressions=%s/tests/openssl.supp" % OVS_SRC] + cmd else: cmd = ["sudo"] + cmd opts = opts + ["-vconsole:off", "--detach"] -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Avoid C preprocessor trick where macro has the same name as a function.
On Mon, Jul 29, 2013 at 03:22:52PM -0400, Ed Maste wrote: > On 29 July 2013 12:57, Ben Pfaff wrote: > > In C, one can do preprocessor tricks by making a macro expansion include > > the macro's own name. We actually used this in the tree to automatically > > provide function arguments, e.g.: > > > > #define f(x) f(x, __FILE__, __LINE__) > > int f(int x, const char *file, int line); > > > > ... > > > > f(1);/* Expands to a call like f(1, __FILE__, __LINE__); */ > > > > However it's somewhat confusing, so this commit stops using that trick. > > > > Reported-by: Ed Maste > > Signed-off-by: Ben Pfaff > > Acked-by: Ed Maste Thanks for the review. > One minor comment: > > >#define f(x) f(x, __FILE__, __LINE__) > >int f(int x, const char *file, int line); > > Unlike the instances in the source, the example as-is doesn't compile, > which I think helps provide justification for the removal of the trick Oops. > It's missing either an #undef, or the further trick of relying on the > preprocessor only expanding function-like macros if the token > following is a left parenthesis, resulting in the odd-looking int > (f)(int x, const char *file, int line) syntax. I reversed the order of the lines, which should also fix it. I applied this to master, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Add vxlan and flow_dissector to gitignore.
Thanks, merged. Ethan On Mon, Jul 29, 2013 at 2:33 PM, Jesse Gross wrote: > On Mon, Jul 29, 2013 at 2:23 PM, Ethan Jackson wrote: >> Signed-off-by: Ethan Jackson >> --- >> datapath/linux/.gitignore |2 ++ >> 1 file changed, 2 insertions(+) > > Acked-by: Jesse Gross X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] valgrind: Update glibc timer_create() suppression.
On Mon, Jul 29, 2013 at 03:28:09PM -0700, Ethan Jackson wrote: > For some reason the current suppression fails to actually suppress the > warning. > > Signed-off-by: Ethan Jackson The old version worked OK with my glibc but so does the new one so: Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] ovs-dev.py: Use custom suppressions when running valgrind.
On Mon, Jul 29, 2013 at 03:28:08PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] valgrind: Update glibc timer_create() suppression.
Thanks, I merged both of these. Ethan On Mon, Jul 29, 2013 at 3:32 PM, Ben Pfaff wrote: > On Mon, Jul 29, 2013 at 03:28:09PM -0700, Ethan Jackson wrote: >> For some reason the current suppression fails to actually suppress the >> warning. >> >> Signed-off-by: Ethan Jackson > > The old version worked OK with my glibc but so does the new one so: > Acked-by: Ben Pfaff X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/5] datapath: move tunnel-init function to flow.h
This makes ovs-module more in-sync with upstream ovs-module.
Signed-off-by: Pravin B Shelar
---
datapath/flow.h| 16
datapath/tunnel.h | 16
datapath/vport-gre.c |2 +-
datapath/vport-lisp.c |2 +-
datapath/vport-vxlan.c |2 +-
5 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/datapath/flow.h b/datapath/flow.h
index 59c7f6e..f7d694f 100644
--- a/datapath/flow.h
+++ b/datapath/flow.h
@@ -58,6 +58,22 @@ struct ovs_key_ipv4_tunnel {
u8 ipv4_ttl;
};
+static inline void ovs_flow_tun_key_init(struct ovs_key_ipv4_tunnel *tun_key,
+const struct iphdr *iph, __be64 tun_id,
+__be16 tun_flags)
+{
+ tun_key->tun_id = tun_id;
+ tun_key->ipv4_src = iph->saddr;
+ tun_key->ipv4_dst = iph->daddr;
+ tun_key->ipv4_tos = iph->tos;
+ tun_key->ipv4_ttl = iph->ttl;
+ tun_key->tun_flags = tun_flags;
+
+ /* clear struct padding. */
+ memset((unsigned char *) tun_key + OVS_TUNNEL_KEY_SIZE, 0,
+ sizeof(*tun_key) - OVS_TUNNEL_KEY_SIZE);
+}
+
struct sw_flow_key {
struct ovs_key_ipv4_tunnel tun_key; /* Encapsulating tunnel key. */
struct {
diff --git a/datapath/tunnel.h b/datapath/tunnel.h
index 17de7c4..aa52145 100644
--- a/datapath/tunnel.h
+++ b/datapath/tunnel.h
@@ -41,20 +41,4 @@ int ovs_tnl_send(struct vport *vport, struct sk_buff *skb,
void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb,
struct ovs_key_ipv4_tunnel *tun_key);
-static inline void tnl_tun_key_init(struct ovs_key_ipv4_tunnel *tun_key,
-const struct iphdr *iph, __be64 tun_id,
-__be16 tun_flags)
-{
- tun_key->tun_id = tun_id;
- tun_key->ipv4_src = iph->saddr;
- tun_key->ipv4_dst = iph->daddr;
- tun_key->ipv4_tos = iph->tos;
- tun_key->ipv4_ttl = iph->ttl;
- tun_key->tun_flags = tun_flags;
-
- /* clear struct padding. */
- memset((unsigned char*) tun_key + OVS_TUNNEL_KEY_SIZE, 0,
- sizeof(*tun_key) - OVS_TUNNEL_KEY_SIZE);
-}
-
#endif /* TUNNEL_H */
diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
index c74f5fc..301145b 100644
--- a/datapath/vport-gre.c
+++ b/datapath/vport-gre.c
@@ -112,7 +112,7 @@ static int gre_rcv(struct sk_buff *skb,
return PACKET_REJECT;
key = key_to_tunnel_id(tpi->key, tpi->seq);
- tnl_tun_key_init(&tun_key, ip_hdr(skb), key,
filter_tnl_flags(tpi->flags));
+ ovs_flow_tun_key_init(&tun_key, ip_hdr(skb), key,
filter_tnl_flags(tpi->flags));
ovs_vport_receive(vport, skb, &tun_key);
return PACKET_RCVD;
diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
index 54c10ae..6027a16 100644
--- a/datapath/vport-lisp.c
+++ b/datapath/vport-lisp.c
@@ -218,7 +218,7 @@ static int lisp_rcv(struct sock *sk, struct sk_buff *skb)
/* Save outer tunnel values */
iph = ip_hdr(skb);
- tnl_tun_key_init(&tun_key, iph, key, TUNNEL_KEY);
+ ovs_flow_tun_key_init(&tun_key, iph, key, TUNNEL_KEY);
/* Drop non-IP inner packets */
inner_iph = (struct iphdr *)(lisph + 1);
diff --git a/datapath/vport-vxlan.c b/datapath/vport-vxlan.c
index 5546820..8fdec16 100644
--- a/datapath/vport-vxlan.c
+++ b/datapath/vport-vxlan.c
@@ -73,7 +73,7 @@ static int vxlan_rcv(struct vxlan_handler *vh, struct sk_buff
*skb, __be32 vx_vn
/* Save outer tunnel values */
iph = ip_hdr(skb);
key = cpu_to_be64(ntohl(vx_vni) >> 8);
- tnl_tun_key_init(&tun_key, iph, key, TUNNEL_KEY);
+ ovs_flow_tun_key_init(&tun_key, iph, key, TUNNEL_KEY);
ovs_vport_receive(vport, skb, &tun_key);
return PACKET_RCVD;
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/5] datapath: Move find_route() to compat.h
Signed-off-by: Pravin B Shelar
---
datapath/compat.h | 42 ++
datapath/tunnel.c | 38 --
datapath/tunnel.h |4
3 files changed, 42 insertions(+), 42 deletions(-)
diff --git a/datapath/compat.h b/datapath/compat.h
index 4dfd192..1ed5ffe 100644
--- a/datapath/compat.h
+++ b/datapath/compat.h
@@ -19,7 +19,12 @@
#ifndef COMPAT_H
#define COMPAT_H 1
+#include
+#include
#include
+#include
+#include
+
#ifndef HAVE_NLA_NUL_STRING
static inline int CHECK_NUL_STRING(struct nlattr *attr, int maxlen)
@@ -106,4 +111,41 @@ static inline void skb_set_mark(struct sk_buff *skb, u32
mark)
#define inet_sport(sk) (inet_sk(sk)->inet_sport)
#endif
+static inline struct rtable *find_route(struct net *net,
+ __be32 *saddr, __be32 daddr,
+ u8 ipproto, u8 tos, u32 skb_mark)
+{
+ struct rtable *rt;
+ /* Tunnel configuration keeps DSCP part of TOS bits, But Linux
+* router expect RT_TOS bits only. */
+
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,39)
+ struct flowi fl = { .nl_u = { .ip4_u = {
+ .daddr = daddr,
+ .saddr = *saddr,
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,20)
+ .fwmark = skb_mark,
+#endif
+ .tos = RT_TOS(tos) } },
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20)
+ .mark = skb_mark,
+#endif
+ .proto = ipproto };
+
+ if (unlikely(ip_route_output_key(net, &rt, &fl)))
+ return ERR_PTR(-EADDRNOTAVAIL);
+ *saddr = fl.nl_u.ip4_u.saddr;
+ return rt;
+#else
+ struct flowi4 fl = { .daddr = daddr,
+.saddr = *saddr,
+.flowi4_tos = RT_TOS(tos),
+.flowi4_mark = skb_mark,
+.flowi4_proto = ipproto };
+
+ rt = ip_route_output_key(net, &fl);
+ *saddr = fl.saddr;
+ return rt;
+#endif
+}
#endif /* compat.h */
diff --git a/datapath/tunnel.c b/datapath/tunnel.c
index ef46a69..20ec694 100644
--- a/datapath/tunnel.c
+++ b/datapath/tunnel.c
@@ -79,44 +79,6 @@ void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb,
ovs_vport_receive(vport, skb, tun_key);
}
-struct rtable *find_route(struct net *net,
- __be32 *saddr, __be32 daddr, u8 ipproto,
- u8 tos, u32 skb_mark)
-{
- struct rtable *rt;
- /* Tunnel configuration keeps DSCP part of TOS bits, But Linux
-* router expect RT_TOS bits only. */
-
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,39)
- struct flowi fl = { .nl_u = { .ip4_u = {
- .daddr = daddr,
- .saddr = *saddr,
-#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,20)
- .fwmark = skb_mark,
-#endif
- .tos = RT_TOS(tos) } },
-#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,20)
- .mark = skb_mark,
-#endif
- .proto = ipproto };
-
- if (unlikely(ip_route_output_key(net, &rt, &fl)))
- return ERR_PTR(-EADDRNOTAVAIL);
- *saddr = fl.nl_u.ip4_u.saddr;
- return rt;
-#else
- struct flowi4 fl = { .daddr = daddr,
-.saddr = *saddr,
-.flowi4_tos = RT_TOS(tos),
-.flowi4_mark = skb_mark,
-.flowi4_proto = ipproto };
-
- rt = ip_route_output_key(net, &fl);
- *saddr = fl.saddr;
- return rt;
-#endif
-}
-
static bool need_linearize(const struct sk_buff *skb)
{
int i;
diff --git a/datapath/tunnel.h b/datapath/tunnel.h
index aa52145..7625dbe 100644
--- a/datapath/tunnel.h
+++ b/datapath/tunnel.h
@@ -26,10 +26,6 @@
#include "flow.h"
#include "vport.h"
-struct rtable *find_route(struct net *net,
- __be32 *saddr, __be32 daddr, u8 ipproto,
- u8 tos, u32 skb_mark);
-
u16 ovs_tnl_get_src_port(struct sk_buff *skb);
int ovs_tnl_send(struct vport *vport, struct sk_buff *skb,
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 3/5] datapath: Move generic tunnel functions to lisp module.
Generic tunnel rcv and send function are only used by lisp tunneling
module, so It make sense to move them to lisp module.
CC: Lori Jakab
Signed-off-by: Pravin B Shelar
---
datapath/Modules.mk|2 -
datapath/datapath.c|1 -
datapath/datapath.h|1 -
datapath/dp_notify.c |2 +
datapath/tunnel.c | 279
datapath/tunnel.h | 40 ---
datapath/vport-gre.c |4 +-
datapath/vport-lisp.c | 245 +-
datapath/vport-vxlan.c |1 -
9 files changed, 247 insertions(+), 328 deletions(-)
delete mode 100644 datapath/tunnel.c
delete mode 100644 datapath/tunnel.h
diff --git a/datapath/Modules.mk b/datapath/Modules.mk
index 2ce..ccf4dfa 100644
--- a/datapath/Modules.mk
+++ b/datapath/Modules.mk
@@ -12,7 +12,6 @@ openvswitch_sources = \
datapath.c \
dp_notify.c \
flow.c \
- tunnel.c \
vlan.c \
vport.c \
vport-gre.c \
@@ -26,7 +25,6 @@ openvswitch_headers = \
compat.h \
datapath.h \
flow.h \
- tunnel.h \
vlan.h \
vport.h \
vport-internal_dev.h \
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 4330ce3..1b50b53 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -58,7 +58,6 @@
#include "datapath.h"
#include "flow.h"
#include "vlan.h"
-#include "tunnel.h"
#include "vport-internal_dev.h"
#include "vport-netdev.h"
diff --git a/datapath/datapath.h b/datapath/datapath.h
index eda87fd..064211d 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -29,7 +29,6 @@
#include "checksum.h"
#include "compat.h"
#include "flow.h"
-#include "tunnel.h"
#include "vlan.h"
#include "vport.h"
diff --git a/datapath/dp_notify.c b/datapath/dp_notify.c
index ec573a5..d530893 100644
--- a/datapath/dp_notify.c
+++ b/datapath/dp_notify.c
@@ -18,6 +18,8 @@
#include
#include
+#include
+#include
#include "datapath.h"
#include "vport-internal_dev.h"
diff --git a/datapath/tunnel.c b/datapath/tunnel.c
deleted file mode 100644
index 20ec694..000
--- a/datapath/tunnel.c
+++ /dev/null
@@ -1,279 +0,0 @@
-/*
- * Copyright (c) 2007-2012 Nicira, Inc.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 of the GNU General Public
- * License as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
- * 02110-1301, USA
- */
-
-#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include "checksum.h"
-#include "compat.h"
-#include "datapath.h"
-#include "tunnel.h"
-#include "vlan.h"
-#include "vport.h"
-
-/**
- * ovs_tnl_rcv - ingress point for generic tunnel code
- *
- * @vport: port this packet was received on
- * @skb: received packet
- * @tos: ToS from encapsulating IP packet, used to copy ECN bits
- *
- * Must be called with rcu_read_lock.
- *
- * Packets received by this function are in the following state:
- * - skb->data points to the inner Ethernet header.
- * - The inner Ethernet header is in the linear data area.
- * - skb->csum does not include the inner Ethernet header.
- * - The layer pointers are undefined.
- */
-void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb,
-struct ovs_key_ipv4_tunnel *tun_key)
-{
- struct ethhdr *eh;
-
- skb_reset_mac_header(skb);
- eh = eth_hdr(skb);
-
- if (likely(ntohs(eh->h_proto) >= ETH_P_802_3_MIN))
- skb->protocol = eh->h_proto;
- else
- skb->protocol = htons(ETH_P_802_2);
-
- skb_dst_drop(skb);
- nf_reset(skb);
- skb_clear_rxhash(skb);
- secpath_reset(skb);
- vlan_set_tci(skb, 0);
-
- if (unlikely(compute_ip_summed(skb, false))) {
- kfree_skb(skb);
- return;
- }
-
- ovs_vport_receive(vport, skb, tun_key);
-}
-
-static bool need_linearize(const struct sk_buff *skb)
-{
- int i;
-
- if (unlikely(skb_shinfo(skb)->frag_list))
- return true;
-
- /*
-* Generally speaking we should linearize if there are paged frags.
-* However, if all of the refcounts are 1 we know nobody else can
-* change them from underneath us and we can skip the linearization.
-*/
- for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
- if
(unlikely(page_count(skb_frag_page(&skb_shinfo(skb)->frags[i])
[ovs-dev] [PATCH 4/5] datapath lisp: use iptunnel_pull_header().
CC: Lori Jakab
Signed-off-by: Pravin B Shelar
---
datapath/vport-lisp.c | 48 ++--
1 files changed, 2 insertions(+), 46 deletions(-)
diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
index 6e37b2f..847cb39 100644
--- a/datapath/vport-lisp.c
+++ b/datapath/vport-lisp.c
@@ -206,48 +206,6 @@ static void lisp_build_header(const struct vport *vport,
lisph->u2.word2.locator_status_bits = 1;
}
-/**
- * ovs_tnl_rcv - ingress point for generic tunnel code
- *
- * @vport: port this packet was received on
- * @skb: received packet
- * @tos: ToS from encapsulating IP packet, used to copy ECN bits
- *
- * Must be called with rcu_read_lock.
- *
- * Packets received by this function are in the following state:
- * - skb->data points to the inner Ethernet header.
- * - The inner Ethernet header is in the linear data area.
- * - skb->csum does not include the inner Ethernet header.
- * - The layer pointers are undefined.
- */
-static void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb,
- struct ovs_key_ipv4_tunnel *tun_key)
-{
- struct ethhdr *eh;
-
- skb_reset_mac_header(skb);
- eh = eth_hdr(skb);
-
- if (likely(ntohs(eh->h_proto) >= ETH_P_802_3_MIN))
- skb->protocol = eh->h_proto;
- else
- skb->protocol = htons(ETH_P_802_2);
-
- skb_dst_drop(skb);
- nf_reset(skb);
- skb_clear_rxhash(skb);
- secpath_reset(skb);
- vlan_set_tci(skb, 0);
-
- if (unlikely(compute_ip_summed(skb, false))) {
- kfree_skb(skb);
- return;
- }
-
- ovs_vport_receive(vport, skb, tun_key);
-}
-
/* Called with rcu_read_lock and BH disabled. */
static int lisp_rcv(struct sock *sk, struct sk_buff *skb)
{
@@ -263,13 +221,11 @@ static int lisp_rcv(struct sock *sk, struct sk_buff *skb)
if (unlikely(!lisp_port))
goto error;
- if (unlikely(!pskb_may_pull(skb, LISP_HLEN)))
+ if (iptunnel_pull_header(skb, LISP_HLEN, htons(ETH_P_TEB)))
goto error;
lisph = lisp_hdr(skb);
- skb_pull_rcsum(skb, LISP_HLEN);
-
if (lisph->instance_id_present != 1)
key = 0;
else
@@ -301,7 +257,7 @@ static int lisp_rcv(struct sock *sk, struct sk_buff *skb)
ovs_skb_postpush_rcsum(skb, skb->data, ETH_HLEN);
- ovs_tnl_rcv(vport_from_priv(lisp_port), skb, &tun_key);
+ ovs_vport_receive(vport_from_priv(lisp_port), skb, &tun_key);
goto out;
error:
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 5/5] datapath lisp: Use ovs offload compat functionality.
OVS already has compat functions to handle GSO packets.
Following patch get rid of GSO packet handling in lisp
and use ovs iptunnel_xmit() function for same.
CC: Lori Jakab
Signed-off-by: Pravin B Shelar
---
datapath/vport-lisp.c | 209 -
1 files changed, 50 insertions(+), 159 deletions(-)
diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
index 847cb39..9ffa74f 100644
--- a/datapath/vport-lisp.c
+++ b/datapath/vport-lisp.c
@@ -35,6 +35,7 @@
#include
#include "datapath.h"
+#include "gso.h"
#include "vport.h"
/*
@@ -177,35 +178,6 @@ static u16 ovs_tnl_get_src_port(struct sk_buff *skb)
return (((u64) hash * range) >> 32) + low;
}
-static void lisp_build_header(const struct vport *vport,
- struct sk_buff *skb,
- int tunnel_hlen)
-{
- struct lisp_port *lisp_port = lisp_vport(vport);
- struct udphdr *udph = udp_hdr(skb);
- struct lisphdr *lisph = (struct lisphdr *)(udph + 1);
- const struct ovs_key_ipv4_tunnel *tun_key = OVS_CB(skb)->tun_key;
-
- udph->dest = lisp_port->dst_port;
- udph->source = htons(ovs_tnl_get_src_port(skb));
- udph->check = 0;
- udph->len = htons(skb->len - skb_transport_offset(skb));
-
- lisph->nonce_present = 0; /* We don't support echo nonce
algorithm */
- lisph->locator_status_bits_present = 1; /* Set LSB */
- lisph->solicit_echo_nonce = 0; /* No echo noncing */
- lisph->map_version_present = 0; /* No mapping versioning, nonce instead
*/
- lisph->instance_id_present = 1; /* Store the tun_id as Instance ID */
- lisph->reserved_flags = 0; /* Reserved flags, set to 0 */
-
- lisph->u1.nonce[0] = 0;
- lisph->u1.nonce[1] = 0;
- lisph->u1.nonce[2] = 0;
-
- tunnel_id_to_instance_id(tun_key->tun_id,
&lisph->u2.word2.instance_id[0]);
- lisph->u2.word2.locator_status_bits = 1;
-}
-
/* Called with rcu_read_lock and BH disabled. */
static int lisp_rcv(struct sock *sk, struct sk_buff *skb)
{
@@ -376,86 +348,56 @@ error:
return ERR_PTR(err);
}
-static bool need_linearize(const struct sk_buff *skb)
+static void lisp_build_header(struct sk_buff *skb)
{
- int i;
+ struct udphdr *udph = udp_hdr(skb);
+ struct lisphdr *lisph = (struct lisphdr *)(udph + 1);
+ const struct ovs_key_ipv4_tunnel *tun_key = OVS_CB(skb)->tun_key;
- if (unlikely(skb_shinfo(skb)->frag_list))
- return true;
+ lisph->nonce_present = 0; /* We don't support echo nonce
algorithm */
+ lisph->locator_status_bits_present = 1; /* Set LSB */
+ lisph->solicit_echo_nonce = 0; /* No echo noncing */
+ lisph->map_version_present = 0; /* No mapping versioning, nonce instead
*/
+ lisph->instance_id_present = 1; /* Store the tun_id as Instance ID */
+ lisph->reserved_flags = 0; /* Reserved flags, set to 0 */
- /*
-* Generally speaking we should linearize if there are paged frags.
-* However, if all of the refcounts are 1 we know nobody else can
-* change them from underneath us and we can skip the linearization.
-*/
- for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
- if
(unlikely(page_count(skb_frag_page(&skb_shinfo(skb)->frags[i])) > 1))
- return true;
+ lisph->u1.nonce[0] = 0;
+ lisph->u1.nonce[1] = 0;
+ lisph->u1.nonce[2] = 0;
- return false;
+ tunnel_id_to_instance_id(tun_key->tun_id,
&lisph->u2.word2.instance_id[0]);
+ lisph->u2.word2.locator_status_bits = 1;
}
-static struct sk_buff *handle_offloads(struct sk_buff *skb)
+static void handle_offloads(struct sk_buff *skb)
{
- int err;
+ if (skb_is_gso(skb))
+ OVS_GSO_CB(skb)->fix_segment = lisp_build_header;
+ else if (get_ip_summed(skb) != OVS_CSUM_PARTIAL)
+ set_ip_summed(skb, OVS_CSUM_NONE);
forward_ip_summed(skb, true);
-
- if (skb_is_gso(skb)) {
- struct sk_buff *nskb;
-
- nskb = __skb_gso_segment(skb, 0, false);
- if (IS_ERR(nskb)) {
- err = PTR_ERR(nskb);
- goto error;
- }
-
- consume_skb(skb);
- skb = nskb;
- } else if (get_ip_summed(skb) == OVS_CSUM_PARTIAL) {
- /* Pages aren't locked and could change at any time.
-* If this happens after we compute the checksum, the
-* checksum will be wrong. We linearize now to avoid
-* this problem.
-*/
- if (unlikely(need_linearize(skb))) {
- err = __skb_linearize(skb);
- if (unlikely(err))
- goto error;
- }
-
- err = skb_checksum_help(skb);
- if (unlike
Re: [ovs-dev] [thread-safety 07/11] bond: Make the bond module thread safe.
On Fri, Jul 26, 2013 at 06:07:08PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson Any particular reason you chose to use a read/write lock here? I've read the patch but I haven't yet audited the r/w choice in each case. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 08/11] mac-learning: Make the mac-learning module thread safe.
On Fri, Jul 26, 2013 at 06:07:09PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson Why does the client do the locking? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/5] datapath: move tunnel-init function to flow.h
On Mon, Jul 29, 2013 at 3:47 PM, Pravin B Shelar wrote: > This makes ovs-module more in-sync with upstream ovs-module. > > Signed-off-by: Pravin B Shelar Kyle/Lori, do you guys want to take a first look at this series? X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 09/11] tunnel: Make the ofproto-dpif tunnel module thread safe.
On Fri, Jul 26, 2013 at 06:07:10PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson GCC says: ../ofproto/tunnel.c: In function ‘tnl_port_del’: ../ofproto/tunnel.c:191: error: unused variable ‘tnl_port’ The logging code in tnl_port_receive() is really fucking weird (not your fault). I assume it used to make sense. There's a doubled space after the ? in tnl_port_send() here: out_port = tnl_port ? tnl_port->match.odp_port : ODPP_NONE; In tnl_port_send() and tnl_port_receive(), the local variable 'tnl_port' may be marked const. With those comments, Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [thread-safety 01/11] clang: Add annotations for thread safety check.
On Mon, Jul 29, 2013 at 10:47 AM, Ben Pfaff wrote:
> @@ -22,59 +22,55 @@
> #include
> #include "ovs-atomic.h"
> #include "util.h"
> +
> +/* Mutex. */
>
> struct OVS_LOCKABLE ovs_mutex {
> pthread_mutex_t lock;
> const char *where;
> };
>
> -struct OVS_LOCKABLE ovs_rwlock {
> -pthread_rwlock_t lock;
> -const char *where;
> -};
> -
>
Hey Ben,
Want to ask why do you remove the rwlock here?
Kind Regards,
Alex Wang,
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v16 4/4] datapath: Enable multiple levels of MPLS pop
This is now sane as recirculation is implemented Signed-off-by: Simon Horman --- v16 * First post --- datapath/datapath.c | 11 +-- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/datapath/datapath.c b/datapath/datapath.c index c8143b9..eb14b23 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -1007,16 +1007,7 @@ static int validate_and_copy_actions__(const struct nlattr *attr, if (!eth_p_mpls(eth_types->types[i])) return -EINVAL; - /* Disallow subsequent L2.5+ set and mpls_pop actions -* as there is no check here to ensure that the new -* eth_type is valid and thus set actions could -* write off the end of the packet or otherwise -* corrupt it. -* -* Support for these actions is planned using packet -* recirculation. -*/ - eth_types_set(eth_types, htons(0)); + eth_types_set(eth_types, nla_get_be16(a)); break; } -- 1.8.3.2 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v16 0/4] Add packet recirculation
[ CC Ethan Jackson. Ethan, this makes changes to ofproto-dpif and ofproto-dpif-xlate. We would appreciate some feedback with how these changes fit with your ongoing refactoring in this area. ] Recirculation is a technique to allow a frame to re-enter frame processing. This is intended to be used after actions have been applied to the frame with modify the frame in some way that makes it possible for richer processing to occur. An example is and indeed targeted use case is MPLS. If an MPLS frame has an mpls_pop action applied with the IPv4 ethernet type then it becomes possible to decode the IPv4 portion of the frame. This may be used to construct a facet that modifies the IPv4 portion of the frame. This is not possible prior to the mpls_pop action as the contents of the frame after the MPLS stack is not known to be IPv4. Design: * New recirculation action. ovs-vswitchd adds a recirculation action to the end of a list of datapath actions for a flow when the actions are truncated because insufficient flow match information is available to add the next OpenFlow action. The recirculation action is preceded by an action to set the skb_mark to an id which can be used to scope a facet lookup of a recirculated packet. e.g. pop_mpls(0x0800),dec_ttl becomes pop_mpls(0x800),set(skb_mark(id)),recirculate * Datapath behaviour Then the datapath encounters a recirculate action it: + Recalculates the flow key based on the packet which will typically have been modified by previous actions + As the recirculate action is preceded by a set(skb_mark(id)) action, the new match key will now include skb_mark=id. + Performs a lookup using the new match key + Processes the packet if a facet matches the key or; + Makes an upcall if necessary * No facet behaviour + Loop: 1) translate actions 2) If there is a recirculate action, execute packet and go back to 1) for remaining actions. Base/Pre-requisites: This series applies to the git master branch on openvswtich.org and does not have any pre-requisites. Availability: To aid review and testing this series is available in git at: git://github.com/horms/openvswitch.git devel/mpls-recirculate-v16 N.B: The above branch is based on a merge of the openvswtich.org master branch and devel/mpls-v2.35. As such it supports both MPLS in the kernel datapath and recirculation. Change Log Highlights: v16 * Ensure that locks are released and memory freed in the case of recirculation in dpif_netdev_execute() * Separate controller and MPLS tests - MPLS adds a number of tests and there are already a number of controller tests. It seems to make sense to separate them. * Do not add reference count to facet - This does not appear to be necessary any more as situations where the parent-facet of a recirculated child-facet does not exist may be handled using the drop rule. * Do not add 'strict' parameter from get_facet_chain. This is no longer useful as recirculated facets chains may me incomplete if parent-facets may be removed before child-facets. This is quite possible now that there is no reference count. * Clarify comments relating to recirculated facet chain revalidation * Now based on MPLS datapath "[PATCH v2.35 0/6] MPLS actions and matches" Accordingly, a patch is added to this series to allow multiple levels of MPLS pop in the kernel datapath. This depends on MPLS support for the kernel datapath, which is added in the series mentioned, and recirculation, which is added by this series. v15 * Add additional flow removal tests to cover double,multiple recirculation * Rebase for ofproto-dpif-xlate modularization * Replace rule pointers in facet with offsets - Use rule offset to determine if facet was created by recirculation - Refactor xin->ofpacts initialisation to handle cases where the rule has disappeared but the facets still exist v14 * Enhance "ofproto-dpif: Add 'force-miss-model' configuration" patch to use strings. * Enhance "Add MPLS recirculation tests" to allow testing of remote datapaths - this was used to create an environment to test the kernel datapath * Allow matching of zero skb_mark - Include skb_mark in flow key passed from datapath - Include skb_mark in flow key constructed from flow if the mask is non-zero * Set skb_mark mask for recirculated facets during revalidation * Run through facets in reverse order in facet_revalidate(), so that children are always destroyed before their parents. * Recirculate on more than one pop otherwise intermediate eth type is lost v13 * Add patch to allow configuration of flow miss handling to force handling with or without facets. This is used by the patch to exercise recirculation using ofproto-dpif. * Extensive rebase for megaflow changes v12 * Rebase * Add much more extensive tests by Joe Stringer * Ensure pre_push_mpls_lse is never used uninitialised in do_xlate_actions() * Copy flow tunnel when
[ovs-dev] [PATCH v16 3/4] Add MPLS recirculation tests
From: Joe Stringer
This patch introduces a python script to generate about 1500 tests for
permutations of mpls_push,mpls_pop,dec_mpls_ttl,dec_ttl where
recirculation occurs up to (and including) three times.
Signed-off-by: Joe Stringer
Signed-off-by: Simon Horman
---
v16
* No change
v15
* Add additional flow removal tests to cover double,multiple recirculation
v14
* Allow testing remote datapaths
- Expose egress bridge, datapath under test, destination mac parameters
* Add option to remove flows after each test
v13
* Use the new force-miss-model config option to test MPLS on ofproto-dpif
through the code paths for both handle_flow_miss_with_facet() and
handle_flow_miss_without_facet().
v12
* First post
---
tests/automake.mk | 1 +
tests/ofproto-dpif.at | 153
tests/test-mpls.py| 491 ++
3 files changed, 645 insertions(+)
create mode 100644 tests/test-mpls.py
diff --git a/tests/automake.mk b/tests/automake.mk
index 755d88e..9e6935f 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -326,6 +326,7 @@ CHECK_PYFILES = \
tests/test-jsonrpc.py \
tests/test-ovsdb.py \
tests/test-reconnect.py \
+ tests/test-mpls.py \
tests/MockXenAPI.py \
tests/test-unix-socket.py \
tests/test-unixctl.py \
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index dded496..9383194 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1301,6 +1301,159 @@ NXST_FLOW reply:
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - MPLS recirculation])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=with-facets
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 1])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS recirculation, no facets])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=without-facets
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 1])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS recirculation, delete rules between])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([$PYTHON $srcdir/test-mpls.py -c 1])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS double-recirculate])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=with-facets
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..150..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 2 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS double-recirculate, no facets])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=without-facets
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..150..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 2 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS double-recirculate, delete rules between])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..150..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py -c 2 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS multi-recirculate])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=with-facets
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..550..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 3 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS multi-recirculate, no facets])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=without-facets
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..550..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 3 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS multi-recirculate, delete rules between])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..550..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py -c 3 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS multi-recirculate 2])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=with-facets
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {600..1200..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 3 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MP
