[ovs-dev] [PATCH] ofproto-dpif: Tighten up megaflow wildcard handling.
A number of use-cases weren't handled properly when determining what can
be wildcarded for megaflows. This commit both catches additional fields
that cannot be wildcarded and loosens a few other cases.
Bug #17979
Signed-off-by: Justin Pettit
---
lib/flow.c |2 +-
ofproto/netflow.c| 12 +++
ofproto/netflow.h|2 +
ofproto/ofproto-dpif-ipfix.c | 19 +++
ofproto/ofproto-dpif-ipfix.h |3 ++
ofproto/ofproto-dpif-xlate.c | 38 +-
ofproto/ofproto-dpif.c |4 ++
tests/ofproto-dpif.at| 72 +-
8 files changed, 100 insertions(+), 52 deletions(-)
diff --git a/lib/flow.c b/lib/flow.c
index d38e3ab..3e50734 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -790,12 +790,12 @@ flow_mask_hash_fields(struct flow_wildcards *wc, enum
nx_hash_fields fields)
memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
-memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
+wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
break;
default:
diff --git a/ofproto/netflow.c b/ofproto/netflow.c
index 7366986..7efb026 100644
--- a/ofproto/netflow.c
+++ b/ofproto/netflow.c
@@ -51,6 +51,18 @@ struct netflow {
long long int reconfig_time; /* When we reconfigured the timeouts. */
};
+void
+netflow_mask_wc(struct flow_wildcards *wc)
+{
+memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
+memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
+memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
+memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
+memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
+wc->masks.nw_tos |= IP_DSCP_MASK;
+}
+
static void
gen_netflow_rec(struct netflow *nf, struct netflow_flow *nf_flow,
struct ofexpired *expired,
diff --git a/ofproto/netflow.h b/ofproto/netflow.h
index c01ff15..9691545 100644
--- a/ofproto/netflow.h
+++ b/ofproto/netflow.h
@@ -64,6 +64,8 @@ void netflow_expire(struct netflow *, struct netflow_flow *,
bool netflow_run(struct netflow *);
void netflow_wait(struct netflow *);
+void netflow_mask_wc(struct flow_wildcards *);
+
void netflow_flow_init(struct netflow_flow *);
void netflow_flow_clear(struct netflow_flow *);
void netflow_flow_update_time(struct netflow *, struct netflow_flow *,
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 9de8b1c..f76596a 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -659,6 +659,25 @@ ipfix_send_template_msg(struct dpif_ipfix_exporter
*exporter,
ofpbuf_uninit(&msg);
}
+void
+ipfix_mask_wc(struct flow_wildcards *wc)
+{
+memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
+memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
+memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
+memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
+memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
+memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
+memset(&wc->masks.nw_ttl, 0xff, sizeof wc->masks.nw_ttl);
+memset(&wc->masks.nw_tos, 0xff, sizeof wc->masks.nw_tos);
+memset(&wc->masks.ipv6_src, 0xff, sizeof wc->masks.ipv6_src);
+memset(&wc->masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst);
+memset(&wc->masks.ipv6_label, 0xff, sizeof wc->masks.ipv6_label);
+memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
+memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
+}
+
static void
ipfix_send_data_msg(struct dpif_ipfix_exporter *exporter, struct ofpbuf
*packet,
const struct flow *flow, uint64_t packet_delta_count,
diff --git a/ofproto/ofproto-dpif-ipfix.h b/ofproto/ofproto-dpif-ipfix.h
index 26b02f1..b89e757 100644
--- a/ofproto/ofproto-dpif-ipfix.h
+++ b/ofproto/ofproto-dpif-ipfix.h
@@ -21,6 +21,7 @@
#include
struct flow;
+struct flow_wildcards;
struct ofpbuf;
struct ofproto_ipfix_bridge_exporter_options;
struct ofproto_ipfix_flow_exporter_options;
@@ -33,6 +34,8 @@ void dpif_ipfix_set_options(
const struct ofproto_ipfix_bridge_exporter_options *,
const struct ofproto_ipfix_flow_exporter_options *, size_t);
+void ipfix_mask_wc(struct flow_wildcards *);
+
void dpif_ipfix_bridge_sample(struct dpi
[ovs-dev] [PATCH] ofproto-dpif: Reconfigure when IPFIX is enabled/disabled.
The presense or absence of IPFIX affects wildcarding, and there needs to
cause a revalidation when changed.
Found by inspecting Ethan's similar patch on NetFlow.
Signed-off-by: Justin Pettit
---
Since I just fixed IPFIX wildcarding in the previous patch, I'll
probably fold this patch into that one.
---
ofproto/ofproto-dpif.c |2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 5766861..0d7481d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1633,12 +1633,14 @@ set_ipfix(
if (bridge_exporter_options || flow_exporters_options) {
if (!di) {
di = ofproto->ipfix = dpif_ipfix_create();
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
dpif_ipfix_set_options(
di, bridge_exporter_options, flow_exporters_options,
n_flow_exporters_options);
} else {
if (di) {
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
dpif_ipfix_destroy(di);
ofproto->ipfix = NULL;
}
--
1.7.5.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2] feature: Create specific types for ofp and odp port
Thanks very much, Ben for the comments,
On Tue, Jun 18, 2013 at 1:10 PM, Ben Pfaff wrote:
> Thanks again for working on this.
>
> It looks like your test builds are not configured to build a kernel
> module: this patch triggers tons of errors in the kernel build due to
> the change to OVSP_LOCAL, because OVS_FORCE is not defined in the
> kernel. I am not sure of that is the best possible fix (we are
> constrained by the kernel API here), but the following incremental
> does work:
Sorry, I didn't test it with kernel module enabled. Thanks for pointing
this out.
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index add1287..bde3ba2 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -116,7 +116,7 @@ struct ovs_vport_stats {
> };
>
> /* Fixed logical ports. */
> -#define OVSP_LOCAL ((OVS_FORCE odp_port_t) 0)
> +#define OVSP_LOCAL ((__u32)0)
>
I think I will keep OVSP_LOCAL and OVSP_NONE u32. And create another macro
ODPP_NONE of type odp_port_t.
> /* Packet transfer. */
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 58435d7..823e5b3 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -277,7 +277,8 @@ create_dp_netdev(const char *name, const struct
> dpif_class *class,
> hmap_init(&dp->flow_table);
> list_init(&dp->port_list);
>
> -error = do_add_port(dp, name, "internal", OVSP_LOCAL);
> +error = do_add_port(dp, name, "internal",
> +(OVS_FORCE odp_port_t) OVSP_LOCAL);
> if (error) {
> dp_netdev_free(dp);
> return error;
> @@ -468,8 +469,8 @@ static int
> dpif_netdev_port_del(struct dpif *dpif, odp_port_t port_no)
> {
> struct dp_netdev *dp = get_dp_netdev(dpif);
> -return (port_no == OVSP_LOCAL ?
> - EINVAL : do_del_port(dp, port_no));
> +return (odp_to_u32(port_no) == OVSP_LOCAL ?
> +EINVAL : do_del_port(dp, port_no));
> }
>
> static bool
>
>
> I find the new #defines at the top of openflow-1.0.h harder to read
> than the previous enum. How about this, instead:
>
> /* Ranges. */
> #define OFPP_MAXOFP_PORT_C(0xff00) /* Max # of switch ports. */
> #define OFPP_FIRST_RESV OFP_PORT_C(0xfff8) /* First assigned reserved
> port. */
> #define OFPP_LAST_RESV OFP_PORT_C(0x) /* Last assigned reserved port.
> */
>
> /* Reserved output "ports". */
> #define OFPP_IN_PORTOFP_PORT_C(0xfff8) /* Where the packet came in. */
> #define OFPP_TABLE OFP_PORT_C(0xfff9) /* Perfor actions in flow
> table. */
> #define OFPP_NORMAL OFP_PORT_C(0xfffa) /* Process with normal L2/L3. */
> #define OFPP_FLOOD OFP_PORT_C(0xfffb) /* All ports except input port
> and
> * ports disabled by STP. */
> #define OFPP_ALLOFP_PORT_C(0xfffc) /* All ports except input port.
> */
> #define OFPP_CONTROLLER OFP_PORT_C(0xfffd) /* Send to controller. */
> #define OFPP_LOCAL OFP_PORT_C(0xfffe) /* Local openflow "port". */
> #define OFPP_NONE OFP_PORT_C(0x) /* Not associated with any
> port. */
>
> also adding macros for constants in types.h:
>
> diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h
> index 7e1e59d..b292769 100644
> --- a/include/openvswitch/types.h
> +++ b/include/openvswitch/types.h
> @@ -66,4 +66,8 @@ typedef uint16_t OVS_BITWISE ofp_port_t;
> typedef uint32_t OVS_BITWISE odp_port_t;
> typedef uint32_t OVS_BITWISE ofp11_port_t;
>
> +#define OFP_PORT_C(X) ((OVS_FORCE ofp_port_t) X)
> +#define ODP_PORT_C(X) ((OVS_FORCE odp_port_t) X)
> +#define OFP11_PORT_C(X) ((OVS_FORCE ofp11_port_t) X)
> +
> #endif /* openvswitch/types.h */
>
This is cleaner. I'll adopt it. Thanks
>
> Many of the new functions in flow.h are defined to take 'const'
> parameters that don't make much sense. For example:
> static inline uint16_t ofp_to_u16(const ofp_port_t ofp_port);
> 'const' doesn't make sense on plain pass-by-value parameters like this
> because they do not communicate any useful information to the reader:
> the reader already knows that the function cannot modify the variable
> that it passes in, because it is passed by value. (In fact, this is
> true enough that C ignores the presence of qualifiers here for the
> purpose of considering compatibility of functions. Try compiling
> this:
> int x(const int);
> int x(int);
> int y(const int *);
> int y(int *);
> and you will see that the compiler does not complain about the change
> in the prototype for x(), but it does for y().)
>
Thanks. I will remove it.
> I don't really like ofp_htons() and the other _htons() and _ntohs()
> functions. They make the reader have to know, or be willing to look
> up, what another set of functions does, and writing ofp_htons(port)
> instead of htons(ofp_to_u16(port)) does not make code much shorter.
> Same for hash_*_port() and *_port_increment().
>
Sounds reasonable, I'd like to make the changes.
Re: [ovs-dev] [PATCH] ofproto-dpif: Tighten up megaflow wildcard handling.
On Wed, Jun 19, 2013 at 12:02:10AM -0700, Justin Pettit wrote: > A number of use-cases weren't handled properly when determining what can > be wildcarded for megaflows. This commit both catches additional fields > that cannot be wildcarded and loosens a few other cases. > > Bug #17979 > > Signed-off-by: Justin Pettit Why does ipfix need to un-wildcard fields? I don't understand the change in compose_output_action__(). It adds the ECN bits to the mask but I don't see anything there that depends on those bits. In do_xlate_actions(), why do OFPACT_SET_VLAN_VID and OFPACT_SET_VLAN_PCP have new dependencies? They do not read the fields that they add to the mask. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Reconfigure when IPFIX is enabled/disabled.
On Wed, Jun 19, 2013 at 12:32:39AM -0700, Justin Pettit wrote: > The presense or absence of IPFIX affects wildcarding, and there needs to presense => presence there => therefore (?) > cause a revalidation when changed. > > Found by inspecting Ethan's similar patch on NetFlow. > > Signed-off-by: Justin Pettit > --- > Since I just fixed IPFIX wildcarding in the previous patch, I'll > probably fold this patch into that one. Why can't IPFIX handle wildcarding? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2] feature: Create specific types for ofp and odp port
On Wed, Jun 19, 2013 at 08:32:30AM -0700, Alex Wang wrote: > On Tue, Jun 18, 2013 at 1:10 PM, Ben Pfaff wrote: > > nl_attr_get_odp_port() and nl_msg_put_odp_port() functions might be > > handy. > > Sure! honestly, it seems that I could not make the right decision on what > to provide and > what not to. Especially after adding so many utility functions > (*htons,*hash, odp/ofp_to_u), > more to learn ;D It's a difficult balance. I am sure that other experienced programmers would weigh the tradeoffs differently. In this case, I think that the benefits of *_htons() (shorter code) are outweighed by the cost of knowing what those functions do. If we already had lots of *_htons() functions for other types around, so that it was obvious what the new ones do, then I might think differently. On the other hands, there are lots of nl_msg_put_*() and nl_attr_get_*() functions that all follow a similar pattern, so it seems reasonable to create another one that also follows that pattern, because it doesn't make the programmer think much to figure out what it very likely does. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2] feature: Create specific types for ofp and odp port
On Wed, Jun 19, 2013 at 9:29 AM, Ben Pfaff wrote: > On Wed, Jun 19, 2013 at 08:32:30AM -0700, Alex Wang wrote: > > On Tue, Jun 18, 2013 at 1:10 PM, Ben Pfaff wrote: > > > nl_attr_get_odp_port() and nl_msg_put_odp_port() functions might be > > > handy. > > > > Sure! honestly, it seems that I could not make the right decision on what > > to provide and > > what not to. Especially after adding so many utility functions > > (*htons,*hash, odp/ofp_to_u), > > more to learn ;D > > It's a difficult balance. I am sure that other experienced > programmers would weigh the tradeoffs differently. In this case, I > think that the benefits of *_htons() (shorter code) are outweighed by > the cost of knowing what those functions do. If we already had lots > of *_htons() functions for other types around, so that it was obvious > what the new ones do, then I might think differently. > > On the other hands, there are lots of nl_msg_put_*() and > nl_attr_get_*() functions that all follow a similar pattern, so it > seems reasonable to create another one that also follows that pattern, > because it doesn't make the programmer think much to figure out what > it very likely does. > > Thanks, > > Ben. > Thanks Ben, for the detailed explanation, very important to know. ;D ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCHv2] ofproto-dpif: Tighten up megaflow wildcard handling.
A number of use-cases weren't handled properly when determining what can
be wildcarded for megaflows. This commit both catches additional fields
that cannot be wildcarded and loosens a few other cases.
Bug #17979
Signed-off-by: Justin Pettit
---
v1 -> v2: Remove IPFIX un-wildcarding.
---
lib/flow.c |2 +-
ofproto/netflow.c| 12 +++
ofproto/netflow.h|2 +
ofproto/ofproto-dpif-xlate.c | 35 +++-
ofproto/ofproto-dpif.c |4 ++
tests/ofproto-dpif.at| 72 +-
6 files changed, 75 insertions(+), 52 deletions(-)
diff --git a/lib/flow.c b/lib/flow.c
index d38e3ab..3e50734 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -790,12 +790,12 @@ flow_mask_hash_fields(struct flow_wildcards *wc, enum
nx_hash_fields fields)
memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
-memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
+wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
break;
default:
diff --git a/ofproto/netflow.c b/ofproto/netflow.c
index 7366986..7efb026 100644
--- a/ofproto/netflow.c
+++ b/ofproto/netflow.c
@@ -51,6 +51,18 @@ struct netflow {
long long int reconfig_time; /* When we reconfigured the timeouts. */
};
+void
+netflow_mask_wc(struct flow_wildcards *wc)
+{
+memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
+memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
+memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
+memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
+memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
+wc->masks.nw_tos |= IP_DSCP_MASK;
+}
+
static void
gen_netflow_rec(struct netflow *nf, struct netflow_flow *nf_flow,
struct ofexpired *expired,
diff --git a/ofproto/netflow.h b/ofproto/netflow.h
index c01ff15..9691545 100644
--- a/ofproto/netflow.h
+++ b/ofproto/netflow.h
@@ -64,6 +64,8 @@ void netflow_expire(struct netflow *, struct netflow_flow *,
bool netflow_run(struct netflow *);
void netflow_wait(struct netflow *);
+void netflow_mask_wc(struct flow_wildcards *);
+
void netflow_flow_init(struct netflow_flow *);
void netflow_flow_clear(struct netflow_flow *);
void netflow_flow_update_time(struct netflow *, struct netflow_flow *,
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 1c234ec..8ba4fe8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -552,7 +552,7 @@ xlate_normal(struct xlate_ctx *ctx)
memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
-memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
+wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
in_bundle = lookup_input_bundle(ctx->ofproto, flow->in_port,
ctx->xin->packet != NULL, &in_port);
@@ -814,6 +814,7 @@ process_special(struct xlate_ctx *ctx, const struct flow
*flow,
return SLOW_BFD;
} else if (ofport->bundle && ofport->bundle->lacp
&& flow->dl_type == htons(ETH_TYPE_LACP)) {
+memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
if (packet) {
lacp_process_packet(ofport->bundle->lacp, ofport, packet);
}
@@ -833,6 +834,7 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
bool check_stp)
{
const struct ofport_dpif *ofport = get_ofp_port(ctx->ofproto, ofp_port);
+struct flow_wildcards *wc = &ctx->xout->wc;
struct flow *flow = &ctx->xin->flow;
ovs_be16 flow_vlan_tci;
uint32_t flow_skb_mark;
@@ -900,6 +902,7 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
flow_nw_tos = flow->nw_tos;
if (ofproto_dpif_dscp_from_priority(ofport, flow->skb_priority, &dscp)) {
+wc->masks.nw_tos |= IP_ECN_MASK;
flow->nw_tos &= ~IP_DSCP_MASK;
flow->nw_tos |= dscp;
}
@@ -928,7 +931,11 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
flow->tunnel = flow_tnl; /* Restore tunnel metadata */
} else {
uint16_t vlandev_port;
+
odp_port = ofport->odp_port;
+if (!hmap_is_empty(&ctx->ofproto->realdev_vid_map)) {
+wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
+}
Re: [ovs-dev] [PATCH] ofproto-dpif: Tighten up megaflow wildcard handling.
On Jun 19, 2013, at 9:21 AM, Ben Pfaff wrote: > Why does ipfix need to un-wildcard fields? Right. I was thinking it was more like NetFlow than sFlow. Since we're sampling, we don't need to match on the fields. > I don't understand the change in compose_output_action__(). It adds > the ECN bits to the mask but I don't see anything there that depends > on those bits. > > In do_xlate_actions(), why do OFPACT_SET_VLAN_VID and > OFPACT_SET_VLAN_PCP have new dependencies? They do not read the > fields that they add to the mask. In both of these cases, we're updating partial fields with a whole field "set", so I think the old value matters. As we discussed off-line, I sent v2 of the patch with the IPFIX changes removed. --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Reconfigure when IPFIX is enabled/disabled.
On Jun 19, 2013, at 9:21 AM, Ben Pfaff wrote: >> Since I just fixed IPFIX wildcarding in the previous patch, I'll >> probably fold this patch into that one. > > Why can't IPFIX handle wildcarding? Yeah, I'm dropping this patch. Thanks. --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Tighten up megaflow wildcard handling.
On Wed, Jun 19, 2013 at 10:03:56AM -0700, Justin Pettit wrote: > On Jun 19, 2013, at 9:21 AM, Ben Pfaff wrote: > > I don't understand the change in compose_output_action__(). It adds > > the ECN bits to the mask but I don't see anything there that depends > > on those bits. > > > > In do_xlate_actions(), why do OFPACT_SET_VLAN_VID and > > OFPACT_SET_VLAN_PCP have new dependencies? They do not read the > > fields that they add to the mask. > > In both of these cases, we're updating partial fields with a whole > field "set", so I think the old value matters. We need to generalize that principle. The kernel sets large groups of fields with a single action. For example, there is a single action to set the IPv4 source and destination address, TOS, TTL, IP protocol, and fragment bits (*), so any action that sets any one of those needs to add a dependency on the rest. (*) The kernel won't allow actually changing the IP protocol or fragment bits. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] ofproto-dpif: Tighten up megaflow wildcard handling.
On Wed, Jun 19, 2013 at 10:00:54AM -0700, Justin Pettit wrote: > A number of use-cases weren't handled properly when determining what can > be wildcarded for megaflows. This commit both catches additional fields > that cannot be wildcarded and loosens a few other cases. > > Bug #17979 > > Signed-off-by: Justin Pettit I think the point that I made in my email from a few minutes ago will require some big revisions here, so I'll skip reviewing until that's ready. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dpctl: Add mega flow support
On Tue, Jun 18, 2013 at 04:15:10PM -0700, Andy Zhou wrote: > Added support to allow mega flow specified and displayed. ovs-dpctl tool > is mainly used as debugging tool. > > This patch also implements the low level user space routines to send > and receive mega flow netlink messages. Those netlink suppor > routines are required for forthcoming user space mega flow patches. > > Added a unit test to test parsing and display of mega flows. > > Ethan contributed the ovs-dpctl mega flow output function. > > Co-authored-by: Ethan Jackson > Signed-off-by: Ethan Jackson > Signed-off-by: Andy Zhou I know there was an offline discussion of how to handle eth_type and specifically its absence when Ethernet II is not in use. I wasn't there for that discussion, can you explain the conclusions? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovs-xapi-sync: Make bridge-id caching logic easier to understand.
The previous code had many subtleties that were easy to miss. This code
is intended to be more obviously correct.
Signed-off-by: Ben Pfaff
---
.../usr_share_openvswitch_scripts_ovs-xapi-sync| 32 ++-
1 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
index 649ddbe..a19ebfb 100755
--- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
+++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
@@ -1,5 +1,5 @@
#!/usr/bin/python
-# Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
+# Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
@@ -313,25 +313,27 @@ def main():
new_bridges = {}
for row in idl.tables["Bridge"].rows.itervalues():
-if row.name in bridges:
-nbd = bridges[row.name]
-else:
-# New bridge.
+bridge_id = bridges.get(row.name)
+if bridge_id is None:
+# Configure the new bridge.
update_fail_mode(row)
update_in_band_mgmt(row)
-nbd = get_bridge_id(row.name)
-bridge_id = nbd
-if bridge_id is None:
-bridge_id = row.external_ids.get("xs-network-uuids")
-if bridge_id and len(bridge_id.split(";")) > 1:
-bridge_ids = bridge_id.split(";")
-bridge_id = get_single_bridge_id(bridge_ids, "")
+# Get the correct bridge_id, if we can.
+bridge_id = get_bridge_id(row.name)
+if bridge_id is None:
+xs_network_uuids = row.external_ids.get("xs-network-uuids")
+if xs_network_uuids:
+bridge_ids = xs_network_uuids.split(";")
+if len(bridge_ids) == 1:
+bridge_id = bridge_ids[0]
+else:
+bridge_id = get_single_bridge_id(bridge_ids,
+ row.name)
+set_external_id(row, "bridge-id", bridge_id)
if bridge_id is not None:
-set_external_id(row, "bridge-id", bridge_id.split(";")[0])
-
-new_bridges[row.name] = nbd
+new_bridges[row.name] = bridge_id
bridges = new_bridges
iface_by_name = {}
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-xapi-sync: Cache the bridge-id value for non nicira-bridge-id too.
On Tue, Jun 18, 2013 at 02:49:27PM -0700, Ben Pfaff wrote:
> On Tue, Jun 18, 2013 at 02:45:46PM -0700, Gurucharan Shetty wrote:
> > On Tue, Jun 18, 2013 at 2:35 PM, Ben Pfaff wrote:
> >
> > > On Tue, Jun 18, 2013 at 02:24:47PM -0700, Gurucharan Shetty wrote:
> > > > On Tue, Jun 18, 2013 at 1:59 PM, Ben Pfaff wrote:
> > > >
> > > > > On Tue, Jun 18, 2013 at 01:30:28PM -0700, Gurucharan Shetty wrote:
> > > > > > On Tue, Jun 18, 2013 at 11:05 AM, Ben Pfaff wrote:
> > > > > >
> > > > > > > On Mon, Jun 17, 2013 at 01:25:01AM -0700, Gurucharan Shetty wrote:
> > > > > > > > Currently we connect to xapi in case there are multiple
> > > > > > > > external_ids:xs-network-uuids to get the single bridge id
> > > everytime
> > > > > > > > we have a change in the database for all the interested columns
> > > in
> > > > > > > > ovs-xapi-sync. The xs-network-uuids value can also change
> > > whenever
> > > > > > > > new VLANs are added or deleted, which is a common use case. The
> > > > > > > > disadvantage with this approach is that we query XAPI more often
> > > > > > > > and set the bridge-id as "" if we don't get a valid response for
> > > > > > > > our query. This can take down the logical connectivity for all
> > > the
> > > > > > > > VMs on that xenserver.
> > > > > > > >
> > > > > > > > Instead of looking at the PIF records for all the
> > > xs-network-uuids,
> > > > > > > > we can instead just look at the xapi record which has the same
> > > bridge
> > > > > > > > name as the OVS bridge name and then cache its uuid. This value
> > > will
> > > > > > > > hold true till the OVS bridge is recreated in which case we will
> > > > > re-read
> > > > > > > > the value.
> > > > > > > >
> > > > > > > > Signed-off-by: Gurucharan Shetty
> > > > > > >
> > > > > > > I think that the tolerance for XAPI failures is incomplete,
> > > because we
> > > > > > > call update_fail_mode(), update_in_band_mgmt(), and
> > > > > > > get_bridge_id()
> > > > > > > only once for a bridge, even if XAPI fails to respond on the first
> > > > > > > attempt.
> > > > > > >
> > > > > > Yes. We can make some improvements. Do you mind, if I come up with a
> > > > > > separate patch
> > > > > > for this, since the current one talks about caching non
> > > nicira-bridge-id.
> > > > > > (get_bridge_id() gets
> > > > > > the nicira-bridge-id)
> > > > >
> > > > > OK.
> > > > >
> > > > > > > I am not sure why the set_external_id() call splits bridge_id on
> > > ';'.
> > > > > > > Can bridge_id contain ';' at this point?
> > > > > > >
> > > > > > The case wherein bridge-id can have ";" is if nicira-bridge-id has a
> > > ";".
> > > > > > If you feel, that is not a valid use case, I can get rid of it.
> > > > >
> > > > > I guess that it is existing code, so it is better not to change it,
> > > > > especially in an unrelated patch.
> > > > >
> > > > > > > I am not sure why bridge_id and bridge_id_cache are different
> > > > > > > variables. When do they have different values?
> > > > > > >
> > > > > > In case get_single_bridge_id() gets us a "", we don't want to cache
> > > it.
> > > > > > Hence 2 separate variables.
> > > > >
> > > > > I see. "" != None even though Python treats both as false.
> > > > >
> > > > > This code is really confusing. Every time I look at it, I get more
> > > > > confused.
> > > > >
> > > > > I am probably doing something stupid here again, but how about this
> > > > > version:
> > > > >
> > > > > new_bridges = {}
> > > > > for row in idl.tables["Bridge"].rows.itervalues():
> > > > > bridge_id = bridges.get(row.name)
> > > > > if bridge_id is None:
> > > > > # Configure the new bridge.
> > > > > update_fail_mode(row)
> > > > > update_in_band_mgmt(row)
> > > > >
> > > > > # Get the correct bridge_id, if we can.
> > > > > bridge_id = get_bridge_id(row.name)
> > > > > if bridge_id is None:
> > > > > xs_network_uuids =
> > > > > row.external_ids.get("xs-network-uuids")
> > > > > if xs_network_uuids:
> > > > > bridge_ids = xs_network_uuids.split(";")
> > > > > if len(bridge_ids) == 1:
> > > > > bridge_id = bridge_ids[0]
> > > > > else:
> > > > > bridge_id =
> > > get_single_bridge_id(bridge_ids,
> > > > > row.name)
> > > > >
> > > > So in this case, we will not be setting the external_ids:bridge_id as
> > > > "",
> > > > if get_single_bridge_id() does not get correct records from xapi and we
> > > > will simply retain the old value in the database. I guess, this was not
> > > > your intent?
> > >
> > > If our connection to XAPI is malfunctioning in some way, then I don't
> > > know whether it is better to clear the database fields or retain them.
> > > If you think that i
Re: [ovs-dev] [PATCH] ovs-xapi-sync: Make bridge-id caching logic easier to understand.
On Wed, Jun 19, 2013 at 10:26:14AM -0700, Ben Pfaff wrote: > The previous code had many subtleties that were easy to miss. This code > is intended to be more obviously correct. > > Signed-off-by: Ben Pfaff Oops, I generated this against a bad tree. I'll send out a fixed version in a minute. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCHv2] ovs-xapi-sync: Make bridge-id caching logic easier to understand.
The previous code had many subtleties that were easy to miss. This code
is intended to be more obviously correct.
Signed-off-by: Ben Pfaff
---
v1->v2: Regenerate against current master.
diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
index 46b729d..1c8ad51 100755
--- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
+++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
@@ -1,5 +1,5 @@
#!/usr/bin/python
-# Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
+# Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
@@ -294,27 +294,27 @@ def main():
new_bridges = {}
for row in idl.tables["Bridge"].rows.itervalues():
-if row.name in bridges:
-bridge_id_cache = bridges[row.name]
-else:
-# New bridge.
+bridge_id = bridges.get(row.name)
+if bridge_id is None:
+# Configure the new bridge.
update_fail_mode(row)
update_in_band_mgmt(row)
-bridge_id_cache = get_bridge_id(row.name)
-bridge_id = bridge_id_cache
-if bridge_id is None:
-bridge_id = row.external_ids.get("xs-network-uuids")
-if bridge_id and len(bridge_id.split(";")) > 1:
-bridge_ids = bridge_id.split(";")
-bridge_id = get_single_bridge_id(bridge_ids, row.name, "")
-if bridge_id:
-bridge_id_cache = bridge_id
+# Get the correct bridge_id, if we can.
+bridge_id = get_bridge_id(row.name)
+if bridge_id is None:
+xs_network_uuids = row.external_ids.get("xs-network-uuids")
+if xs_network_uuids:
+bridge_ids = xs_network_uuids.split(";")
+if len(bridge_ids) == 1:
+bridge_id = bridge_ids[0]
+else:
+bridge_id = get_single_bridge_id(bridge_ids,
+ row.name)
+set_external_id(row, "bridge-id", bridge_id)
if bridge_id is not None:
-set_external_id(row, "bridge-id", bridge_id.split(";")[0])
-
-new_bridges[row.name] = bridge_id_cache
+new_bridges[row.name] = bridge_id
bridges = new_bridges
iface_by_name = {}
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] ovs-xapi-sync: Make bridge-id caching logic easier to understand.
On Wed, Jun 19, 2013 at 10:32 AM, Ben Pfaff wrote:
> The previous code had many subtleties that were easy to miss. This code
> is intended to be more obviously correct.
>
> Signed-off-by: Ben Pfaff
>
I was about to send this. Looks good to me, please apply.
Thanks,
Guru
> ---
> v1->v2: Regenerate against current master.
>
> diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
> b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
> index 46b729d..1c8ad51 100755
> --- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
> +++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
> @@ -1,5 +1,5 @@
> #!/usr/bin/python
> -# Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> +# Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> #
> # Licensed under the Apache License, Version 2.0 (the "License");
> # you may not use this file except in compliance with the License.
> @@ -294,27 +294,27 @@ def main():
>
> new_bridges = {}
> for row in idl.tables["Bridge"].rows.itervalues():
> -if row.name in bridges:
> -bridge_id_cache = bridges[row.name]
> -else:
> -# New bridge.
> +bridge_id = bridges.get(row.name)
> +if bridge_id is None:
> +# Configure the new bridge.
> update_fail_mode(row)
> update_in_band_mgmt(row)
> -bridge_id_cache = get_bridge_id(row.name)
>
> -bridge_id = bridge_id_cache
> -if bridge_id is None:
> -bridge_id = row.external_ids.get("xs-network-uuids")
> -if bridge_id and len(bridge_id.split(";")) > 1:
> -bridge_ids = bridge_id.split(";")
> -bridge_id = get_single_bridge_id(bridge_ids, row.name,
> "")
> -if bridge_id:
> -bridge_id_cache = bridge_id
> +# Get the correct bridge_id, if we can.
> +bridge_id = get_bridge_id(row.name)
> +if bridge_id is None:
> +xs_network_uuids =
> row.external_ids.get("xs-network-uuids")
> +if xs_network_uuids:
> +bridge_ids = xs_network_uuids.split(";")
> +if len(bridge_ids) == 1:
> +bridge_id = bridge_ids[0]
> +else:
> +bridge_id = get_single_bridge_id(bridge_ids,
> + row.name)
> +set_external_id(row, "bridge-id", bridge_id)
>
> if bridge_id is not None:
> -set_external_id(row, "bridge-id", bridge_id.split(";")[0])
> -
> -new_bridges[row.name] = bridge_id_cache
> +new_bridges[row.name] = bridge_id
> bridges = new_bridges
>
> iface_by_name = {}
> --
> 1.7.2.5
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] ovs-xapi-sync: Make bridge-id caching logic easier to understand.
On Wed, Jun 19, 2013 at 10:38 AM, Ben Pfaff wrote: > On Wed, Jun 19, 2013 at 10:37:08AM -0700, Gurucharan Shetty wrote: > > On Wed, Jun 19, 2013 at 10:32 AM, Ben Pfaff wrote: > > > > > The previous code had many subtleties that were easy to miss. This > code > > > is intended to be more obviously correct. > > > > > > Signed-off-by: Ben Pfaff > > > > > I was about to send this. Looks good to me, please apply. > > I didn't test it. I guess you must have a test rig based on the > previous patches. Is it worth testing there? > I tested this. > > Thanks, > > Ben. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dpctl: Add mega flow support
We will continue to allow missing eth_type in the netlink attribute to imply Ethernet II type. 802.3 frames requires a specific eth_type attribute. With Mega flows, we further require a missing eth_type in the key attribute to have a exact match (ox) in the eth_type of the mask attribute (if present). In this patch, dpctl will output eth_type(0/0x) to indicate mssing eth_type key attribute with a exact match mask, and corresponding parsing logic to support this special case. IMHO, it will be better to be able to drop the '0' in the output (e.g. eth_type(/0x)). I plan to address this when we change how odp parser away from using sscanf(). Other suggestions are welcome. Please let me know if you need more information. --andy On Wed, Jun 19, 2013 at 10:20 AM, Ben Pfaff wrote: > On Tue, Jun 18, 2013 at 04:15:10PM -0700, Andy Zhou wrote: > > Added support to allow mega flow specified and displayed. ovs-dpctl tool > > is mainly used as debugging tool. > > > > This patch also implements the low level user space routines to send > > and receive mega flow netlink messages. Those netlink suppor > > routines are required for forthcoming user space mega flow patches. > > > > Added a unit test to test parsing and display of mega flows. > > > > Ethan contributed the ovs-dpctl mega flow output function. > > > > Co-authored-by: Ethan Jackson > > Signed-off-by: Ethan Jackson > > Signed-off-by: Andy Zhou > > I know there was an offline discussion of how to handle eth_type and > specifically its absence when Ethernet II is not in use. I wasn't > there for that discussion, can you explain the conclusions? > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovs-xapi-sync: Increase the tolerance level for xapi failures for bridges.
Specifically for the case, where we know that a bridge record
should exist in xapi, if we don't get any bridge records, log
the failure and retry again later.
Signed-off-by: Gurucharan Shetty
---
.../usr_share_openvswitch_scripts_ovs-xapi-sync| 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
index 0633cbb..1622c18 100755
--- a/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
+++ b/xenserver/usr_share_openvswitch_scripts_ovs-xapi-sync
@@ -103,15 +103,21 @@ def get_single_bridge_id(bridge_ids, br_name,
default=None):
xapi_down = True
return default
+
# By default, the "bridge-id" external id in the Bridge table is the
# same as "xs-network-uuids". This may be overridden by defining a
# "nicira-bridge-id" key in the "other_config" field of the network
# record of XAPI. If nicira-bridge-id is undefined returns default.
# On error returns None.
def get_bridge_id(br_name, default=None):
+global xapi_down
+
rec = get_network_by_bridge(br_name)
if rec:
return rec['other_config'].get('nicira-bridge-id', default)
+
+vlog.warn("Failed to get Xapi record for %s." %(br_name))
+xapi_down = True
return None
@@ -185,8 +191,12 @@ def set_external_id(row, key, value):
# XenServer does not call interface-reconfigure on internal networks,
# which is where the fail-mode would normally be set.
def update_fail_mode(row):
+global xapi_down
+
rec = get_network_by_bridge(row.name)
if not rec:
+vlog.warn("Failed to get Xapi record for %s." %(row.name))
+xapi_down = True
return
fail_mode = rec['other_config'].get('vswitch-controller-fail-mode')
@@ -207,8 +217,12 @@ def update_fail_mode(row):
def update_in_band_mgmt(row):
+global xapi_down
+
rec = get_network_by_bridge(row.name)
if not rec:
+vlog.warn("Failed to get Xapi record for %s." %(row.name))
+xapi_down = True
return
dib = rec['other_config'].get('vswitch-disable-in-band')
@@ -299,9 +313,11 @@ def main():
# Configure the new bridge.
update_fail_mode(row)
update_in_band_mgmt(row)
-
# Get the correct bridge_id, if we can.
bridge_id = get_bridge_id(row.name)
+if xapi_down:
+# Try again later.
+continue
if bridge_id is None:
xs_network_uuids = row.external_ids.get("xs-network-uuids")
if xs_network_uuids:
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] ovs-xapi-sync: Make bridge-id caching logic easier to understand.
On Wed, Jun 19, 2013 at 10:37:08AM -0700, Gurucharan Shetty wrote: > On Wed, Jun 19, 2013 at 10:32 AM, Ben Pfaff wrote: > > > The previous code had many subtleties that were easy to miss. This code > > is intended to be more obviously correct. > > > > Signed-off-by: Ben Pfaff > > > I was about to send this. Looks good to me, please apply. I didn't test it. I guess you must have a test rig based on the previous patches. Is it worth testing there? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] ovs-xapi-sync: Make bridge-id caching logic easier to understand.
On Wed, Jun 19, 2013 at 10:39:43AM -0700, Gurucharan Shetty wrote: > On Wed, Jun 19, 2013 at 10:38 AM, Ben Pfaff wrote: > > > On Wed, Jun 19, 2013 at 10:37:08AM -0700, Gurucharan Shetty wrote: > > > On Wed, Jun 19, 2013 at 10:32 AM, Ben Pfaff wrote: > > > > > > > The previous code had many subtleties that were easy to miss. This > > code > > > > is intended to be more obviously correct. > > > > > > > > Signed-off-by: Ben Pfaff > > > > > > > I was about to send this. Looks good to me, please apply. > > > > I didn't test it. I guess you must have a test rig based on the > > previous patches. Is it worth testing there? > > > I tested this. That was fast. Thanks, I pushed this to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv2] ovs-xapi-sync: Make bridge-id caching logic easier to understand.
On Wed, Jun 19, 2013 at 10:50 AM, Ben Pfaff wrote: > On Wed, Jun 19, 2013 at 10:39:43AM -0700, Gurucharan Shetty wrote: > > On Wed, Jun 19, 2013 at 10:38 AM, Ben Pfaff wrote: > > > > > On Wed, Jun 19, 2013 at 10:37:08AM -0700, Gurucharan Shetty wrote: > > > > On Wed, Jun 19, 2013 at 10:32 AM, Ben Pfaff wrote: > > > > > > > > > The previous code had many subtleties that were easy to miss. This > > > code > > > > > is intended to be more obviously correct. > > > > > > > > > > Signed-off-by: Ben Pfaff > > > > > > > > > I was about to send this. Looks good to me, please apply. > > > > > > I didn't test it. I guess you must have a test rig based on the > > > previous patches. Is it worth testing there? > > > > > I tested this. > > That was fast. > I was testing the version you sent yesterday at the time you sent this one out. > Thanks, I pushed this to master. > Thanks ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [threads 06/11] New function ovs_strerror() as a thread-safe replacement for strerror().
Signed-off-by: Ben Pfaff
---
configure.ac |1 +
lib/util.c | 48 +---
lib/util.h |3 ++-
3 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/configure.ac b/configure.ac
index 6db4a00..734b2ff 100644
--- a/configure.ac
+++ b/configure.ac
@@ -45,6 +45,7 @@ AC_SEARCH_LIBS([pow], [m])
AC_SEARCH_LIBS([clock_gettime], [rt])
AC_SEARCH_LIBS([timer_create], [rt])
AC_SEARCH_LIBS([pthread_sigmask], [pthread])
+AC_FUNC_STRERROR_R
OVS_CHECK_ESX
OVS_CHECK_COVERAGE
diff --git a/lib/util.c b/lib/util.c
index 2a06461..6ee8b5c 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -18,6 +18,7 @@
#include "util.h"
#include
#include
+#include
#include
#include
#include
@@ -45,6 +46,9 @@ const char *subprogram_name = "";
/* --version option output. */
static char *program_version;
+/* Buffer used by ovs_strerror(). */
+DEFINE_PER_THREAD_DATA(struct { char s[128]; }, strerror_buffer, { "" });
+
void
ovs_assert_failure(const char *where, const char *function,
const char *condition)
@@ -307,19 +311,41 @@ ovs_error_valist(int err_no, const char *format, va_list
args)
const char *
ovs_retval_to_string(int retval)
{
-static char unknown[48];
+return (!retval ? ""
+: retval == EOF ? "End of file"
+: ovs_strerror(retval));
+}
-if (!retval) {
-return "";
-}
-if (retval > 0) {
-return strerror(retval);
-}
-if (retval == EOF) {
-return "End of file";
+const char *
+ovs_strerror(int error)
+{
+enum { BUFSIZE = sizeof strerror_buffer_get()->s };
+int save_errno;
+char *buffer;
+char *s;
+
+save_errno = errno;
+buffer = strerror_buffer_get()->s;
+
+#if STRERROR_R_CHAR_P
+/* GNU style strerror_r() might return an immutable static string, or it
+ * might write and return 'buffer', but in either case we can pass the
+ * returned string directly to the caller. */
+s = strerror_r(error, buffer, BUFSIZE);
+#else /* strerror_r() returns an int. */
+s = buffer;
+if (strerror_r(error, buffer, BUFSIZE)) {
+/* strerror_r() is only allowed to fail on ERANGE (because the buffer
+ * is too short). We don't check the actual failure reason because
+ * POSIX requires strerror_r() to return the error but old glibc
+ * (before 2.13) returns -1 and sets errno. */
+snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
}
-snprintf(unknown, sizeof unknown, "***unknown return value: %d***",
retval);
-return unknown;
+#endif
+
+errno = save_errno;
+
+return s;
}
/* Sets global "program_name" and "program_version" variables. Should
diff --git a/lib/util.h b/lib/util.h
index f5589e3..d7fbe09 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -213,6 +213,7 @@ void ovs_error(int err_no, const char *format, ...)
PRINTF_FORMAT(2, 3);
void ovs_error_valist(int err_no, const char *format, va_list)
PRINTF_FORMAT(2, 0);
const char *ovs_retval_to_string(int);
+const char *ovs_strerror(int);
void ovs_hex_dump(FILE *, const void *, size_t, uintptr_t offset, bool ascii);
bool str_to_int(const char *, int base, int *);
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [threads 08/11] random: Make thread-safe.
Signed-off-by: Ben Pfaff
---
lib/random.c | 26 +-
1 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/lib/random.c b/lib/random.c
index 45d428c..2572c1e 100644
--- a/lib/random.c
+++ b/lib/random.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -22,6 +22,8 @@
#include
#include "entropy.h"
+#include "hash.h"
+#include "ovs-thread.h"
#include "timeval.h"
#include "util.h"
@@ -37,21 +39,25 @@
* cryptographic-quality randomness. */
/* Current random state. */
-static uint32_t seed;
+DEFINE_PER_THREAD_DATA(uint32_t, seed, 0);
static uint32_t random_next(void);
void
random_init(void)
{
-while (!seed) {
+uint32_t *seedp = seed_get();
+while (!*seedp) {
struct timeval tv;
uint32_t entropy;
+pthread_t self;
xgettimeofday(&tv);
get_entropy_or_die(&entropy, 4);
+self = pthread_self();
-seed = tv.tv_sec ^ tv.tv_usec ^ entropy;
+*seedp = (tv.tv_sec ^ tv.tv_usec ^ entropy
+ ^ hash_bytes(&self, sizeof self, 0));
}
}
@@ -59,7 +65,7 @@ void
random_set_seed(uint32_t seed_)
{
ovs_assert(seed_);
-seed = seed_;
+*seed_get() = seed_;
}
void
@@ -120,9 +126,11 @@ random_range(int max)
static uint32_t
random_next(void)
{
-seed ^= seed << 13;
-seed ^= seed >> 17;
-seed ^= seed << 5;
+uint32_t *seedp = seed_get__();
-return seed;
+*seedp ^= *seedp << 13;
+*seedp ^= *seedp >> 17;
+*seedp ^= *seedp << 5;
+
+return *seedp;
}
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [threads 00/11] basic thread support library
This series has two purposes: * Add a basic thread support library to the tree. * Get rid of all calls to C library functions that POSIX describes as inherently unsafe in a multithreaded program, and then add a make-time check that flags an error if any calls get reintroduced. Ben Pfaff (11): ovs-thread: New module, initially just with pthreads wrapper functions. ovs-thread: Add per-thread data support. ovs-atomic: New library for atomic operations. ovs-thread: Add support for convenient once-only initializers. ovs-thread: Add support for various thread-related assertions. New function ovs_strerror() as a thread-safe replacement for strerror(). Replace all uses of strerror() by ovs_strerror(), for thread safety. random: Make thread-safe. Use random_*() instead of rand(), for thread safety. sparse: Remove prototypes for thread-unsafe functions from headers. Makefile: Blacklist functions that threaded programs cannot use safely. Makefile.am | 26 ++- build-aux/thread-safety-blacklist | 90 + configure.ac |8 +- include/sparse/automake.mk|1 + include/sparse/math.h |5 +- include/sparse/netinet/in.h |3 +- include/sparse/pthread.h | 61 ++ lib/automake.mk |9 + lib/command-line.c|4 +- lib/compiler.h| 36 - lib/daemon.c | 43 ++--- lib/dpif-linux.c |9 +- lib/dpif-netdev.c |4 +- lib/dpif.c| 23 ++- lib/entropy.c |4 +- lib/fatal-signal.c|6 +- lib/json.c|5 +- lib/jsonrpc.c |7 +- lib/learning-switch.c |6 +- lib/lockfile.c| 11 +- lib/netdev-bsd.c | 32 ++-- lib/netdev-dummy.c|5 +- lib/netdev-linux.c| 63 --- lib/netdev.c | 10 +- lib/netlink-notifier.c|7 +- lib/netlink-socket.c | 26 ++-- lib/ofp-errors.c | 10 +- lib/ofp-version-opt.c |3 + lib/ovs-atomic-c11.h | 62 ++ lib/ovs-atomic-gcc4+.c| 68 +++ lib/ovs-atomic-gcc4+.h| 267 ++ lib/ovs-atomic-gcc4.7+.h | 141 ++ lib/ovs-atomic-pthreads.c | 61 ++ lib/ovs-atomic-pthreads.h | 157 lib/ovs-atomic.h | 250 + lib/ovs-thread.c | 194 +++ lib/ovs-thread.h | 370 + lib/poll-loop.c |4 +- lib/process.c | 10 +- lib/random.c | 26 ++- lib/rconn.c |8 +- lib/reconnect.c |6 +- lib/rtbsd.c |8 +- lib/sflow_agent.c |3 +- lib/signals.c |4 +- lib/socket-util.c | 40 ++-- lib/stream-fd.c |4 +- lib/stream-ssl.c | 19 +- lib/stream-tcp.c |4 +- lib/stream-unix.c |7 +- lib/timeval.c |8 +- lib/unixctl.c |2 +- lib/util.c| 57 +-- lib/util.h|3 +- lib/vconn-stream.c|6 +- lib/vconn.c |2 +- lib/vlandev.c |6 +- lib/vlog.c|4 +- lib/worker.c |8 +- m4/openvswitch.m4 | 42 - ofproto/collectors.c |6 +- ofproto/connmgr.c |6 +- ofproto/in-band.c | 12 +- ofproto/ofproto-dpif-sflow.c |2 +- ofproto/ofproto-dpif.c| 14 +- ofproto/ofproto.c | 24 ++-- ovsdb/jsonrpc-server.c|6 +- ovsdb/ovsdb-server.c |7 +- tests/automake.mk |5 + tests/library.at |4 + tests/test-atomic.c | 94 ++ tests/test-classifier.c | 25 ++-- tests/test-hindex.c |3 +- tests/test-hmap.c |5 +- tests/test-netflow.c |2 +- tests/test-sflow.c|2 +- tests/test-util.c |4 +- tests/test-vconn.c| 12 +- utilities/ovs-controller.c|2 +- utilities/ovs-dpctl.c |8 +- utilities/ovs-ofctl.c | 10 +- vswitchd/bridge.c | 18 +- vswitchd/ovs-vswitchd.c |4 +- vswitchd/system-stats.c | 20 ++- vswitchd/xenserver.c
[ovs-dev] [threads 03/11] ovs-atomic: New library for atomic operations.
This library should prove useful for the threading changes coming up. The following commit introduces one (very simple) user. Signed-off-by: Ben Pfaff --- configure.ac |6 +- lib/automake.mk |7 + lib/ovs-atomic-c11.h | 62 +++ lib/ovs-atomic-gcc4+.c| 68 lib/ovs-atomic-gcc4+.h| 267 + lib/ovs-atomic-gcc4.7+.h | 141 lib/ovs-atomic-pthreads.c | 61 ++ lib/ovs-atomic-pthreads.h | 157 ++ lib/ovs-atomic.h | 250 ++ m4/openvswitch.m4 | 21 tests/automake.mk |5 + tests/library.at |4 + tests/test-atomic.c | 94 13 files changed, 1142 insertions(+), 1 deletions(-) create mode 100644 lib/ovs-atomic-c11.h create mode 100644 lib/ovs-atomic-gcc4+.c create mode 100644 lib/ovs-atomic-gcc4+.h create mode 100644 lib/ovs-atomic-gcc4.7+.h create mode 100644 lib/ovs-atomic-pthreads.c create mode 100644 lib/ovs-atomic-pthreads.h create mode 100644 lib/ovs-atomic.h create mode 100644 tests/test-atomic.c diff --git a/configure.ac b/configure.ac index a6c68a9..6db4a00 100644 --- a/configure.ac +++ b/configure.ac @@ -64,7 +64,7 @@ AC_CHECK_MEMBERS([struct stat.st_mtim.tv_nsec, struct stat.st_mtimensec], [], [], [[#include ]]) AC_CHECK_MEMBERS([struct ifreq.ifr_flagshigh], [], [], [[#include ]]) AC_CHECK_FUNCS([mlockall strnlen getloadavg statvfs getmntent_r]) -AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h]) +AC_CHECK_HEADERS([mntent.h sys/statvfs.h linux/types.h linux/if_ether.h stdatomic.h]) AC_CHECK_HEADERS([net/if_mib.h], [], [], [[#include #include ]]) @@ -81,6 +81,10 @@ OVS_CHECK_GROFF OVS_CHECK_GNU_MAKE OVS_CHECK_CACHE_TIME OVS_CHECK___THREAD +OVS_CHECK_ATOMIC_ALWAYS_LOCK_FREE(1) +OVS_CHECK_ATOMIC_ALWAYS_LOCK_FREE(2) +OVS_CHECK_ATOMIC_ALWAYS_LOCK_FREE(4) +OVS_CHECK_ATOMIC_ALWAYS_LOCK_FREE(8) OVS_ENABLE_OPTION([-Wall]) OVS_ENABLE_OPTION([-Wno-sign-compare]) diff --git a/lib/automake.mk b/lib/automake.mk index c6de4fe..c8550c3 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -121,6 +121,13 @@ lib_libopenvswitch_a_SOURCES = \ lib/ofp-version-opt.c \ lib/ofpbuf.c \ lib/ofpbuf.h \ + lib/ovs-atomic-c11.h \ + lib/ovs-atomic-gcc4+.c \ + lib/ovs-atomic-gcc4+.h \ + lib/ovs-atomic-gcc4.7+.h \ + lib/ovs-atomic-pthreads.c \ + lib/ovs-atomic-pthreads.h \ + lib/ovs-atomic.h \ lib/ovs-thread.c \ lib/ovs-thread.h \ lib/ovsdb-data.c \ diff --git a/lib/ovs-atomic-c11.h b/lib/ovs-atomic-c11.h new file mode 100644 index 000..26fa25e --- /dev/null +++ b/lib/ovs-atomic-c11.h @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2013 Nicira, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/* This header implements atomic operation primitives on compilers that + * have built-in support for C11 */ +#ifndef IN_OVS_ATOMIC_H +#error "This header should only be included indirectly via ovs-atomic.h." +#endif + +#include + +/* Nonstandard atomic types. */ +typedef _Atomic uint8_t atomic_uint8_t; +typedef _Atomic uint16_t atomic_uint16_t; +typedef _Atomic uint32_t atomic_uint32_t; +typedef _Atomic uint64_t atomic_uint64_t; + +typedef _Atomic int8_tatomic_int8_t; +typedef _Atomic int16_t atomic_int16_t; +typedef _Atomic int32_t atomic_int32_t; +typedef _Atomic int64_t atomic_int64_t; + +#define atomic_read(SRC, DST) \ +atomic_read_explicit(SRC, DST, memory_order_seq_cst) +#define atomic_read_explicit(SRC, DST, ORDER) \ +(*(DST) = atomic_load_explicit(SRC, ORDER), \ + (void) 0) + +#define atomic_add(RMW, ARG, ORIG) \ +atomic_add_explicit(RMW, ARG, ORIG, memory_order_seq_cst) +#define atomic_sub(RMW, ARG, ORIG) \ +atomic_sub_explicit(RMW, ARG, ORIG, memory_order_seq_cst) +#define atomic_or(RMW, ARG, ORIG) \ +atomic_or_explicit(RMW, ARG, ORIG, memory_order_seq_cst) +#define atomic_xor(RMW, ARG, ORIG) \ +atomic_xor_explicit(RMW, ARG, ORIG, memory_order_seq_cst) +#define atomic_and(RMW, ARG, ORIG) \ +atomic_and_explicit(RMW, ARG, ORIG, memory_order_seq_cst) + +#define atomic_add_explicit(RMW, ARG, ORIG, ORDER) \ +(*(ORIG) = atomic_fetch_add_explicit(RMW, ARG, ORDER), (void) 0) +#define atomic_sub_explicit(RMW, ARG, ORIG, ORDER) \ +(*(ORIG) = atomic_f
[ovs-dev] [threads 01/11] ovs-thread: New module, initially just with pthreads wrapper functions.
The only tricky part here is that I'm throwing in annotations to allow
"sparse" to report unbalanced locking.
Signed-off-by: Ben Pfaff
---
include/sparse/automake.mk |1 +
include/sparse/pthread.h | 61 +
lib/automake.mk|2 +
lib/compiler.h | 36 ++-
lib/ovs-thread.c | 108
lib/ovs-thread.h | 89
6 files changed, 296 insertions(+), 1 deletions(-)
create mode 100644 include/sparse/pthread.h
create mode 100644 lib/ovs-thread.c
create mode 100644 lib/ovs-thread.h
diff --git a/include/sparse/automake.mk b/include/sparse/automake.mk
index 1a77500..45ae1f5 100644
--- a/include/sparse/automake.mk
+++ b/include/sparse/automake.mk
@@ -4,5 +4,6 @@ noinst_HEADERS += \
include/sparse/math.h \
include/sparse/netinet/in.h \
include/sparse/netinet/ip6.h \
+include/sparse/pthread.h \
include/sparse/sys/socket.h \
include/sparse/sys/wait.h
diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
new file mode 100644
index 000..1e54e95
--- /dev/null
+++ b/include/sparse/pthread.h
@@ -0,0 +1,61 @@
+/*
+ * Copyright (c) 2013 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef __CHECKER__
+#error "Use this header only with sparse. It is not a correct implementation."
+#endif
+
+#include_next
+
+#include "compiler.h"
+
+int pthread_mutex_lock(pthread_mutex_t *mutex) OVS_ACQUIRES(mutex);
+int pthread_mutex_unlock(pthread_mutex_t *mutex) OVS_RELEASES(mutex);
+
+int pthread_rwlock_rdlock(pthread_rwlock_t *rwlock) OVS_ACQUIRES(rwlock);
+int pthread_rwlock_wrlock(pthread_rwlock_t *rwlock) OVS_ACQUIRES(rwlock);
+int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) OVS_RELEASES(rwlock);
+
+int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
+OVS_MUST_HOLD(mutex);
+
+#define pthread_mutex_trylock(MUTEX)\
+({ \
+int retval = pthread_mutex_trylock(mutex); \
+if (!retval) { \
+OVS_ACQUIRE(MUTEX); \
+} \
+retval; \
+})
+
+#define pthread_rwlock_tryrdlock(RWLOCK)\
+({ \
+int retval = pthread_rwlock_tryrdlock(rwlock); \
+if (!retval) { \
+OVS_ACQUIRE(RWLOCK);\
+} \
+retval; \
+})
+#define pthread_rwlock_trywrlock(RWLOCK)\
+({ \
+int retval = pthread_rwlock_trywrlock(rwlock); \
+if (!retval) { \
+OVS_ACQUIRE(RWLOCK);\
+} \
+retval; \
+})
+
+
diff --git a/lib/automake.mk b/lib/automake.mk
index eec71dd..c6de4fe 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -121,6 +121,8 @@ lib_libopenvswitch_a_SOURCES = \
lib/ofp-version-opt.c \
lib/ofpbuf.c \
lib/ofpbuf.h \
+ lib/ovs-thread.c \
+ lib/ovs-thread.h \
lib/ovsdb-data.c \
lib/ovsdb-data.h \
lib/ovsdb-error.c \
diff --git a/lib/compiler.h b/lib/compiler.h
index 760389d..f3cbe96 100644
--- a/lib/compiler.h
+++ b/lib/compiler.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -41,6 +41,40 @@
#define OVS_UNLIKELY(CONDITION) (!!(CONDITION))
#endif
+#ifdef __CHECKER__
+/* "sparse" annotations for mutexes and mutex-like constructs.
+ *
+ * In a function prototype, OVS_ACQUIRES(MUTEX) indicates that the function
+ * must be called without MUTEX acquired and that it returns with MUTEX
+ * acquired. OVS_RELEASES(MUTEX) indicates the reverse. OVS_MUST_HOLD
+ * indicates that the function must be called with MUTEX acquired b
[ovs-dev] [threads 05/11] ovs-thread: Add support for various thread-related assertions.
Signed-off-by: Ben Pfaff
---
lib/command-line.c|4 ++-
lib/daemon.c | 12 +++-
lib/ofp-version-opt.c |3 ++
lib/ovs-thread.c | 68 +
lib/ovs-thread.h | 11
lib/process.c |4 +++
lib/timeval.c |2 +
lib/util.c|4 +++
8 files changed, 100 insertions(+), 8 deletions(-)
diff --git a/lib/command-line.c b/lib/command-line.c
index 70b1f4d..7800c0b 100644
--- a/lib/command-line.c
+++ b/lib/command-line.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -19,6 +19,7 @@
#include
#include
#include
+#include "ovs-thread.h"
#include "util.h"
#include "vlog.h"
@@ -110,6 +111,7 @@ proctitle_init(int argc, char **argv)
{
int i;
+assert_single_threaded();
if (!argc || !argv[0]) {
/* This situation should never occur, but... */
return;
diff --git a/lib/daemon.c b/lib/daemon.c
index e12bc14..56b32b8 100644
--- a/lib/daemon.c
+++ b/lib/daemon.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -29,6 +29,7 @@
#include "fatal-signal.h"
#include "dirs.h"
#include "lockfile.h"
+#include "ovs-thread.h"
#include "process.h"
#include "socket-util.h"
#include "timeval.h"
@@ -88,6 +89,7 @@ make_pidfile_name(const char *name)
void
set_pidfile(const char *name)
{
+assert_single_threaded();
free(pidfile);
pidfile = make_pidfile_name(name);
}
@@ -280,9 +282,7 @@ daemonize(void)
pid_t
fork_and_clean_up(void)
{
-pid_t pid;
-
-pid = fork();
+pid_t pid = xfork();
if (pid > 0) {
/* Running in parent process. */
fatal_signal_fork();
@@ -290,10 +290,7 @@ fork_and_clean_up(void)
/* Running in child process. */
time_postfork();
lockfile_postfork();
-} else {
-VLOG_FATAL("fork failed (%s)", strerror(errno));
}
-
return pid;
}
@@ -504,6 +501,7 @@ close_standard_fds(void)
void
daemonize_start(void)
{
+assert_single_threaded();
daemonize_fd = -1;
if (detach) {
diff --git a/lib/ofp-version-opt.c b/lib/ofp-version-opt.c
index 35d79e6..84e83d8 100644
--- a/lib/ofp-version-opt.c
+++ b/lib/ofp-version-opt.c
@@ -1,6 +1,7 @@
#include
#include "ofp-util.h"
#include "ofp-version-opt.h"
+#include "ovs-thread.h"
#include "vlog.h"
#include "dynamic-string.h"
@@ -17,12 +18,14 @@ get_allowed_ofp_versions(void)
void
set_allowed_ofp_versions(const char *string)
{
+assert_single_threaded();
allowed_versions = ofputil_versions_from_string(string);
}
void
mask_allowed_ofp_versions(uint32_t bitmap)
{
+assert_single_threaded();
allowed_versions &= bitmap;
}
diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index d69ec5e..56303f7 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -17,7 +17,12 @@
#include
#include "ovs-thread.h"
#include
+#include
+#include
+#include
#include "compiler.h"
+#include "poll-loop.h"
+#include "socket-util.h"
#include "util.h"
#ifdef __CHECKER__
@@ -26,6 +31,18 @@
* cut-and-paste. Since "sparse" is just a checker, not a compiler, it
* doesn't matter that we don't define them. */
#else
+#include "vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(ovs_thread);
+
+/* If there is a reason that we cannot fork anymore (unless the fork will be
+ * immediately followed by an exec), then this points to a string that
+ * explains why. */
+static const char *must_not_fork;
+
+/* True if we created any threads beyond the main initial thread. */
+static bool multithreaded;
+
#define XPTHREAD_FUNC1(FUNCTION, PARAM1)\
void\
x##FUNCTION(PARAM1 arg1)\
@@ -100,6 +117,9 @@ xpthread_create(pthread_t *threadp, pthread_attr_t *attr,
pthread_t thread;
int error;
+forbid_forking("multiple threads exist");
+multithreaded = true;
+
error = pthread_create(threadp ? threadp : &thread, attr, start, arg);
if (error) {
ovs_abort(error, "pthread_create failed");
@@ -123,4 +143,52 @@ ovsthread_once_done(struct ovsthread_once *once)
atomic_store(&once->done, true);
xpthread_mutex_unlock(&once->mutex);
}
+
+/* Asserts that the process has not yet created any threads (beyond the initial
+ * thread). */
+void
+(assert_single_threaded)(const char *where)
+{
+if (multithreaded) {
+VLOG_FATAL("%s: attempted operation not allowed when multithreaded",
+ where);
+}
+}
+
+/* Forks the cu
[ovs-dev] [threads 09/11] Use random_*() instead of rand(), for thread safety.
None of these test programs are threaded, but has little cost and means
that "grep" doesn't turn up any instances of these thread-unsafe functions
in our tree.
Signed-off-by: Ben Pfaff
---
tests/test-classifier.c | 25 +
tests/test-hindex.c |3 ++-
tests/test-hmap.c |5 +++--
tests/test-util.c |4 ++--
4 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/tests/test-classifier.c b/tests/test-classifier.c
index f616494..981251b 100644
--- a/tests/test-classifier.c
+++ b/tests/test-classifier.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -34,6 +34,7 @@
#include "flow.h"
#include "ofp-util.h"
#include "packets.h"
+#include "random.h"
#include "unaligned.h"
#undef NDEBUG
@@ -404,7 +405,7 @@ compare_classifiers(struct classifier *cls, struct tcls
*tcls)
struct flow flow;
unsigned int x;
-x = rand () % N_FLOW_VALUES;
+x = random_uint32() % N_FLOW_VALUES;
memset(&flow, 0, sizeof flow);
flow.nw_src = nw_src_values[get_value(&x, N_NW_SRC_VALUES)];
flow.nw_dst = nw_dst_values[get_value(&x, N_NW_DST_VALUES)];
@@ -579,7 +580,7 @@ static void
shuffle(unsigned int *p, size_t n)
{
for (; n > 1; n--, p++) {
-unsigned int *q = &p[rand() % n];
+unsigned int *q = &p[random_range(n)];
unsigned int tmp = *p;
*p = *q;
*q = tmp;
@@ -590,7 +591,7 @@ static void
shuffle_u32s(uint32_t *p, size_t n)
{
for (; n > 1; n--, p++) {
-uint32_t *q = &p[rand() % n];
+uint32_t *q = &p[random_range(n)];
uint32_t tmp = *p;
*p = *q;
*q = tmp;
@@ -879,7 +880,7 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char
*argv[] OVS_UNUSED)
int i;
do {
-wcf = rand() & ((1u << CLS_N_FIELDS) - 1);
+wcf = random_uint32() & ((1u << CLS_N_FIELDS) - 1);
value_mask = ~wcf & ((1u << CLS_N_FIELDS) - 1);
} while ((1 << count_ones(value_mask)) < N_RULES);
@@ -887,10 +888,10 @@ test_many_rules_in_one_table(int argc OVS_UNUSED, char
*argv[] OVS_UNUSED)
tcls_init(&tcls);
for (i = 0; i < N_RULES; i++) {
-unsigned int priority = rand();
+unsigned int priority = random_uint32();
do {
-value_pats[i] = rand() & value_mask;
+value_pats[i] = random_uint32() & value_mask;
} while (array_contains(value_pats, i, value_pats[i]));
rules[i] = make_rule(wcf, priority, value_pats[i]);
@@ -928,7 +929,7 @@ test_many_rules_in_n_tables(int n_tables)
assert(n_tables < 10);
for (i = 0; i < n_tables; i++) {
do {
-wcfs[i] = rand() & ((1u << CLS_N_FIELDS) - 1);
+wcfs[i] = random_uint32() & ((1u << CLS_N_FIELDS) - 1);
} while (array_contains(wcfs, i, wcfs[i]));
}
@@ -937,7 +938,7 @@ test_many_rules_in_n_tables(int n_tables)
struct classifier cls;
struct tcls tcls;
-srand(iteration);
+random_set_seed(iteration + 1);
for (i = 0; i < MAX_RULES; i++) {
priorities[i] = i * 129;
}
@@ -949,8 +950,8 @@ test_many_rules_in_n_tables(int n_tables)
for (i = 0; i < MAX_RULES; i++) {
struct test_rule *rule;
unsigned int priority = priorities[i];
-int wcf = wcfs[rand() % n_tables];
-int value_pat = rand() & ((1u << CLS_N_FIELDS) - 1);
+int wcf = wcfs[random_range(n_tables)];
+int value_pat = random_uint32() & ((1u << CLS_N_FIELDS) - 1);
rule = make_rule(wcf, priority, value_pat);
tcls_insert(&tcls, rule);
classifier_insert(&cls, &rule->cls_rule);
@@ -963,7 +964,7 @@ test_many_rules_in_n_tables(int n_tables)
struct test_rule *target;
struct cls_cursor cursor;
-target = clone_rule(tcls.rules[rand() % tcls.n_rules]);
+target = clone_rule(tcls.rules[random_uint32() % tcls.n_rules]);
cls_cursor_init(&cursor, &cls, &target->cls_rule);
CLS_CURSOR_FOR_EACH_SAFE (rule, next_rule, cls_rule, &cursor) {
diff --git a/tests/test-hindex.c b/tests/test-hindex.c
index b5fe9f0..7a3ef72 100644
--- a/tests/test-hindex.c
+++ b/tests/test-hindex.c
@@ -21,6 +21,7 @@
#include "hindex.h"
#include
#include "hash.h"
+#include "random.h"
#include "util.h"
#undef NDEBUG
@@ -108,7 +109,7 @@ static void
shuffle(int *p, size_t n)
{
for (; n > 1; n--, p++) {
-int *q = &p[rand() % n];
+int *q = &p[random_range(n)];
int tmp = *p;
*p = *q;
*q = tmp;
diff --git a/tests/test-hmap.c b/tests/test-hmap.c
index c202ea
[ovs-dev] [threads 11/11] Makefile: Blacklist functions that threaded programs cannot use safely.
Some functions that POSIX says cannot be used safely in multithreaded programs are not on the initial blacklist: - getenv() should be safe in real implementations in the absence of changes to the environment. (putenv() and setenv() are blacklisted.) - We only use getopt() before spawning extra threads, and I expect this to continue to be true. Signed-off-by: Ben Pfaff --- Makefile.am | 14 ++ build-aux/thread-safety-blacklist | 90 + 2 files changed, 104 insertions(+), 0 deletions(-) create mode 100644 build-aux/thread-safety-blacklist diff --git a/Makefile.am b/Makefile.am index 488aed2..08aea0f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -210,6 +210,20 @@ check-assert-h-usage: fi .PHONY: check-assert-h-usage +ALL_LOCAL += thread-safety-check +thread-safety-check: + @if test -e '$(srcdir)'/.git && (git --version) >/dev/null 2>&1 && \ + grep -n -f '$(srcdir)'/build-aux/thread-safety-blacklist \ + `git ls-files '$(srcdir)' | grep '\.[ch]$$' \ +| $(EGREP) -v '^datapath|^lib/sflow|^third-party'` \ + | $(EGREP) -v ':[]*/?\*'; \ + then \ + echo "See above for list of calls to functions that are"; \ + echo "blacklisted due to thread safety issues"; \ + exit 1; \ + fi +EXTRA_DIST += build-aux/thread-safety-blacklist + if HAVE_GROFF ALL_LOCAL += manpage-check manpage-check: $(man_MANS) $(dist_man_MANS) $(noinst_man_MANS) diff --git a/build-aux/thread-safety-blacklist b/build-aux/thread-safety-blacklist new file mode 100644 index 000..42560df --- /dev/null +++ b/build-aux/thread-safety-blacklist @@ -0,0 +1,90 @@ +\basctime( +\bbasename( +\bcatgets( +\bcrypt( +\bctermid( +\bctime( +\bdbm_clearerr( +\bdbm_close( +\bdbm_delete( +\bdbm_error( +\bdbm_fetch( +\bdbm_firstkey( +\bdbm_nextkey( +\bdbm_open( +\bdbm_store( +\bdirname( +\bdlerror( +\bdrand48( +\becvt( +\bencrypt( +\bendgrent( +\bendpwent( +\bendutxent( +\bfcvt( +\bftw( +\bgcvt( +\bgetc_unlocked( +\bgetchar_unlocked( +\bgetdate( +\bgetgrent( +\bgetgrgid( +\bgetgrnam( +\bgethostbyaddr( +\bgethostbyname( +\bgethostent( +\bgetlogin( +\bgetmntent( +\bgetnetbyaddr( +\bgetnetbyname( +\bgetnetent( +\bgetprotobyname( +\bgetprotobynumber( +\bgetprotoent( +\bgetpwent( +\bgetpwnam( +\bgetpwuid( +\bgetservbyname( +\bgetservbyport( +\bgetservent( +\bgetutxent( +\bgetutxid( +\bgetutxline( +\bgmtime( +\bhcreate( +\bhdestroy( +\bhsearch( +\binet_ntoa( +\bl64a( +\blgamma( +\blgammaf( +\blgammal( +\blocaleconv( +\blocaltime( +\blrand48( +\bmrand48( +\bnftw( +\bnl_langinfo( +\bptsname( +\bputc_unlocked( +\bputchar_unlocked( +\bputenv( +\bpututxline( +\brand( +\bsetenv( +\bsetgrent( +\bsetkey( +\bsetpwent( +\bsetutxent( +\bsigprocmask( +\bstrerror( +\bstrsignal( +\bstrtok( +\bsystem( +\btmpnam( +\bttyname( +\bunsetenv( +\bwcrtomb( +\bwcsrtombs( +\bwcstombs( +\bwctomb( -- 1.7.2.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [threads 04/11] ovs-thread: Add support for convenient once-only initializers.
pthread_once() is portable but it does not allow passing any parameters to
the initialization function, which is often inconvenient, because it means
that the function can only access data declared at file scope. This commit
introduces an alternative with a more convenient interface.
Signed-off-by: Ben Pfaff
---
Makefile.am | 12
lib/ovs-thread.c | 18 +
lib/ovs-thread.h | 76 ++
3 files changed, 100 insertions(+), 6 deletions(-)
diff --git a/Makefile.am b/Makefile.am
index 193b19e..488aed2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -185,17 +185,17 @@ config-h-check:
fi
.PHONY: config-h-check
-# Check that "struct vlog_ratelimit" is always declared "static".
-ALL_LOCAL += rate-limit-check
-rate-limit-check:
+# Check that certain data structures are always declared "static".
+ALL_LOCAL += static-check
+static-check:
@if test -e $(srcdir)/.git && (git --version) >/dev/null 2>&1 && \
- git --no-pager grep -n -E '^[ ]+struct vlog_rate_limit.*='
$(srcdir); \
+ git --no-pager grep -n -E '^[ ]+(struct
vlog_rate_limit|pthread_once_t|struct ovsthread_once).*=' $(srcdir); \
then \
echo "See above for list of violations of the rule that "; \
- echo "'struct vlog_rate_limit' must always be 'static'"; \
+ echo "certain data structures must always be 'static'"; \
exit 1; \
fi
-.PHONY: rate-limit-check
+.PHONY: static-check
# Check that assert.h is not used outside a whitelist of files.
ALL_LOCAL += check-assert-h-usage
diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
index f48a659..d69ec5e 100644
--- a/lib/ovs-thread.c
+++ b/lib/ovs-thread.c
@@ -105,4 +105,22 @@ xpthread_create(pthread_t *threadp, pthread_attr_t *attr,
ovs_abort(error, "pthread_create failed");
}
}
+
+bool
+ovsthread_once_start__(struct ovsthread_once *once)
+{
+xpthread_mutex_lock(&once->mutex);
+if (!ovsthread_once_is_done__(once)) {
+return false;
+}
+xpthread_mutex_unlock(&once->mutex);
+return true;
+}
+
+void OVS_RELEASES(once)
+ovsthread_once_done(struct ovsthread_once *once)
+{
+atomic_store(&once->done, true);
+xpthread_mutex_unlock(&once->mutex);
+}
#endif
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 752e44f..4779030 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -18,6 +18,7 @@
#define OVS_THREAD_H 1
#include
+#include "ovs-atomic.h"
#include "util.h"
/* glibc has some non-portable mutex types and initializers:
@@ -279,5 +280,80 @@ void xpthread_create(pthread_t *, pthread_attr_t *, void
*(*)(void *), void *);
return NAME##_set__(value); \
}
+/* Convenient once-only execution.
+ *
+ *
+ * Problem
+ * ===
+ *
+ * POSIX provides pthread_once_t and pthread_once() as primitives for running a
+ * set of code only once per process execution. They are used like this:
+ *
+ * static void run_once(void) { ...initialization... }
+ * static pthread_once_t once = PTHREAD_ONCE_INIT;
+ * ...
+ * pthread_once(&once, run_once);
+ *
+ * pthread_once() does not allow passing any parameters to the initialization
+ * function, which is often inconvenient, because it means that the function
+ * can only access data declared at file scope.
+ *
+ *
+ * Solution
+ *
+ *
+ * Use ovsthread_once, like this, instead:
+ *
+ * static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
+ *
+ * if (ovsthread_once_start(&once)) {
+ * ...initialization...
+ * ovsthread_once_done(&once);
+ * }
+ */
+
+struct ovsthread_once {
+atomic_bool done;
+pthread_mutex_t mutex;
+};
+
+#define OVSTHREAD_ONCE_INITIALIZER \
+{ \
+ATOMIC_VAR_INIT(false), \
+PTHREAD_ADAPTIVE_MUTEX_INITIALIZER, \
+}
+
+static inline bool ovsthread_once_start(struct ovsthread_once *);
+void ovsthread_once_done(struct ovsthread_once *once) OVS_RELEASES(once);
+
+bool ovsthread_once_start__(struct ovsthread_once *);
+
+static inline bool
+ovsthread_once_is_done__(const struct ovsthread_once *once)
+{
+bool done;
+
+atomic_read_explicit(&once->done, &done, memory_order_relaxed);
+return done;
+}
+
+/* Returns true if this is the first call to ovsthread_once_start() for
+ * 'once'. In this case, the caller should perform whatever initialization
+ * actions it needs to do, then call ovsthread_once_done() for 'once'.
+ *
+ * Returns false if this is not the first call to ovsthread_once_start() for
+ * 'once'. In this case, the call will not return until after
+ * ovsthread_once_done() has been called. */
+static inline bool
+ovsthread_once_start(struct ovsthread_once *once)
+{
+return OVS_UNLIKELY(!ovsthread_once_is_done__(once)
+&& !ovsthread_once_start__(once));
+}
+
+#ifdef __CHECKER__
Re: [ovs-dev] [PATCH] ovs-vsctl: Improve error message for "ovs-vsctl del-port ".
Thanks. Is that a review?
On Tue, Jun 18, 2013 at 09:27:18PM -0700, Reid Price wrote:
> Thanks, that seems like the right amount of information
>
> -Reid
>
>
> On Tue, Jun 18, 2013 at 9:02 PM, Ben Pfaff wrote:
>
> > Previously, commands like this:
> > ovs-vsctl add-br br0
> > ovs-vsctl del-port br0
> > yielded an error message like:
> > no port named br0
> > which is confusing. This commit improves the error message to:
> > cannot delete port br0 because it is the local port for bridge br0
> > (deleting this port requires deleting the entire bridge)
> >
> > Bug #17994.
> > Reported-by: Reid Price
> > Signed-off-by: Ben Pfaff
> > ---
> > tests/ovs-vsctl.at|6 +-
> > utilities/ovs-vsctl.c | 10 +++---
> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > index ec08cd4..fa2c3ff 100644
> > --- a/tests/ovs-vsctl.at
> > +++ b/tests/ovs-vsctl.at
> > @@ -323,6 +323,10 @@ AT_CHECK([RUN_OVS_VSCTL(
> > ], [], [OVS_VSCTL_CLEANUP])
> > AT_CHECK([RUN_OVS_VSCTL([--may-exist add-port b b1])], [0], [], [],
> >[OVS_VSCTL_CLEANUP])
> > +AT_CHECK([RUN_OVS_VSCTL([del-port a])], [1], [],
> > + [ovs-vsctl: cannot delete port a because it is the local port for
> > bridge a (deleting this port requires deleting the entire bridge)
> > +],
> > + [OVS_VSCTL_CLEANUP])
> > AT_CHECK([RUN_OVS_VSCTL([--may-exist add-port a b1])], [1], [],
> >[ovs-vsctl: "--may-exist add-port a b1" but b1 is actually attached to
> > bridge b
> > ],
> > @@ -1246,4 +1250,4 @@ AT_CHECK([sed "/|bridge|WARN|/d" ovs-vswitchd.log >
> > ovs-vswitchd.log], [0], [],
> > # Delete the port
> > AT_CHECK([ovs-vsctl del-port br0 reserved_name], [0], [], [])])
> > OVS_VSWITCHD_STOP
> > -AT_CLEANUP
> > \ No newline at end of file
> > +AT_CLEANUP
> > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > index 19ab472..2d8c7c7 100644
> > --- a/utilities/ovs-vsctl.c
> > +++ b/utilities/ovs-vsctl.c
> > @@ -2016,13 +2016,17 @@ cmd_del_port(struct vsctl_context *ctx)
> > {
> > bool must_exist = !shash_find(&ctx->options, "--if-exists");
> > bool with_iface = shash_find(&ctx->options, "--with-iface") != NULL;
> > +const char *target = ctx->argv[ctx->argc - 1];
> > struct vsctl_port *port;
> >
> > vsctl_context_populate_cache(ctx);
> > -if (!with_iface) {
> > -port = find_port(ctx, ctx->argv[ctx->argc - 1], must_exist);
> > +if (find_bridge(ctx, target, false)) {
> > +vsctl_fatal("cannot delete port %s because it is the local port "
> > +"for bridge %s (deleting this port requires deleting "
> > +"the entire bridge)", target, target);
> > +} else if (!with_iface) {
> > +port = find_port(ctx, target, must_exist);
> > } else {
> > -const char *target = ctx->argv[ctx->argc - 1];
> > struct vsctl_iface *iface;
> >
> > port = find_port(ctx, target, false);
> > --
> > 1.7.10.4
> >
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-xapi-sync: Increase the tolerance level for xapi failures for bridges.
On Wed, Jun 19, 2013 at 02:03:15AM -0700, Gurucharan Shetty wrote: > Specifically for the case, where we know that a bridge record > should exist in xapi, if we don't get any bridge records, log > the failure and retry again later. > > Signed-off-by: Gurucharan Shetty Could we add a helper function for the common code here? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dpctl: Add mega flow support
I don't really care about the formatting, only about the kernel ABI. Pre-megaflows, the ABI was: typemask matches --- eth_type(0x600+) specified Ethertype II Ethertype. any non-Ethernet II frame Now, my understanding is that the above continue to be valid, with the same meanings, but the following are also supported: typemask matches --- eth_type(0x600+)eth_type(0x) specified Ethertype II Ethertype. eth_type(0x600+)eth_type(0)any Ethertype II frame eth_type(0x) any non-Ethernet II frame Is that right? On Wed, Jun 19, 2013 at 10:40:28AM -0700, Andy Zhou wrote: > We will continue to allow missing eth_type in the netlink attribute to > imply Ethernet II type. 802.3 frames requires a specific eth_type > attribute. I don't understand the first sentence. We have never interpreted a missing eth_type as implying an Ethernet II frame; the opposite, in fact: a missing eth_type matches only non-Ethernet II frames. > With Mega flows, we further require a missing eth_type in the key attribute > to have a exact match (ox) in the eth_type of the mask attribute (if > present). That's really weird. What's the rationale? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2] feature: Create specific types for ofp and odp port
On Tue, Jun 18, 2013 at 1:10 PM, Ben Pfaff wrote:
> Thanks again for working on this.
>
> It looks like your test builds are not configured to build a kernel
> module: this patch triggers tons of errors in the kernel build due to
> the change to OVSP_LOCAL, because OVS_FORCE is not defined in the
> kernel. I am not sure of that is the best possible fix (we are
> constrained by the kernel API here), but the following incremental
> does work:
>
> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> index add1287..bde3ba2 100644
> --- a/include/linux/openvswitch.h
> +++ b/include/linux/openvswitch.h
> @@ -116,7 +116,7 @@ struct ovs_vport_stats {
> };
>
> /* Fixed logical ports. */
> -#define OVSP_LOCAL ((OVS_FORCE odp_port_t) 0)
> +#define OVSP_LOCAL ((__u32)0)
>
OVSP_LOCAL is mostly used in kernel module. OVSP_NONE is only used in user
space to indicate unknown odp port.
I want to rename OVSP_NONE to ODPP_NONE and change its type to odp_port_t.
Does it make sense?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2] feature: Create specific types for ofp and odp port
On Wed, Jun 19, 2013 at 01:46:17PM -0700, Alex Wang wrote:
> On Tue, Jun 18, 2013 at 1:10 PM, Ben Pfaff wrote:
>
> > Thanks again for working on this.
> >
> > It looks like your test builds are not configured to build a kernel
> > module: this patch triggers tons of errors in the kernel build due to
> > the change to OVSP_LOCAL, because OVS_FORCE is not defined in the
> > kernel. I am not sure of that is the best possible fix (we are
> > constrained by the kernel API here), but the following incremental
> > does work:
> >
> > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> > index add1287..bde3ba2 100644
> > --- a/include/linux/openvswitch.h
> > +++ b/include/linux/openvswitch.h
> > @@ -116,7 +116,7 @@ struct ovs_vport_stats {
> > };
> >
> > /* Fixed logical ports. */
> > -#define OVSP_LOCAL ((OVS_FORCE odp_port_t) 0)
> > +#define OVSP_LOCAL ((__u32)0)
> >
>
>
> OVSP_LOCAL is mostly used in kernel module. OVSP_NONE is only used in user
> space to indicate unknown odp port.
>
> I want to rename OVSP_NONE to ODPP_NONE and change its type to odp_port_t.
> Does it make sense?
That's a pretty clean solution. If you do that, then you can also:
#define ODPP_LOCAL ((OVS_FORCE odp_port_t) OVSP_LOCAL)
and use ODPP_LOCAL where we currently use OVSP_LOCAL.
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2] feature: Create specific types for ofp and odp port
Thanks,
On Wed, Jun 19, 2013 at 1:48 PM, Ben Pfaff wrote:
> On Wed, Jun 19, 2013 at 01:46:17PM -0700, Alex Wang wrote:
> > On Tue, Jun 18, 2013 at 1:10 PM, Ben Pfaff wrote:
> >
> > > Thanks again for working on this.
> > >
> > > It looks like your test builds are not configured to build a kernel
> > > module: this patch triggers tons of errors in the kernel build due to
> > > the change to OVSP_LOCAL, because OVS_FORCE is not defined in the
> > > kernel. I am not sure of that is the best possible fix (we are
> > > constrained by the kernel API here), but the following incremental
> > > does work:
> > >
> > > diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
> > > index add1287..bde3ba2 100644
> > > --- a/include/linux/openvswitch.h
> > > +++ b/include/linux/openvswitch.h
> > > @@ -116,7 +116,7 @@ struct ovs_vport_stats {
> > > };
> > >
> > > /* Fixed logical ports. */
> > > -#define OVSP_LOCAL ((OVS_FORCE odp_port_t) 0)
> > > +#define OVSP_LOCAL ((__u32)0)
> > >
> >
> >
> > OVSP_LOCAL is mostly used in kernel module. OVSP_NONE is only used in
> user
> > space to indicate unknown odp port.
> >
> > I want to rename OVSP_NONE to ODPP_NONE and change its type to
> odp_port_t.
> > Does it make sense?
>
> That's a pretty clean solution. If you do that, then you can also:
> #define ODPP_LOCAL ((OVS_FORCE odp_port_t) OVSP_LOCAL)
> and use ODPP_LOCAL where we currently use OVSP_LOCAL.
>
> Thanks,
>
> Ben.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofp-errors: Fix typos in error enumeration names.
OFPGMFC is so nice these constants said it twice.
Signed-off-by: Ben Pfaff
---
lib/ofp-errors.h |8
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index 593241d..fd6f03f 100644
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -388,13 +388,13 @@ enum ofperr {
OFPERR_OFPGMFC_BAD_COMMAND,
/* OF1.2+(6,12). Error in bucket. */
-OFPERR_OFPGMFC_OFPGMFC_BAD_BUCKET,
+OFPERR_OFPGMFC_BAD_BUCKET,
/* OF1.2+(6,13). Error in watch port/group. */
-OFPERR_OFPGMFC_OFPGMFC_BAD_WATCH,
+OFPERR_OFPGMFC_BAD_WATCH,
/* OF1.2+(6,14). Permissions error. */
-OFPERR_OFPGMFC_OFPGMFC_EPERM,
+OFPERR_OFPGMFC_EPERM,
/* ## - ## */
/* ## OFPET_PORT_MOD_FAILED ## */
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [threads 02/11] ovs-thread: Add per-thread data support.
POSIX defines a portable pthread_key_t API for per-thread data. GCC and
C11 have two different forms of per-thread data that are generally faster
than the POSIX API, where they are available. This commit adds a
macro-based wrapper, DEFINE_PER_THREAD_DATA, that takes advantage of the
GCC extension where it is available and falls back to the POSIX API
otherwise. (I'm not aware of any compilers that implement the C11 feature,
so this commit doesn't try to use it.)
This commit also adds a convenience wrapper for the POSIX API, via the
DEFINE_PER_THREAD_MALLOCED_DATA macro.
Signed-off-by: Ben Pfaff
---
configure.ac |1 +
lib/ovs-thread.h | 194 +
m4/openvswitch.m4 | 21 ++-
3 files changed, 215 insertions(+), 1 deletions(-)
diff --git a/configure.ac b/configure.ac
index a691963..a6c68a9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -80,6 +80,7 @@ OVS_CHECK_XENSERVER_VERSION
OVS_CHECK_GROFF
OVS_CHECK_GNU_MAKE
OVS_CHECK_CACHE_TIME
+OVS_CHECK___THREAD
OVS_ENABLE_OPTION([-Wall])
OVS_ENABLE_OPTION([-Wno-sign-compare])
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index cafeedf..752e44f 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -85,5 +85,199 @@ void xpthread_cond_wait(pthread_cond_t *, pthread_mutex_t
*mutex)
void xpthread_key_create(pthread_key_t *, void (*destructor)(void *));
void xpthread_create(pthread_t *, pthread_attr_t *, void *(*)(void *), void *);
+
+/* Per-thread data.
+ *
+ * Multiple forms of per-thread data exist, each with its own pluses and
+ * minuses:
+ *
+ * - POSIX per-thread data via pthread_key_t is portable to any pthreads
+ * implementation, and allows a destructor function to be defined. It
+ * only (directly) supports per-thread pointers, which are always
+ * initialized to NULL. It requires once-only allocation of a
+ * pthread_key_t value. It is relatively slow.
+ *
+ * - The __thread keyword works with any data type and initializer, and it
+ * is fast. __thread does not require once-only initialization like
+ * pthread_key_t. However, __thread is GCC-specific and not portable
+ * even to every GCC environment. There is no provision to call a
+ * user-specified destructor when a thread ends.
+ *
+ * - The _Thread_local keyword is similar to __thread, but it is new in C11
+ * and therefore not available pretty much anywhere yet. Compared to
+ * __thread, it has the additional disadvantage that C11 does not define
+ * what happens if one attempts to access a _Thread_local object from a
+ * thread other than the one to which that object belongs.
+ *
+ * Here's a handy summary:
+ *
+ * pthread_key_t__thread _Thread_local
+ * - - -
+ * portability high medium low
+ * speedlowhighhigh
+ * supports destructors?yes no no
+ * needs key allocation?yes no no
+ * arbitrary initializer?no yes yes
+ * cross-thread access? yes yes no
+ */
+
+/* DEFINE_PER_THREAD_DATA(TYPE, NAME, INITIALIZER).
+ *
+ * One should prefer to use POSIX per-thread data, via pthread_key_t, when its
+ * performance is acceptable, because of its portability (see the table above).
+ * This macro is an alternatives that takes advantage of __thread, for its
+ * performance, when it is available, and falls back to POSIX per-thread data
+ * otherwise.
+ *
+ * Defines per-thread variable NAME with the given TYPE, initialized to
+ * INITIALIZER (which must be valid as an initializer for a variable with
+ * static lifetime).
+ *
+ * The public interface to the variable is:
+ *
+ *TYPE *NAME_get(void)
+ *TYPE *NAME_get__(void)
+ *
+ * Returns the address of this thread's instance of NAME.
+ *
+ * Use NAME_get() in a context where this might be the first use of the
+ * per-thread variable in the program. Use NAME_get__(), which avoids a
+ * conditional test and is thus slightly faster, in a context where one
+ * knows that NAME_get() has already been called previously.
+ *
+ * There are no "NAME_set()" or "NAME_set__()" functions. To set the value of
+ * the per-thread variable, dereference the pointer returned by TYPE_get() or
+ * TYPE_get__(), e.g. *TYPE_get() = 0.
+ */
+#if HAVE___THREAD
+#define DEFINE_PER_THREAD_DATA(TYPE, NAME, ...) \
+typedef TYPE NAME##_type; \
+static __thread NAME##_type NAME##_var = __VA_ARGS__; \
+\
+static NAME##_type *\
+NAME##_get__(void) \
+{
[ovs-dev] [threads 10/11] sparse: Remove prototypes for thread-unsafe functions from headers.
This ensures that attempts to use them cause sparse to complain. Signed-off-by: Ben Pfaff --- include/sparse/math.h |5 + include/sparse/netinet/in.h |3 +-- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/include/sparse/math.h b/include/sparse/math.h index f94c527..c2d6156 100644 --- a/include/sparse/math.h +++ b/include/sparse/math.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011 Nicira, Inc. + * Copyright (c) 2011, 2013 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -108,9 +108,6 @@ double jn(int, double); double ldexp(double, int); float ldexpf(float, int); long double ldexpl(long double, int); -double lgamma(double); -float lgammaf(float); -long double lgammal(long double); long long llrint(double); long long llrintf(float); long long llrintl(long double); diff --git a/include/sparse/netinet/in.h b/include/sparse/netinet/in.h index b3924c3..87d48d6 100644 --- a/include/sparse/netinet/in.h +++ b/include/sparse/netinet/in.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011 Nicira, Inc. + * Copyright (c) 2011, 2013 Nicira, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -109,7 +109,6 @@ static inline uint16_t ntohs(ovs_be16 x) in_addr_t inet_addr(const char *); int inet_aton (const char *, struct in_addr *); -char *inet_ntoa(struct in_addr); const char *inet_ntop(int, const void *, char *, socklen_t); int inet_pton(int, const char *, void *); -- 1.7.2.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofp-parse: Fix parsing of out_port.
Signed-off-by: Ben Pfaff
---
lib/ofp-parse.c|4 ++--
tests/ovs-ofctl.at |4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 1c5c761..fdec1d4 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -950,7 +950,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command,
const char *str_,
if (!strcmp(name, "table")) {
fm->table_id = str_to_table_id(value);
} else if (!strcmp(name, "out_port")) {
-if (!ofputil_port_from_string(name, &fm->out_port)) {
+if (!ofputil_port_from_string(value, &fm->out_port)) {
ofp_fatal(str_, verbose, "%s is not a valid OpenFlow port",
name);
}
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 6198677..6cc4c51 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -3,7 +3,7 @@ AT_BANNER([ovs-ofctl])
AT_SETUP([ovs-ofctl parse-flows (OpenFlow 1.0)])
AT_DATA([flows.txt], [[
# comment
-tcp,tp_src=123,actions=flood
+tcp,tp_src=123,out_port=5,actions=flood
in_port=LOCAL dl_vlan=9 dl_src=00:0A:E4:25:6B:B0 actions=drop
udp dl_vlan_pcp=7 idle_timeout=5 actions=strip_vlan output:0
tcp,nw_src=192.168.0.3,tp_dst=80 actions=set_queue:37,output:1
@@ -20,7 +20,7 @@ AT_CHECK([ovs-ofctl parse-flows flows.txt
AT_CHECK([[sed 's/ (xid=0x[0-9a-fA-F]*)//' stdout]], [0],
[[usable protocols: any
chosen protocol: OpenFlow10-table_id
-OFPT_FLOW_MOD: ADD tcp,tp_src=123 actions=FLOOD
+OFPT_FLOW_MOD: ADD tcp,tp_src=123 out_port:5 actions=FLOOD
OFPT_FLOW_MOD: ADD in_port=LOCAL,dl_vlan=9,dl_src=00:0a:e4:25:6b:b0
actions=drop
OFPT_FLOW_MOD: ADD udp,dl_vlan_pcp=7 idle:5 actions=strip_vlan,output:0
OFPT_FLOW_MOD: ADD tcp,nw_src=192.168.0.3,tp_dst=80
actions=set_queue:37,output:1
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: Mega flow implementation
Jesse, The following patch is an incremental patch on top of yours. -- Fix a few small bugs introduced by the cleanup patch. -- Make mask list per table. I believe this implementation will address the rcu lock issues we discussed off line. Would you please review? Thanks, --andy On Tue, Jun 18, 2013 at 4:35 PM, Jesse Gross wrote: > On Tue, Jun 18, 2013 at 4:15 PM, Andy Zhou wrote: > > Add wildcarded flow support in kernel datapath. > > > > Wildcarded flow can improve OVS flow set up performance by avoid sending > > matching new flows to the user space program. The exact performance boost > > will largely dependent on wildcarded flow hit rate. > > > > In case all new flows hits wildcard flows, the flow set up rate is > > within 5% of that of linux bridge module. > > > > Pravin has made significant contributions to this patch. Including API > > clean ups and bug fixes. > > > > Co-authored-by: Pravin B Shelar > > Signed-off-by: Pravin B Shelar > > Signed-off-by: Andy Zhou > > I made some incremental changes, can you take a look to see if they > seem reasonable to you? > > Here are the main blocks: > - Additional interface documentation. > - Fix memory leak when installing a new flow if there is an > allocation failure for the mask. > - Require Ethernet addresses to be present in the key, as before. > - Make SNAP handling a little more integrated with the rest of the > parsing and validation code. > - Make ovs_flow_free() and ovs_deferred_flow_free() have exactly the > same behavior, just with the addition of RCU, to avoid possible > confusion. > - Return errors directly rather than through an intermediate variable > in ovs_flow_extract() and children. > - Enforce an exact match for the outer EtherType for vlan packets, > similar to what we do for other protocols. > - Miscellaneous style fixes. > 0001-datapath-Make-mask-list-per-table.patch Description: Binary data ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 00/11] basic thread support library
On 19 June 2013 16:17, Ben Pfaff wrote: > This series has two purposes: > > * Add a basic thread support library to the tree. > > * Get rid of all calls to C library functions that POSIX describes > as inherently unsafe in a multithreaded program, and then add a > make-time check that flags an error if any calls get reintroduced. This fails to build on FreeBSD for me, because it appears to rely on header leakage for stdint.h types. In file included from lib/ovs-atomic.h:237, from lib/ovs-thread.h:23, from lib/command-line.c:22: lib/ovs-atomic-c11.h:26: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'uint8_t' ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 00/11] basic thread support library
On Wed, Jun 19, 2013 at 05:20:11PM -0400, Ed Maste wrote: > On 19 June 2013 16:17, Ben Pfaff wrote: > > This series has two purposes: > > > > * Add a basic thread support library to the tree. > > > > * Get rid of all calls to C library functions that POSIX describes > > as inherently unsafe in a multithreaded program, and then add a > > make-time check that flags an error if any calls get reintroduced. > > This fails to build on FreeBSD for me, because it appears to rely on > header leakage for stdint.h types. > > In file included from lib/ovs-atomic.h:237, > from lib/ovs-thread.h:23, > from lib/command-line.c:22: > lib/ovs-atomic-c11.h:26: error: expected '=', ',', ';', 'asm' or > '__attribute__' before 'uint8_t' Does this mean that FreeBSD has ? Is it fully functional? That's the one implementation I wasn't able to test, because I didn't know of any C11 implementations with . ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 06/11] New function ovs_strerror() as a thread-safe replacement for strerror().
On 19 June 2013 16:17, Ben Pfaff wrote:
> +#if STRERROR_R_CHAR_P
> +/* GNU style strerror_r() might return an immutable static string, or it
> + * might write and return 'buffer', but in either case we can pass the
> + * returned string directly to the caller. */
> +s = strerror_r(error, buffer, BUFSIZE);
> +#else /* strerror_r() returns an int. */
> +s = buffer;
> +if (strerror_r(error, buffer, BUFSIZE)) {
> +/* strerror_r() is only allowed to fail on ERANGE (because the buffer
> + * is too short). We don't check the actual failure reason because
> + * POSIX requires strerror_r() to return the error but old glibc
> + * (before 2.13) returns -1 and sets errno. */
> +snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
This one fails to build with:
lib/util.c:342:26: error: use of undeclared identifier 'ptb'
snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
^
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 00/11] basic thread support library
On 19 June 2013 17:22, Ben Pfaff wrote: > On Wed, Jun 19, 2013 at 05:20:11PM -0400, Ed Maste wrote: >> On 19 June 2013 16:17, Ben Pfaff wrote: >> > This series has two purposes: >> > >> > * Add a basic thread support library to the tree. >> > >> > * Get rid of all calls to C library functions that POSIX describes >> > as inherently unsafe in a multithreaded program, and then add a >> > make-time check that flags an error if any calls get reintroduced. >> >> This fails to build on FreeBSD for me, because it appears to rely on >> header leakage for stdint.h types. >> >> In file included from lib/ovs-atomic.h:237, >> from lib/ovs-thread.h:23, >> from lib/command-line.c:22: >> lib/ovs-atomic-c11.h:26: error: expected '=', ',', ';', 'asm' or >> '__attribute__' before 'uint8_t' > > Does this mean that FreeBSD has ? Is it fully > functional? That's the one implementation I wasn't able to test, > because I didn't know of any C11 implementations with . Indeed we do, and it should be functional. SVN history is here if you're interested: http://svnweb.freebsd.org/base/head/sys/sys/stdatomic.h That said, it seems it wasn't stdint.h per se that was the problem; I needed to change: -typedef _Atomic uint8_t atomic_uint8_t; +typedef _Atomic(uint8_t) atomic_uint8_t; etc. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 06/11] New function ovs_strerror() as a thread-safe replacement for strerror().
On Wed, Jun 19, 2013 at 05:42:44PM -0400, Ed Maste wrote:
> On 19 June 2013 16:17, Ben Pfaff wrote:
> > +#if STRERROR_R_CHAR_P
> > +/* GNU style strerror_r() might return an immutable static string, or
> > it
> > + * might write and return 'buffer', but in either case we can pass the
> > + * returned string directly to the caller. */
> > +s = strerror_r(error, buffer, BUFSIZE);
> > +#else /* strerror_r() returns an int. */
> > +s = buffer;
> > +if (strerror_r(error, buffer, BUFSIZE)) {
> > +/* strerror_r() is only allowed to fail on ERANGE (because the
> > buffer
> > + * is too short). We don't check the actual failure reason because
> > + * POSIX requires strerror_r() to return the error but old glibc
> > + * (before 2.13) returns -1 and sets errno. */
> > +snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
>
> This one fails to build with:
>
> lib/util.c:342:26: error: use of undeclared identifier 'ptb'
> snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
> ^
Oops, code from an older revision that I failed to re-test after
changing the implementation pattern.
Thanks, I'll fix that.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dpctl: Add mega flow support
It is a good idea to capture this information in a table. The following table should be accurate: Pre-megaflow: typemask matches --- eth_type(0x600+) specified Ethertype II, or valid 802.3 SNAP packet with valid eth_type. Ethertype. any non-Ethernet II frame, except valid 802.3 SNAP packet with valid eth_type. Post-megaflow: typemask matches --- eth_type(0x600+)eth_type(0x) specified Ethertype II Ethertype, or valid 802.3 SNAP packet with valid eth_type. eth_type(0x600+)eth_type(0)any Ethertype II frame or non-Ethernet II frame. eth_type(0x) any non-Ethernet II frame, except valid 802.3 SNAP packet with valid eth_type. --andy On Wed, Jun 19, 2013 at 1:42 PM, Ben Pfaff wrote: > I don't really care about the formatting, only about the kernel ABI. > > Pre-megaflows, the ABI was: > > typemask matches > --- > eth_type(0x600+) specified Ethertype II > Ethertype. >any non-Ethernet II frame > > Now, my understanding is that the above continue to be valid, with the > same meanings, but the following are also supported: > > typemask matches > --- > eth_type(0x600+)eth_type(0x) specified Ethertype II > Ethertype. > eth_type(0x600+)eth_type(0)any Ethertype II frame > eth_type(0x) any non-Ethernet II frame > > Is that right? > > On Wed, Jun 19, 2013 at 10:40:28AM -0700, Andy Zhou wrote: > > We will continue to allow missing eth_type in the netlink attribute to > > imply Ethernet II type. 802.3 frames requires a specific eth_type > > attribute. > > I don't understand the first sentence. We have never interpreted a > missing eth_type as implying an Ethernet II frame; the opposite, in > fact: a missing eth_type matches only non-Ethernet II frames. > > > With Mega flows, we further require a missing eth_type in the key > attribute > > to have a exact match (ox) in the eth_type of the mask attribute (if > > present). > > That's really weird. What's the rationale? > > Thanks, > > Ben. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 00/11] basic thread support library
On Wed, Jun 19, 2013 at 05:45:30PM -0400, Ed Maste wrote: > On 19 June 2013 17:22, Ben Pfaff wrote: > > On Wed, Jun 19, 2013 at 05:20:11PM -0400, Ed Maste wrote: > >> On 19 June 2013 16:17, Ben Pfaff wrote: > >> > This series has two purposes: > >> > > >> > * Add a basic thread support library to the tree. > >> > > >> > * Get rid of all calls to C library functions that POSIX describes > >> > as inherently unsafe in a multithreaded program, and then add a > >> > make-time check that flags an error if any calls get reintroduced. > >> > >> This fails to build on FreeBSD for me, because it appears to rely on > >> header leakage for stdint.h types. > >> > >> In file included from lib/ovs-atomic.h:237, > >> from lib/ovs-thread.h:23, > >> from lib/command-line.c:22: > >> lib/ovs-atomic-c11.h:26: error: expected '=', ',', ';', 'asm' or > >> '__attribute__' before 'uint8_t' > > > > Does this mean that FreeBSD has ? Is it fully > > functional? That's the one implementation I wasn't able to test, > > because I didn't know of any C11 implementations with . > > Indeed we do, and it should be functional. SVN history is here if > you're interested: > http://svnweb.freebsd.org/base/head/sys/sys/stdatomic.h > > That said, it seems it wasn't stdint.h per se that was the problem; I > needed to change: > > -typedef _Atomic uint8_t atomic_uint8_t; > +typedef _Atomic(uint8_t) atomic_uint8_t; Oh, that's odd. C11 appears to say that the parentheses are optional with _Atomic in this case (you can use _Atomic(...) as a type specifier or _Atomic by itself as a type qualifier) but I'll change it since that fixes the problem. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dpctl: Add mega flow support
OK, can we add that in the tree somewhere, maybe datapath/README? Thanks, Ben. On Wed, Jun 19, 2013 at 02:52:52PM -0700, Andy Zhou wrote: > It is a good idea to capture this information in a table. The following > table should be accurate: > > Pre-megaflow: > > typemask matches > --- > eth_type(0x600+) specified Ethertype II, or valid 802.3 > SNAP >packet with valid > eth_type. > Ethertype. > any non-Ethernet II frame, except > valid 802.3 SNAP >packet with valid > eth_type. > > Post-megaflow: > > typemask matches > --- > eth_type(0x600+)eth_type(0x) specified Ethertype II > Ethertype, or valid 802.3 SNAP packet with valid eth_type. > eth_type(0x600+)eth_type(0)any Ethertype II frame or > non-Ethernet II frame. > eth_type(0x) any non-Ethernet II frame, > except valid 802.3 SNAP packet with valid eth_type. > > --andy > > > > > On Wed, Jun 19, 2013 at 1:42 PM, Ben Pfaff wrote: > > > I don't really care about the formatting, only about the kernel ABI. > > > > Pre-megaflows, the ABI was: > > > > typemask matches > > --- > > eth_type(0x600+) specified Ethertype II > > Ethertype. > >any non-Ethernet II frame > > > > Now, my understanding is that the above continue to be valid, with the > > same meanings, but the following are also supported: > > > > typemask matches > > --- > > eth_type(0x600+)eth_type(0x) specified Ethertype II > > Ethertype. > > eth_type(0x600+)eth_type(0)any Ethertype II frame > > eth_type(0x) any non-Ethernet II frame > > > > Is that right? > > > > On Wed, Jun 19, 2013 at 10:40:28AM -0700, Andy Zhou wrote: > > > We will continue to allow missing eth_type in the netlink attribute to > > > imply Ethernet II type. 802.3 frames requires a specific eth_type > > > attribute. > > > > I don't understand the first sentence. We have never interpreted a > > missing eth_type as implying an Ethernet II frame; the opposite, in > > fact: a missing eth_type matches only non-Ethernet II frames. > > > > > With Mega flows, we further require a missing eth_type in the key > > attribute > > > to have a exact match (ox) in the eth_type of the mask attribute (if > > > present). > > > > That's really weird. What's the rationale? > > > > Thanks, > > > > Ben. > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dpctl: Add mega flow support
Good idea. There may be other aspects of the mega related kernel ABI needs document as well. Wonder if Justin and Jesse have more inputs on this. On Wed, Jun 19, 2013 at 2:55 PM, Ben Pfaff wrote: > OK, can we add that in the tree somewhere, maybe datapath/README? > > Thanks, > > Ben. > > On Wed, Jun 19, 2013 at 02:52:52PM -0700, Andy Zhou wrote: > > It is a good idea to capture this information in a table. The following > > table should be accurate: > > > > Pre-megaflow: > > > > typemask matches > > --- > > eth_type(0x600+) specified Ethertype II, or valid > 802.3 > > SNAP > >packet with valid > > eth_type. > > Ethertype. > > any non-Ethernet II frame, > except > > valid 802.3 SNAP > >packet with valid > > eth_type. > > > > Post-megaflow: > > > > typemask matches > > --- > > eth_type(0x600+)eth_type(0x) specified Ethertype II > > Ethertype, or valid 802.3 SNAP packet with valid eth_type. > > eth_type(0x600+)eth_type(0)any Ethertype II frame or > > non-Ethernet II frame. > > eth_type(0x) any non-Ethernet II frame, > > except valid 802.3 SNAP packet with valid eth_type. > > > > --andy > > > > > > > > > > On Wed, Jun 19, 2013 at 1:42 PM, Ben Pfaff wrote: > > > > > I don't really care about the formatting, only about the kernel ABI. > > > > > > Pre-megaflows, the ABI was: > > > > > > typemask matches > > > --- > > > eth_type(0x600+) specified Ethertype II > > > Ethertype. > > >any non-Ethernet II frame > > > > > > Now, my understanding is that the above continue to be valid, with the > > > same meanings, but the following are also supported: > > > > > > typemask matches > > > --- > > > eth_type(0x600+)eth_type(0x) specified Ethertype II > > > Ethertype. > > > eth_type(0x600+)eth_type(0)any Ethertype II frame > > > eth_type(0x) any non-Ethernet II frame > > > > > > Is that right? > > > > > > On Wed, Jun 19, 2013 at 10:40:28AM -0700, Andy Zhou wrote: > > > > We will continue to allow missing eth_type in the netlink attribute > to > > > > imply Ethernet II type. 802.3 frames requires a specific eth_type > > > > attribute. > > > > > > I don't understand the first sentence. We have never interpreted a > > > missing eth_type as implying an Ethernet II frame; the opposite, in > > > fact: a missing eth_type matches only non-Ethernet II frames. > > > > > > > With Mega flows, we further require a missing eth_type in the key > > > attribute > > > > to have a exact match (ox) in the eth_type of the mask attribute > (if > > > > present). > > > > > > That's really weird. What's the rationale? > > > > > > Thanks, > > > > > > Ben. > > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 06/11] New function ovs_strerror() as a thread-safe replacement for strerror().
On Wed, Jun 19, 2013 at 02:47:23PM -0700, Ben Pfaff wrote:
> On Wed, Jun 19, 2013 at 05:42:44PM -0400, Ed Maste wrote:
> > On 19 June 2013 16:17, Ben Pfaff wrote:
> > > +#if STRERROR_R_CHAR_P
> > > +/* GNU style strerror_r() might return an immutable static string,
> > > or it
> > > + * might write and return 'buffer', but in either case we can pass
> > > the
> > > + * returned string directly to the caller. */
> > > +s = strerror_r(error, buffer, BUFSIZE);
> > > +#else /* strerror_r() returns an int. */
> > > +s = buffer;
> > > +if (strerror_r(error, buffer, BUFSIZE)) {
> > > +/* strerror_r() is only allowed to fail on ERANGE (because the
> > > buffer
> > > + * is too short). We don't check the actual failure reason
> > > because
> > > + * POSIX requires strerror_r() to return the error but old glibc
> > > + * (before 2.13) returns -1 and sets errno. */
> > > +snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
> >
> > This one fails to build with:
> >
> > lib/util.c:342:26: error: use of undeclared identifier 'ptb'
> > snprintf(buffer, ptb.bufsize, "Unknown error %d", error);
> > ^
>
> Oops, code from an older revision that I failed to re-test after
> changing the implementation pattern.
>
> Thanks, I'll fix that.
I pushed a fix for this issue (just s/ptb.bufsize/BUFSIZE/) and the
ovs-atomic-c11.h issue you also mentioned to the "threads" branch at
https://github.com/blp/ovs-reviews/commits/threads
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 00/11] basic thread support library
On Wed, Jun 19, 2013 at 02:54:11PM -0700, Ben Pfaff wrote: > On Wed, Jun 19, 2013 at 05:45:30PM -0400, Ed Maste wrote: > > On 19 June 2013 17:22, Ben Pfaff wrote: > > > On Wed, Jun 19, 2013 at 05:20:11PM -0400, Ed Maste wrote: > > >> On 19 June 2013 16:17, Ben Pfaff wrote: > > >> > This series has two purposes: > > >> > > > >> > * Add a basic thread support library to the tree. > > >> > > > >> > * Get rid of all calls to C library functions that POSIX describes > > >> > as inherently unsafe in a multithreaded program, and then add a > > >> > make-time check that flags an error if any calls get > > >> > reintroduced. > > >> > > >> This fails to build on FreeBSD for me, because it appears to rely on > > >> header leakage for stdint.h types. > > >> > > >> In file included from lib/ovs-atomic.h:237, > > >> from lib/ovs-thread.h:23, > > >> from lib/command-line.c:22: > > >> lib/ovs-atomic-c11.h:26: error: expected '=', ',', ';', 'asm' or > > >> '__attribute__' before 'uint8_t' > > > > > > Does this mean that FreeBSD has ? Is it fully > > > functional? That's the one implementation I wasn't able to test, > > > because I didn't know of any C11 implementations with . > > > > Indeed we do, and it should be functional. SVN history is here if > > you're interested: > > http://svnweb.freebsd.org/base/head/sys/sys/stdatomic.h > > > > That said, it seems it wasn't stdint.h per se that was the problem; I > > needed to change: > > > > -typedef _Atomic uint8_t atomic_uint8_t; > > +typedef _Atomic(uint8_t) atomic_uint8_t; > > Oh, that's odd. C11 appears to say that the parentheses are optional > with _Atomic in this case (you can use _Atomic(...) as a type > specifier or _Atomic by itself as a type qualifier) but I'll change it > since that fixes the problem. I made that change in the latest version I pushed to the reviews repo. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: Mega flow implementation
What is the reason need for iterating through the mask list itself when destroying the table instead of doing it as the flows are deleted? Doing it that way would both avoid the need to have multiple list deletion functions and the differences between the RCU and non-RCU versions of ovs_flow_free(). One issue that comes to mind is the RCU barrier when the module is unloaded but there are other ways to fix that. On Wed, Jun 19, 2013 at 2:09 PM, Andy Zhou wrote: > Jesse, > > The following patch is an incremental patch on top of yours. > -- Fix a few small bugs introduced by the cleanup patch. > -- Make mask list per table. > > I believe this implementation will address the rcu lock issues we discussed > off line. > > Would you please review? > > Thanks, > > --andy > > On Tue, Jun 18, 2013 at 4:35 PM, Jesse Gross wrote: >> >> On Tue, Jun 18, 2013 at 4:15 PM, Andy Zhou wrote: >> > Add wildcarded flow support in kernel datapath. >> > >> > Wildcarded flow can improve OVS flow set up performance by avoid sending >> > matching new flows to the user space program. The exact performance >> > boost >> > will largely dependent on wildcarded flow hit rate. >> > >> > In case all new flows hits wildcard flows, the flow set up rate is >> > within 5% of that of linux bridge module. >> > >> > Pravin has made significant contributions to this patch. Including API >> > clean ups and bug fixes. >> > >> > Co-authored-by: Pravin B Shelar >> > Signed-off-by: Pravin B Shelar >> > Signed-off-by: Andy Zhou >> >> I made some incremental changes, can you take a look to see if they >> seem reasonable to you? >> >> Here are the main blocks: >> - Additional interface documentation. >> - Fix memory leak when installing a new flow if there is an >> allocation failure for the mask. >> - Require Ethernet addresses to be present in the key, as before. >> - Make SNAP handling a little more integrated with the rest of the >> parsing and validation code. >> - Make ovs_flow_free() and ovs_deferred_flow_free() have exactly the >> same behavior, just with the addition of RCU, to avoid possible >> confusion. >> - Return errors directly rather than through an intermediate variable >> in ovs_flow_extract() and children. >> - Enforce an exact match for the outer EtherType for vlan packets, >> similar to what we do for other protocols. >> - Miscellaneous style fixes. > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: Mega flow implementation
When table is destroyed, there is really no need to keep track of the mask's reference counting -- The masks will have to be destroyed any ways. During run time, the mask_list deletion needs to be protected by the ovs_lock, But we should not require ovs_lock to be held during table destroy. right? So it seems we will have two ways of doing ovs_flow_free() any ways, I will continue to think about how to merge these two cases. Please let me know if you have any suggestions. --andy On Wed, Jun 19, 2013 at 4:09 PM, Jesse Gross wrote: > What is the reason need for iterating through the mask list itself > when destroying the table instead of doing it as the flows are > deleted? Doing it that way would both avoid the need to have multiple > list deletion functions and the differences between the RCU and > non-RCU versions of ovs_flow_free(). > > One issue that comes to mind is the RCU barrier when the module is > unloaded but there are other ways to fix that. > > On Wed, Jun 19, 2013 at 2:09 PM, Andy Zhou wrote: > > Jesse, > > > > The following patch is an incremental patch on top of yours. > > -- Fix a few small bugs introduced by the cleanup patch. > > -- Make mask list per table. > > > > I believe this implementation will address the rcu lock issues we > discussed > > off line. > > > > Would you please review? > > > > Thanks, > > > > --andy > > > > On Tue, Jun 18, 2013 at 4:35 PM, Jesse Gross wrote: > >> > >> On Tue, Jun 18, 2013 at 4:15 PM, Andy Zhou wrote: > >> > Add wildcarded flow support in kernel datapath. > >> > > >> > Wildcarded flow can improve OVS flow set up performance by avoid > sending > >> > matching new flows to the user space program. The exact performance > >> > boost > >> > will largely dependent on wildcarded flow hit rate. > >> > > >> > In case all new flows hits wildcard flows, the flow set up rate is > >> > within 5% of that of linux bridge module. > >> > > >> > Pravin has made significant contributions to this patch. Including API > >> > clean ups and bug fixes. > >> > > >> > Co-authored-by: Pravin B Shelar > >> > Signed-off-by: Pravin B Shelar > >> > Signed-off-by: Andy Zhou > >> > >> I made some incremental changes, can you take a look to see if they > >> seem reasonable to you? > >> > >> Here are the main blocks: > >> - Additional interface documentation. > >> - Fix memory leak when installing a new flow if there is an > >> allocation failure for the mask. > >> - Require Ethernet addresses to be present in the key, as before. > >> - Make SNAP handling a little more integrated with the rest of the > >> parsing and validation code. > >> - Make ovs_flow_free() and ovs_deferred_flow_free() have exactly the > >> same behavior, just with the addition of RCU, to avoid possible > >> confusion. > >> - Return errors directly rather than through an intermediate variable > >> in ovs_flow_extract() and children. > >> - Enforce an exact match for the outer EtherType for vlan packets, > >> similar to what we do for other protocols. > >> - Miscellaneous style fixes. > > > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: Mega flow implementation
I think at this point, the use of a separate RCU callback for the masks is hurting more than it is helping and it would be easier to just piggyback off the flow/table destroy RCU. I'll try to write a patch to see if I can get it to work. On Wed, Jun 19, 2013 at 4:23 PM, Andy Zhou wrote: > When table is destroyed, there is really no need to keep track of the mask's > reference counting -- The masks will have to be destroyed any ways. > > During run time, the mask_list deletion needs to be protected by the > ovs_lock, But we should not require ovs_lock to be held during table > destroy. right? So it seems we will have two ways of doing ovs_flow_free() > any ways, > > I will continue to think about how to merge these two cases. Please let me > know if you have any suggestions. > > --andy > > > On Wed, Jun 19, 2013 at 4:09 PM, Jesse Gross wrote: >> >> What is the reason need for iterating through the mask list itself >> when destroying the table instead of doing it as the flows are >> deleted? Doing it that way would both avoid the need to have multiple >> list deletion functions and the differences between the RCU and >> non-RCU versions of ovs_flow_free(). >> >> One issue that comes to mind is the RCU barrier when the module is >> unloaded but there are other ways to fix that. >> >> On Wed, Jun 19, 2013 at 2:09 PM, Andy Zhou wrote: >> > Jesse, >> > >> > The following patch is an incremental patch on top of yours. >> > -- Fix a few small bugs introduced by the cleanup patch. >> > -- Make mask list per table. >> > >> > I believe this implementation will address the rcu lock issues we >> > discussed >> > off line. >> > >> > Would you please review? >> > >> > Thanks, >> > >> > --andy >> > >> > On Tue, Jun 18, 2013 at 4:35 PM, Jesse Gross wrote: >> >> >> >> On Tue, Jun 18, 2013 at 4:15 PM, Andy Zhou wrote: >> >> > Add wildcarded flow support in kernel datapath. >> >> > >> >> > Wildcarded flow can improve OVS flow set up performance by avoid >> >> > sending >> >> > matching new flows to the user space program. The exact performance >> >> > boost >> >> > will largely dependent on wildcarded flow hit rate. >> >> > >> >> > In case all new flows hits wildcard flows, the flow set up rate is >> >> > within 5% of that of linux bridge module. >> >> > >> >> > Pravin has made significant contributions to this patch. Including >> >> > API >> >> > clean ups and bug fixes. >> >> > >> >> > Co-authored-by: Pravin B Shelar >> >> > Signed-off-by: Pravin B Shelar >> >> > Signed-off-by: Andy Zhou >> >> >> >> I made some incremental changes, can you take a look to see if they >> >> seem reasonable to you? >> >> >> >> Here are the main blocks: >> >> - Additional interface documentation. >> >> - Fix memory leak when installing a new flow if there is an >> >> allocation failure for the mask. >> >> - Require Ethernet addresses to be present in the key, as before. >> >> - Make SNAP handling a little more integrated with the rest of the >> >> parsing and validation code. >> >> - Make ovs_flow_free() and ovs_deferred_flow_free() have exactly the >> >> same behavior, just with the addition of RCU, to avoid possible >> >> confusion. >> >> - Return errors directly rather than through an intermediate variable >> >> in ovs_flow_extract() and children. >> >> - Enforce an exact match for the outer EtherType for vlan packets, >> >> similar to what we do for other protocols. >> >> - Miscellaneous style fixes. >> > >> > > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] vswitchd: Update flow-eviction-threshold documentation
Patch 27a88d1373cbfcceac6d901bbf1c17051aa7845f caused the vswitchd documentation and the code to digress. This brings them back in line. Signed-off-by: Joe Stringer --- vswitchd/vswitch.xml |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index c6750d6..b962849 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -133,7 +133,7 @@ usage and packet loss. - The default is 1000. Values below 100 will be rounded up to 100. + The default is 2500. Values below 100 will be rounded up to 100. -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v13 4/4] Add MPLS recirculation tests
From: Joe Stringer
This patch introduces a python script to generate about 1500 tests for
permutations of mpls_push,mpls_pop,dec_mpls_ttl,dec_ttl where
recirculation occurs up to (and including) three times.
Signed-off-by: Joe Stringer
Signed-off-by: Simon Horman
---
v13
* Use the new force-miss-model config option to test MPLS on ofproto-dpif
through the code paths for both handle_flow_miss_with_facet() and
handle_flow_miss_without_facet().
v12
* First post
---
tests/automake.mk | 1 +
tests/ofproto-dpif.at | 114 +
tests/test-mpls.py| 455 ++
3 files changed, 570 insertions(+)
create mode 100644 tests/test-mpls.py
diff --git a/tests/automake.mk b/tests/automake.mk
index 1af41cc..ff2ee2e 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -320,6 +320,7 @@ CHECK_PYFILES = \
tests/test-jsonrpc.py \
tests/test-ovsdb.py \
tests/test-reconnect.py \
+ tests/test-mpls.py \
tests/MockXenAPI.py \
tests/test-unix-socket.py \
tests/test-unixctl.py \
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index bb86541..7c6d423 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1224,6 +1224,120 @@ NXST_FLOW reply:
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - MPLS recirculation])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=1
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 1])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS recirculation, no facets])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=2
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 1])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS double recirculation])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=1
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..150..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 2 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS double recirculation, no facets])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=2
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..150..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 2 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS multiple recirculation])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=1
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..550..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 3 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS multiple recirculation, no facets])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=2
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {0..550..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 3 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS multiple recirculation 2])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=1
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {600..1200..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 3 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - MPLS multiple recirculation 2, no facets])
+OVS_VSWITCHD_START([dnl
+ add-port br0 p1 -- set Interface p1 type=dummy -- \
+ set Open_vSwitch . other_config:force-miss-model=2
+])
+
+AT_CAPTURE_FILE([ofctl_monitor.log])
+
+for i in {600..1200..50}; do
+AT_CHECK([$PYTHON $srcdir/test-mpls.py 3 $i])
+done
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - VLAN handling])
OVS_VSWITCHD_START(
[set Bridge br0 fail-mode=standalone -- \
diff --git a/tests/test-mpls.py b/tests/test-mpls.py
new file mode 100644
index 000..748472e
--- /dev/null
+++ b/tests/test-mpls.py
@@ -0,0 +1,455 @@
+# Copyright (c) 2013 Joe Stringer
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at:
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import argparse
+i
[ovs-dev] [PATCH v13 0/4] Add packet recirculation
Recirculation is a technique to allow a frame to re-enter
frame processing. This is intended to be used after actions
have been applied to the frame with modify the frame in
some way that makes it possible for richer processing to occur.
An example is and indeed targeted use case is MPLS. If an MPLS frame has an
mpls_pop action applied with the IPv4 ethernet type then it becomes
possible to decode the IPv4 portion of the frame. This may be used to
construct a facet that modifies the IPv4 portion of the frame. This is not
possible prior to the mpls_pop action as the contents of the frame after
the MPLS stack is not known to be IPv4.
Design:
* New recirculation action.
ovs-vswitchd adds a recirculation action to the end of a list of
datapath actions for a flow when the actions are truncated because
insufficient flow match information is available to add the next
OpenFlow action. The recirculation action is preceded by an action
to set the skb_mark to an id which can be used to scope a facet lookup
of a recirculated packet.
e.g. pop_mpls(0x0800),dec_ttl becomes
pop_mpls(0x800),set(skb_mark(id)),recirculate
* Datapath behaviour
Then the datapath encounters a recirculate action it:
+ Recalculates the flow key based on the packet
which will typically have been modified by previous actions
+ As the recirculate action is preceded by a set(skb_mark(id)) action,
the new match key will now include skb_mark=id.
+ Performs a lookup using the new match key
+ Processes the packet if a facet matches the key or;
+ Makes an upcall if necessary
* No facet behaviour
+ Loop:
1) translate actions
2) If there is a recirculate action, execute packet
and go back to 1) for remaining actions.
Base/Pre-requisites:
This series applies to the git master branch on openvswtich.org
and does not have any pre-requisites.
Availability:
For reference this series is available in git at:
git://github.com/horms/openvswitch.git devel/mpls-recirculate.v13
Change Log:
v13
* Add patch to allow configuration of flow miss handling to force
handling with or without facets. This is used by the
patch to exercise recirculation using ofproto-dpif.
* Extensive rebase for megaflow changes
v12
* Rebase
* Add much more extensive tests by Joe Stringer
* Ensure pre_push_mpls_lse is never used uninitialised in do_xlate_actions()
* Copy flow tunnel when recirculating in xlate_and_recirculate().
This avoids a bug that occurred when recirculating more than once
and thus using flow_storage as the flow passed to flow_extract()
and flow_storage.tunnel as the tunnel passed to flow_extract().
The tunnel parameter passed to flow_extract() must not be
the tunnel element of its flow parameter.
* In xlate_and_recirculate() ensure that there is sufficient headroom in
packet for both the key, which is present if xlate_and_recirculate()
is called by handle_flow_miss_without_facet(), and an MPLS LSE in case
mpls_push() occurs.
* As suggested by Ben Pfaff
- Do not unnecessarily rename flow variable in dp_netdev_port_input()
- Rename tun_key_from_attr() as odp_tun_key_from_attr() as it is now
exported
v11
* Add patches to handle multiple levels of push and pop
* Addressed review by Jarno Rajahalme
v10
* As suggested by Ben Pfaff
- Rename lib/execute-actions.[ch] as lib/odp-execute.[ch]
- Sort headers in alphabetical order
- Minimise includes in lib/odp-execute.h
- Include lib/odp-execute.h immediately after config.h in lib/odp-execute.c
- Correct coding style of prototype of tun_key_from_attr
- Remove dubious memset from set_tunnel_action
- use {} with if statement for OVS_ACTION_ATTR_OUTPUT in execute_actions()
- Do not put required side effects in conditions for ovs_assert();
- Remove execute_actions_for_recircualtion() helper and
use odp_execute_actions directly()
v9
* As suggested by Jesse Gross
- Follow the convention on other code that when passing around
a void pointer and casting it, the variable named with an underscore
is the void pointer.
- Move extraction of userspace data into execute_actions().
Previously it was in the userspace callback passed to execute_actions().
- Remove ovs_assert() calls from execute_actions().
They seem unnecessary at best.
- Reduce the number of new parameters added to functions,
in many cases to zero, by using the skb_priority, skb_mark
and tunnel elements of struct flow.
- Add patch to make use of execute_actions() to execute controller actions
v8
* For a learn action recirculate regardless of the value of
ctx->may_learn in do_xlate_actions(). Previously recirculation
only occurred if ctx->may_learn was true. That is a bug as it causes
revalidation to fail. Flagged by the tests developed as part of a patch
to allow multiple MPLS push and poop actions.
* Update tests removing unnecessary dl_type pre-requisite
v7
* Rebase
* Add skb_priority, skb_mark and tun_key parameters to dp_netdev_port_input()
This allows th
[ovs-dev] [PATCH v13 1/4] ofproto-dpif: Add 'force-miss-model' configuration
From: Joe Stringer
This adds support for specifying flow miss handling behaviour at
runtime, through a new "other-config" option in the Open_vSwitch table.
This is an extension to flow-eviction-threshold.
By default, the behaviour is the same as before. If force-miss-model is
set to 1, then flow miss handling will always result in the creation of
new facets and flow-eviction-threshold will be ignored. If
force-miss-model is set to 2, then flow miss handling will never result
in the creation of new facets (effectively the same as setting the
flow-eviction-threshold to 0, which is not currently configurable).
We intend to use this configuration option in the testsuite to force
particular code paths to be used, allowing us to improve test coverage.
Signed-off-by: Joe Stringer
Signed-off-by: Simon Horman
---
v13
* First post
---
ofproto/ofproto-dpif.c | 9 +
ofproto/ofproto-provider.h | 4
ofproto/ofproto.c | 17 +
ofproto/ofproto.h | 9 +
vswitchd/bridge.c | 4
vswitchd/vswitch.xml | 23 +++
6 files changed, 66 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e3acbc6..037dfef 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3347,6 +3347,15 @@ flow_miss_should_make_facet(struct flow_miss *miss,
struct flow_wildcards *wc)
struct dpif_backer *backer = miss->ofproto->backer;
uint32_t hash;
+switch (flow_miss_model) {
+case OFPROTO_HANDLE_MISS_WITH_FACET:
+return true;
+case OFPROTO_HANDLE_MISS_WITHOUT_FACET:
+return false;
+default:
+break;
+}
+
if (!backer->governor) {
size_t n_subfacets;
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 41b589f..692d53c 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -238,6 +238,10 @@ struct rule {
* ofproto-dpif implementation */
extern unsigned flow_eviction_threshold;
+/* Determines which model to use for handling misses in the ofproto-dpif
+ * implementation */
+extern unsigned flow_miss_model;
+
static inline struct rule *
rule_from_cls_rule(const struct cls_rule *cls_rule)
{
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index eabe850..2996ce5 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -221,6 +221,7 @@ static size_t n_ofproto_classes;
static size_t allocated_ofproto_classes;
unsigned flow_eviction_threshold = OFPROTO_FLOW_EVICTION_THRESHOLD_DEFAULT;
+unsigned flow_miss_model = OFPROTO_HANDLE_MISS_AUTO;
/* Map from datapath name to struct ofproto, for use by unixctl commands. */
static struct hmap all_ofprotos = HMAP_INITIALIZER(&all_ofprotos);
@@ -573,6 +574,22 @@ ofproto_set_flow_eviction_threshold(unsigned threshold)
threshold);
}
+/* Sets the path for handling flow misses. */
+void
+ofproto_set_flow_miss_model(unsigned model)
+{
+switch (model) {
+case OFPROTO_HANDLE_MISS_WITH_FACET:
+VLOG_INFO("Handling all misses by creating facets.\n");
+case OFPROTO_HANDLE_MISS_WITHOUT_FACET:
+VLOG_INFO("Handling all misses without creating facets.\n");
+}
+
+if (OFPROTO_HANDLE_MISS_AUTO <= model && model < OFPROTO_HANDLE_MISS_MAX) {
+flow_miss_model = model;
+}
+}
+
/* If forward_bpdu is true, the NORMAL action will forward frames with
* reserved (e.g. STP) destination Ethernet addresses. if forward_bpdu is
false,
* the NORMAL action will drop these frames. */
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 5f8244c..664e9e4 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -216,6 +216,14 @@ int ofproto_port_dump_done(struct ofproto_port_dump *);
#define OFPROTO_FLOW_EVICTION_THRESHOLD_DEFAULT 2500
#define OFPROTO_FLOW_EVICTION_THRESHOLD_MIN 100
+/* How flow misses should be handled in ofproto-dpif */
+enum ofproto_flow_miss_model {
+OFPROTO_HANDLE_MISS_AUTO, /* Based on flow eviction threshold. */
+OFPROTO_HANDLE_MISS_WITH_FACET, /* Always create facets. */
+OFPROTO_HANDLE_MISS_WITHOUT_FACET, /* Always handle without facets.*/
+OFPROTO_HANDLE_MISS_MAX
+};
+
const char *ofproto_port_open_type(const char *datapath_type,
const char *port_type);
int ofproto_port_add(struct ofproto *, struct netdev *, uint16_t *ofp_portp);
@@ -237,6 +245,7 @@ void ofproto_set_extra_in_band_remotes(struct ofproto *,
const struct sockaddr_in *, size_t n);
void ofproto_set_in_band_queue(struct ofproto *, int queue_id);
void ofproto_set_flow_eviction_threshold(unsigned threshold);
+void ofproto_set_flow_miss_model(unsigned model);
void ofproto_set_forward_bpdu(struct ofproto *, bool forward_bpdu);
void ofproto_set_mac_table_config(struct ofproto *, unsigned idle_time,
size_t max_entries);
diff --git a/vswitchd/bridge.c b/vswitch
Re: [ovs-dev] [PATCH v2.33] datapath: Add basic MPLS support to kernel
On Tue, Jun 18, 2013 at 04:06:49PM +0900, Simon Horman wrote: > Allow datapath to recognize and extract MPLS labels into flow keys > and execute actions which push, pop, and set labels on packets. > > Based heavily on work by Leo Alterman, Ravi K, Isaku Yamahata and Joe > Stringer. > > Cc: Ravi K > Cc: Leo Alterman > Cc: Isaku Yamahata > Cc: Joe Stringer > Signed-off-by: Simon Horman > > --- > > This patch depends on "gre: Restructure tunneling" which it aims > to be compatible with. To clarify. The dependency relates to a conflict when applying this patch which modifies datapath/linux/compat/gso.[ch], files that are created by "gre: Restructure tunneling". I believe it would be trivial to reverse the dependency so that this patch creates those files and "gre: Restructure tunneling" applies on top of it as the two patches add different functions to those files. As such I think it would be better to describe this patch as compatible with "gre: Restructure tunneling" rather than dependent on it. > > This is the remaining patch of the series "MPLS actions and matches". > To aid review it and its dependency are available in git at: > > git://github.com/horms/openvswitch.git devel/mpls-v2.33 > > v2.33 > * Ensure that inner_protocol is always set to to the current > skb->protocol value in ovs_execute_actions(). This ensures > it is set to the correct value in the absence of a push_mpls action. > Also remove setting of inner_protocol in push_mpls() as > it duplicates the code now in ovs_execute_actions(). > * Call __skb_gso_segment() instead of skb_gso_segment() from > rpl___skb_gso_segment() in the case that HAVE___SKB_GSO_SEGMENT is set. > This was a typo. > > v2.32 > * As suggested by Jesse Gross > - Use int instead of size_t in validate_and_copy_actions__(). > - Fix crazy edit mess in pop_mpls() action comment > - Move eth_p_mpls() into mpls.h > - Refactor skb_gso_segment MPLS handling into rpl_skb_gso_segment > Address Jesse's comments regarding this code: > "Can we push this completely into the skb_gso_segment() compatibility > code? It's both nicer and may make the interactions with the vlan code > less confusing." > - Move GSO compatibility code into linux/compat/gso.* > - Set skb->protocol on mpls_push and mpls_pop in the presence > of an offloaded VLAN. > > v2.31 > * As suggested by Jesse Gross > - There is no need to make mac_header_end inline as it is not in a header > file > - Remove dubious if (*skb_ethertype == ethertype) optimisation from > set_ethertype > - Only set skb->protocol in push_mpls() or pop_mpls() for non-VLAN packets > - Use MAX_ETH_TYPES instead of SAMPLE_ACTION_DEPTH for array size > of types in struct eth_types. This corrects a typo/thinko. > - Correct eth type tracking logic such that start isn't advanced > when entering a sample action, ensuring that all possibly types > are checked when verifying nested actions. > * Define HAVE_INNER_PROTOCOL based on kernel version. > inner_protocol has been merged into net-next and should appear in > v3.11 so there is no longer a need for a acinclude.m4 test to check for it. > * Add MPLS GSO compatibility code. > This is for use on kernels that do not have MPLS GSO support. > Thanks to Joe Stringer for his work on this. > > v2.30 > * As suggested by Jesse Gross > - Use skb_cow_head in push_mpls to ensure there is sufficient headroom for > skb_push > - Call make_writable with skb->mac_len instead of skb->mac_len + MPLS_HLEN > in push_mpls as only the first skb->mac_len bytes of existing packet data > are modified. > - Rename skb_mac_header_end as mac_header_end, this seems > to be a more appropriate name for a local function. > - Remove OVS_CSUM_COMPLETE code from set_ethertype(). > Inside OVS the ethernet header is not covered by OVS_CSUM_COMPLETE. > - Use __skb_pull() instead of skb_pull() in pop_mpls() > - Decrement and decrement skb->mac_len when poping and pushing VLAN tags. > Previously mac_len was reset, but this would result in forgetting > the MPLS label stack. > - Remove spurious comment from before do_execute_actions(). > - Move OVS_KEY_ATTR_MPLS attribute to its final, upstreamable, location. > - Correct ethertype check for OVS_ACTION_ATTR_POP_MPLS case in > validate_and_copy_actions() to check for MPLS ethertypes rather than > ETH_P_IP. > - Rewrite tracking of eth types used to verify actions in the presence > of sample actions. There is a large comment above struct eth_types > describing the new implementation. > > v2.29 > * Break include/ and lib/ portions of the patch out into a > separate patch "datapath: Add basic MPLS support to kernel" > * Update for new MPLS GSO scheme > - skb->protocol is set to the new ethertype of the packet > on MPLS push and pop > - When pushing the first MPLS LSE onto a previously non-MPLS > packet set skb->inner_prot
Re: [ovs-dev] [PATCH net-next v2 00/12] openvswitch: gre tunneling support.
From: Pravin B Shelar Date: Mon, 17 Jun 2013 17:49:21 -0700 > Following patch series adds support for gre tunneling. > First six patches extend kernel gre and ip_tunnel modules > api so that there is more code sharing between gre modules > and ovs. Rest of patches adds ovs tunneling infrastructre > and gre protocol vport. > > V2 fixes two patches according to comments from Jesse. Series applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 00/11] basic thread support library
On 19 June 2013 17:54, Ben Pfaff wrote: > Oh, that's odd. C11 appears to say that the parentheses are optional > with _Atomic in this case (you can use _Atomic(...) as a type > specifier or _Atomic by itself as a type qualifier) but I'll change it > since that fixes the problem. Ahh, it looks like _Atomic as a type qualifier is a recent addition to clang: http://llvm.org/viewvc/llvm-project?view=revision&revision=178210 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 06/11] New function ovs_strerror() as a thread-safe replacement for strerror().
On 19 June 2013 18:48, Ben Pfaff wrote: > I pushed a fix for this issue (just s/ptb.bufsize/BUFSIZE/) and the > ovs-atomic-c11.h issue you also mentioned to the "threads" branch at > https://github.com/blp/ovs-reviews/commits/threads Thanks Ben, with those two changes your threads branch passes unit tests for me on FreeBSD 9. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: Mega flow implementation
OK, that idea turned out to be a bad one since masks really do have a lifetime that is separate from an individual flow. However, here's a patch that does the unification a different way. What do you think? On Wed, Jun 19, 2013 at 4:32 PM, Jesse Gross wrote: > I think at this point, the use of a separate RCU callback for the > masks is hurting more than it is helping and it would be easier to > just piggyback off the flow/table destroy RCU. I'll try to write a > patch to see if I can get it to work. > > On Wed, Jun 19, 2013 at 4:23 PM, Andy Zhou wrote: >> When table is destroyed, there is really no need to keep track of the mask's >> reference counting -- The masks will have to be destroyed any ways. >> >> During run time, the mask_list deletion needs to be protected by the >> ovs_lock, But we should not require ovs_lock to be held during table >> destroy. right? So it seems we will have two ways of doing ovs_flow_free() >> any ways, >> >> I will continue to think about how to merge these two cases. Please let me >> know if you have any suggestions. >> >> --andy >> >> >> On Wed, Jun 19, 2013 at 4:09 PM, Jesse Gross wrote: >>> >>> What is the reason need for iterating through the mask list itself >>> when destroying the table instead of doing it as the flows are >>> deleted? Doing it that way would both avoid the need to have multiple >>> list deletion functions and the differences between the RCU and >>> non-RCU versions of ovs_flow_free(). >>> >>> One issue that comes to mind is the RCU barrier when the module is >>> unloaded but there are other ways to fix that. >>> >>> On Wed, Jun 19, 2013 at 2:09 PM, Andy Zhou wrote: >>> > Jesse, >>> > >>> > The following patch is an incremental patch on top of yours. >>> > -- Fix a few small bugs introduced by the cleanup patch. >>> > -- Make mask list per table. >>> > >>> > I believe this implementation will address the rcu lock issues we >>> > discussed >>> > off line. >>> > >>> > Would you please review? >>> > >>> > Thanks, >>> > >>> > --andy >>> > >>> > On Tue, Jun 18, 2013 at 4:35 PM, Jesse Gross wrote: >>> >> >>> >> On Tue, Jun 18, 2013 at 4:15 PM, Andy Zhou wrote: >>> >> > Add wildcarded flow support in kernel datapath. >>> >> > >>> >> > Wildcarded flow can improve OVS flow set up performance by avoid >>> >> > sending >>> >> > matching new flows to the user space program. The exact performance >>> >> > boost >>> >> > will largely dependent on wildcarded flow hit rate. >>> >> > >>> >> > In case all new flows hits wildcard flows, the flow set up rate is >>> >> > within 5% of that of linux bridge module. >>> >> > >>> >> > Pravin has made significant contributions to this patch. Including >>> >> > API >>> >> > clean ups and bug fixes. >>> >> > >>> >> > Co-authored-by: Pravin B Shelar >>> >> > Signed-off-by: Pravin B Shelar >>> >> > Signed-off-by: Andy Zhou >>> >> >>> >> I made some incremental changes, can you take a look to see if they >>> >> seem reasonable to you? >>> >> >>> >> Here are the main blocks: >>> >> - Additional interface documentation. >>> >> - Fix memory leak when installing a new flow if there is an >>> >> allocation failure for the mask. >>> >> - Require Ethernet addresses to be present in the key, as before. >>> >> - Make SNAP handling a little more integrated with the rest of the >>> >> parsing and validation code. >>> >> - Make ovs_flow_free() and ovs_deferred_flow_free() have exactly the >>> >> same behavior, just with the addition of RCU, to avoid possible >>> >> confusion. >>> >> - Return errors directly rather than through an intermediate variable >>> >> in ovs_flow_extract() and children. >>> >> - Enforce an exact match for the outer EtherType for vlan packets, >>> >> similar to what we do for other protocols. >>> >> - Miscellaneous style fixes. >>> > >>> > >> >> 0001-Mask-deletion.patch Description: Binary data ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCHv3] ofproto-dpif: Tighten up megaflow wildcard handling.
A number of use-cases weren't handled properly when determining what can
be wildcarded for megaflows. This commit both catches additional fields
that cannot be wildcarded and loosens a few other cases.
Bug #17979
Signed-off-by: Justin Pettit
---
v1 -> v2: Remove IPFIX un-wildcarding.
v2 -> v3: Un-wildcard other fields affected by DPIF set actions.
---
lib/flow.c |2 +-
lib/odp-util.c | 73 +-
lib/odp-util.h |4 +-
ofproto/netflow.c| 12 ++
ofproto/netflow.h|2 +
ofproto/ofproto-dpif-xlate.c | 41 ++-
ofproto/ofproto-dpif.c |4 ++
tests/ofproto-dpif.at| 88 +-
8 files changed, 149 insertions(+), 77 deletions(-)
diff --git a/lib/flow.c b/lib/flow.c
index d38e3ab..3e50734 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -790,12 +790,12 @@ flow_mask_hash_fields(struct flow_wildcards *wc, enum
nx_hash_fields fields)
memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
-memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
memset(&wc->masks.tp_src, 0xff, sizeof wc->masks.tp_src);
memset(&wc->masks.tp_dst, 0xff, sizeof wc->masks.tp_dst);
+wc->masks.vlan_tci |= htons(VLAN_VID_MASK | VLAN_CFI);
break;
default:
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 588b3b6..c4127ea 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2233,7 +2233,8 @@ commit_odp_tunnel_action(const struct flow *flow, struct
flow *base,
static void
commit_set_ether_addr_action(const struct flow *flow, struct flow *base,
- struct ofpbuf *odp_actions)
+ struct ofpbuf *odp_actions,
+ struct flow_wildcards *wc)
{
struct ovs_key_ethernet eth_key;
@@ -2242,6 +2243,9 @@ commit_set_ether_addr_action(const struct flow *flow,
struct flow *base,
return;
}
+memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
+memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
+
memcpy(base->dl_src, flow->dl_src, ETH_ADDR_LEN);
memcpy(base->dl_dst, flow->dl_dst, ETH_ADDR_LEN);
@@ -2254,12 +2258,14 @@ commit_set_ether_addr_action(const struct flow *flow,
struct flow *base,
static void
commit_vlan_action(const struct flow *flow, struct flow *base,
- struct ofpbuf *odp_actions)
+ struct ofpbuf *odp_actions, struct flow_wildcards *wc)
{
if (base->vlan_tci == flow->vlan_tci) {
return;
}
+memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
+
if (base->vlan_tci & htons(VLAN_CFI)) {
nl_msg_put_flag(odp_actions, OVS_ACTION_ATTR_POP_VLAN);
}
@@ -2277,13 +2283,15 @@ commit_vlan_action(const struct flow *flow, struct flow
*base,
static void
commit_mpls_action(const struct flow *flow, struct flow *base,
- struct ofpbuf *odp_actions)
+ struct ofpbuf *odp_actions, struct flow_wildcards *wc)
{
if (flow->mpls_lse == base->mpls_lse &&
flow->mpls_depth == base->mpls_depth) {
return;
}
+memset(&wc->masks.mpls_lse, 0xff, sizeof wc->masks.mpls_lse);
+
if (flow->mpls_depth < base->mpls_depth) {
if (base->mpls_depth - flow->mpls_depth > 1) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(10, 10);
@@ -2321,7 +2329,7 @@ commit_mpls_action(const struct flow *flow, struct flow
*base,
static void
commit_set_ipv4_action(const struct flow *flow, struct flow *base,
- struct ofpbuf *odp_actions)
+ struct ofpbuf *odp_actions, struct flow_wildcards *wc)
{
struct ovs_key_ipv4 ipv4_key;
@@ -2333,6 +2341,13 @@ commit_set_ipv4_action(const struct flow *flow, struct
flow *base,
return;
}
+memset(&wc->masks.nw_src, 0xff, sizeof wc->masks.nw_src);
+memset(&wc->masks.nw_dst, 0xff, sizeof wc->masks.nw_dst);
+memset(&wc->masks.nw_tos, 0xff, sizeof wc->masks.nw_tos);
+memset(&wc->masks.nw_ttl, 0xff, sizeof wc->masks.nw_ttl);
+memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+memset(&wc->masks.nw_frag, 0xff, sizeof wc->masks.nw_frag);
+
ipv4_key.ipv4_src = base->nw_src = flow->nw_src;
ipv4_key.ipv4_dst = base->nw_dst = flow->nw_dst;
ipv4_key.ipv4_tos = base->nw_tos = flow->nw_tos;
@@ -2346,7 +2361,7 @@ commit_set_ipv4_action(const struct flow *flow, struct
flow *base,
static void
commit_set_ipv6_action(const struct flow *flow, struct flo
Re: [ovs-dev] [PATCH 2/2] ovs-dpctl: Add mega flow support
I added most of the ones that seemed important during my cleanups yesterday. The only other thing that comes to mind is a description of how updates are handled (such as the fact that the new mask is ignored). On Wed, Jun 19, 2013 at 3:04 PM, Andy Zhou wrote: > Good idea. There may be other aspects of the mega related kernel ABI needs > document as well. Wonder if Justin and Jesse have more inputs on this. > > > On Wed, Jun 19, 2013 at 2:55 PM, Ben Pfaff wrote: >> >> OK, can we add that in the tree somewhere, maybe datapath/README? >> >> Thanks, >> >> Ben. >> >> On Wed, Jun 19, 2013 at 02:52:52PM -0700, Andy Zhou wrote: >> > It is a good idea to capture this information in a table. The >> > following >> > table should be accurate: >> > >> > Pre-megaflow: >> > >> > typemask matches >> > --- >> > eth_type(0x600+) specified Ethertype II, or valid >> > 802.3 >> > SNAP >> >packet with valid >> > eth_type. >> > Ethertype. >> > any non-Ethernet II frame, >> > except >> > valid 802.3 SNAP >> >packet with valid >> > eth_type. >> > >> > Post-megaflow: >> > >> > typemask matches >> > --- >> > eth_type(0x600+)eth_type(0x) specified Ethertype II >> > Ethertype, or valid 802.3 SNAP packet with valid eth_type. >> > eth_type(0x600+)eth_type(0)any Ethertype II frame or >> > non-Ethernet II frame. >> > eth_type(0x) any non-Ethernet II frame, >> > except valid 802.3 SNAP packet with valid eth_type. >> > >> > --andy >> > >> > >> > >> > >> > On Wed, Jun 19, 2013 at 1:42 PM, Ben Pfaff wrote: >> > >> > > I don't really care about the formatting, only about the kernel ABI. >> > > >> > > Pre-megaflows, the ABI was: >> > > >> > > typemask matches >> > > --- >> > > eth_type(0x600+) specified Ethertype II >> > > Ethertype. >> > >any non-Ethernet II frame >> > > >> > > Now, my understanding is that the above continue to be valid, with the >> > > same meanings, but the following are also supported: >> > > >> > > typemask matches >> > > --- >> > > eth_type(0x600+)eth_type(0x) specified Ethertype II >> > > Ethertype. >> > > eth_type(0x600+)eth_type(0)any Ethertype II frame >> > > eth_type(0x) any non-Ethernet II frame >> > > >> > > Is that right? >> > > >> > > On Wed, Jun 19, 2013 at 10:40:28AM -0700, Andy Zhou wrote: >> > > > We will continue to allow missing eth_type in the netlink attribute >> > > > to >> > > > imply Ethernet II type. 802.3 frames requires a specific eth_type >> > > > attribute. >> > > >> > > I don't understand the first sentence. We have never interpreted a >> > > missing eth_type as implying an Ethernet II frame; the opposite, in >> > > fact: a missing eth_type matches only non-Ethernet II frames. >> > > >> > > > With Mega flows, we further require a missing eth_type in the key >> > > attribute >> > > > to have a exact match (ox) in the eth_type of the mask attribute >> > > > (if >> > > > present). >> > > >> > > That's really weird. What's the rationale? >> > > >> > > Thanks, >> > > >> > > Ben. >> > > > > > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-dpctl: Add mega flow support
Good to hear, thank you. On Wed, Jun 19, 2013 at 06:42:59PM -0700, Jesse Gross wrote: > I added most of the ones that seemed important during my cleanups > yesterday. The only other thing that comes to mind is a description of > how updates are handled (such as the fact that the new mask is > ignored). > > On Wed, Jun 19, 2013 at 3:04 PM, Andy Zhou wrote: > > Good idea. There may be other aspects of the mega related kernel ABI needs > > document as well. Wonder if Justin and Jesse have more inputs on this. > > > > > > On Wed, Jun 19, 2013 at 2:55 PM, Ben Pfaff wrote: > >> > >> OK, can we add that in the tree somewhere, maybe datapath/README? > >> > >> Thanks, > >> > >> Ben. > >> > >> On Wed, Jun 19, 2013 at 02:52:52PM -0700, Andy Zhou wrote: > >> > It is a good idea to capture this information in a table. The > >> > following > >> > table should be accurate: > >> > > >> > Pre-megaflow: > >> > > >> > typemask matches > >> > --- > >> > eth_type(0x600+) specified Ethertype II, or valid > >> > 802.3 > >> > SNAP > >> >packet with valid > >> > eth_type. > >> > Ethertype. > >> > any non-Ethernet II frame, > >> > except > >> > valid 802.3 SNAP > >> >packet with valid > >> > eth_type. > >> > > >> > Post-megaflow: > >> > > >> > typemask matches > >> > --- > >> > eth_type(0x600+)eth_type(0x) specified Ethertype II > >> > Ethertype, or valid 802.3 SNAP packet with valid eth_type. > >> > eth_type(0x600+)eth_type(0)any Ethertype II frame or > >> > non-Ethernet II frame. > >> > eth_type(0x) any non-Ethernet II frame, > >> > except valid 802.3 SNAP packet with valid eth_type. > >> > > >> > --andy > >> > > >> > > >> > > >> > > >> > On Wed, Jun 19, 2013 at 1:42 PM, Ben Pfaff wrote: > >> > > >> > > I don't really care about the formatting, only about the kernel ABI. > >> > > > >> > > Pre-megaflows, the ABI was: > >> > > > >> > > typemask matches > >> > > --- > >> > > eth_type(0x600+) specified Ethertype II > >> > > Ethertype. > >> > >any non-Ethernet II frame > >> > > > >> > > Now, my understanding is that the above continue to be valid, with the > >> > > same meanings, but the following are also supported: > >> > > > >> > > typemask matches > >> > > --- > >> > > eth_type(0x600+)eth_type(0x) specified Ethertype II > >> > > Ethertype. > >> > > eth_type(0x600+)eth_type(0)any Ethertype II frame > >> > > eth_type(0x) any non-Ethernet II frame > >> > > > >> > > Is that right? > >> > > > >> > > On Wed, Jun 19, 2013 at 10:40:28AM -0700, Andy Zhou wrote: > >> > > > We will continue to allow missing eth_type in the netlink attribute > >> > > > to > >> > > > imply Ethernet II type. 802.3 frames requires a specific eth_type > >> > > > attribute. > >> > > > >> > > I don't understand the first sentence. We have never interpreted a > >> > > missing eth_type as implying an Ethernet II frame; the opposite, in > >> > > fact: a missing eth_type matches only non-Ethernet II frames. > >> > > > >> > > > With Mega flows, we further require a missing eth_type in the key > >> > > attribute > >> > > > to have a exact match (ox) in the eth_type of the mask attribute > >> > > > (if > >> > > > present). > >> > > > >> > > That's really weird. What's the rationale? > >> > > > >> > > Thanks, > >> > > > >> > > Ben. > >> > > > > > > > > > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-vsctl: Improve error message for "ovs-vsctl del-port ".
Yes, especially if it passed the unit test, looks correct.
On Wed, Jun 19, 2013 at 3:26 PM, Ben Pfaff wrote:
> Thanks. Is that a review?
>
> On Tue, Jun 18, 2013 at 09:27:18PM -0700, Reid Price wrote:
> > Thanks, that seems like the right amount of information
> >
> > -Reid
> >
> >
> > On Tue, Jun 18, 2013 at 9:02 PM, Ben Pfaff wrote:
> >
> > > Previously, commands like this:
> > > ovs-vsctl add-br br0
> > > ovs-vsctl del-port br0
> > > yielded an error message like:
> > > no port named br0
> > > which is confusing. This commit improves the error message to:
> > > cannot delete port br0 because it is the local port for bridge br0
> > > (deleting this port requires deleting the entire bridge)
> > >
> > > Bug #17994.
> > > Reported-by: Reid Price
> > > Signed-off-by: Ben Pfaff
> > > ---
> > > tests/ovs-vsctl.at|6 +-
> > > utilities/ovs-vsctl.c | 10 +++---
> > > 2 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > > index ec08cd4..fa2c3ff 100644
> > > --- a/tests/ovs-vsctl.at
> > > +++ b/tests/ovs-vsctl.at
> > > @@ -323,6 +323,10 @@ AT_CHECK([RUN_OVS_VSCTL(
> > > ], [], [OVS_VSCTL_CLEANUP])
> > > AT_CHECK([RUN_OVS_VSCTL([--may-exist add-port b b1])], [0], [], [],
> > >[OVS_VSCTL_CLEANUP])
> > > +AT_CHECK([RUN_OVS_VSCTL([del-port a])], [1], [],
> > > + [ovs-vsctl: cannot delete port a because it is the local port for
> > > bridge a (deleting this port requires deleting the entire bridge)
> > > +],
> > > + [OVS_VSCTL_CLEANUP])
> > > AT_CHECK([RUN_OVS_VSCTL([--may-exist add-port a b1])], [1], [],
> > >[ovs-vsctl: "--may-exist add-port a b1" but b1 is actually attached
> to
> > > bridge b
> > > ],
> > > @@ -1246,4 +1250,4 @@ AT_CHECK([sed "/|bridge|WARN|/d"
> ovs-vswitchd.log >
> > > ovs-vswitchd.log], [0], [],
> > > # Delete the port
> > > AT_CHECK([ovs-vsctl del-port br0 reserved_name], [0], [], [])])
> > > OVS_VSWITCHD_STOP
> > > -AT_CLEANUP
> > > \ No newline at end of file
> > > +AT_CLEANUP
> > > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > > index 19ab472..2d8c7c7 100644
> > > --- a/utilities/ovs-vsctl.c
> > > +++ b/utilities/ovs-vsctl.c
> > > @@ -2016,13 +2016,17 @@ cmd_del_port(struct vsctl_context *ctx)
> > > {
> > > bool must_exist = !shash_find(&ctx->options, "--if-exists");
> > > bool with_iface = shash_find(&ctx->options, "--with-iface") !=
> NULL;
> > > +const char *target = ctx->argv[ctx->argc - 1];
> > > struct vsctl_port *port;
> > >
> > > vsctl_context_populate_cache(ctx);
> > > -if (!with_iface) {
> > > -port = find_port(ctx, ctx->argv[ctx->argc - 1], must_exist);
> > > +if (find_bridge(ctx, target, false)) {
> > > +vsctl_fatal("cannot delete port %s because it is the local
> port "
> > > +"for bridge %s (deleting this port requires
> deleting "
> > > +"the entire bridge)", target, target);
> > > +} else if (!with_iface) {
> > > +port = find_port(ctx, target, must_exist);
> > > } else {
> > > -const char *target = ctx->argv[ctx->argc - 1];
> > > struct vsctl_iface *iface;
> > >
> > > port = find_port(ctx, target, false);
> > > --
> > > 1.7.10.4
> > >
> > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv3] ofproto-dpif: Tighten up megaflow wildcard handling.
On Wed, Jun 19, 2013 at 06:38:25PM -0700, Justin Pettit wrote: > A number of use-cases weren't handled properly when determining what can > be wildcarded for megaflows. This commit both catches additional fields > that cannot be wildcarded and loosens a few other cases. > > Bug #17979 > > Signed-off-by: Justin Pettit > --- > v1 -> v2: Remove IPFIX un-wildcarding. > v2 -> v3: Un-wildcard other fields affected by DPIF set actions. Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv3] ofproto-dpif: Tighten up megaflow wildcard handling.
On Jun 19, 2013, at 10:44 PM, Ben Pfaff wrote: > On Wed, Jun 19, 2013 at 06:38:25PM -0700, Justin Pettit wrote: >> A number of use-cases weren't handled properly when determining what can >> be wildcarded for megaflows. This commit both catches additional fields >> that cannot be wildcarded and loosens a few other cases. >> >> Bug #17979 >> >> Signed-off-by: Justin Pettit >> --- >> v1 -> v2: Remove IPFIX un-wildcarding. >> v2 -> v3: Un-wildcard other fields affected by DPIF set actions. > > Acked-by: Ben Pfaff Ohhh, an "Acked-by"…you're brave. I'll push this shortly. Thanks! --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-vsctl: Improve error message for "ovs-vsctl del-port ".
Good enough, thanks. Applied.
On Thu, Jun 20, 2013 at 12:31:09AM -0500, Reid Price wrote:
> Yes, especially if it passed the unit test, looks correct.
>
>
> On Wed, Jun 19, 2013 at 3:26 PM, Ben Pfaff wrote:
>
> > Thanks. Is that a review?
> >
> > On Tue, Jun 18, 2013 at 09:27:18PM -0700, Reid Price wrote:
> > > Thanks, that seems like the right amount of information
> > >
> > > -Reid
> > >
> > >
> > > On Tue, Jun 18, 2013 at 9:02 PM, Ben Pfaff wrote:
> > >
> > > > Previously, commands like this:
> > > > ovs-vsctl add-br br0
> > > > ovs-vsctl del-port br0
> > > > yielded an error message like:
> > > > no port named br0
> > > > which is confusing. This commit improves the error message to:
> > > > cannot delete port br0 because it is the local port for bridge br0
> > > > (deleting this port requires deleting the entire bridge)
> > > >
> > > > Bug #17994.
> > > > Reported-by: Reid Price
> > > > Signed-off-by: Ben Pfaff
> > > > ---
> > > > tests/ovs-vsctl.at|6 +-
> > > > utilities/ovs-vsctl.c | 10 +++---
> > > > 2 files changed, 12 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/tests/ovs-vsctl.at b/tests/ovs-vsctl.at
> > > > index ec08cd4..fa2c3ff 100644
> > > > --- a/tests/ovs-vsctl.at
> > > > +++ b/tests/ovs-vsctl.at
> > > > @@ -323,6 +323,10 @@ AT_CHECK([RUN_OVS_VSCTL(
> > > > ], [], [OVS_VSCTL_CLEANUP])
> > > > AT_CHECK([RUN_OVS_VSCTL([--may-exist add-port b b1])], [0], [], [],
> > > >[OVS_VSCTL_CLEANUP])
> > > > +AT_CHECK([RUN_OVS_VSCTL([del-port a])], [1], [],
> > > > + [ovs-vsctl: cannot delete port a because it is the local port for
> > > > bridge a (deleting this port requires deleting the entire bridge)
> > > > +],
> > > > + [OVS_VSCTL_CLEANUP])
> > > > AT_CHECK([RUN_OVS_VSCTL([--may-exist add-port a b1])], [1], [],
> > > >[ovs-vsctl: "--may-exist add-port a b1" but b1 is actually attached
> > to
> > > > bridge b
> > > > ],
> > > > @@ -1246,4 +1250,4 @@ AT_CHECK([sed "/|bridge|WARN|/d"
> > ovs-vswitchd.log >
> > > > ovs-vswitchd.log], [0], [],
> > > > # Delete the port
> > > > AT_CHECK([ovs-vsctl del-port br0 reserved_name], [0], [], [])])
> > > > OVS_VSWITCHD_STOP
> > > > -AT_CLEANUP
> > > > \ No newline at end of file
> > > > +AT_CLEANUP
> > > > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > > > index 19ab472..2d8c7c7 100644
> > > > --- a/utilities/ovs-vsctl.c
> > > > +++ b/utilities/ovs-vsctl.c
> > > > @@ -2016,13 +2016,17 @@ cmd_del_port(struct vsctl_context *ctx)
> > > > {
> > > > bool must_exist = !shash_find(&ctx->options, "--if-exists");
> > > > bool with_iface = shash_find(&ctx->options, "--with-iface") !=
> > NULL;
> > > > +const char *target = ctx->argv[ctx->argc - 1];
> > > > struct vsctl_port *port;
> > > >
> > > > vsctl_context_populate_cache(ctx);
> > > > -if (!with_iface) {
> > > > -port = find_port(ctx, ctx->argv[ctx->argc - 1], must_exist);
> > > > +if (find_bridge(ctx, target, false)) {
> > > > +vsctl_fatal("cannot delete port %s because it is the local
> > port "
> > > > +"for bridge %s (deleting this port requires
> > deleting "
> > > > +"the entire bridge)", target, target);
> > > > +} else if (!with_iface) {
> > > > +port = find_port(ctx, target, must_exist);
> > > > } else {
> > > > -const char *target = ctx->argv[ctx->argc - 1];
> > > > struct vsctl_iface *iface;
> > > >
> > > > port = find_port(ctx, target, false);
> > > > --
> > > > 1.7.10.4
> > > >
> > > >
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] datapath: Mega flow implementation
This patch seems to work fine. Please feel free to push. I have two concerns mainly on the style side: 1. In the same file flow.c, we are mixing the _deferred_ functions with the style of passing 'deferred' as a parameter in the same file. The correctness of caller in setting 'deferred' value does not look obviously correct. 2. In function ovw_flow_free() ... ovs_sw_flow_mask_del_ref((struct sw_flow_mask __force *)flow->mask, deferred); ... The type cast to (flow->mask) does not look good to me. It also defeats some useful lockdep checks. I am sure you are aware of those trade-offs and have determined that the benefits out weights any style concerns, right? --andy On Wed, Jun 19, 2013 at 6:31 PM, Jesse Gross wrote: > OK, that idea turned out to be a bad one since masks really do have a > lifetime that is separate from an individual flow. However, here's a > patch that does the unification a different way. > > What do you think? > > On Wed, Jun 19, 2013 at 4:32 PM, Jesse Gross wrote: > > I think at this point, the use of a separate RCU callback for the > > masks is hurting more than it is helping and it would be easier to > > just piggyback off the flow/table destroy RCU. I'll try to write a > > patch to see if I can get it to work. > > > > On Wed, Jun 19, 2013 at 4:23 PM, Andy Zhou wrote: > >> When table is destroyed, there is really no need to keep track of the > mask's > >> reference counting -- The masks will have to be destroyed any ways. > >> > >> During run time, the mask_list deletion needs to be protected by the > >> ovs_lock, But we should not require ovs_lock to be held during table > >> destroy. right? So it seems we will have two ways of doing > ovs_flow_free() > >> any ways, > >> > >> I will continue to think about how to merge these two cases. Please let > me > >> know if you have any suggestions. > >> > >> --andy > >> > >> > >> On Wed, Jun 19, 2013 at 4:09 PM, Jesse Gross wrote: > >>> > >>> What is the reason need for iterating through the mask list itself > >>> when destroying the table instead of doing it as the flows are > >>> deleted? Doing it that way would both avoid the need to have multiple > >>> list deletion functions and the differences between the RCU and > >>> non-RCU versions of ovs_flow_free(). > >>> > >>> One issue that comes to mind is the RCU barrier when the module is > >>> unloaded but there are other ways to fix that. > >>> > >>> On Wed, Jun 19, 2013 at 2:09 PM, Andy Zhou wrote: > >>> > Jesse, > >>> > > >>> > The following patch is an incremental patch on top of yours. > >>> > -- Fix a few small bugs introduced by the cleanup patch. > >>> > -- Make mask list per table. > >>> > > >>> > I believe this implementation will address the rcu lock issues we > >>> > discussed > >>> > off line. > >>> > > >>> > Would you please review? > >>> > > >>> > Thanks, > >>> > > >>> > --andy > >>> > > >>> > On Tue, Jun 18, 2013 at 4:35 PM, Jesse Gross > wrote: > >>> >> > >>> >> On Tue, Jun 18, 2013 at 4:15 PM, Andy Zhou > wrote: > >>> >> > Add wildcarded flow support in kernel datapath. > >>> >> > > >>> >> > Wildcarded flow can improve OVS flow set up performance by avoid > >>> >> > sending > >>> >> > matching new flows to the user space program. The exact > performance > >>> >> > boost > >>> >> > will largely dependent on wildcarded flow hit rate. > >>> >> > > >>> >> > In case all new flows hits wildcard flows, the flow set up rate is > >>> >> > within 5% of that of linux bridge module. > >>> >> > > >>> >> > Pravin has made significant contributions to this patch. Including > >>> >> > API > >>> >> > clean ups and bug fixes. > >>> >> > > >>> >> > Co-authored-by: Pravin B Shelar > >>> >> > Signed-off-by: Pravin B Shelar > >>> >> > Signed-off-by: Andy Zhou > >>> >> > >>> >> I made some incremental changes, can you take a look to see if they > >>> >> seem reasonable to you? > >>> >> > >>> >> Here are the main blocks: > >>> >> - Additional interface documentation. > >>> >> - Fix memory leak when installing a new flow if there is an > >>> >> allocation failure for the mask. > >>> >> - Require Ethernet addresses to be present in the key, as before. > >>> >> - Make SNAP handling a little more integrated with the rest of the > >>> >> parsing and validation code. > >>> >> - Make ovs_flow_free() and ovs_deferred_flow_free() have exactly > the > >>> >> same behavior, just with the addition of RCU, to avoid possible > >>> >> confusion. > >>> >> - Return errors directly rather than through an intermediate > variable > >>> >> in ovs_flow_extract() and children. > >>> >> - Enforce an exact match for the outer EtherType for vlan packets, > >>> >> similar to what we do for other protocols. > >>> >> - Miscellaneous style fixes. > >>> > > >>> > > >> > >> > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
