Re: [ovs-dev] [PATCH] [RFC] ovn-controller: Experiment with restricting access to columns.
On Fri, Jun 15, 2018 at 10:11:41AM -0400, Mark Michelson wrote: > On 06/13/2018 11:29 PM, Han Zhou wrote: > >On Wed, Jun 13, 2018 at 3:37 PM, Ben Pfaff wrote: > >> > >>To make ovn-controller recompute incrementally, we need accurate > >>dependencies for each function that reads or writes a table. It's > >>currently difficult to be sure about these dependencies, and certainly > >>difficult to maintain them over time, because there's no way to actually > >>enforce them. > >> > >>This commit experiments with an approach that allows for fairly > >>fine-grained access control within ovn-controller to tables and columns. > >>It's based on generating a new version of the IDL data structures for each > >>case where we want different access control. All of these data structures > >>have the same format, but the columns that a given piece of code is not > >>supposed to touch are renamed to discourage programmers from using them, > >>e.g. they're given names suffixed with "__accessdenied". (This means > >>that there is no runtime overhead to the access control since it only > >>requires a cast to convert between the regular and restricted versions.) > >>In addition, when a columns is supposed to be read-only, functions for > >>modifying the column are not supplied. > >> > >>This commit only tries out this experiment for a single file within > >>ovn-controller, the BFD implementation (mostly because that's > >>alphabetically first, no other real reason). It would require a little > >>more work to apply it everywhere, but it's probably not a huge deal. > >> > >>Comments? > >> > >>CC: Han Zhou > >>Signed-off-by: Ben Pfaff > >>--- > >> ovn/controller/automake.mk | 5 + > >> ovn/controller/bfd-vswitch-idl.def | 21 > >> ovn/controller/bfd.c | 20 ++-- > >> ovn/controller/bfd.h | 8 +- > >> ovn/controller/ovn-controller.c| 13 ++- > >> ovsdb/ovsdb-idlc.in| 223 ++ > >++- > >> 6 files changed, 268 insertions(+), 22 deletions(-) > >> create mode 100644 ovn/controller/bfd-vswitch-idl.def > >> > > > >I wanted to have a quick test but it didn't pass the compile: > >In file included from ovn/controller/bfd.c:17:0: > >ovn/controller/bfd.h:19:44: fatal error: ovn/controller/bfd-vswitch-idl.h: > >No such file or directory > > Here's a different datapoint in the same category. > > I got a slightly different error when I tried to compile. > ovn/controller/bfd-vswitch-idl.h was auto-generated and everything worked up > until the very end: > > "The following files are in git but not the distribution: > ovn/controller/bfd-vswitch-idl.def" > > The make command I ran was `make sandbox SANDBOXFLAGS="--ovn"` > > I tried running `make distclean` then reconfiguring, but this didn't help. > > For comparison, Han, these are my software versions, in case that might be > why auto-generation worked for me but not you: > gcc version is 7.3.1 > make version is 4.2.1 > autoconf version is 2.69 I've fixed that locally now. It needed EXTRA_DIST += ovn/controller/bfd-vswitch-idl.def. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] [RFC] ovn-controller: Experiment with restricting access to columns.
On Fri, Jun 15, 2018 at 07:22:38PM -0700, Han Zhou wrote: > On Fri, Jun 15, 2018 at 7:11 AM, Mark Michelson wrote: > > > On 06/13/2018 11:29 PM, Han Zhou wrote: > > > >> On Wed, Jun 13, 2018 at 3:37 PM, Ben Pfaff wrote: > >> > >>> > >>> To make ovn-controller recompute incrementally, we need accurate > >>> dependencies for each function that reads or writes a table. It's > >>> currently difficult to be sure about these dependencies, and certainly > >>> difficult to maintain them over time, because there's no way to actually > >>> enforce them. > >>> > >>> This commit experiments with an approach that allows for fairly > >>> fine-grained access control within ovn-controller to tables and columns. > >>> It's based on generating a new version of the IDL data structures for > >>> each > >>> case where we want different access control. All of these data > >>> structures > >>> have the same format, but the columns that a given piece of code is not > >>> supposed to touch are renamed to discourage programmers from using them, > >>> e.g. they're given names suffixed with "__accessdenied". (This means > >>> that there is no runtime overhead to the access control since it only > >>> requires a cast to convert between the regular and restricted versions.) > >>> In addition, when a columns is supposed to be read-only, functions for > >>> modifying the column are not supplied. > >>> > >>> This commit only tries out this experiment for a single file within > >>> ovn-controller, the BFD implementation (mostly because that's > >>> alphabetically first, no other real reason). It would require a little > >>> more work to apply it everywhere, but it's probably not a huge deal. > >>> > >>> Comments? > >>> > >>> CC: Han Zhou > >>> Signed-off-by: Ben Pfaff > >>> --- > >>> ovn/controller/automake.mk | 5 + > >>> ovn/controller/bfd-vswitch-idl.def | 21 > >>> ovn/controller/bfd.c | 20 ++-- > >>> ovn/controller/bfd.h | 8 +- > >>> ovn/controller/ovn-controller.c| 13 ++- > >>> ovsdb/ovsdb-idlc.in| 223 > >>> ++ > >>> > >> ++- > >> > >>> 6 files changed, 268 insertions(+), 22 deletions(-) > >>> create mode 100644 ovn/controller/bfd-vswitch-idl.def > >>> > >>> > >> I wanted to have a quick test but it didn't pass the compile: > >> In file included from ovn/controller/bfd.c:17:0: > >> ovn/controller/bfd.h:19:44: fatal error: ovn/controller/bfd-vswitch-idl > >> .h: > >> No such file or directory > >> > > > > Here's a different datapoint in the same category. > > > > I got a slightly different error when I tried to compile. > > ovn/controller/bfd-vswitch-idl.h was auto-generated and everything worked > > up until the very end: > > > > "The following files are in git but not the distribution: > > ovn/controller/bfd-vswitch-idl.def" > > > > The make command I ran was `make sandbox SANDBOXFLAGS="--ovn"` > > > > I tried running `make distclean` then reconfiguring, but this didn't help. > > > > For comparison, Han, these are my software versions, in case that might be > > why auto-generation worked for me but not you: > > gcc version is 7.3.1 > > make version is 4.2.1 > > autoconf version is 2.69 > > > > Hey Mark, thanks for sharing. I figured out that my error was due to the > space v.s. tab in the receipt line in makefile (tab should be used): > > + > +$(ovn_controller_ovn_controller_SOURCES:.c=.$(OBJEXT)): \ > + ovn/controller/bfd-vswitch-idl.h > +ovn/controller/bfd-vswitch-idl.h: lib/vswitch-idl.ovsidl > ovn/controller/bfd-vswitch-idl.def ovsdb/ovsdb-idlc.in > + $(AM_V_GEN)$(OVSDB_IDLC) c-idl-subset lib/vswitch-idl.ovsidl > $(srcdir)/ovn/controller/bfd-vswitch-idl.def > $@.tmp && mv $@.tmp $@ > > The original patch was using tab. I should not copy/paste from browser :( > Now I can compile without any issues, and I didn't encounter the problem > you mentioned. Oh, that's good news. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] [RFC] ovn-controller: Experiment with restricting access to columns.
On Fri, Jun 15, 2018 at 7:11 AM, Mark Michelson wrote: > On 06/13/2018 11:29 PM, Han Zhou wrote: > >> On Wed, Jun 13, 2018 at 3:37 PM, Ben Pfaff wrote: >> >>> >>> To make ovn-controller recompute incrementally, we need accurate >>> dependencies for each function that reads or writes a table. It's >>> currently difficult to be sure about these dependencies, and certainly >>> difficult to maintain them over time, because there's no way to actually >>> enforce them. >>> >>> This commit experiments with an approach that allows for fairly >>> fine-grained access control within ovn-controller to tables and columns. >>> It's based on generating a new version of the IDL data structures for >>> each >>> case where we want different access control. All of these data >>> structures >>> have the same format, but the columns that a given piece of code is not >>> supposed to touch are renamed to discourage programmers from using them, >>> e.g. they're given names suffixed with "__accessdenied". (This means >>> that there is no runtime overhead to the access control since it only >>> requires a cast to convert between the regular and restricted versions.) >>> In addition, when a columns is supposed to be read-only, functions for >>> modifying the column are not supplied. >>> >>> This commit only tries out this experiment for a single file within >>> ovn-controller, the BFD implementation (mostly because that's >>> alphabetically first, no other real reason). It would require a little >>> more work to apply it everywhere, but it's probably not a huge deal. >>> >>> Comments? >>> >>> CC: Han Zhou >>> Signed-off-by: Ben Pfaff >>> --- >>> ovn/controller/automake.mk | 5 + >>> ovn/controller/bfd-vswitch-idl.def | 21 >>> ovn/controller/bfd.c | 20 ++-- >>> ovn/controller/bfd.h | 8 +- >>> ovn/controller/ovn-controller.c| 13 ++- >>> ovsdb/ovsdb-idlc.in| 223 >>> ++ >>> >> ++- >> >>> 6 files changed, 268 insertions(+), 22 deletions(-) >>> create mode 100644 ovn/controller/bfd-vswitch-idl.def >>> >>> >> I wanted to have a quick test but it didn't pass the compile: >> In file included from ovn/controller/bfd.c:17:0: >> ovn/controller/bfd.h:19:44: fatal error: ovn/controller/bfd-vswitch-idl >> .h: >> No such file or directory >> > > Here's a different datapoint in the same category. > > I got a slightly different error when I tried to compile. > ovn/controller/bfd-vswitch-idl.h was auto-generated and everything worked > up until the very end: > > "The following files are in git but not the distribution: > ovn/controller/bfd-vswitch-idl.def" > > The make command I ran was `make sandbox SANDBOXFLAGS="--ovn"` > > I tried running `make distclean` then reconfiguring, but this didn't help. > > For comparison, Han, these are my software versions, in case that might be > why auto-generation worked for me but not you: > gcc version is 7.3.1 > make version is 4.2.1 > autoconf version is 2.69 > > Hey Mark, thanks for sharing. I figured out that my error was due to the space v.s. tab in the receipt line in makefile (tab should be used): + +$(ovn_controller_ovn_controller_SOURCES:.c=.$(OBJEXT)): \ + ovn/controller/bfd-vswitch-idl.h +ovn/controller/bfd-vswitch-idl.h: lib/vswitch-idl.ovsidl ovn/controller/bfd-vswitch-idl.def ovsdb/ovsdb-idlc.in + $(AM_V_GEN)$(OVSDB_IDLC) c-idl-subset lib/vswitch-idl.ovsidl $(srcdir)/ovn/controller/bfd-vswitch-idl.def > $@.tmp && mv $@.tmp $@ The original patch was using tab. I should not copy/paste from browser :( Now I can compile without any issues, and I didn't encounter the problem you mentioned. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] lib: Build action_set in one scan of action_list
> I like how it actually deletes more lines (161) > than it adds (106). A good result. :) > What do you think of this version? Looks good to me, I'm not a macro person so it was strange at first glance. It makes a lot of sense though, and it's certainly more extensible than my mechanism and what we had before. The unified list and classification make the action set logic far simpler, too. I like it. -- Kyle web: https://mcfelix.me ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] ofp-print: Move significant formatting code into more specific .c files.
Signed-off-by: Ben Pfaff
---
v1->v2: Rebase due to changes on master.
include/openvswitch/ofp-bundle.h | 11 +-
include/openvswitch/ofp-connection.h | 18 +-
include/openvswitch/ofp-ipfix.h | 5 +
include/openvswitch/ofp-match.h | 9 +-
include/openvswitch/ofp-meter.h | 12 +
include/openvswitch/ofp-monitor.h| 4 +
include/openvswitch/ofp-port.h | 3 +
include/openvswitch/ofp-queue.h | 14 +
include/openvswitch/ofp-table.h | 6 +
lib/ofp-bundle.c | 70 +++
lib/ofp-connection.c | 197 +++
lib/ofp-ipfix.c | 51 ++
lib/ofp-match.c | 59 +++
lib/ofp-meter.c | 178 +++
lib/ofp-monitor.c| 27 +
lib/ofp-port.c | 145 ++
lib/ofp-print.c | 976 ++-
lib/ofp-queue.c | 196 +++
lib/ofp-table.c | 35 ++
19 files changed, 1080 insertions(+), 936 deletions(-)
diff --git a/include/openvswitch/ofp-bundle.h b/include/openvswitch/ofp-bundle.h
index 78a44d6f0088..b4c29c0d9c43 100644
--- a/include/openvswitch/ofp-bundle.h
+++ b/include/openvswitch/ofp-bundle.h
@@ -36,9 +36,11 @@ struct ofputil_bundle_ctrl_msg {
enum ofperr ofputil_decode_bundle_ctrl(const struct ofp_header *,
struct ofputil_bundle_ctrl_msg *);
-
struct ofpbuf *ofputil_encode_bundle_ctrl_request(
enum ofp_version, struct ofputil_bundle_ctrl_msg *);
+void ofputil_format_bundle_ctrl_request(
+struct ds *, const struct ofputil_bundle_ctrl_msg *);
+
struct ofpbuf *ofputil_encode_bundle_ctrl_reply(
const struct ofp_header *, struct ofputil_bundle_ctrl_msg *);
@@ -51,10 +53,15 @@ struct ofputil_bundle_add_msg {
struct ofpbuf *ofputil_encode_bundle_add(enum ofp_version,
struct ofputil_bundle_add_msg *);
-
enum ofperr ofputil_decode_bundle_add(const struct ofp_header *,
struct ofputil_bundle_add_msg *,
enum ofptype *);
+void ofputil_format_bundle_add(struct ds *,
+ const struct ofputil_bundle_add_msg *,
+ const struct ofputil_port_map *,
+ const struct ofputil_table_map *,
+ int verbosity);
+
/* Bundle message as produced by ofp-parse. */
struct ofputil_bundle_msg {
diff --git a/include/openvswitch/ofp-connection.h
b/include/openvswitch/ofp-connection.h
index 540ce9ed9b21..5fb143157d23 100644
--- a/include/openvswitch/ofp-connection.h
+++ b/include/openvswitch/ofp-connection.h
@@ -31,22 +31,26 @@ struct ofputil_role_request {
uint64_t generation_id;
};
+enum ofperr ofputil_decode_role_message(const struct ofp_header *,
+struct ofputil_role_request *);
+void ofputil_format_role_message(struct ds *,
+ const struct ofputil_role_request *);
+struct ofpbuf *ofputil_encode_role_reply(const struct ofp_header *,
+ const struct ofputil_role_request *);
+
+/* Abstract OFPT_ROLE_STATUS. */
struct ofputil_role_status {
enum ofp12_controller_role role;
enum ofp14_controller_role_reason reason;
uint64_t generation_id;
};
-enum ofperr ofputil_decode_role_message(const struct ofp_header *,
-struct ofputil_role_request *);
-struct ofpbuf *ofputil_encode_role_reply(const struct ofp_header *,
- const struct ofputil_role_request *);
-
struct ofpbuf *ofputil_encode_role_status(const struct ofputil_role_status *,
enum ofputil_protocol);
-
enum ofperr ofputil_decode_role_status(const struct ofp_header *,
struct ofputil_role_status *);
+void ofputil_format_role_status(struct ds *,
+const struct ofputil_role_status *);
enum ofputil_async_msg_type {
/* Standard asynchronous messages. */
@@ -79,6 +83,8 @@ struct ofpbuf *ofputil_encode_get_async_reply(
const struct ofp_header *, const struct ofputil_async_cfg *);
struct ofpbuf *ofputil_encode_set_async_config(
const struct ofputil_async_cfg *, uint32_t oams, enum ofp_version);
+void ofputil_format_set_async_config(struct ds *,
+ const struct ofputil_async_cfg *);
struct ofputil_async_cfg ofputil_async_cfg_default(enum ofp_version);
diff --git a/include/openvswitch/ofp-ipfix.h b/include/openvswitch/ofp-ipfix.h
index 75eefc005877..34ff2b9b20bf 100644
--- a/include/openvswitch/ofp-ipfix.h
+++ b/include/openvswitch/ofp-ipfix.h
@@ -19,6 +19,7 @@
#include "openflow/openflow.h"
+struct ds;
struct ofpbuf;
struct ovs_list;
@@ -44,6 +45,10 @@ vo
Re: [ovs-dev] [PATCH] rconn: Introduce new invariant to fix assertion failure in corner case.
On Wed, May 23, 2018 at 09:28:59PM -0700, Ben Pfaff wrote:
> On Wed, May 23, 2018 at 06:06:44PM -0700, Han Zhou wrote:
> > On Wed, May 23, 2018 at 5:14 PM, Ben Pfaff wrote:
> > >
> > > Until now, rconn_get_version() has only reported the OpenFlow version in
> > > use when the rconn is actually connected. This makes sense, but it has a
> > > harsh consequence. Consider code like this:
> > >
> > > if (rconn_is_connected(rconn) && rconn_get_version(rconn) >= 0) {
> > > for (int i = 0; i < 2; i++) {
> > > struct ofpbuf *b = ofputil_encode_echo_request(
> > > rconn_get_version(rconn));
> > > rconn_send(rconn, b, NULL);
> > > }
> > > }
> > >
> > > Maybe not the smartest code in the world, and probably no one would write
> > > this exact code in any case, but it doesn't look too risky or crazy.
> > >
> > > But it is. The second trip through the loop can assert-fail inside
> > > ofputil_encode_echo_request() because rconn_get_version(rconn) returns -1
> > > instead of a valid OpenFlow version. That happens if the first call to
> > > rconn_send() encounters an error while sending the message and therefore
> > > destroys the underlying vconn and disconnects so that rconn_get_version()
> > > doesn't have a vconn to query for its version.
> > >
> > > In a case like this where all the code to send the messages is close by,
> > we
> > > could just check rconn_get_version() in each loop iteration. We could
> > even
> > > go through the tree and convince ourselves that individual bits of code
> > are
> > > safe, or be conservative and check rconn_get_version() >= 0 in the iffy
> > > cases. But this seems to me like an ongoing source of risk and a way to
> > > get things wrong in corner cases.
> > >
> > > This commit takes a different approach. It introduces a new invariant: if
> > > an rconn has ever been connected, then it returns a valid OpenFlow version
> > > from rconn_get_version(). In addition, if an rconn is currently
> > connected,
> > > then the OpenFlow version it returns is the correct one (that may be
> > > obvious, but there were corner cases before where it returned -1 even
> > > though rconn_is_connected() returned true).
> > >
> > > With this commit, the code above would work OK. If the first call to
> > > rconn_send() encounters an error sending the message, then
> > > rconn_get_version() in the second iteration will return the same value as
> > > in the first iteration. The message passed to rconn_send() will end up
> > > being discarded, but that's much better than either an assertion failure
> > or
> > > having to carefully analyze a lot of our code to deal with one unusual
> > > corner case.
> > >
> > > Reported-by: Han Zhou
> > > Signed-off-by: Ben Pfaff
> > > ---
> > > lib/learning-switch.c | 2 +-
> > > lib/rconn.c | 41 -
> > > lib/vconn.c | 1 +
> > > 3 files changed, 18 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/lib/learning-switch.c b/lib/learning-switch.c
> > > index 04eaa6e8e73f..8102475cae52 100644
> > > --- a/lib/learning-switch.c
> > > +++ b/lib/learning-switch.c
> > > @@ -305,7 +305,7 @@ lswitch_run(struct lswitch *sw)
> > > rconn_run(sw->rconn);
> > >
> > > if (sw->state == S_CONNECTING) {
> > > -if (rconn_get_version(sw->rconn) != -1) {
> > > +if (rconn_is_connected(sw->rconn)) {
> > > lswitch_handshake(sw);
> > > sw->state = S_FEATURES_REPLY;
> > > }
> > > diff --git a/lib/rconn.c b/lib/rconn.c
> > > index 3cad259d0178..a3f811fedbe3 100644
> > > --- a/lib/rconn.c
> > > +++ b/lib/rconn.c
> > > @@ -141,7 +141,8 @@ struct rconn {
> > > struct vconn *monitors[MAXIMUM_MONITORS];
> > > size_t n_monitors;
> > >
> > > -uint32_t allowed_versions;
> > > +uint32_t allowed_versions; /* Acceptable OpenFlow versions. */
> > > +int version;/* Current or most recent version. */
> > > };
> > >
> > > /* Counts packets and bytes queued into an rconn by a given source. */
> > > @@ -182,8 +183,6 @@ static bool is_connected_state(enum state);
> > > static bool is_admitted_msg(const struct ofpbuf *);
> > > static bool rconn_logging_connection_attempts__(const struct rconn *rc)
> > > OVS_REQUIRES(rc->mutex);
> > > -static int rconn_get_version__(const struct rconn *rconn)
> > > -OVS_REQUIRES(rconn->mutex);
> > >
> > > /* The following prototypes duplicate those in rconn.h, but there we
> > weren't
> > > * able to add the OVS_EXCLUDED annotations because the definition of
> > struct
> > > @@ -284,7 +283,9 @@ rconn_create(int probe_interval, int max_backoff,
> > uint8_t dscp,
> > > rconn_set_dscp(rc, dscp);
> > >
> > > rc->n_monitors = 0;
> > > +
> > > rc->allowed_versions = allowed_versions;
> > > +rc->version = -1;
> > >
> > > return rc;
> > > }
> > > @@ -376,8 +377,7 @@ rconn_connect_unreliably(struct rconn *rc,
> > >
[ovs-dev] [PATCH v2] ofp-actions: Split ofpacts_check__() into many functions.
ofpacts_check__() was a huge switch statement with special cases for many
different kinds of actions. This made it unwieldy and put the special
cases far away from the rest of the code related to a given action. This
commit refactors the code to avoid the problem.
Signed-off-by: Ben Pfaff
---
I'm reposting this in hope of getting a review this time.
include/openvswitch/ofp-actions.h | 20 +-
lib/ofp-actions.c | 973 --
lib/ofp-flow.c| 19 +-
ofproto/ofproto-dpif-trace.c | 20 +-
ofproto/ofproto.c | 12 +-
utilities/ovs-ofctl.c | 19 +-
6 files changed, 678 insertions(+), 385 deletions(-)
diff --git a/include/openvswitch/ofp-actions.h
b/include/openvswitch/ofp-actions.h
index b3dd0959d53e..8f5ebaa2be51 100644
--- a/include/openvswitch/ofp-actions.h
+++ b/include/openvswitch/ofp-actions.h
@@ -1029,14 +1029,22 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
*openflow,
const struct vl_mff_map *vl_mff_map,
uint64_t *ofpacts_tlv_bitmap,
struct ofpbuf *ofpacts);
+
+struct ofpact_check_params {
+/* Input. */
+struct match *match;
+ofp_port_t max_ports;
+uint8_t table_id;
+uint8_t n_tables;
+
+/* Output. */
+enum ofputil_protocol usable_protocols;
+};
enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
- struct match *, ofp_port_t max_ports,
- uint8_t table_id, uint8_t n_tables,
- enum ofputil_protocol *usable_protocols);
+ struct ofpact_check_params *);
enum ofperr ofpacts_check_consistency(struct ofpact[], size_t ofpacts_len,
- struct match *, ofp_port_t max_ports,
- uint8_t table_id, uint8_t n_tables,
- enum ofputil_protocol usable_protocols);
+ enum ofputil_protocol needed_protocols,
+ struct ofpact_check_params *);
enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports);
/* Converting ofpacts to OpenFlow. */
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 87797bc9a4fd..a5967ea777fa 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -430,6 +430,8 @@ static char * OVS_WARN_UNUSED_RESULT ofpacts_parse_copy(
const char *s_, const struct ofpact_parse_params *pp,
bool allow_instructions, enum ofpact_type outer_action);
+static void inconsistent_match(enum ofputil_protocol *usable_protocols);
+
/* Returns the ofpact following 'ofpact', except that if 'ofpact' contains
* nested ofpacts it returns the first one. */
struct ofpact *
@@ -687,6 +689,13 @@ format_OUTPUT(const struct ofpact_output *a,
ds_put_format(fp->s, ":%"PRIu16, a->max_len);
}
}
+
+static enum ofperr
+check_OUTPUT(const struct ofpact_output *a,
+ const struct ofpact_check_params *cp)
+{
+return ofpact_check_output_port(a->port, cp->max_ports);
+}
/* Group actions. */
@@ -719,6 +728,13 @@ format_GROUP(const struct ofpact_group *a,
ds_put_format(fp->s, "%sgroup:%s%"PRIu32,
colors.special, colors.end, a->group_id);
}
+
+static enum ofperr
+check_GROUP(const struct ofpact_group *a OVS_UNUSED,
+const struct ofpact_check_params *cp OVS_UNUSED)
+{
+return 0;
+}
/* Action structure for NXAST_CONTROLLER.
*
@@ -1010,6 +1026,13 @@ format_CONTROLLER(const struct ofpact_controller *a,
ds_put_format(fp->s, "%s)%s", colors.paren, colors.end);
}
}
+
+static enum ofperr
+check_CONTROLLER(const struct ofpact_controller *a OVS_UNUSED,
+ const struct ofpact_check_params *cp OVS_UNUSED)
+{
+return 0;
+}
/* Enqueue action. */
struct ofp10_action_enqueue {
@@ -1090,6 +1113,18 @@ format_ENQUEUE(const struct ofpact_enqueue *a,
ofputil_format_port(a->port, fp->port_map, fp->s);
ds_put_format(fp->s, ":%"PRIu32, a->queue);
}
+
+static enum ofperr
+check_ENQUEUE(const struct ofpact_enqueue *a,
+ const struct ofpact_check_params *cp)
+{
+if (ofp_to_u16(a->port) >= ofp_to_u16(cp->max_ports)
+&& a->port != OFPP_IN_PORT
+&& a->port != OFPP_LOCAL) {
+return OFPERR_OFPBAC_BAD_OUT_PORT;
+}
+return 0;
+}
/* Action structure for NXAST_OUTPUT_REG.
*
@@ -1243,6 +1278,13 @@ format_OUTPUT_REG(const struct ofpact_output_reg *a,
ds_put_format(fp->s, "%soutput:%s", colors.special, colors.end);
mf_format_subfield(&a->src, fp->s);
}
+
+static enum ofperr
+check_OUTPUT_REG(const struct ofpact_output_reg *a,
+ const struct ofpact_check_params *cp)
+{
+return mf_check_src(&a->src, cp->match);
+}
/* Action structure for NXAST_BUNDLE and NXAST_BUNDLE_LOAD.
*
@@ -1461,6 +1503,13 @@
Re: [ovs-dev] [PATCH] ovsdb-idl: Correct singleton insert logic
On Tue, May 29, 2018 at 09:15:52AM -0400, Mark Michelson wrote: > Hi Ben, > > Thanks for having a look and providing an update. See my replies in-line > below. > > On 05/25/2018 06:31 PM, Ben Pfaff wrote: > >On Thu, May 17, 2018 at 01:16:55PM -0400, Mark Michelson wrote: > >>When inserting data into a "singleton" table (one that has maxRows == > >>1), there is a check that ensures that the table is currently empty > >>before inserting the row. The intention is to prevent races where > >>multiple clients might attempt to insert rows at the same time. > >> > >>The problem is that this singleton check can cause legitimate > >>transactions to fail. Specifically, a transaction that attempts to > >>delete the current content of the table and insert new data will cause > >>the singleton check to fail since the table currently has data. > >> > >>This patch corrects the issue by keeping a count of the rows being > >>deleted and added to singleton tables. If the total is larger than zero, > >>then the net operation is attempting to insert rows. If the total is > >>less than zero, then the net operation is attempting to remove rows. If > >>the total is zero, then the operation is inserting and deleting an equal > >>number of rows (or is just updating rows). We only add the singleton > >>check if the total is larger than zero. > >> > >>This patch also includes a new test for singleton tables that ensures > >>that the maxRows constraint works as expected. > >> > >>Signed-off-by: Mark Michelson > > > >Good catch. (It's kind of weird to delete and repopulate a singleton > >table, but we should support it.) > > The use case that brought this to our attention was the "set-ssl" commands > for ovn-nbctl and ovn-sbctl. Since the SSL table is a singleton, these > operations delete the current row and repopulate with a new row. The > existing check resulted in failure when when the transaction should have > been allowed. > > > > >I think that the following patch achieves the same end with less > >bookkeeping overhead. What do you think? It doesn't break anything in > >the testsuite, but I didn't check that it actually achieves the purpose. > > > > I did some testing and debugging and this looks like it has the expected > behavior, and I agree that this seems like a simpler approach. So > > Acked-by: Mark Michelson Thanks. I applied this to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovn: Allow for automatic dynamic updates of IPAM
On Fri, Jun 01, 2018 at 03:41:14PM -0400, Mark Michelson wrote: > OVN offers a method of IP address management that allows for an IPv4 subnet or > IPv6 prefix to be specified on a logical switch. Then by specifying a > switch port's address as "dynamic" or " dynamic", OVN will > automatically assign addresses to the switch port. > > While this works great for initial assignment of addresses, addresses do > not automatically adjust when changes are made to the switch's > configuration. For instance: > * If the subnet, ipv6_prefix, or exclude_ips for a logical switch > changes, the affected switch ports are not updated. > * If a switch port with a static IP address is added to the switch, and > that address conflicts with a dynamically assigned IP address, the > dynamic address is not updated. > * If a MAC address switched from being statically assigned to > dynamically assigned, the MAC address would not be updated. > * If a statically assigned MAC address changed, then the IPv6 address > would not be updated. I spent some time looking this over and discovered some things that I'd like to fix before it goes in: https://patchwork.ozlabs.org/project/openvswitch/list/?series=50436&state=* Would you mind reviewing those? Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 3/3] ovn-northd: Always allocate ipam_info for an ovn_datapath.
Until now, the ipam_info struct for a datapath has been allocated on
demand. This leads to slightly complication in the code in places, and
there is hardly any benefit since ipam_info is only about 48 bytes anyway.
This commit just inlines it into struct ovn_datapath.
Signed-off-by: Ben Pfaff
---
ovn/northd/ovn-northd.c | 90 +++--
1 file changed, 35 insertions(+), 55 deletions(-)
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 8755fa1f40c0..2eb6ace39cba 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -427,7 +427,7 @@ struct ovn_datapath {
bool has_unknown;
/* IPAM data. */
-struct ipam_info *ipam_info;
+struct ipam_info ipam_info;
/* OVN northd only needs to know about the logical router gateway port for
* NAT on a distributed router. This "distributed gateway port" is
@@ -480,10 +480,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
ovn_datapath *od)
* use it. */
hmap_remove(datapaths, &od->key_node);
destroy_tnlids(&od->port_tnlids);
-if (od->ipam_info) {
-bitmap_free(od->ipam_info->allocated_ipv4s);
-free(od->ipam_info);
-}
+bitmap_free(od->ipam_info.allocated_ipv4s);
free(od->router_ports);
free(od);
}
@@ -535,26 +532,14 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
return;
}
-const char *subnet_str = smap_get(&od->nbs->other_config, "subnet");
const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix");
+od->ipam_info.ipv6_prefix_set = ipv6_prefix && ipv6_parse(
+ipv6_prefix, &od->ipam_info.ipv6_prefix);
-if (ipv6_prefix) {
-if (!od->ipam_info) {
-od->ipam_info = xzalloc(sizeof *od->ipam_info);
-}
-od->ipam_info->ipv6_prefix_set = ipv6_parse(
-ipv6_prefix, &od->ipam_info->ipv6_prefix);
-} else {
-if (od->ipam_info) {
-od->ipam_info->ipv6_prefix_set = false;
-}
-}
-
+const char *subnet_str = smap_get(&od->nbs->other_config, "subnet");
if (!subnet_str) {
-if (od->ipam_info) {
-bitmap_free(od->ipam_info->allocated_ipv4s);
-od->ipam_info->allocated_ipv4s = NULL;
-}
+bitmap_free(od->ipam_info.allocated_ipv4s);
+od->ipam_info.allocated_ipv4s = NULL;
return;
}
@@ -566,24 +551,18 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
VLOG_WARN_RL(&rl, "bad 'subnet' %s", subnet_str);
free(error);
-if (od->ipam_info) {
-bitmap_free(od->ipam_info->allocated_ipv4s);
-od->ipam_info->allocated_ipv4s = NULL;
-}
+bitmap_free(od->ipam_info.allocated_ipv4s);
+od->ipam_info.allocated_ipv4s = NULL;
return;
}
-
-if (!od->ipam_info) {
-od->ipam_info = xzalloc(sizeof *od->ipam_info);
-}
-od->ipam_info->start_ipv4 = ntohl(subnet) + 1;
-od->ipam_info->total_ipv4s = ~ntohl(mask);
-od->ipam_info->allocated_ipv4s =
-bitmap_allocate(od->ipam_info->total_ipv4s);
+od->ipam_info.start_ipv4 = ntohl(subnet) + 1;
+od->ipam_info.total_ipv4s = ~ntohl(mask);
+od->ipam_info.allocated_ipv4s =
+bitmap_allocate(od->ipam_info.total_ipv4s);
/* Mark first IP as taken */
-bitmap_set1(od->ipam_info->allocated_ipv4s, 0);
+bitmap_set1(od->ipam_info.allocated_ipv4s, 0);
/* Check if there are any reserver IPs (list) to be excluded from IPAM */
const char *exclude_ip_list = smap_get(&od->nbs->other_config,
@@ -617,11 +596,11 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
}
/* Clamp start...end to fit the subnet. */
-start = MAX(od->ipam_info->start_ipv4, start);
-end = MIN(od->ipam_info->start_ipv4 + od->ipam_info->total_ipv4s, end);
+start = MAX(od->ipam_info.start_ipv4, start);
+end = MIN(od->ipam_info.start_ipv4 + od->ipam_info.total_ipv4s, end);
if (end > start) {
-bitmap_set_multiple(od->ipam_info->allocated_ipv4s,
-start - od->ipam_info->start_ipv4,
+bitmap_set_multiple(od->ipam_info.allocated_ipv4s,
+start - od->ipam_info.start_ipv4,
end - start, 1);
} else {
lexer_error(&lexer, "excluded addresses not in subnet");
@@ -953,14 +932,14 @@ ipam_insert_mac(struct eth_addr *ea, bool check)
static void
ipam_insert_ip(struct ovn_datapath *od, uint32_t ip)
{
-if (!od || !od->ipam_info || !od->ipam_info->allocated_ipv4s) {
+if (!od || !od->ipam_info.allocated_ipv4s) {
return;
}
-if (ip >= od->ipam_info->start_ipv4 &&
-ip < (od->ipam_info->start_ipv4 + od->ipam_info->total_ipv4s)) {
-bitmap_set1(od->ipam_info->allocated_ipv4s,
-ip - od->ipam_info-
[ovs-dev] [PATCH 2/3] ovn-northd: Make it possible to disable IPAM once enabled.
The code for doing IPAM never gave up if IPAM was un-configured later.
Found by inspection.
Signed-off-by: Ben Pfaff
---
ovn/northd/ovn-northd.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 344b595ad3c1..8755fa1f40c0 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -544,9 +544,17 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
}
od->ipam_info->ipv6_prefix_set = ipv6_parse(
ipv6_prefix, &od->ipam_info->ipv6_prefix);
+} else {
+if (od->ipam_info) {
+od->ipam_info->ipv6_prefix_set = false;
+}
}
if (!subnet_str) {
+if (od->ipam_info) {
+bitmap_free(od->ipam_info->allocated_ipv4s);
+od->ipam_info->allocated_ipv4s = NULL;
+}
return;
}
@@ -557,6 +565,12 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
= VLOG_RATE_LIMIT_INIT(5, 1);
VLOG_WARN_RL(&rl, "bad 'subnet' %s", subnet_str);
free(error);
+
+if (od->ipam_info) {
+bitmap_free(od->ipam_info->allocated_ipv4s);
+od->ipam_info->allocated_ipv4s = NULL;
+}
+
return;
}
--
2.16.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/3] ovn-northd: Fix memory leak when IPv6 IPAM is in use.
Every iteration of the main loop allocated a new IPAM structure.
Found by inspection.
Signed-off-by: Ben Pfaff
---
ovn/northd/ovn-northd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 74eefc6caeba..344b595ad3c1 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -539,7 +539,9 @@ init_ipam_info_for_datapath(struct ovn_datapath *od)
const char *ipv6_prefix = smap_get(&od->nbs->other_config, "ipv6_prefix");
if (ipv6_prefix) {
-od->ipam_info = xzalloc(sizeof *od->ipam_info);
+if (!od->ipam_info) {
+od->ipam_info = xzalloc(sizeof *od->ipam_info);
+}
od->ipam_info->ipv6_prefix_set = ipv6_parse(
ipv6_prefix, &od->ipam_info->ipv6_prefix);
}
--
2.16.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 0/3] ovn-northd IPAM fixes
While reviewing https://patchwork.ozlabs.org/patch/924319/, I discovered some bugs in IPAM that seem worth fixing. The first two patches below are minimal so that they can be backported. The third is an improvement that doesn't need backporting. Ben Pfaff (3): ovn-northd: Fix memory leak when IPv6 IPAM is in use. ovn-northd: Make it possible to disable IPAM once enabled. ovn-northd: Always allocate ipam_info for an ovn_datapath. ovn/northd/ovn-northd.c | 78 +++-- 1 file changed, 37 insertions(+), 41 deletions(-) -- 2.16.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] datapath: ensure UFO traffic is actually fragmented
On Fri, Jun 15, 2018 at 1:21 PM, Gregory Rose wrote: > On 6/15/2018 1:05 PM, Ben Pfaff wrote: >> >> On Tue, May 29, 2018 at 06:06:19PM +, Neal Shrader via dev wrote: >>> >>> While investigating a kernel panic, our team noticed that UDP traffic >>> recieved by an STT tunnel will always have a gso_type set as SKB_GSO_UDP. >>> After decap, we also noticed that traffic that had this flag set had its >>> fragmentation type set as OVS_FRAG_TYPE_FIRST during key extraction. >>> >>> When the connection tracker encounters this, it assumes it's already >>> dealing with fragmented traffic, which might not be the case. This >>> patch simply ensures we're dealing with an actual fragment before sending >>> the skb off to be reassembled. >>> >>> Reported-by: Johannes Erdfelt >>> Reported-at: >>> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html >>> Signed-off-by: Neal Shrader >> >> Thanks a lot for the patch. >> >> Greg, have you taken a look at this? > > > I had it flagged for review but have not yet had a chance to get to it. > I'll do so now. > I do not think this is right approach to fix the issue. I have posted my comment on discuss mailing thread: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] ovsdb-server: Don't log closing session at program termination.
When ovsdb-server closes a remote connection, it logs a message about it
that includes the reason. Until now this has included sessions that it
closes when it exits. That meant that, when --run was used, there was a
race between noticing that the subprocess exited and noticing that the
session that that subprocess (presumably) had open had been closed. If
it noticed the latter first, nothing was logged (because it didn't log
anything if a session was closed in the ordinary way by the client). If
it noticed the former first, it logged a message about closing the session
itself.
This is a benign race that causes no real problems--except that the tests
didn't expect to see the log message from the former case and fail with
errors like the following:
1826. ovsdb-server.at:92: testing truncating database log with bad
transaction ...
./ovsdb-server.at:96: ovsdb-tool create db schema
stderr:
stdout:
./ovsdb-server.at:104: ovsdb-server --remote=punix:socket db --run="sh
txnfile"
--- /dev/null 2018-04-24 08:50:58.76900 +
+++
/root/openvswitch-2.9.2/rpm/rpmbuild/BUILD/openvswitch-2.9.2/tests/testsuite.dir/at-groups/1826/stderr
2018-05-29 14:29:56.529257295 +
@@ -0,0 +1,2 @@
+2018-05-29T14:29:56Z|1|ovsdb_jsonrpc_server|INFO|unix#0: disconnecting
(removing ordinals database due to server termination)
+2018-05-29T14:29:56Z|2|ovsdb_jsonrpc_server|INFO|unix#0: disconnecting
(removing _Server database due to server termination)
This fixes the race. This particular log message isn't too useful since
it's pretty obvious that ovsdb-server is closing those sessions, since
after all it's exiting!
Reported-by: Sanket Sudake
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046840.html
Signed-off-by: Ben Pfaff
---
ovsdb/jsonrpc-server.c | 12 +++-
ovsdb/ovsdb-server.c | 4 +---
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 6c58f4102e29..7c7a277f0d49 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -173,8 +173,9 @@ ovsdb_jsonrpc_server_add_db(struct ovsdb_jsonrpc_server
*svr, struct ovsdb *db)
/* Removes 'db' from the set of databases served out by 'svr'.
*
- * 'comment' should be a human-readable reason for removing the database. This
- * function frees it. */
+ * 'comment' should be a human-readable reason for removing the database, for
+ * use in log messages, or NULL to suppress logging. This function frees
+ * it. */
void
ovsdb_jsonrpc_server_remove_db(struct ovsdb_jsonrpc_server *svr,
struct ovsdb *db, char *comment)
@@ -339,7 +340,7 @@ ovsdb_jsonrpc_server_free_remote_status(
/* Makes all of the JSON-RPC sessions managed by 'svr' disconnect. (They
* will then generally reconnect.). Uses 'comment' as a human-readable comment
- * for logging. Frees 'comment'.
+ * for logging (it may be NULL to suppress logging). Frees 'comment'.
*
* If 'force' is true, disconnects all sessions. Otherwise, disconnects only
* sesions that aren't database change aware. */
@@ -644,7 +645,8 @@ ovsdb_jsonrpc_session_close_all(struct ovsdb_jsonrpc_remote
*remote)
/* Makes all of the JSON-RPC sessions managed by 'remote' disconnect. (They
* will then generally reconnect.). 'comment' should be a human-readable
- * explanation of the reason for disconnection, for use in log messages.
+ * explanation of the reason for disconnection, for use in log messages, or
+ * NULL to suppress logging.
*
* If 'force' is true, disconnects all sessions. Otherwise, disconnects only
* sesions that aren't database change aware. */
@@ -657,7 +659,7 @@ ovsdb_jsonrpc_session_reconnect_all(struct
ovsdb_jsonrpc_remote *remote,
LIST_FOR_EACH_SAFE (s, next, node, &remote->sessions) {
if (force || !s->db_change_aware) {
jsonrpc_session_force_reconnect(s->js);
-if (jsonrpc_session_is_connected(s->js)) {
+if (comment && jsonrpc_session_is_connected(s->js)) {
VLOG_INFO("%s: disconnecting (%s)",
jsonrpc_session_get_name(s->js), comment);
}
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 68f7acae9fa3..8d213b27aae1 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -459,9 +459,7 @@ main(int argc, char *argv[])
SHASH_FOR_EACH_SAFE(node, next, &all_dbs) {
struct db *db = node->data;
-close_db(&server_config, db,
- xasprintf("removing %s database due to server termination",
- db->db->name));
+close_db(&server_config, db, NULL);
shash_delete(&all_dbs, node);
}
ovsdb_jsonrpc_server_destroy(jsonrpc);
--
2.16.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/2] test-unixctl.py: Don't suppress exceptions.
A user reported a failure of test 2364 "vlog - RFC5424 facility - Python2"
with an exit code that says that the test-unixctl process died from an
uncaught exception. Unfortunately the exception didn't show up in the log.
This commit should make the exception show up (it deletes some boilerplate
we use in our Python-based daemons to make them restart themselves on
failure, which isn't needed or appropriate for a test script).
Reported-by: Sanket Sudake
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046840.html
Signed-off-by: Ben Pfaff
---
tests/test-unixctl.py | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/tests/test-unixctl.py b/tests/test-unixctl.py
index 5de51d31ecfd..4fa27b09f82d 100644
--- a/tests/test-unixctl.py
+++ b/tests/test-unixctl.py
@@ -13,7 +13,6 @@
# limitations under the License.
import argparse
-import sys
import ovs.daemon
import ovs.unixctl
@@ -88,11 +87,4 @@ def main():
if __name__ == '__main__':
-try:
-main()
-except SystemExit:
-# Let system.exit() calls complete normally
-raise
-except:
-vlog.exception("traceback")
-sys.exit(ovs.daemon.RESTART_EXIT_CODE)
+main()
--
2.16.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] dpcls: Miniflow match of packet and subtable
Hi, Ben, Your understand of my point 2 is correct. But more specifically, by "0" or "1" I mean the bit in "flowmap map" of the miniflow. Just as a remind, here is the code comment of the flowmap: * The map member hold one bit for each uint64_t in a "struct flow". Each * 0-bit indicates that the corresponding uint64_t is zero, each 1-bit that it * *may* be nonzero (see below how this applies to minimasks). My point 1 ((also Harry first brought it up) is: for packet key, a bit "0" in the miniflow flowmap means the corresponding protocol field definitely does not exist in the packet header (This seems true from miniflow_extract(). For example, 0 in flowmap that maps to L4 field, means the packet does not have L4 fields). My point 2, as you restated, is: a bit "1" in the flowmap of the subtable mask means the corresponding protocol field has to be present in the packet header to find a match (for example, if the miniflow flowmap of subtable mask shows "1" for L4 field, then the packet has to have L4 header to match). If both points hold, then Harry's proposal should work. Which is if the packet header miniflow flowmap has a bit "0" but the same bit in the subtable mask is "1", then we should just skip the subtable, by only comparing the flowmap, which is short. And as you suggested, there are exceptions like the VLAN example you mentioned that we have to check very carefully. > -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Friday, June 15, 2018 12:39 PM > To: Van Haaren, Harry > Cc: jpet...@ovn.org; William Tu ; > db...@vmware.com; Gobriel, Sameh ; Wang, > Yipeng1 ; ovs-dev@openvswitch.org > Subject: Re: dpcls: Miniflow match of packet and subtable > > I think I understand Yipeng's point 2. Please allow me to restate it: > if a flow's field has a 1-bit in its mask, then the field must exist in the > packet > for the flow to match. This is true. (There are some technical exceptions: > for > example, OVS and OpenFlow handle VLANs in an unusual way that allows > matching on VLAN fields even when they are not > present.) > > I don't understand point 1 yet. Can you restate or expand on it, or give an > example? > > On Fri, Jun 15, 2018 at 10:25:43AM +, Van Haaren, Harry wrote: > > Hi Ben, Justin, William and Darrell, > > > > Please see below email regarding performance optimization, I believe > > we can speed up the mf_get_next_in_map() and related dpcls_lookup() > > code significantly if we can confirm that the suggestion below is valid. > > > > Your input and feedback would be very helpful. > > > > Regards, -Harry > > > > > > > > > -Original Message- > > > From: Wang, Yipeng1 > > > Sent: Friday, June 1, 2018 9:46 PM > > > To: Van Haaren, Harry ; > > > ovs-dev@openvswitch.org > > > Cc: Gobriel, Sameh ; jpet...@ovn.org; Ben > > > Pfaff > > > (b...@ovn.org) ; William Tu ; > > > db...@vmware.com > > > Subject: RE: dpcls: Miniflow match of packet and subtable > > > > > > Thanks Harry for explanation. I got your points :) > > > > > > So I guess your proposal should work with two assumptions: 1) For > > > packet miniflow bitmap, a bit as "0" means the packet header does > > > not have the corresponding field. And 2) For subtable mask miniflow, > > > a bit as "1" means all the rules in the subtable will have the > > > corresponding field. Thus, you could safely skip the subtable if the > > > packet header has a "0" on certain field but the mask of the subtable has > a "1". > > > > > > For the second proposal, if the rule added and the packet header > > > searched agree on the same hashing range, it should work I think. > > > > > > I add guys that I believe are more experts on miniflow extraction > > > and megaflow translation to the CC list. > > > > > > Thanks > > > Yipeng > > > > > > >-Original Message- > > > >From: Van Haaren, Harry > > > >Sent: Friday, June 1, 2018 2:30 AM > > > >To: Wang, Yipeng1 ; ovs- > d...@openvswitch.org > > > >Cc: Gobriel, Sameh > > > >Subject: RE: dpcls: Miniflow match of packet and subtable > > > > > > > >> From: Wang, Yipeng1 > > > >> Sent: Tuesday, May 22, 2018 11:49 PM > > > >> To: Van Haaren, Harry ; > > > >> ovs-dev@openvswitch.org > > > >> Cc: Gobriel, Sameh > > > >> Subject: RE: dpcls: Miniflow match of packet and subtable > > > >> > > > >> Hi, Harry, > > > >> > > > >> Welcome! > > > > > > > >Thanks! > > > > > > > >> Please see my reply inlined: > > > > > > > >My responses also inline, prefixed with [HvH] > > > > > > > >> >-Original Message- > > > >> >From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- > > > >> boun...@openvswitch.org] On Behalf Of Van Haaren, Harry > > > >> >Sent: Friday, May 18, 2018 3:10 AM > > > >> >To: ovs-dev@openvswitch.org > > > >> >Subject: [ovs-dev] dpcls: Miniflow match of packet and subtable > > > >> > > > > >> >Hi, > > > >> > > > > >> >My first post to OvS list - a quick hello! You may have seen me > > > >> >on the > > > DPDK > > > >> mailing list, where I send
Re: [ovs-dev] [PATCH] ovsdb-idl: Remove unnecessary code in track clear.
On Fri, Jun 15, 2018 at 1:03 PM, Ben Pfaff wrote:
>
> On Wed, May 30, 2018 at 10:08:26AM -0700, Han Zhou wrote:
> > In ovsdb_idl_db_track_clear(), it needs to free the deleted row.
> > However, it unnecessary to call ovsdb_idl_row_clear_old(), because
> > this has been called in ovsdb_idl_row_destroy(). It is also confusing
> > because it is called only if:
> > if (ovsdb_idl_row_is_orphan(row))
> > This is contradict with the check in ovsdb_idl_row_clear_old():
> > if (!ovsdb_idl_row_is_orphan(row))
> >
> > Signed-off-by: Han Zhou
> > ---
> > lib/ovsdb-idl.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> > index c6ff78e..958f924 100644
> > --- a/lib/ovsdb-idl.c
> > +++ b/lib/ovsdb-idl.c
> > @@ -1710,7 +1710,6 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db)
> > ovs_list_remove(&row->track_node);
> > ovs_list_init(&row->track_node);
> > if (ovsdb_idl_row_is_orphan(row)) {
> > -ovsdb_idl_row_clear_old(row);
> > free(row);
> > }
> > }
>
> Thanks for looking at this code.
>
> This code is definitely useless since, as you say,
> ovsdb_idl_row_clear_old() does nothing for orphan rows.
>
> I understand the tracking feature very poorly. This change implies that
> a tracked row never has data, since it never gets freed here. Or maybe
> it implies that any data in a tracked row is getting leaked. Do you
> understand which or those is true?
Hi Ben,
Currently the tracked row doesn't maintain any data, so there is no leak.
Here is the later patch that tries to keep the data until track_clear:
https://patchwork.ozlabs.org/patch/924268/, but it is independent of this
change.
Thanks,
Han
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] datapath: ensure UFO traffic is actually fragmented
On 6/15/2018 1:05 PM, Ben Pfaff wrote: On Tue, May 29, 2018 at 06:06:19PM +, Neal Shrader via dev wrote: While investigating a kernel panic, our team noticed that UDP traffic recieved by an STT tunnel will always have a gso_type set as SKB_GSO_UDP. After decap, we also noticed that traffic that had this flag set had its fragmentation type set as OVS_FRAG_TYPE_FIRST during key extraction. When the connection tracker encounters this, it assumes it's already dealing with fragmented traffic, which might not be the case. This patch simply ensures we're dealing with an actual fragment before sending the skb off to be reassembled. Reported-by: Johannes Erdfelt Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html Signed-off-by: Neal Shrader Thanks a lot for the patch. Greg, have you taken a look at this? I had it flagged for review but have not yet had a chance to get to it. I'll do so now. Thanks, - Greg ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] datapath: ensure UFO traffic is actually fragmented
On Tue, May 29, 2018 at 06:06:19PM +, Neal Shrader via dev wrote: > While investigating a kernel panic, our team noticed that UDP traffic > recieved by an STT tunnel will always have a gso_type set as SKB_GSO_UDP. > After decap, we also noticed that traffic that had this flag set had its > fragmentation type set as OVS_FRAG_TYPE_FIRST during key extraction. > > When the connection tracker encounters this, it assumes it's already > dealing with fragmented traffic, which might not be the case. This > patch simply ensures we're dealing with an actual fragment before sending > the skb off to be reassembled. > > Reported-by: Johannes Erdfelt > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2018-May/046800.html > Signed-off-by: Neal Shrader Thanks a lot for the patch. Greg, have you taken a look at this? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovsdb-idl: Remove unnecessary code in track clear.
On Wed, May 30, 2018 at 10:08:26AM -0700, Han Zhou wrote:
> In ovsdb_idl_db_track_clear(), it needs to free the deleted row.
> However, it unnecessary to call ovsdb_idl_row_clear_old(), because
> this has been called in ovsdb_idl_row_destroy(). It is also confusing
> because it is called only if:
> if (ovsdb_idl_row_is_orphan(row))
> This is contradict with the check in ovsdb_idl_row_clear_old():
> if (!ovsdb_idl_row_is_orphan(row))
>
> Signed-off-by: Han Zhou
> ---
> lib/ovsdb-idl.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index c6ff78e..958f924 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -1710,7 +1710,6 @@ ovsdb_idl_db_track_clear(struct ovsdb_idl_db *db)
> ovs_list_remove(&row->track_node);
> ovs_list_init(&row->track_node);
> if (ovsdb_idl_row_is_orphan(row)) {
> -ovsdb_idl_row_clear_old(row);
> free(row);
> }
> }
Thanks for looking at this code.
This code is definitely useless since, as you say,
ovsdb_idl_row_clear_old() does nothing for orphan rows.
I understand the tracking feature very poorly. This change implies that
a tracked row never has data, since it never gets freed here. Or maybe
it implies that any data in a tracked row is getting leaked. Do you
understand which or those is true?
Thanks,
Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] dpcls: Miniflow match of packet and subtable
I think I understand Yipeng's point 2. Please allow me to restate it: if a flow's field has a 1-bit in its mask, then the field must exist in the packet for the flow to match. This is true. (There are some technical exceptions: for example, OVS and OpenFlow handle VLANs in an unusual way that allows matching on VLAN fields even when they are not present.) I don't understand point 1 yet. Can you restate or expand on it, or give an example? On Fri, Jun 15, 2018 at 10:25:43AM +, Van Haaren, Harry wrote: > Hi Ben, Justin, William and Darrell, > > Please see below email regarding performance optimization, I believe we can > speed up the mf_get_next_in_map() and related dpcls_lookup() code > significantly > if we can confirm that the suggestion below is valid. > > Your input and feedback would be very helpful. > > Regards, -Harry > > > > > -Original Message- > > From: Wang, Yipeng1 > > Sent: Friday, June 1, 2018 9:46 PM > > To: Van Haaren, Harry ; ovs-dev@openvswitch.org > > Cc: Gobriel, Sameh ; jpet...@ovn.org; Ben Pfaff > > (b...@ovn.org) ; William Tu ; > > db...@vmware.com > > Subject: RE: dpcls: Miniflow match of packet and subtable > > > > Thanks Harry for explanation. I got your points :) > > > > So I guess your proposal should work with two assumptions: 1) For packet > > miniflow bitmap, a bit as "0" means the packet header does not have the > > corresponding field. And 2) For subtable mask miniflow, a bit as "1" means > > all > > the rules in the subtable will have the corresponding field. Thus, you could > > safely skip the subtable if the packet header has a "0" on certain field but > > the mask of the subtable has a "1". > > > > For the second proposal, if the rule added and the packet header searched > > agree on the same hashing range, it should work I think. > > > > I add guys that I believe are more experts on miniflow extraction and > > megaflow > > translation to the CC list. > > > > Thanks > > Yipeng > > > > >-Original Message- > > >From: Van Haaren, Harry > > >Sent: Friday, June 1, 2018 2:30 AM > > >To: Wang, Yipeng1 ; ovs-dev@openvswitch.org > > >Cc: Gobriel, Sameh > > >Subject: RE: dpcls: Miniflow match of packet and subtable > > > > > >> From: Wang, Yipeng1 > > >> Sent: Tuesday, May 22, 2018 11:49 PM > > >> To: Van Haaren, Harry ; > > >> ovs-dev@openvswitch.org > > >> Cc: Gobriel, Sameh > > >> Subject: RE: dpcls: Miniflow match of packet and subtable > > >> > > >> Hi, Harry, > > >> > > >> Welcome! > > > > > >Thanks! > > > > > >> Please see my reply inlined: > > > > > >My responses also inline, prefixed with [HvH] > > > > > >> >-Original Message- > > >> >From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev- > > >> boun...@openvswitch.org] On Behalf Of Van Haaren, Harry > > >> >Sent: Friday, May 18, 2018 3:10 AM > > >> >To: ovs-dev@openvswitch.org > > >> >Subject: [ovs-dev] dpcls: Miniflow match of packet and subtable > > >> > > > >> >Hi, > > >> > > > >> >My first post to OvS list - a quick hello! You may have seen me on the > > DPDK > > >> mailing list, where I send most of my patches. I'm looking > > >> >forward to working with yee folks of the OvS community :) > > >> > > > >> >I've been looking at optimizing the datapath classifier, and stumbled > > >> >into > > >> a few concepts that I don't think I understand correctly. I've a > > >> >question below, any input or suggestions where to investigate next > > welcome! > > >> > > > >> >When matching the miniflows between packets and subtables, I believe a > > >> packet cannot match a subtable if the packet does not have > > >> >at least the miniflow bits set that the subtable miniflow has. > > >> >Eg: > > >> >osubtable matches on nw_src (bit 63 of mf) > > >> >oThe packet miniflow does NOT have bit 63 set > > >> >oIs it possible for this to packet to match the subtable? If yes, > > >> >how? > > >> > > > >> [Wang, Yipeng] If the subtable mask set the nw_src to be "cared (meaning > > >> 1s > > >> in mask)", then incoming packets should consider these bits as "cared" > > >> during subtable lookup. Even if the packet has those bits as all 0s, it > > does > > >> not mean these bits are not "cared". It is still possible that this key > > >> matches one of the rules in this subtable. For example the rule in the > > table > > >> has nw_src as 0, e.g., an IP address of 0.0.0.0. Then a packet with IP > > >> of > > >> 0.0.0.0 could match it. > > > > > >[HvH] > > >Yes I agree with you, the above 0.0.0.0 case the actual *contents* of the > > >miniflow could match zero, however the packet miniflow *bits* would > > >not have the IP-bit set, hence the metadata can be identified as not usable > > >for this subtable. > > > > > >Let me graph it up; > > > > > >/* bits as per offsetof(struct flow, FIELD_NAME) */ > > >#define DP_PORT (52) > > >#define IPv4 (63) > > >#define IPv6 (73) > > > > > >Item | MF bits| MF Values (uint64_t blocks of metadata) > > >--
Re: [ovs-dev] [PATCH 1/1] Utilities: Add the simap and netdev_provider dump commands to gdb
On Fri, Jun 01, 2018 at 01:21:31PM +0200, Eelco Chaudron wrote: > This changes add two additional gdb commands: > > - ovs_dump_netdev_provider > - ovs_dump_ovs_list Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] Utilities: Add the ovs_dump_dp_provider command to the gdb script
On Thu, May 31, 2018 at 11:13:19AM +0200, Eelco Chaudron wrote: > This change adds the ovs_dump_dp_provider command, which allows > dumping of all the registered registered_dpif_class structures. > > In addition it has some small internal cleanups. > > Signed-off-by: Eelco Chaudron Thanks, applied to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 0/3] add TCP/UDP port unreachable support to OVN logical router
On Fri, Jun 15, 2018 at 11:49:06AM +0200, Lorenzo Bianconi wrote: > Add TCP reset/ICMP port unreachable messages in reply to IP packets directed > to > the logical router's IP addresses Thanks for implementing this! I noticed a needlessly inefficient pattern here, where the actions are a fixed string yet the code copies it into a dynamic string and later frees it. Would you mind just using a string literal in those cases? Thanks, Ben. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v5 0/3] Use VLANs for VLAN packets redirected to a gateway chassis
On Thu, Jun 07, 2018 at 02:59:46PM +0530, vkomm...@redhat.com wrote: > From: Venkata Anil > > This patch avoids tunneling and instead uses source tenant vlan network > across hypervisors for traffic from vlan network on local hypervisor > towards gateway hypervisor hosting redirect chassiss port. > > On the local hypervisor, when the packet enters logical router ingress > pipeline from tenant vlan network, router will set REGBIT_NAT_REDIRECT > and redirect the packet to gateway hypervisor, which is hosting the > chassis redirect port, using tenant vlan network. > Packet travelling across hypervisors will have source vlan tag and > distributed gateway port MAC as destination MAC (other packet data > unchanged). > > Gateway hypervisor will check the vlan tag and destination MAC and > resubmit it to router logical ingress pipeline for routing and finding > the logical output port(i.e it treats this packet as coming from the > local patch port connected to tenant vlan network for routing). > > No changes done for return path as return path to source hypervisor > always uses tenant vlan networks. Thanks a lot for revising the patch series. We've had a lot of churn in ovn-controller over the last week, and it has caused some patch rejects for this patch series. Would you mind rebasing and reposting it? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v9 0/7] OVS-DPDK flow offload with rte_flow
Hi Shahaf, Thanks for pointing NIC/protocol support. Find inline comments: >>When you say DECAP functionality is broken you mean the flow is not actually >>inserted to the HW right? [Mallesh]: Yes, DECAP flows are not inserted to HW. >>The datapath should still DECAP the flow correctly. [Mallesh]: Agree, However the datapath failed to DECAP. If VXLAN protocol support in netdev_dpdk_validate_flow failed (return -1) then, should be fall back normal or default behavior of datapath right? Have you tried w/o VXLAN tunnel rules? [Mallesh]: Yes, tried, but same behavior. Best regards, -/Mallesh ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] netdev-dpdk: fix snprintf call
On Fri, Jun 15, 2018 at 05:56:28PM +0300, Ilya Maximets wrote: > > lib/netdev-dpdk.c: In function : > > lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the > > last format character [-Wformat-truncation=] > > snprintf(vhost_vring, 16, "vring_%d_size", i); > > ^ > > > > Since vring_num is 16 bits, the largest value ever would only be 17 bytes, > > including the terminating nul. Stretch it to 18 bytes (as a precaution > > against a signed value, which again would never happen). > > Looks like commit message is a bit outdated. Last sentence doesn't > make sense in v2. > > Other than that: > Acked-by: Ilya Maximets Thanks, I applied this to master. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] lib: Add initalize when xmalloc ofproto: Add initalize when recv from sdn controller
[Dropping invalid email address findtheonly...@example.com.] Thanks. I believe that we've found and fixed related memory errors before. What version of OVS are you using? On Fri, Jun 15, 2018 at 02:48:11PM +0800, 孙文杰 wrote: > Yes, I can. > > When the first packets send to sdn contoller(we use onos), and the onos > will send the packets back to ovs, if the buffer is not initalized, there > may cause some Illegal memory pointer. > > There is a panic stack caused in my env: > > #0 0x00914647 in memcpy_from_metadata (dst=0x7ffe2e6fdd90, > src=0x7ffe2e6fdef0, loc=0x2784e40) at lib/tun-metadata.c:451 > #1 0x0091446c in tun_metadata_get_fmd (tnl=0x7ffe2e6fdeb0, > flow_metadata=0x31f7288) at lib/tun-metadata.c:396 > #2 0x0083a4f9 in flow_get_metadata (flow=0x7ffe2e6fdeb0, > flow_metadata=0x31f7288) at lib/flow.c:1045 > #3 0x007e5b37 in process_upcall (udpif=0x2cef410, > upcall=0x7ffe2e6fe220, odp_actions=0x7ffe2e6fed50, wc=0x0) at > ofproto/ofproto-dpif-upcall.c:1488 > #4 0x007e525c in upcall_cb (packet=0x2cfcd60, flow=0x7ffe2e6feea0, > ufid=0x7ffe2e6ff140, pmd_id=4294967295, type=DPIF_UC_ACTION, > userdata=0x31f2934, actions=0x7ffe2e6fed50, wc=0x0, put_actions=0x0, > aux=0x2cef410) > at ofproto/ofproto-dpif-upcall.c:1264 > #5 0x0082b9ef in dp_netdev_upcall (pmd=0x284e6d0, > packet_=0x2cfcd60, flow=0x7ffe2e6feea0, wc=0x0, ufid=0x7ffe2e6ff140, > type=DPIF_UC_ACTION, userdata=0x31f2934, actions=0x7ffe2e6fed50, > put_actions=0x0) at lib/dpif-netdev.c:4990 > #6 0x0082d15e in dp_execute_userspace_action (pmd=0x284e6d0, > packet=0x2cfcd60, may_steal=false, flow=0x7ffe2e6feea0, > ufid=0x7ffe2e6ff140, actions=0x7ffe2e6fed50, userdata=0x31f2934) at > lib/dpif-netdev.c:5568 > #7 0x0082d97e in dp_execute_cb (aux_=0x7ffe2e6ff650, > packets_=0x7ffe2e6ff680, a=0x31f2928, may_steal=false) at > lib/dpif-netdev.c:5742 > #8 0x0086e868 in odp_execute_actions (dp=0x7ffe2e6ff650, > batch=0x7ffe2e6ff680, steal=false, actions=0x31f2928, actions_len=64, > dp_execute_action=0x82d1e4 ) at lib/odp-execute.c:717 > #9 0x0082e09a in dp_netdev_execute_actions (pmd=0x284e6d0, > packets=0x7ffe2e6ff680, may_steal=false, flow=0x2cfd1c0, actions=0x31f2928, > actions_len=64) at lib/dpif-netdev.c:5945 > #10 0x00826781 in dpif_netdev_execute (dpif=0x2cef370, > execute=0x7ffe2e6ff888) at lib/dpif-netdev.c:2954 > #11 0x0082687e in dpif_netdev_operate (dpif=0x2cef370, > ops=0x7ffe2e6ff8d8, n_ops=1) at lib/dpif-netdev.c:2984 > #12 0x00831fde in dpif_operate (dpif=0x2cef370, ops=0x7ffe2e6ff8d8, > n_ops=1) at lib/dpif.c:1359 > #13 0x00831f3b in dpif_execute (dpif=0x2cef370, > execute=0x7ffe2e6ff900) at lib/dpif.c:1324 > #14 0x007d0778 in packet_execute (ofproto_=0x27b4b10, > opo=0x7ffe2e6ffde0) at ofproto/ofproto-dpif.c:4661 > #15 0x007b864e in ofproto_packet_out_finish (ofproto=0x27b4b10, > opo=0x7ffe2e6ffde0) at ofproto/ofproto.c:3543 > #16 0x007b87b4 in handle_packet_out (ofconn=0x31cf500, > oh=0x2cffe40) at ofproto/ofproto.c:3584 > #17 0x007c1c94 in handle_openflow__ (ofconn=0x31cf500, > msg=0x2784b20) at ofproto/ofproto.c:8071 > #18 0x007c2072 in handle_openflow (ofconn=0x31cf500, > ofp_msg=0x2784b20) at ofproto/ofproto.c:8246 > #19 0x0080296c in ofconn_run (ofconn=0x31cf500, > handle_openflow=0x7c204f ) at ofproto/connmgr.c:1432 > #20 0x00800177 in connmgr_run (mgr=0x2765bd0, > handle_openflow=0x7c204f ) at ofproto/connmgr.c:363 > #21 0x007b479f in ofproto_run (p=0x27b4b10) at > ofproto/ofproto.c:1816 > #22 0x007a4d31 in bridge_run__ () at vswitchd/bridge.c:2941 > #23 0x007a4f0f in bridge_run () at vswitchd/bridge.c:2999 > #24 0x007aa58b in main (argc=4, argv=0x7ffe2e700f78) at > vswitchd/ovs-vswitchd.c:119 > > > > Ben Pfaff 于2018年6月15日周五 上午1:43写道： > > > On Thu, Jun 14, 2018 at 04:18:27PM +0800, findtheonly...@gmail.com wrote: > > > From: findtheonlway > > > > > > When recv packet from onos, the ovs may report an error or > > > crash due to the buffer is uninitalized. > > > > > > Signed-off-by: findtheonlway > > > Signed-off-by: sunwenjie > > > > Can you provide an example? It's not really acceptable to make > > xmalloc() initialize every allocation. > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] meter: Correct comment describing parse_ofp_meter_mod_str().
On Fri, Jun 15, 2018 at 01:44:50AM -0700, Justin Pettit wrote: > Signed-off-by: Justin Pettit Acked-by: Ben Pfaff ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovn-controller: Only add comment in binding_cleanup() in case of changes.
Thanks, applied to master.
On Fri, Jun 15, 2018 at 09:20:49AM -0400, Mark Michelson wrote:
> Acked-by: Mark Michelson
>
> On 06/14/2018 03:36 PM, Ben Pfaff wrote:
> >This makes the comment more meaningful.
> >
> >Signed-off-by: Ben Pfaff
> >---
> > ovn/controller/binding.c | 13 -
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> >diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> >index 2b27f3cbd9ad..021ecddcff77 100644
> >--- a/ovn/controller/binding.c
> >+++ b/ovn/controller/binding.c
> >@@ -628,11 +628,6 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > return true;
> > }
> >-ovsdb_idl_txn_add_comment(
> >-ovnsb_idl_txn,
> >-"ovn-controller: removing all port bindings for '%s'",
> >-chassis_rec->name);
> >-
> > const struct sbrec_port_binding *binding_rec;
> > bool any_changes = false;
> > SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table) {
> >@@ -641,5 +636,13 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > any_changes = true;
> > }
> > }
> >+
> >+if (any_changes) {
> >+ovsdb_idl_txn_add_comment(
> >+ovnsb_idl_txn,
> >+"ovn-controller: removing all port bindings for '%s'",
> >+chassis_rec->name);
> >+}
> >+
> > return !any_changes;
> > }
> >
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] lib: Build action_set in one scan of action_list
On Wed, Jun 06, 2018 at 03:17:59PM +0100, Kyle Simpson wrote:
> The previous implementation scans the action set of each WRITE_ACTIONS
> command 13--17 times when moving the actions over. This change builds
> up the list as a single scan, which should be more efficient.
>
> Signed-off-by: Kyle Simpson
Hmm, this is interesting.
I observe that "set_queue" can be treated like a "set or move" action
since its ordering with those actions doesn't actually matter.
With that, and a few other changes, I get the following, which is pretty
simple and consolidates logic between ofpact_is_set_or_move_action() and
ofpact_is_allowed_in_actions_set(), which is nice, although it does add
preprocessor tricks. I like how it actually deletes more lines (161)
than it adds (106).
What do you think of this version?
--8<--cut here-->8--
From: Kyle Simpson
Date: Wed, 6 Jun 2018 15:17:59 +0100
Subject: [PATCH] lib: Build action_set in one scan of action_list
The previous implementation scans the action set of each WRITE_ACTIONS
command 13--17 times when moving the actions over. This change builds
up the list as a single scan, which should be more efficient.
Signed-off-by: Kyle Simpson
Signed-off-by: Ben Pfaff
---
lib/ofp-actions.c | 267 ++
1 file changed, 106 insertions(+), 161 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 87797bc9a4fd..e91e0b252390 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -6956,17 +6956,77 @@ ofpacts_pull_openflow_actions(struct ofpbuf *openflow,
ofpacts_tlv_bitmap);
}
-/* OpenFlow 1.1 actions. */
+/* OpenFlow 1.1 action sets. */
+/* Append ofpact 'a' onto the tail of 'out' */
+static void
+ofpact_copy(struct ofpbuf *out, const struct ofpact *a)
+{
+ofpbuf_put(out, a, OFPACT_ALIGN(a->len));
+}
-/* True if an action sets the value of a field
- * in a way that is compatibile with the action set.
- * The field can be set via either a set or a move action.
- * False otherwise. */
-static bool
-ofpact_is_set_or_move_action(const struct ofpact *a)
+/* The order in which actions in an action set get executed. This is only for
+ * the actions where only the last instance added is used. */
+#define ACTION_SET_ORDER\
+SLOT(OFPACT_STRIP_VLAN) \
+SLOT(OFPACT_POP_MPLS) \
+SLOT(OFPACT_DECAP) \
+SLOT(OFPACT_ENCAP) \
+SLOT(OFPACT_PUSH_MPLS) \
+SLOT(OFPACT_PUSH_VLAN) \
+SLOT(OFPACT_DEC_TTL)\
+SLOT(OFPACT_DEC_MPLS_TTL) \
+SLOT(OFPACT_DEC_NSH_TTL)
+
+/* Priority for "final actions" in an action set. An action set only gets
+ * executed at all if at least one of these actions is present. If more than
+ * one is present, then only the one later in this list is executed (and if
+ * more than one of a given type, the one later in the action set). */
+#define ACTION_SET_FINAL_PRIORITY \
+FINAL(OFPACT_CT)\
+FINAL(OFPACT_CT_CLEAR) \
+FINAL(OFPACT_RESUBMIT) \
+FINAL(OFPACT_OUTPUT)\
+FINAL(OFPACT_GROUP)
+
+enum action_set_class {
+/* Actions that individually can usefully appear only once in an action
+ * set. If they do appear more than once, then only the last instance is
+ * honored. */
+#define SLOT(OFPACT) ACTION_SLOT_##OFPACT,
+ACTION_SET_ORDER
+#undef SLOT
+
+/* Final actions. */
+#define FINAL(OFPACT) ACTION_SLOT_##OFPACT,
+ACTION_SET_FINAL_PRIORITY
+#undef FINAL
+
+/* Actions that can appear in an action set more than once and are executed
+ * in order. */
+ACTION_SLOT_SET_OR_MOVE,
+
+/* Actions that shouldn't appear in the action set at all. */
+ACTION_SLOT_INVALID
+};
+
+/* Count the action set slots. */
+#define SLOT(OFPACT) +1
+enum { N_ACTION_SLOTS = ACTION_SET_ORDER };
+#undef SLOT
+
+static enum action_set_class
+action_set_classify(const struct ofpact *a)
{
switch (a->type) {
+#define SLOT(OFPACT) case OFPACT: return ACTION_SLOT_##OFPACT;
+ACTION_SET_ORDER
+#undef SLOT
+
+#define FINAL(OFPACT) case OFPACT: return ACTION_SLOT_##OFPACT;
+ACTION_SET_FINAL_PRIORITY
+#undef FINAL
+
case OFPACT_SET_FIELD:
case OFPACT_REG_MOVE:
case OFPACT_SET_ETH_DST:
@@ -6985,47 +7045,35 @@ ofpact_is_set_or_move_action(const struct ofpact *a)
case OFPACT_SET_TUNNEL:
case OFPACT_SET_VLAN_PCP:
case OFPACT_SET_VLAN_VID:
-return true;
+return ACTION_SLOT_SET_OR_MOVE;
+
case OFPACT_BUNDLE:
case OFPACT_CLEAR_ACTIONS:
-case OFPACT_CT:
-case OFPACT_CT_CLEAR:
case OFPACT_CLONE:
case OFPACT_NAT:
case OFPACT_CONTROLLER:
-case OFPACT_DEC_MPL
Re: [ovs-dev] [PATCH v2] netdev-dpdk: fix snprintf call
> lib/netdev-dpdk.c: In function :
> lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the last
> format character [-Wformat-truncation=]
> snprintf(vhost_vring, 16, "vring_%d_size", i);
> ^
>
> Since vring_num is 16 bits, the largest value ever would only be 17 bytes,
> including the terminating nul. Stretch it to 18 bytes (as a precaution
> against a signed value, which again would never happen).
Looks like commit message is a bit outdated. Last sentence doesn't
make sense in v2.
Other than that:
Acked-by: Ilya Maximets
>
> Suggested-by: Ben Pfaff
> Signed-off-by: Aaron Conole
> ---
> lib/netdev-dpdk.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index a76e489a2..1bde9cfe7 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2883,11 +2883,10 @@ netdev_dpdk_vhost_user_get_status(const struct netdev
> *netdev,
>
> for (int i = 0; i < vring_num; i++) {
> struct rte_vhost_vring vring;
> -char vhost_vring[16];
>
> rte_vhost_get_vhost_vring(vid, i, &vring);
> -snprintf(vhost_vring, 16, "vring_%d_size", i);
> -smap_add_format(args, vhost_vring, "%d", vring.size);
> +smap_add_nocopy(args, xasprintf("vring_%d_size", i),
> +xasprintf("%d", vring.size));
> }
>
> ovs_mutex_unlock(&dev->mutex);
> --
> 2.14.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] [RFC] ovn-controller: Experiment with restricting access to columns.
On 06/13/2018 11:29 PM, Han Zhou wrote: On Wed, Jun 13, 2018 at 3:37 PM, Ben Pfaff wrote: To make ovn-controller recompute incrementally, we need accurate dependencies for each function that reads or writes a table. It's currently difficult to be sure about these dependencies, and certainly difficult to maintain them over time, because there's no way to actually enforce them. This commit experiments with an approach that allows for fairly fine-grained access control within ovn-controller to tables and columns. It's based on generating a new version of the IDL data structures for each case where we want different access control. All of these data structures have the same format, but the columns that a given piece of code is not supposed to touch are renamed to discourage programmers from using them, e.g. they're given names suffixed with "__accessdenied". (This means that there is no runtime overhead to the access control since it only requires a cast to convert between the regular and restricted versions.) In addition, when a columns is supposed to be read-only, functions for modifying the column are not supplied. This commit only tries out this experiment for a single file within ovn-controller, the BFD implementation (mostly because that's alphabetically first, no other real reason). It would require a little more work to apply it everywhere, but it's probably not a huge deal. Comments? CC: Han Zhou Signed-off-by: Ben Pfaff --- ovn/controller/automake.mk | 5 + ovn/controller/bfd-vswitch-idl.def | 21 ovn/controller/bfd.c | 20 ++-- ovn/controller/bfd.h | 8 +- ovn/controller/ovn-controller.c| 13 ++- ovsdb/ovsdb-idlc.in| 223 ++ ++- 6 files changed, 268 insertions(+), 22 deletions(-) create mode 100644 ovn/controller/bfd-vswitch-idl.def I wanted to have a quick test but it didn't pass the compile: In file included from ovn/controller/bfd.c:17:0: ovn/controller/bfd.h:19:44: fatal error: ovn/controller/bfd-vswitch-idl.h: No such file or directory Here's a different datapoint in the same category. I got a slightly different error when I tried to compile. ovn/controller/bfd-vswitch-idl.h was auto-generated and everything worked up until the very end: "The following files are in git but not the distribution: ovn/controller/bfd-vswitch-idl.def" The make command I ran was `make sandbox SANDBOXFLAGS="--ovn"` I tried running `make distclean` then reconfiguring, but this didn't help. For comparison, Han, these are my software versions, in case that might be why auto-generation worked for me but not you: gcc version is 7.3.1 make version is 4.2.1 autoconf version is 2.69 I didn't debug, but by looking at the code I got the idea. It ensures the purpose is declared through the generated data type and violations are captured at compile time. I think this is a brilliant way to achieve the check with such a small change, however, I am not sure how is it supposed to help for generating the dependency. I think instead of caring about whether it is read-only, we should care about whether it is write-only. For example, a engine node A reads on Table T1's column C1, reads & writes on Table T2's column C2, and writes on T3's C3. In this case A depends on T1 and T2, but not depends on T3. So I think what matters to the dependency generation is whether it is Write-Only. Read-Only is no different from ReadWrite. If my understanding is correct (that we don't care about the difference between RO and RW), for WO columns, we can simply just don't monitor its change. Then we don't even need this feature from dependency generation point of view. Did I miss something? Another thing, however, I think is really important for the dependency generation is the columns that reference other tables, e.g. bfd_run() doesn't has table "ovsrec_port" as input, but in fact it depends on this table by referencing from ovsrec_bridge->ports. We need the table being referenced appear in the input parameters. The approach in the patch may not solve that problem directly, but something similar may work: we could declare the usage for a referenced column as "direct" or "indirect". "indirect" means the column can be accessed only with "get functions" instead of pointers. The "get functions" requires an additional parameter that is the instance of the table being referenced. To call the "get function" to access the column, the caller function needs to have the table being referenced passed in as parameter, so we will be able to catch the dependency. I am not sure if this approach is too heavy, or is tricky to implement. Thanks, Han ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] OVN: add ICMP time exceeded support to OVN logical router
> On Thu, Jun 14, 2018 at 05:27:18PM +0200, Lorenzo Bianconi wrote:
>> Using icmp4 action, send an ICMP time exceeded frame whenever
>> an OVN logical router receives an IPv4 packets whose TTL has
>> expired (ip.ttl == {0, 1})
>>
>> Signed-off-by: Lorenzo Bianconi
>> ---
>> Changes since v1:
>> - use ovn-nbctl --wait=hv sync instead of sleep in automatic test
>
> Thanks. I applied this to master.
>
> Do you plan to add the same feature for IPv6 logical routers?
Yes, I will work on it
Regards,
Lorenzo
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ovn-controller: Only add comment in binding_cleanup() in case of changes.
Acked-by: Mark Michelson
On 06/14/2018 03:36 PM, Ben Pfaff wrote:
This makes the comment more meaningful.
Signed-off-by: Ben Pfaff
---
ovn/controller/binding.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 2b27f3cbd9ad..021ecddcff77 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -628,11 +628,6 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
return true;
}
-ovsdb_idl_txn_add_comment(
-ovnsb_idl_txn,
-"ovn-controller: removing all port bindings for '%s'",
-chassis_rec->name);
-
const struct sbrec_port_binding *binding_rec;
bool any_changes = false;
SBREC_PORT_BINDING_TABLE_FOR_EACH (binding_rec, port_binding_table) {
@@ -641,5 +636,13 @@ binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
any_changes = true;
}
}
+
+if (any_changes) {
+ovsdb_idl_txn_add_comment(
+ovnsb_idl_txn,
+"ovn-controller: removing all port bindings for '%s'",
+chassis_rec->name);
+}
+
return !any_changes;
}
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] netdev-dpdk: fix snprintf call
lib/netdev-dpdk.c: In function :
lib/netdev-dpdk.c:2865:49: warning: output may be truncated before the last
format character [-Wformat-truncation=]
snprintf(vhost_vring, 16, "vring_%d_size", i);
^
Since vring_num is 16 bits, the largest value ever would only be 17 bytes,
including the terminating nul. Stretch it to 18 bytes (as a precaution
against a signed value, which again would never happen).
Suggested-by: Ben Pfaff
Signed-off-by: Aaron Conole
---
lib/netdev-dpdk.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index a76e489a2..1bde9cfe7 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2883,11 +2883,10 @@ netdev_dpdk_vhost_user_get_status(const struct netdev
*netdev,
for (int i = 0; i < vring_num; i++) {
struct rte_vhost_vring vring;
-char vhost_vring[16];
rte_vhost_get_vhost_vring(vid, i, &vring);
-snprintf(vhost_vring, 16, "vring_%d_size", i);
-smap_add_format(args, vhost_vring, "%d", vring.size);
+smap_add_nocopy(args, xasprintf("vring_%d_size", i),
+xasprintf("%d", vring.size));
}
ovs_mutex_unlock(&dev->mutex);
--
2.14.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] dpcls: Miniflow match of packet and subtable
Hi Ben, Justin, William and Darrell,
Please see below email regarding performance optimization, I believe we can
speed up the mf_get_next_in_map() and related dpcls_lookup() code significantly
if we can confirm that the suggestion below is valid.
Your input and feedback would be very helpful.
Regards, -Harry
> -Original Message-
> From: Wang, Yipeng1
> Sent: Friday, June 1, 2018 9:46 PM
> To: Van Haaren, Harry ; ovs-dev@openvswitch.org
> Cc: Gobriel, Sameh ; jpet...@ovn.org; Ben Pfaff
> (b...@ovn.org) ; William Tu ;
> db...@vmware.com
> Subject: RE: dpcls: Miniflow match of packet and subtable
>
> Thanks Harry for explanation. I got your points :)
>
> So I guess your proposal should work with two assumptions: 1) For packet
> miniflow bitmap, a bit as "0" means the packet header does not have the
> corresponding field. And 2) For subtable mask miniflow, a bit as "1" means all
> the rules in the subtable will have the corresponding field. Thus, you could
> safely skip the subtable if the packet header has a "0" on certain field but
> the mask of the subtable has a "1".
>
> For the second proposal, if the rule added and the packet header searched
> agree on the same hashing range, it should work I think.
>
> I add guys that I believe are more experts on miniflow extraction and megaflow
> translation to the CC list.
>
> Thanks
> Yipeng
>
> >-Original Message-
> >From: Van Haaren, Harry
> >Sent: Friday, June 1, 2018 2:30 AM
> >To: Wang, Yipeng1 ; ovs-dev@openvswitch.org
> >Cc: Gobriel, Sameh
> >Subject: RE: dpcls: Miniflow match of packet and subtable
> >
> >> From: Wang, Yipeng1
> >> Sent: Tuesday, May 22, 2018 11:49 PM
> >> To: Van Haaren, Harry ; ovs-dev@openvswitch.org
> >> Cc: Gobriel, Sameh
> >> Subject: RE: dpcls: Miniflow match of packet and subtable
> >>
> >> Hi, Harry,
> >>
> >> Welcome!
> >
> >Thanks!
> >
> >> Please see my reply inlined:
> >
> >My responses also inline, prefixed with [HvH]
> >
> >> >-Original Message-
> >> >From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> >> boun...@openvswitch.org] On Behalf Of Van Haaren, Harry
> >> >Sent: Friday, May 18, 2018 3:10 AM
> >> >To: ovs-dev@openvswitch.org
> >> >Subject: [ovs-dev] dpcls: Miniflow match of packet and subtable
> >> >
> >> >Hi,
> >> >
> >> >My first post to OvS list - a quick hello! You may have seen me on the
> DPDK
> >> mailing list, where I send most of my patches. I'm looking
> >> >forward to working with yee folks of the OvS community :)
> >> >
> >> >I've been looking at optimizing the datapath classifier, and stumbled into
> >> a few concepts that I don't think I understand correctly. I've a
> >> >question below, any input or suggestions where to investigate next
> welcome!
> >> >
> >> >When matching the miniflows between packets and subtables, I believe a
> >> packet cannot match a subtable if the packet does not have
> >> >at least the miniflow bits set that the subtable miniflow has.
> >> >Eg:
> >> >osubtable matches on nw_src (bit 63 of mf)
> >> >oThe packet miniflow does NOT have bit 63 set
> >> >oIs it possible for this to packet to match the subtable? If yes, how?
> >> >
> >> [Wang, Yipeng] If the subtable mask set the nw_src to be "cared (meaning 1s
> >> in mask)", then incoming packets should consider these bits as "cared"
> >> during subtable lookup. Even if the packet has those bits as all 0s, it
> does
> >> not mean these bits are not "cared". It is still possible that this key
> >> matches one of the rules in this subtable. For example the rule in the
> table
> >> has nw_src as 0, e.g., an IP address of 0.0.0.0. Then a packet with IP of
> >> 0.0.0.0 could match it.
> >
> >[HvH]
> >Yes I agree with you, the above 0.0.0.0 case the actual *contents* of the
> >miniflow could match zero, however the packet miniflow *bits* would
> >not have the IP-bit set, hence the metadata can be identified as not usable
> >for this subtable.
> >
> >Let me graph it up;
> >
> >/* bits as per offsetof(struct flow, FIELD_NAME) */
> >#define DP_PORT (52)
> >#define IPv4 (63)
> >#define IPv6 (73)
> >
> >Item | MF bits| MF Values (uint64_t blocks of metadata)
> >---
> >Subtable | IPv4 | 0x (mask) | not-used | not-used | ...
> >--- Rule | IPv4 | 1.2.3.4 | not-used | not-used | ...
> >
> >Eg: valid packet matching subtable:
> >Packet | DP_PORT, IPv4 | 3 (dp port) | 5.6.7.8 (IPv4) |
> >
> >Eg: Packet that *cannot* match subtable
> >Packet | DP_PORT, IPv6 | 3 (dp port) | 11:22:33:44.. (IPv6) |
> >
> >
> >As such, a generalization that I *think* is valid, is this:
> >
> >if( (packet.mf_bits & subtable.mf_bits) != subtable.mf_bits) {
> > // packet *cannot* match this rule it doesn't have required mf metadata
> >}
> >
> >
> >There is one possible case where even though the metadata is not present
> >it *could* (from a hash calculation point-of-vie
[ovs-dev] [PATCH 3/3] OVN: add protocol unreachable support to OVN router ports
Add priority-70 flows to generate ICMP protocol unreachable messages
in reply to packets directed to the router's IP address on IP protocols
other than UDP, TCP, and ICMP
Signed-off-by: Lorenzo Bianconi
---
ovn/northd/ovn-northd.8.xml | 4
ovn/northd/ovn-northd.c | 17 +
tests/ovn.at| 1 +
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 18a481b3d..cfd35115e 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -1342,10 +1342,6 @@ nd_na {
These flows should not match IP fragments with nonzero offset.
-
-
- Details TBD. Not yet implemented.
-
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index c4b04ae1c..0365eafb1 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -5179,6 +5179,23 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
"next; };");
ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80,
ds_cstr(&match), ds_cstr(&actions));
+
+ds_clear(&match);
+ds_clear(&actions);
+
+ds_put_format(&match,
+ "ip4 && ip4.dst == %s && !ip.later_frag",
+ op->lrp_networks.ipv4_addrs[i].addr_s);
+ds_put_format(&actions,
+ "icmp4 {"
+ "eth.dst <-> eth.src; "
+ "ip4.dst <-> ip4.src; "
+ "ip.ttl = 255; "
+ "icmp4.type = 3; "
+ "icmp4.code = 2; "
+ "next; };");
+ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 70,
+ ds_cstr(&match), ds_cstr(&actions));
}
ds_clear(&match);
diff --git a/tests/ovn.at b/tests/ovn.at
index 4648a303c..6553d17c6 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10444,6 +10444,7 @@ OVN_POPULATE_ARP
ovn-nbctl --wait=hv sync
test_ip_packet 1 1 0001 ff01 $(ip_to_hex 192 168 1 1)
$(ip_to_hex 192 168 1 254) 11 7dae fcfc 0303
+test_ip_packet 1 1 0001 ff01 $(ip_to_hex 192 168 1 1)
$(ip_to_hex 192 168 1 254) 84 7dae fcfd 0302
OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected])
test_tcp_syn_packet 2 2 0002 ff02 $(ip_to_hex 192 168 2 1)
$(ip_to_hex 192 168 2 254) 8b40 3039 7bae 4486
--
2.17.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/3] OVN: add TCP port unreachable support to OVN logical router
Add priority-80 flows to generate TCP reset messages in reply to
TCP datagrams directed to the router's IP address since the
logical router doesn't accept any TCP traffic
Signed-off-by: Lorenzo Bianconi
---
ovn/northd/ovn-northd.8.xml | 4
ovn/northd/ovn-northd.c | 33 -
tests/ovn.at| 31 +++
3 files changed, 63 insertions(+), 5 deletions(-)
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index a33de3d95..18a481b3d 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -1330,10 +1330,6 @@ nd_na {
These flows should not match IP fragments with nonzero offset.
-
-
- Details TBD. Not yet implemented.
-
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 0e137efde..c4b04ae1c 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -5147,7 +5147,7 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
ds_cstr(&match), ds_cstr(&actions));
}
-/* UDP port unreachable */
+/* UDP/TCP port unreachable */
for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
ds_clear(&match);
ds_clear(&actions);
@@ -5165,6 +5165,20 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
"next; };");
ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80,
ds_cstr(&match), ds_cstr(&actions));
+
+ds_clear(&match);
+ds_clear(&actions);
+
+ds_put_format(&match,
+ "ip4 && ip4.dst == %s && !ip.later_frag && tcp",
+ op->lrp_networks.ipv4_addrs[i].addr_s);
+ds_put_format(&actions,
+ "tcp_reset {"
+ "eth.dst <-> eth.src; "
+ "ip4.dst <-> ip4.src; "
+ "next; };");
+ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80,
+ ds_cstr(&match), ds_cstr(&actions));
}
ds_clear(&match);
@@ -5286,6 +5300,23 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 90,
ds_cstr(&match), ds_cstr(&actions));
}
+
+/* TCP port unreachable */
+for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
+ds_clear(&match);
+ds_clear(&actions);
+
+ds_put_format(&match,
+ "ip6 && ip6.dst == %s && !ip.later_frag && tcp",
+ op->lrp_networks.ipv6_addrs[i].addr_s);
+ds_put_format(&actions,
+ "tcp_reset {"
+ "eth.dst <-> eth.src; "
+ "ip6.dst <-> ip6.src; "
+ "next; };");
+ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80,
+ ds_cstr(&match), ds_cstr(&actions));
+}
}
/* NAT, Defrag and load balancing. */
diff --git a/tests/ovn.at b/tests/ovn.at
index ee807f9c3..4648a303c 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10379,6 +10379,34 @@ test_ip_packet() {
as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
}
+# test_tcp_syn_packet INPORT HV ETH_SRC ETH_DST IPV4_SRC IPV4_ROUTER IP_CHKSUM
TCP_SPORT TCP_DPORT TCP_CHKSUM EXP_IP_CHKSUM EXP_TCP_RST_CHKSUM
+#
+# Causes a packet to be received on INPORT of the hypervisor HV. The packet is
an TCP syn segment with
+# ETH_SRC, ETH_DST, IPV4_SRC, IPV4_ROUTER, IP_CHKSUM, TCP_SPORT, TCP_DPORT,
TCP_CHKSUM as specified.
+# EXP_IP_CHKSUM and EXP_TCP_RST_CHKSUM are the ip and tcp checksums of the tcp
reset segment generated by OVN logical router
+#
+# INPORT is an lport number, e.g. 11 for vif11.
+# HV is an hypervisor number
+# ETH_SRC and ETH_DST are each 12 hex digits.
+# IPV4_SRC and IPV4_ROUTER are each 8 hex digits.
+# TCP_SPORT and TCP_DPORT are 4 hex digits.
+# IP_CHKSUM, TCP_CHKSUM, EXP_IP_CHSUM and EXP_TCP_RST_CHKSUM are each 4 hex
digits
+test_tcp_syn_packet() {
+local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv4_src=$5 ip_router=$6
ip_chksum=$7
+local tcp_sport=$8 tcp_dport=$9 tcp_chksum=${10}
+local exp_ip_chksum=${11} exp_tcp_rst_chksum=${12}
+shift 12
+
+local ip_ttl=ff
+local
packet=${eth_dst}${eth_src}080045284000${ip_ttl}06${ip_chksum}${ipv4_src}${ip_router}${tcp_sport}${tcp_dport}000150027210${tcp_chksum}
+
+local tcp_rst_ttl=fe
+local
reply=${eth_src}${eth_dst}080045284000${tcp_rst_ttl}06${exp_ip_chksum}${ip_router}${ipv4_src}${tcp_dport}${tcp_sport}00015004${exp_tcp_rst_chksum}
+echo $reply >> vif$inport.expected
+
+as hv$hv ovs-appctl netdev-dummy/receive v
[ovs-dev] [PATCH 1/3] OVN: add UDP port unreachable support to OVN logical router
Add priority-80 flows to generate ICMP port unreachable messages in
reply to UDP datagrams directed to the router's IP address since the
logical router doesn't accept any UDP traffic
Signed-off-by: Lorenzo Bianconi
---
ovn/northd/ovn-northd.8.xml | 4 --
ovn/northd/ovn-northd.c | 20 ++
tests/ovn.at| 76 +
3 files changed, 96 insertions(+), 4 deletions(-)
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
index 759d3dace..a33de3d95 100644
--- a/ovn/northd/ovn-northd.8.xml
+++ b/ovn/northd/ovn-northd.8.xml
@@ -1317,10 +1317,6 @@ nd_na {
These flows should not match IP fragments with nonzero offset.
-
-
- Details TBD. Not yet implemented.
-
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 74eefc6ca..0e137efde 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -5147,6 +5147,26 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap
*ports,
ds_cstr(&match), ds_cstr(&actions));
}
+/* UDP port unreachable */
+for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
+ds_clear(&match);
+ds_clear(&actions);
+
+ds_put_format(&match,
+ "ip4 && ip4.dst == %s && !ip.later_frag && udp",
+ op->lrp_networks.ipv4_addrs[i].addr_s);
+ds_put_format(&actions,
+ "icmp4 {"
+ "eth.dst <-> eth.src; "
+ "ip4.dst <-> ip4.src; "
+ "ip.ttl = 255; "
+ "icmp4.type = 3; "
+ "icmp4.code = 3; "
+ "next; };");
+ovn_lflow_add(lflows, op->od, S_ROUTER_IN_IP_INPUT, 80,
+ ds_cstr(&match), ds_cstr(&actions));
+}
+
ds_clear(&match);
ds_put_cstr(&match, "ip4.dst == {");
bool has_drop_ips = false;
diff --git a/tests/ovn.at b/tests/ovn.at
index b586afb4e..ee807f9c3 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -10344,3 +10344,79 @@ OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected])
OVN_CLEANUP([hv1], [hv2])
AT_CLEANUP
+
+AT_SETUP([ovn -- router port unreachable])
+AT_KEYWORDS([router-port-unreachable])
+AT_SKIP_IF([test $HAVE_PYTHON = no])
+ovn_start
+
+# test_ip_packet INPORT HV ETH_SRC ETH_DST IPV4_SRC IPV4_ROUTER L4_PROTCOL
IP_CHKSUM EXP_IP_CHKSUM EXP_ICMP_CHKSUM EXP_ICMP_CODE
+#
+# Causes a packet to be received on INPORT of the hypervisor HV. The packet is
an IPv4 packet with
+# ETH_SRC, ETH_DST, IPV4_SRC, IPV4_ROUTER, L4_PROTCOL, IP_CHKSUM as specified.
+# EXP_IP_CHKSUM and EXP_ICMP_CHKSUM are the ip and icmp checksums of the icmp
frame generated by OVN logical router
+# EXP_ICMP_CODE are code and type of the icmp frame generated by OVN logical
router
+#
+# INPORT is a lport number, e.g. 11 for vif11.
+# HV is a hypervisor number
+# ETH_SRC and ETH_DST are each 12 hex digits.
+# IPV4_SRC and IPV4_ROUTER are each 8 hex digits.
+# IP_CHKSUM, EXP_IP_CHSUM and EXP_ICMP_CHKSUM are each 4 hex digits
+test_ip_packet() {
+local inport=$1 hv=$2 eth_src=$3 eth_dst=$4 ipv4_src=$5 ip_router=$6
l4_proto=$7 ip_chksum=$8
+local exp_ip_chksum=$9 exp_icmp_chksum=${10} exp_icmp_code=${11}
+shift 11
+
+local ip_ttl=ff
+local
packet=${eth_dst}${eth_src}080045144000${ip_ttl}${l4_proto}${ip_chksum}${ipv4_src}${ip_router}
+
+local reply_icmp_ttl=fe
+local icmp_data=
+local reply_icmp_payload=${exp_icmp_code}${exp_icmp_chksum}${icmp_data}
+local
reply=${eth_src}${eth_dst}0800451c4000${reply_icmp_ttl}01${exp_ip_chksum}${ip_router}${ipv4_src}${reply_icmp_payload}
+echo $reply >> vif$inport.expected
+
+as hv$hv ovs-appctl netdev-dummy/receive vif$inport $packet
+}
+
+ip_to_hex() {
+printf "%02x%02x%02x%02x" "$@"
+}
+
+for i in 1 2; do
+net_add n$i
+ovn-nbctl ls-add sw$i
+
+sim_add hv$i
+as hv$i
+ovs-vsctl add-br br-phys
+ovn_attach n$i br-phys 192.168.$i.1
+
+ovn-nbctl lsp-add sw$i sw$i-p${i}0 -- \
+lsp-set-addresses sw$i-p${i}0 "00:00:00:00:00:0$i 192.168.$i.1"
+
+ovs-vsctl -- add-port br-int vif$i -- \
+set interface vif$i \
+external-ids:iface-id=sw$i-p${i}0 \
+options:tx_pcap=hv$i/vif$i-tx.pcap \
+options:rxq_pcap=hv$i/vif$i-rx.pcap \
+ofport-request=$i
+done
+
+ovn-nbctl lr-add lr0
+for i in 1 2; do
+ovn-nbctl lrp-add lr0 lrp$i 00:00:00:00:ff:0$i 192.168.$i.254/24
+ovn-nbctl -- lsp-add sw$i lrp$i-attachment \
+ -- set Logical_Switch_Port lrp$i-attachment type=router \
+options:router-port=lrp$i addresses='"00:00:00:00:ff:'0$i'"'
+done
+
+OVN_POPULATE_ARP
+# allow some time for ovn-northd and ovn-controller to catch up.
+ovn-nbctl --wait=hv sync
+
+test_ip_
[ovs-dev] [PATCH 0/3] add TCP/UDP port unreachable support to OVN logical router
Add TCP reset/ICMP port unreachable messages in reply to IP packets directed to the logical router's IP addresses Lorenzo Bianconi (3): OVN: add UDP port unreachable support to OVN logical router OVN: add TCP port unreachable support to OVN logical router OVN: add protocol unreachable support to OVN router ports ovn/northd/ovn-northd.8.xml | 12 ovn/northd/ovn-northd.c | 68 +++ tests/ovn.at| 108 3 files changed, 176 insertions(+), 12 deletions(-) -- 2.17.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] meter: Correct comment describing parse_ofp_meter_mod_str().
Signed-off-by: Justin Pettit --- lib/ofp-meter.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/ofp-meter.c b/lib/ofp-meter.c index 4f77f508d396..de6f2b3a0fc2 100644 --- a/lib/ofp-meter.c +++ b/lib/ofp-meter.c @@ -603,12 +603,13 @@ parse_ofp_meter_mod_str__(struct ofputil_meter_mod *mm, char *string, return NULL; } -/* Convert 'str_' (as described in the Flow Syntax section of the ovs-ofctl man - * page) into 'mm' for sending the specified meter_mod 'command' to a switch. +/* Convert 'str_' (as described in the Meter Syntax section of the + * ovs-ofctl man page) into 'mm' for sending the specified meter_mod + * 'command' to a switch. * * Returns NULL if successful, otherwise a malloc()'d string describing the * error. The caller is responsible for freeing the returned string. - * If successful, 'mm->meter.bands' must be free()d by the caller. */ + * If successful, 'mm->meter.bands' must be free()'d by the caller. */ char * OVS_WARN_UNUSED_RESULT parse_ofp_meter_mod_str(struct ofputil_meter_mod *mm, const char *str_, int command, enum ofputil_protocol *usable_protocols) -- 2.7.4 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/5] dpctl.man: Correct argument to "dump-flows".
> On Jun 14, 2018, at 9:41 PM, Ben Pfaff wrote: > > On Thu, Jun 14, 2018 at 12:00:43PM -0700, Justin Pettit wrote: >> Signed-off-by: Justin Pettit > > For the series: > Acked-by: Ben Pfaff Thanks. I pushed the series to master. I also cherry-picked the first patch to branch-2.8 and branch-2.9, since it fixes documentation errors that far back. --Justin ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/5] dpctl: Use common code to open dpif with optional name.
> On Jun 14, 2018, at 10:40 PM, Darrell Ball wrote:
>
>> On Thu, Jun 14, 2018 at 12:00 PM, Justin Pettit wrote:
>> Signed-off-by: Justin Pettit
>> ---
>> lib/dpctl.c | 158
>> +++-
>> 1 file changed, 38 insertions(+), 120 deletions(-)
>>
>> diff --git a/lib/dpctl.c b/lib/dpctl.c
>> index ec8c51e4b0a7..f522785a5f97 100644
>> --- a/lib/dpctl.c
>> +++ b/lib/dpctl.c
>> @@ -187,6 +187,31 @@ parsed_dpif_open(const char *arg_, bool create, struct
>> dpif **dpifp)
>> return result;
>> }
>>
>> +/* Open a dpif with an optional name argument.
>> + *
>> + * The datapath name is not a mandatory parameter for this command. If
>> + * it is not specified -- so 'argc' < 'max_args' -- we retrieve it from
>> + * the current setup, assuming only one exists. On success stores the
>> + * opened dpif in '*dpif'. */
>> +static int
>> +opt_dpif_open(int argc, const char *argv[], struct dpctl_params *dpctl_p,
>> + struct dpif **dpif, uint8_t max_args)
>> +{
>> ...
>>
> When I wrote the generic function dpctl_ct_open_dp() for a few APIs related
> to a specific commit, which you now
> renamed to opt_dpif_open() here, I intended to come back and use it for the
> other callers here as well :-).
> Glad to see you got to it.
Yes, thanks for starting the work. I just noticed a few places it could also
be leveraged when reviewing your fragment patches.
> Minor comment is ‘dpif’ could be last parameter, since it is an ‘out’
> parameter
Good point. I changed it.
> Also, I am curious why you used ‘opt’ prefix, since this API is needed to
> retrieve a dp even if one is not specified as one of
> the arguments. You could use something like dpctl_open_dp_(), with a trailing
> underscore to specify an internal API.
The "opt" was used to indicate that the name argument was optional. I usually
think of names with a trailing underscores as ones that makes up a function
called by one without the underscores. I think the way it's written follows
the convention of other functions in the file such as parsed_dpif_open(). I
think between that and the comment that describes its use, it should be pretty
clear.
Thanks for the feedback.
--Justin
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] 64Byte packet performance regression on 2.9 from 2.7
> Hi, > I just upgraded from OvS 2.7 + DPDK 16.11 to OvS2.9 + DPDK 17.11 and > running into performance issue with 64 Byte packet rate. One interesting > thing that I notice that even at very light load from IXIA the processing > cycles on all the PMD threads run close to 100% of the cpu cycle on 2.9 > OvS, but on OvS 2.7 even under full load the processing cycles remain at > 75% of the cpu cycles. > > Attaching the FlameGraphs of both the versions, the only thing that pops > out to me is the new way invoking netdev_send() is on 2.9 is being invoked > via dp_netdev_pmd_flush_output_packets() which seems to be adding another > ~11% to the whole rx to tx path. > > I also did try the tx-flush-interval to 50 and more it does seem to help, > but not significant enough to match the 2.7 performance. > > > Any help or ideas would be really great. Thanks, Shahaji Hello, Shahaji. Could you, please, describe your testing scenario in more details? Also, mail-list filters attachments, so they are not available. You need to publish them somewhere else or write in text format inside the letter. About the performance itself: Some performance degradation because of output batching is expected for tests with low number of flows or simple PHY-PHY tests. It was mainly targeted for cases with relatively large number of flows, for amortizing of vhost-user penalties (PHY-VM-PHY, VM-VM cases), OVS bonding cases. If your test involves vhost-user ports, then you should also consider vhost-user performance regression in stable DPDK 17.11 because of fixes for CVE-2018-1059. Related bug: https://dpdk.org/tracker/show_bug.cgi?id=48 It'll be good if you'll be able to test OVS 2.8 + DPDK 17.05. There was too many changes since 2.7. It'll be hard to track down the root cause. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
