[ovs-dev] OpenvSwitch version for Ubuntu Saucy + Kernel 3.9/3.10
Hi OpenvSwitch Devs Ubuntu Saucy development has been open for a while and the Ubuntu kernel team are currently targeting either Linux 3.9 or 3.10 for this release. Currently the 1.9.0 DKMS source is unhappy with 3.9 - I've hacked on it for a bit and although it definitely possible to backport/fixup enough to make it work with Linux >= 3.9 I wondered whether 1.10 would make a better base; I was able to get things up and running with the following commits: f8dfbcb77d7678fa19b4de0f94118b713f74ffb8: datapath: hlist: drop the node parameter from iterators 29285b6286493965a36ae3eff5983457f85cd087: datapath: Backport simplified hlist iterators. plus the normal change to datapath.c to increase the range of supported kernel versions. Also I think the VXLAN support in 1.10 is of interest to OpenStack users (the Havana release should grow support for this). The version of OpenvSwitch that ships with Saucy will also be provided to support OpenStack Havana on Ubuntu 12.04. Any thoughts/guidance as always much appreciated! Cheers James -- James Page Ubuntu Core Developer Debian Maintainer james.p...@ubuntu.com ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: handle_flow_miss_common() without facets.
handle_flow_miss_common() is intended to be called whether or not a miss needs a facet. Found by inspection. Signed-off-by: Ethan Jackson --- ofproto/ofproto-dpif.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index b547298..ae59eda 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3683,6 +3683,8 @@ handle_flow_miss_without_facet(struct flow_miss *miss, COVERAGE_INC(facet_suppress); +handle_flow_miss_common(rule, packet, &miss->flow); + ofpbuf_use_stub(&odp_actions, op->stub, sizeof op->stub); dpif_flow_stats_extract(&miss->flow, packet, now, &stats); -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] openvswitch: use -EEXIST as return error to be more precise
On Tue, May 28, 2013 at 3:31 PM, Fan Du wrote: > ovs_vport_cmd_new create a new vport with port num specified by user. > when this port num is already occupied, it's more clear to return > -EEXIST as the reason than -EBUSY. > > Signed-off-by: Fan Du This error code is part of the userspace ABI so you can't change it without breaking existing applications. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] debian: Don't fail ovs-controller restart if daemon not running.
On Tue, May 28, 2013 at 05:24:16PM -0700, Gurucharan Shetty wrote: > Reported-by: Maxime Brun > Signed-off-by: Gurucharan Shetty Looks good, thanks. I thought we had converted all this stuff to use ovs-lib.sh, but I guess not. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: handle_flow_miss_common() without facets.
On Wed, May 29, 2013 at 10:21:28AM -0700, Ethan Jackson wrote: > handle_flow_miss_common() is intended to be called whether or not a > miss needs a facet. Found by inspection. > > Signed-off-by: Ethan Jackson Looks good, thanks. Is it worth a backport? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: Consolidate facet stat logic.
The logic for updating statistics at the facet level had been
spread through ofproto-dpif in a rather confusing manner. This
patch consolidates as much of this logic as is reasonable into
facet_push_stats().
On a side note, I'd expect this patch to have a marginal positive
performance impact when using learning (though I haven't bothered
to measure it). It combines facet_learn() and facet_push_stats()
into one step allowing us to avoid a redundant xlate_actions().
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif.c | 148
1 file changed, 50 insertions(+), 98 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ae59eda..881b29f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -121,7 +121,6 @@ static struct rule_dpif *rule_dpif_miss_rule(struct
ofproto_dpif *ofproto,
static void rule_credit_stats(struct rule_dpif *,
const struct dpif_flow_stats *);
-static void flow_push_stats(struct facet *, const struct dpif_flow_stats *);
static tag_type rule_calculate_tag(const struct flow *,
const struct minimask *, uint32_t basis);
static void rule_invalidate(const struct rule_dpif *);
@@ -325,9 +324,6 @@ static void action_xlate_ctx_init(struct action_xlate_ctx *,
static void xlate_actions(struct action_xlate_ctx *,
const struct ofpact *ofpacts, size_t ofpacts_len,
struct ofpbuf *odp_actions);
-static void xlate_actions_for_side_effects(struct action_xlate_ctx *,
- const struct ofpact *ofpacts,
- size_t ofpacts_len);
static void xlate_table_action(struct action_xlate_ctx *, uint16_t in_port,
uint8_t table_id, bool may_packet_in);
@@ -419,7 +415,6 @@ static void subfacet_destroy_batch(struct ofproto_dpif *,
struct subfacet **, int n);
static void subfacet_reset_dp_stats(struct subfacet *,
struct dpif_flow_stats *);
-static void subfacet_update_time(struct subfacet *, long long int used);
static void subfacet_update_stats(struct subfacet *,
const struct dpif_flow_stats *);
static void subfacet_make_actions(struct subfacet *,
@@ -520,9 +515,8 @@ static bool facet_check_consistency(struct facet *);
static void facet_flush_stats(struct facet *);
-static void facet_update_time(struct facet *, long long int used);
static void facet_reset_counters(struct facet *);
-static void facet_push_stats(struct facet *);
+static void facet_push_stats(struct facet *, bool may_learn);
static void facet_learn(struct facet *);
static void facet_account(struct facet *);
static void push_all_stats(void);
@@ -4331,26 +4325,29 @@ update_subfacet_stats(struct subfacet *subfacet,
const struct dpif_flow_stats *stats)
{
struct facet *facet = subfacet->facet;
+struct dpif_flow_stats diff;
+
+diff.tcp_flags = stats->tcp_flags;
+diff.used = stats->used;
if (stats->n_packets >= subfacet->dp_packet_count) {
-uint64_t extra = stats->n_packets - subfacet->dp_packet_count;
-facet->packet_count += extra;
+diff.n_packets = stats->n_packets - subfacet->dp_packet_count;
} else {
VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
+diff.n_packets = 0;
}
if (stats->n_bytes >= subfacet->dp_byte_count) {
-facet->byte_count += stats->n_bytes - subfacet->dp_byte_count;
+diff.n_bytes = stats->n_bytes - subfacet->dp_byte_count;
} else {
VLOG_WARN_RL(&rl, "unexpected byte count from datapath");
+diff.n_bytes = 0;
}
subfacet->dp_packet_count = stats->n_packets;
subfacet->dp_byte_count = stats->n_bytes;
+subfacet_update_stats(subfacet, &diff);
-facet->tcp_flags |= stats->tcp_flags;
-
-subfacet_update_time(subfacet, stats->used);
if (facet->accounted_bytes < facet->byte_count) {
facet_learn(facet);
facet_account(facet);
@@ -4406,7 +4403,6 @@ update_stats(struct dpif_backer *backer)
while (dpif_flow_dump_next(&dump, &key, &key_len, NULL, NULL, &stats)) {
struct flow flow;
struct subfacet *subfacet;
-struct ofport_dpif *ofport;
uint32_t key_hash;
if (ofproto_receive(backer, NULL, key, key_len, &flow, NULL, &ofproto,
@@ -4417,11 +4413,6 @@ update_stats(struct dpif_backer *backer)
ofproto->total_subfacet_count += hmap_count(&ofproto->subfacets);
ofproto->n_update_stats++;
-ofport = get_ofp_port(ofproto, flow.in_port);
-if (ofport && ofport->tnl_port) {
-netdev_vport_inc_rx(ofport->up.netdev, stats);
-}
-
key_hash = odp_flow_key_hash(key, key_len);
subfacet = subfacet_find(ofproto, k
Re: [ovs-dev] [PATCH] ofproto-dpif: Consolidate facet stat logic.
Oops, this patch ends up deleteing, xlate_actions_for_side_effects(),
but then I need it again in the xlate series. I think instead of
basing this on master, I'll base it on the xlate series once it's
merged. May as well hold off reviewing it until then.
Ethan
On Wed, May 29, 2013 at 2:38 PM, Ethan Jackson wrote:
> The logic for updating statistics at the facet level had been
> spread through ofproto-dpif in a rather confusing manner. This
> patch consolidates as much of this logic as is reasonable into
> facet_push_stats().
>
> On a side note, I'd expect this patch to have a marginal positive
> performance impact when using learning (though I haven't bothered
> to measure it). It combines facet_learn() and facet_push_stats()
> into one step allowing us to avoid a redundant xlate_actions().
>
> Signed-off-by: Ethan Jackson
> ---
> ofproto/ofproto-dpif.c | 148
>
> 1 file changed, 50 insertions(+), 98 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ae59eda..881b29f 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -121,7 +121,6 @@ static struct rule_dpif *rule_dpif_miss_rule(struct
> ofproto_dpif *ofproto,
>
> static void rule_credit_stats(struct rule_dpif *,
>const struct dpif_flow_stats *);
> -static void flow_push_stats(struct facet *, const struct dpif_flow_stats *);
> static tag_type rule_calculate_tag(const struct flow *,
> const struct minimask *, uint32_t basis);
> static void rule_invalidate(const struct rule_dpif *);
> @@ -325,9 +324,6 @@ static void action_xlate_ctx_init(struct action_xlate_ctx
> *,
> static void xlate_actions(struct action_xlate_ctx *,
>const struct ofpact *ofpacts, size_t ofpacts_len,
>struct ofpbuf *odp_actions);
> -static void xlate_actions_for_side_effects(struct action_xlate_ctx *,
> - const struct ofpact *ofpacts,
> - size_t ofpacts_len);
> static void xlate_table_action(struct action_xlate_ctx *, uint16_t in_port,
> uint8_t table_id, bool may_packet_in);
>
> @@ -419,7 +415,6 @@ static void subfacet_destroy_batch(struct ofproto_dpif *,
> struct subfacet **, int n);
> static void subfacet_reset_dp_stats(struct subfacet *,
> struct dpif_flow_stats *);
> -static void subfacet_update_time(struct subfacet *, long long int used);
> static void subfacet_update_stats(struct subfacet *,
>const struct dpif_flow_stats *);
> static void subfacet_make_actions(struct subfacet *,
> @@ -520,9 +515,8 @@ static bool facet_check_consistency(struct facet *);
>
> static void facet_flush_stats(struct facet *);
>
> -static void facet_update_time(struct facet *, long long int used);
> static void facet_reset_counters(struct facet *);
> -static void facet_push_stats(struct facet *);
> +static void facet_push_stats(struct facet *, bool may_learn);
> static void facet_learn(struct facet *);
> static void facet_account(struct facet *);
> static void push_all_stats(void);
> @@ -4331,26 +4325,29 @@ update_subfacet_stats(struct subfacet *subfacet,
>const struct dpif_flow_stats *stats)
> {
> struct facet *facet = subfacet->facet;
> +struct dpif_flow_stats diff;
> +
> +diff.tcp_flags = stats->tcp_flags;
> +diff.used = stats->used;
>
> if (stats->n_packets >= subfacet->dp_packet_count) {
> -uint64_t extra = stats->n_packets - subfacet->dp_packet_count;
> -facet->packet_count += extra;
> +diff.n_packets = stats->n_packets - subfacet->dp_packet_count;
> } else {
> VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
> +diff.n_packets = 0;
> }
>
> if (stats->n_bytes >= subfacet->dp_byte_count) {
> -facet->byte_count += stats->n_bytes - subfacet->dp_byte_count;
> +diff.n_bytes = stats->n_bytes - subfacet->dp_byte_count;
> } else {
> VLOG_WARN_RL(&rl, "unexpected byte count from datapath");
> +diff.n_bytes = 0;
> }
>
> subfacet->dp_packet_count = stats->n_packets;
> subfacet->dp_byte_count = stats->n_bytes;
> +subfacet_update_stats(subfacet, &diff);
>
> -facet->tcp_flags |= stats->tcp_flags;
> -
> -subfacet_update_time(subfacet, stats->used);
> if (facet->accounted_bytes < facet->byte_count) {
> facet_learn(facet);
> facet_account(facet);
> @@ -4406,7 +4403,6 @@ update_stats(struct dpif_backer *backer)
> while (dpif_flow_dump_next(&dump, &key, &key_len, NULL, NULL, &stats)) {
> struct flow flow;
> struct subfacet *subfacet;
> -struct ofport_dpif *ofport;
> uint32_t key_hash;
>
> if (ofpro
Re: [ovs-dev] [PATCH] ofproto-dpif: Consolidate facet stat logic.
OK, thanks.
On Wed, May 29, 2013 at 02:41:55PM -0500, Ethan Jackson wrote:
> Oops, this patch ends up deleteing, xlate_actions_for_side_effects(),
> but then I need it again in the xlate series. I think instead of
> basing this on master, I'll base it on the xlate series once it's
> merged. May as well hold off reviewing it until then.
>
> Ethan
>
> On Wed, May 29, 2013 at 2:38 PM, Ethan Jackson wrote:
> > The logic for updating statistics at the facet level had been
> > spread through ofproto-dpif in a rather confusing manner. This
> > patch consolidates as much of this logic as is reasonable into
> > facet_push_stats().
> >
> > On a side note, I'd expect this patch to have a marginal positive
> > performance impact when using learning (though I haven't bothered
> > to measure it). It combines facet_learn() and facet_push_stats()
> > into one step allowing us to avoid a redundant xlate_actions().
> >
> > Signed-off-by: Ethan Jackson
> > ---
> > ofproto/ofproto-dpif.c | 148
> >
> > 1 file changed, 50 insertions(+), 98 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index ae59eda..881b29f 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -121,7 +121,6 @@ static struct rule_dpif *rule_dpif_miss_rule(struct
> > ofproto_dpif *ofproto,
> >
> > static void rule_credit_stats(struct rule_dpif *,
> >const struct dpif_flow_stats *);
> > -static void flow_push_stats(struct facet *, const struct dpif_flow_stats
> > *);
> > static tag_type rule_calculate_tag(const struct flow *,
> > const struct minimask *, uint32_t
> > basis);
> > static void rule_invalidate(const struct rule_dpif *);
> > @@ -325,9 +324,6 @@ static void action_xlate_ctx_init(struct
> > action_xlate_ctx *,
> > static void xlate_actions(struct action_xlate_ctx *,
> >const struct ofpact *ofpacts, size_t ofpacts_len,
> >struct ofpbuf *odp_actions);
> > -static void xlate_actions_for_side_effects(struct action_xlate_ctx *,
> > - const struct ofpact *ofpacts,
> > - size_t ofpacts_len);
> > static void xlate_table_action(struct action_xlate_ctx *, uint16_t in_port,
> > uint8_t table_id, bool may_packet_in);
> >
> > @@ -419,7 +415,6 @@ static void subfacet_destroy_batch(struct ofproto_dpif
> > *,
> > struct subfacet **, int n);
> > static void subfacet_reset_dp_stats(struct subfacet *,
> > struct dpif_flow_stats *);
> > -static void subfacet_update_time(struct subfacet *, long long int used);
> > static void subfacet_update_stats(struct subfacet *,
> >const struct dpif_flow_stats *);
> > static void subfacet_make_actions(struct subfacet *,
> > @@ -520,9 +515,8 @@ static bool facet_check_consistency(struct facet *);
> >
> > static void facet_flush_stats(struct facet *);
> >
> > -static void facet_update_time(struct facet *, long long int used);
> > static void facet_reset_counters(struct facet *);
> > -static void facet_push_stats(struct facet *);
> > +static void facet_push_stats(struct facet *, bool may_learn);
> > static void facet_learn(struct facet *);
> > static void facet_account(struct facet *);
> > static void push_all_stats(void);
> > @@ -4331,26 +4325,29 @@ update_subfacet_stats(struct subfacet *subfacet,
> >const struct dpif_flow_stats *stats)
> > {
> > struct facet *facet = subfacet->facet;
> > +struct dpif_flow_stats diff;
> > +
> > +diff.tcp_flags = stats->tcp_flags;
> > +diff.used = stats->used;
> >
> > if (stats->n_packets >= subfacet->dp_packet_count) {
> > -uint64_t extra = stats->n_packets - subfacet->dp_packet_count;
> > -facet->packet_count += extra;
> > +diff.n_packets = stats->n_packets - subfacet->dp_packet_count;
> > } else {
> > VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
> > +diff.n_packets = 0;
> > }
> >
> > if (stats->n_bytes >= subfacet->dp_byte_count) {
> > -facet->byte_count += stats->n_bytes - subfacet->dp_byte_count;
> > +diff.n_bytes = stats->n_bytes - subfacet->dp_byte_count;
> > } else {
> > VLOG_WARN_RL(&rl, "unexpected byte count from datapath");
> > +diff.n_bytes = 0;
> > }
> >
> > subfacet->dp_packet_count = stats->n_packets;
> > subfacet->dp_byte_count = stats->n_bytes;
> > +subfacet_update_stats(subfacet, &diff);
> >
> > -facet->tcp_flags |= stats->tcp_flags;
> > -
> > -subfacet_update_time(subfacet, stats->used);
> > if (facet->accounted_bytes < facet->byte_count) {
> > facet_learn(facet);
> > facet_account(facet);
> > @@
Re: [ovs-dev] [PATCH] ofproto-dpif: handle_flow_miss_common() without facets.
Probably. I need to backport the xlate series as well. I'll do it all in one go when I merge that. On Wed, May 29, 2013 at 2:02 PM, Ben Pfaff wrote: > On Wed, May 29, 2013 at 10:21:28AM -0700, Ethan Jackson wrote: >> handle_flow_miss_common() is intended to be called whether or not a >> miss needs a facet. Found by inspection. >> >> Signed-off-by: Ethan Jackson > > Looks good, thanks. > > Is it worth a backport? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: handle_flow_miss_common() without > facets. (Ethan Jackson)
> Message: 3 > Date: Wed, 29 May 2013 10:21:28 -0700 > From: Ethan Jackson > Subject: [ovs-dev] [PATCH] ofproto-dpif: handle_flow_miss_common() > without facets. > To: dev@openvswitch.org > Message-ID: <1369848088-47098-1-git-send-email-et...@nicira.com> > > handle_flow_miss_common() is intended to be called whether or not a > miss needs a facet. Found by inspection. > > Signed-off-by: Ethan Jackson > --- > ofproto/ofproto-dpif.c |2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index b547298..ae59eda 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -3683,6 +3683,8 @@ handle_flow_miss_without_facet(struct flow_miss *miss, > > COVERAGE_INC(facet_suppress); > > +handle_flow_miss_common(rule, packet, &miss->flow); > + > ofpbuf_use_stub(&odp_actions, op->stub, sizeof op->stub); > > dpif_flow_stats_extract(&miss->flow, packet, now, &stats); > -- > 1.7.9.5 > Is it necessary to catch this up by a unit test? Best,Jing ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v11 1/6] Add execute_actions
On Wed, May 29, 2013 at 03:06:38PM +0900, Simon Horman wrote: > This moves generic action execution code out of lib/dpif-netedev.c > and into a new file, lib/execute-actions.c. > > This is in preparation for using execute_set_action() > in lib/odp-util.c to handle recirculation/ > > Signed-off-by: Simon Horman Applied to master, thanks. I updated the commit message, which was now slightly inaccurate due to renamings. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [xlate v3 8/8] ofproto-dpif: Revamp xlate_actions() interface.
This incremental (applied on v3) should address your comments on the v2 version
of the patch. I consider this series reviewed at this point, unless there's
something you'd like to make additional comments on.
Ethan
---
ofproto/ofproto-dpif.c | 43 +++
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 89b693a..386434d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -210,6 +210,21 @@ static bool ofbundle_includes_vlan(const struct ofbundle
*, uint16_t vlan);
struct xlate_ctx;
+/* Initial values of fields of the packet that may be changed during
+ * flow processing and needed later. */
+struct initial_vals {
+ /* This is the value of vlan_tci in the packet as actually received from
+* dpif. This is the same as the facet's flow.vlan_tci unless the packet
+* was received via a VLAN splinter. In that case, this value is 0
+* (because the packet as actually received from the dpif had no 802.1Q
+* tag) but the facet's flow.vlan_tci is set to the VLAN that the splinter
+* represents.
+*
+* This member should be removed when the VLAN splinters feature is no
+* longer needed. */
+ovs_be16 vlan_tci;
+};
+
struct xlate_out {
tag_type tags; /* Tags associated with actions. */
enum slow_path_reason slow; /* 0 if fast path may be used. */
@@ -230,7 +245,7 @@ struct xlate_in {
* this flow when actions change header fields. */
struct flow flow;
-const struct initial_vals *initial_vals;
+struct initial_vals initial_vals;
/* The packet corresponding to 'flow', or a null pointer if we are
* revalidating without a packet to refer to. */
@@ -316,21 +331,6 @@ struct xlate_ctx {
bool exit; /* No further actions should be processed. */
};
-/* Initial values of fields of the packet that may be changed during
- * flow processing and needed later. */
-struct initial_vals {
- /* This is the value of vlan_tci in the packet as actually received from
-* dpif. This is the same as the facet's flow.vlan_tci unless the packet
-* was received via a VLAN splinter. In that case, this value is 0
-* (because the packet as actually received from the dpif had no 802.1Q
-* tag) but the facet's flow.vlan_tci is set to the VLAN that the splinter
-* represents.
-*
-* This member should be removed when the VLAN splinters feature is no
-* longer needed. */
-ovs_be16 vlan_tci;
-};
-
static void xlate_in_init(struct xlate_in *, struct ofproto_dpif *,
const struct flow *, const struct initial_vals *,
struct rule_dpif *, uint8_t tcp_flags,
@@ -6856,7 +6856,6 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif
*ofproto,
{
xin->ofproto = ofproto;
xin->flow = *flow;
-xin->initial_vals = initial_vals;
xin->packet = packet;
xin->may_learn = packet != NULL;
xin->rule = rule;
@@ -6866,6 +6865,12 @@ xlate_in_init(struct xlate_in *xin, struct ofproto_dpif
*ofproto,
xin->resubmit_hook = NULL;
xin->report_hook = NULL;
xin->resubmit_stats = NULL;
+
+if (initial_vals) {
+xin->initial_vals = *initial_vals;
+} else {
+xin->initial_vals.vlan_tci = xin->flow.vlan_tci;
+}
}
static void
@@ -6923,9 +6928,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
*xout)
ctx.rule = xin->rule;
ctx.base_flow = ctx.xin->flow;
-if (xin->initial_vals) {
-ctx.base_flow.vlan_tci = xin->initial_vals->vlan_tci;
-}
+ctx.base_flow.vlan_tci = xin->initial_vals.vlan_tci;
memset(&ctx.base_flow.tunnel, 0, sizeof ctx.base_flow.tunnel);
ctx.orig_tunnel_ip_dst = ctx.xin->flow.tunnel.ip_dst;
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/3] ovs-vswitchd: An option to wait for userspace > flow restore to complete. (Gurucharan Shetty)
> While upgrading openvswitch, it helps to restore openflow flows before
> starting packet processing. Typically we want to restart openvswitch,
> add the openflow flows and then start packet processing.
>
> To do this, we look for the other_config:flow-restore-wait column
> in the Open_vSwitch table during startup. If set as true, we disable
> receiving packets from the datapath, expiring or flushing flows and
> running any periodic ofproto activities. This option does not prevent
> the addition and deletion of ports. Once this option is set to false,
> we return to normal processing.
>
> An upcoming commit will use this feature in Open vSwitch startup scripts.
Will this patch leave ongoing traffic not disturbed while upgrading OVS?
I am asking since I notice that the existing ports, which keep packet flowing,
in the fast datapath (i.e., kernel) will be garbage collected in
open_dpif_backer.
Best,Jing
>
> Bug #16086.
> Signed-off-by: Gurucharan Shetty
> ---
> ofproto/ofproto-dpif.c | 55
> +---
> vswitchd/bridge.c | 11 ++
> vswitchd/vswitch.xml | 17 +++
> 3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index c4f7d25..31f04f3 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -72,6 +72,8 @@ COVERAGE_DEFINE(facet_suppress);
> * flow translation. */
> #define MAX_RESUBMIT_RECURSION 64
>
> +extern bool flow_restore_wait;
> +
> /* Number of implemented OpenFlow tables. */
> enum { N_TABLES = 255 };
> enum { TBL_INTERNAL = N_TABLES - 1 };/* Used for internal hidden rules.
> */
> @@ -671,6 +673,7 @@ struct dpif_backer {
> struct tag_set revalidate_set; /* Revalidate only matching facets. */
>
> struct hmap drop_keys; /* Set of dropped odp keys. */
> +bool recv_set_enable; /* Enables or disables receiving packets. */
> };
>
> /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
> @@ -947,6 +950,21 @@ type_run(const char *type)
> push_all_stats();
> }
>
> +/* If vswitchd started with other_config:flow_restore_wait set as "true",
> + * and the configuration has now changed to "false", enable receiving
> + * packets from the datapath. */
> +if (!backer->recv_set_enable && !flow_restore_wait) {
> +backer->recv_set_enable = true;
> +
> +error = dpif_recv_set(backer->dpif, backer->recv_set_enable);
> +if (error) {
> +VLOG_ERR("Failed to enable receiving packets in dpif.");
> +return error;
> +}
> +dpif_flow_flush(backer->dpif);
> +backer->need_revalidate = REV_RECONFIGURE;
> +}
> +
> if (backer->need_revalidate
> || !tag_set_is_empty(&backer->revalidate_set)) {
> struct tag_set revalidate_set = backer->revalidate_set;
> @@ -1040,7 +1058,10 @@ type_run(const char *type)
> }
> }
>
> -if (timer_expired(&backer->next_expiration)) {
> +if (!backer->recv_set_enable) {
> +/* A minimum of 1000 ms delay. */
> +timer_set_duration(&backer->next_expiration, 1000);
> +} else if (timer_expired(&backer->next_expiration)) {
> int delay = expire(backer);
> timer_set_duration(&backer->next_expiration, delay);
> }
> @@ -1106,6 +1127,11 @@ dpif_backer_run_fast(struct dpif_backer *backer, int
> max_batch)
> {
> unsigned int work;
>
> +/* If recv_set_enable is false, we should not handle upcalls. */
> +if (!backer->recv_set_enable) {
> +return 0;
> +}
> +
> /* Handle one or more batches of upcalls, until there's nothing left to
> do
> * or until we do a fixed total amount of work.
> *
> @@ -1305,9 +1331,16 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
> backer->need_revalidate = 0;
> simap_init(&backer->tnl_backers);
> tag_set_init(&backer->revalidate_set);
> +if (flow_restore_wait) {
> +backer->recv_set_enable = false;
> +} else {
> +backer->recv_set_enable = true;
> +}
> *backerp = backer;
>
> -dpif_flow_flush(backer->dpif);
> +if (backer->recv_set_enable) {
> +dpif_flow_flush(backer->dpif);
> +}
>
> /* Loop through the ports already on the datapath and remove any
> * that we don't need anymore. */
> @@ -1331,7 +1364,7 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
>
> shash_add(&all_dpif_backers, type, backer);
>
> -error = dpif_recv_set(backer->dpif, true);
> +error = dpif_recv_set(backer->dpif, backer->recv_set_enable);
> if (error) {
> VLOG_ERR("failed to listen on datapath of type %s: %s",
> type, strerror(error));
> @@ -1569,6 +1602,12 @@ run_fast(struct ofproto *ofproto_)
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> struct ofport_dpif *ofport;
>
> +
Re: [ovs-dev] [PATCH v11 2/6] Add set skb_mark, set_priority and tunnel support to execute_set_action
On Wed, May 29, 2013 at 03:06:39PM +0900, Simon Horman wrote:
> Add set skb_mark support to execute_set_action.
Now renamed odp_execute_actions().
> This also adds support for the user-space datapath
> to honour such actions if they occur before recirculation,
> which will be added by a subsequent patch.
>
> This is in preparation for using execute_set_action()
Ditto.
> to handle recirculation.
>
> Signed-off-by: Simon Horman
> index bd00f18..a2a8ed4 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -159,6 +159,10 @@ static void dp_netdev_execute_actions(struct dp_netdev *,
>struct ofpbuf *, struct flow *,
>const struct nlattr *actions,
>size_t actions_len);
> +static void dp_netdev_port_input(struct dp_netdev *dp,
> + struct dp_netdev_port *port,
> + struct ofpbuf *packet, uint32_t
> skb_priority,
> + uint32_t skb_mark, const struct flow_tnl
> *tnl);
>
> static struct dpif_netdev *
> dpif_netdev_cast(const struct dpif *dpif)
> @@ -1032,20 +1036,21 @@ dp_netdev_flow_used(struct dp_netdev_flow *flow,
> const struct ofpbuf *packet)
>
> static void
> dp_netdev_port_input(struct dp_netdev *dp, struct dp_netdev_port *port,
> - struct ofpbuf *packet)
> + struct ofpbuf *packet, uint32_t skb_priority,
> + uint32_t skb_mark, const struct flow_tnl *tnl)
> {
> -struct dp_netdev_flow *flow;
> +struct dp_netdev_flow *dp_flow;
Any particular reason to rename flow to dp_flow here? It makes the
patch larger and thus less obviously correct.
> struct flow key;
>
> if (packet->size < ETH_HEADER_LEN) {
> return;
> }
> -flow_extract(packet, 0, 0, NULL, port->port_no, &key);
> -flow = dp_netdev_lookup_flow(dp, &key);
> -if (flow) {
> -dp_netdev_flow_used(flow, packet);
> +flow_extract(packet, skb_priority, skb_mark, tnl, port->port_no, &key);
> +dp_flow = dp_netdev_lookup_flow(dp, &key);
> +if (dp_flow) {
> +dp_netdev_flow_used(dp_flow, packet);
> dp_netdev_execute_actions(dp, packet, &key,
> - flow->actions, flow->actions_len);
> + dp_flow->actions, dp_flow->actions_len);
> dp->n_hit++;
> } else {
> dp->n_missed++;
> @@ -1071,7 +1076,7 @@ dpif_netdev_run(struct dpif *dpif)
>
> error = port->rx ? netdev_rx_recv(port->rx, &packet) : EOPNOTSUPP;
> if (!error) {
> -dp_netdev_port_input(dp, port, &packet);
> +dp_netdev_port_input(dp, port, &packet, 0, 0, NULL);
> } else if (error != EAGAIN && error != EOPNOTSUPP) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_ERR_RL(&rl, "error receiving data from %s: %s",
...
> diff --git a/lib/odp-util.h b/lib/odp-util.h
> index a981d17..3635591 100644
> --- a/lib/odp-util.h
> +++ b/lib/odp-util.h
> @@ -87,6 +87,8 @@ struct odputil_keybuf {
> uint32_t keybuf[DIV_ROUND_UP(ODPUTIL_FLOW_KEY_BYTES, 4)];
> };
>
> +enum odp_key_fitness tun_key_from_attr(const struct nlattr *, struct
> flow_tnl *);
> +
Most of the other functions exported by the odp-util module have names
that begin with "odp_". I'd like to maintain that here.
Otherwise this looks good, thanks.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [xlate v3 8/8] ofproto-dpif: Revamp xlate_actions() interface.
On Wed, May 29, 2013 at 01:22:00PM -0700, Ethan Jackson wrote: > This incremental (applied on v3) should address your comments on the > v2 version of the patch. I consider this series reviewed at this > point, unless there's something you'd like to make additional comments > on. No, I'm happy, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto/pktbuf.c: Fix a typo in the comment
This patch fixes a typo in the comment of the pktbuf_retrieve() function. Signed-off-by: Alex Wang --- ofproto/pktbuf.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ofproto/pktbuf.c b/ofproto/pktbuf.c index 902b19d..2ec1f0d 100644 --- a/ofproto/pktbuf.c +++ b/ofproto/pktbuf.c @@ -158,7 +158,7 @@ pktbuf_get_null(void) * 0 if successful, otherwise an OpenFlow error code. * * On success, ordinarily stores the buffered packet in '*bufferp' and the - * datapath port number on which the packet was received in '*in_port'. The + * OpenFlow port number on which the packet was received in '*in_port'. The * caller becomes responsible for freeing the buffer. However, if 'id' * identifies a "null" packet buffer (created with pktbuf_get_null()), stores * NULL in '*bufferp' and UINT16_max in '*in_port'. -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/3] ovs-vswitchd: An option to wait for userspace > flow restore to complete. (Gurucharan Shetty)
On Wed, May 29, 2013 at 1:23 PM, Jing Ai wrote:
> > While upgrading openvswitch, it helps to restore openflow flows before
> > starting packet processing. Typically we want to restart openvswitch,
> > add the openflow flows and then start packet processing.
> >
> > To do this, we look for the other_config:flow-restore-wait column
> > in the Open_vSwitch table during startup. If set as true, we disable
> > receiving packets from the datapath, expiring or flushing flows and
> > running any periodic ofproto activities. This option does not prevent
> > the addition and deletion of ports. Once this option is set to false,
> > we return to normal processing.
> >
> > An upcoming commit will use this feature in Open vSwitch startup scripts.
>
> Will this patch leave ongoing traffic not disturbed while upgrading OVS?
>
Yes. That is one intent for just a "restart --save-flows=yes". The other
intent is to not setup new flows till the userspace flows are restored. For
force-reload-kmod (when kernel module is replaced), the intent is not to
setup new flows without having all the userspace flows replaced.
>
> I am asking since I notice that the existing ports, which keep packet
> flowing, in the fast datapath (i.e., kernel) will be garbage collected in
> open_dpif_backer.
>
I don't think that is true. The goal is to keep the datapath's ports to be
the same as seen in OVSDB. A OVS upgrade can result in either just a
"restart", in which case only userspace is restarted or a
'force-reload-kmod', in which case the kernel module is replaced too (in
this case there will be traffic disruption for a few seconds). In both
cases the expectation is that the kernel will have the ports for the
interfaces in OVSDB. If not, we will add it.
Tahnks,
Guru
>
> Best,
> Jing
>
> >
> > Bug #16086.
> > Signed-off-by: Gurucharan Shetty
> > ---
> > ofproto/ofproto-dpif.c | 55
> +---
> > vswitchd/bridge.c | 11 ++
> > vswitchd/vswitch.xml | 17 +++
> > 3 files changed, 80 insertions(+), 3 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index c4f7d25..31f04f3 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -72,6 +72,8 @@ COVERAGE_DEFINE(facet_suppress);
> > * flow translation. */
> > #define MAX_RESUBMIT_RECURSION 64
> >
> > +extern bool flow_restore_wait;
> > +
> > /* Number of implemented OpenFlow tables. */
> > enum { N_TABLES = 255 };
> > enum { TBL_INTERNAL = N_TABLES - 1 }; /* Used for internal hidden rules.
> */
> > @@ -671,6 +673,7 @@ struct dpif_backer {
> > struct tag_set revalidate_set; /* Revalidate only matching facets. */
> >
> > struct hmap drop_keys; /* Set of dropped odp keys. */
> > + bool recv_set_enable; /* Enables or disables receiving packets. */
> > };
> >
> > /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
> > @@ -947,6 +950,21 @@ type_run(const char *type)
> > push_all_stats();
> > }
> >
> > + /* If vswitchd started with other_config:flow_restore_wait set as
> "true",
> > + * and the configuration has now changed to "false", enable receiving
> > + * packets from the datapath. */
> > + if (!backer->recv_set_enable && !flow_restore_wait) {
> > + backer->recv_set_enable = true;
> > +
> > + error = dpif_recv_set(backer->dpif, backer->recv_set_enable);
> > + if (error) {
> > + VLOG_ERR("Failed to enable receiving packets in dpif.");
> > + return error;
> > + }
> > + dpif_flow_flush(backer->dpif);
> > + backer->need_revalidate = REV_RECONFIGURE;
> > + }
> > +
> > if (backer->need_revalidate
> > || !tag_set_is_empty(&backer->revalidate_set)) {
> > struct tag_set revalidate_set = backer->revalidate_set;
> > @@ -1040,7 +1058,10 @@ type_run(const char *type)
> > }
> > }
> >
> > - if (timer_expired(&backer->next_expiration)) {
> > + if (!backer->recv_set_enable) {
> > + /* A minimum of 1000 ms delay. */
> > + timer_set_duration(&backer->next_expiration, 1000);
> > + } else if (timer_expired(&backer->next_expiration)) {
> > int delay = expire(backer);
> > timer_set_duration(&backer->next_expiration, delay);
> > }
> > @@ -1106,6 +1127,11 @@ dpif_backer_run_fast(struct dpif_backer *backer,
> int max_batch)
> > {
> > unsigned int work;
> >
> > + /* If recv_set_enable is false, we should not handle upcalls. */
> > + if (!backer->recv_set_enable) {
> > + return 0;
> > + }
> > +
> > /* Handle one or more batches of upcalls, until there's nothing left to
> do
> > * or until we do a fixed total amount of work.
> > *
> > @@ -1305,9 +1331,16 @@ open_dpif_backer(const char *type, struct
> dpif_backer **backerp)
> > backer->need_revalidate = 0;
> > simap_init(&backer->tnl_backers);
> > tag_set_init(&backer->revalidate_set);
> > + if (flow_restore_wait) {
> > + backer->recv_set_enable = false;
> > + } else {
> > + backer->recv_set_enable = true;
> > + }
> > *backerp = backer;
> >
> > - dpif_flow_flush(backer->dpif);
> > + if (backer->recv_set_enable) {
> > + dpif_flow_flush(backer-
Re: [ovs-dev] [PATCH] debian: Don't fail ovs-controller restart if daemon not running.
On Wed, May 29, 2013 at 12:00 PM, Ben Pfaff wrote: > On Tue, May 28, 2013 at 05:24:16PM -0700, Gurucharan Shetty wrote: > > Reported-by: Maxime Brun > > Signed-off-by: Gurucharan Shetty > > Looks good, thanks. > > Thank you. I pushed this to master, 1.11, 1.10 and 1.9. I thought we had converted all this stuff to use ovs-lib.sh, but I guess > not. > We have the openvswitch-ipsec and the openvswitch-controller with the old model. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH net-next v5 1/2] net: Use 16bits for *_headers fields of struct skbuff
Simon, On Thu, May 23, 2013 at 11:51 PM, Simon Horman wrote: > In order to mitigate ongoing incresase in the size of struct skbuff > use 16 bit integer offsets rather than pointers for inner_*_headers. > > This appears to reduce the size of struct skbuff from 0xd0 to 0xc0 > bytes on x86_64 with the following all unset. > > CONFIG_XFRM > CONFIG_NF_CONNTRACK > CONFIG_NF_CONNTRACK_MODULE > NET_SKBUFF_NF_DEFRAG_NEEDED > CONFIG_BRIDGE_NETFILTER > CONFIG_NET_SCHED > CONFIG_IPV6_NDISC_NODETYPE > CONFIG_NET_DMA > CONFIG_NETWORK_SECMARK > > Signed-off-by: Simon Horman I'm getting crashes in csum_partial() on several ARM platforms that I bisected down to this patch (and reverting this + "MPLS: Add limited GSO support" due to conflicts) results in a working kernel. The failures started with the 0529 linux-next, didn't exist in 0528. Unfortunately I'm not getting a useful stack from the crashes: [6.495560] Unable to handle kernel paging request at virtual address eb00 [6.502769] pgd = e9084000 [6.505465] [eb00] *pgd= [6.509034] Internal error: Oops: 5 [#1] PREEMPT SMP ARM [6.514329] Modules linked in: [6.517378] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.10.0-rc2-00483-g1a37e41 #27 [6.525015] task: c06e1980 ti: c06d6000 task.ti: c06d6000 [6.530402] PC is at csum_partial+0x40/0x130 [6.534658] LR is at 0x0 [6.537181] pc : []lr : [<>]psr: 000f0113 [6.537181] sp : c06d7d88 ip : e700c060 fp : c06d80c0 [6.548631] r10: e90e2140 r9 : e9006050 r8 : 0001 [6.553839] r7 : e9006020 r6 : e9b7e400 r5 : r4 : [6.560348] r3 : r2 : 73129f9e r1 : e900601c r0 : eb00 [6.566857] Flags: nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel [6.574146] Control: 10c5387d Table: 2908404a DAC: 0015 [6.579874] Process swapper/0 (pid: 0, stack limit = 0xc06d6238) [6.585862] Stack: (0xc06d7d88 to 0xc06d8000) [6.590207] 7d80: c0714fc0 e900601c e9006050 c043a0b8 e92737b0 [6.598365] 7da0: e9273740 c043c1d4 c0010208 c0f83cc0 c06d4cc0 c0f83cc0 e9b32000 [6.606523] 7dc0: c06d4cc0 0001 c06d80c0 e9273758 00989680 0001 e9b7aec0 [6.614680] 7de0: e90e2140 e9b7af30 e9b7e400 e9b7e490 0001 c043c5d4 [6.622838] 7e00: 0001 7cdfa980 0001 e9b7e458 c06d6000 0101 c06d6030 c043c414 [6.630995] 7e20: 00200200 c06d80c0 c002fbe8 0002 7c4716e8 e9b7e458 c073fb80 [6.639153] 7e40: c043c414 e9b7e400 c06d7e68 c002fde8 c0f82678 c0740394 [6.647310] 7e60: c0740594 c0740794 c06d7e68 c06d7e68 0006 0001 0004 c06d8088 [6.655468] 7e80: c06d8080 c06d6000 0001 0004 0101 c002a57c 7c4716e8 0001 [6.663625] 7ea0: c06d37e0 c073f940 c06d80c0 8d4f c06e4970 0020 c06d6018 [6.671783] 7ec0: c02025e8 c06d6028 001d fe000100 c06d6000 [6.679940] 7ee0: c06de42c c002a9a4 c06d3fac c000eaa0 fe00010c c06df074 c06d7f18 c00086f8 [6.688098] 7f00: c0061288 c032b560 600f0013 c06d7f4c c000de60 c06d7f60 0006 [6.696255] 7f20: 7c471300 0001 7c3bbca8 0001 c0f83110 c06e3cd4 [6.704413] 7f40: c06d6000 c06de42c 0015 c06d7f60 c0061288 c032b560 600f0013 [6.712571] 7f60: 7c471300 0001 c0765ac8 c04fe16c c0f83110 c0765ac8 c06e3cd4 [6.720728] 7f80: c032b6b8 4c9c c071ddc7 c06de490 c04fe16c c06d6000 c071ddc7 [6.728886] 7fa0: c06d6000 c000edb4 4c9c c0060b3c c04fe5fc c0f7f9c0 c069f828 [6.737043] 7fc0: c069f2e8 c06cbac0 10c5387d [6.745200] 7fe0: c06de3f4 c06cbabc c06e28e4 406a 411fc090 8074 [6.753359] Code: e0b22003 e0b22004 e0b22005 e0b2200e (e8b04038) [6.759438] ---[ end trace 40c34f89615c2c53 ]--- [6.764041] Kernel panic - not syncing: Fatal exception in interrupt [6.770386] CPU1: stopping [6.773086] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G D 3.10.0-rc2-00483-g1a37e41 #27 -Olof ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [xlate 1.11 0/9] xlate series to branch-1.11
This is the backport of the recently merged xlate series to branch-1.11. I'm sending it out to the list as a backport of this size is slightly unusual, and therefore deserves some explanation. Because of a potentially large performance benefit, we're planning to backport userspace megaflows to 1.11 when Justin is finished with them (soon). This series requires the final patch of this series "ofproto-dpif: Revamp xlate_actions() interface." That patch, doesn't strictly require the entire series, but (in my judgement) managing a half-backported series is going to be a bit of a nightmare, so I've decided to submit the whole thing. I'd a appreciate a review of xlate_in_init(), xlate_out_uninit(), and xlate_actions() in the final patch of the series. Specifically focusing on how the tunnel_ip_tos field of initial_vals is handled. Everything else backported trivially. Once that's reviewed, assuming there are no objections, I'll push this. Ethan Ethan Jackson (9): ofproto-dpif: handle_flow_miss_common() without facets. ofproto-dpif: Avoid redundant facet_find() in facet_lookup_valid(). ofpbuf: New helper ofpbuf_equal(). ofproto-dpif: Ditch SLOW_MATCH slow path reason. ofproto: Ditch SLOW_IN_BAND slow path reason. odp-util: Make slow_path_reasons mutually exclusive. ofproto-dpif: Move odp_actions from subfacet to facet. ofproto-dpif: Rename action_xlate_ctx. ofproto-dpif: Revamp xlate_actions() interface. lib/odp-util.c | 57 +- lib/odp-util.h | 18 +- lib/ofpbuf.h |6 + ofproto/connmgr.c | 15 +- ofproto/connmgr.h | 10 +- ofproto/in-band.c | 31 - ofproto/in-band.h |2 - ofproto/ofproto-dpif.c | 1527 ++-- tests/odp.at |1 - 9 files changed, 742 insertions(+), 925 deletions(-) -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [xlate 1.11 1/9] ofproto-dpif: handle_flow_miss_common() without facets.
handle_flow_miss_common() is intended to be called whether or not a miss needs a facet. Found by inspection. Signed-off-by: Ethan Jackson --- ofproto/ofproto-dpif.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index f74cfcb..e0c88a8 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3637,6 +3637,8 @@ handle_flow_miss_without_facet(struct flow_miss *miss, COVERAGE_INC(facet_suppress); +handle_flow_miss_common(rule, packet, &miss->flow); + ofpbuf_use_stub(&odp_actions, op->stub, sizeof op->stub); dpif_flow_stats_extract(&miss->flow, packet, now, &stats); -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [xlate 1.11 6/9] odp-util: Make slow_path_reasons mutually exclusive.
It's no longer possible for a single datapath flow to be slow
pathed for two different reasons. This patch updates the code to
reflect this fact (marginally simplifying it).
Signed-off-by: Ethan Jackson
---
lib/odp-util.c | 53 +++-
lib/odp-util.h | 10 -
ofproto/ofproto-dpif.c | 45 ++--
3 files changed, 59 insertions(+), 49 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 1fba981..b92a7ca 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -170,11 +170,9 @@ format_odp_sample_action(struct ds *ds, const struct
nlattr *attr)
}
static const char *
-slow_path_reason_to_string(uint32_t data)
+slow_path_reason_to_string(enum slow_path_reason reason)
{
-enum slow_path_reason bit = (enum slow_path_reason) data;
-
-switch (bit) {
+switch (reason) {
case SLOW_CFM:
return "cfm";
case SLOW_LACP:
@@ -183,11 +181,26 @@ slow_path_reason_to_string(uint32_t data)
return "stp";
case SLOW_CONTROLLER:
return "controller";
+case __SLOW_MAX:
default:
return NULL;
}
}
+static enum slow_path_reason
+string_to_slow_path_reason(const char *string)
+{
+enum slow_path_reason i;
+
+for (i = 1; i < __SLOW_MAX; i++) {
+if (!strcmp(string, slow_path_reason_to_string(i))) {
+return i;
+}
+}
+
+return 0;
+}
+
static int
parse_flags(const char *s, const char *(*bit_to_string)(uint32_t),
uint32_t *res)
@@ -282,10 +295,10 @@ format_odp_userspace_action(struct ds *ds, const struct
nlattr *attr)
cookie.sflow.output);
} else if (userdata_len == sizeof cookie.slow_path
&& cookie.type == USER_ACTION_COOKIE_SLOW_PATH) {
-ds_put_cstr(ds, ",slow_path(");
-format_flags(ds, slow_path_reason_to_string,
- cookie.slow_path.reason, ',');
-ds_put_format(ds, ")");
+const char *reason;
+reason = slow_path_reason_to_string(cookie.slow_path.reason);
+reason = reason ? reason : "";
+ds_put_format(ds, ",slow_path(%s)", reason);
} else if (userdata_len == sizeof cookie.flow_sample
&& cookie.type == USER_ACTION_COOKIE_FLOW_SAMPLE) {
ds_put_format(ds, ",flow_sample(probability=%"PRIu16
@@ -496,25 +509,27 @@ parse_odp_action(const char *s, const struct simap
*port_names,
odp_put_userspace_action(pid, &cookie, sizeof cookie.sflow,
actions);
return n;
-} else if (sscanf(s, "userspace(pid=%lli,slow_path%n", &pid, &n) > 0
+} else if (sscanf(s, "userspace(pid=%lli,slow_path(%n", &pid, &n) > 0
&& n > 0) {
union user_action_cookie cookie;
-int res;
+char reason[32];
+
+if (s[n] == ')' && s[n + 1] == ')') {
+reason[0] = '\0';
+n += 2;
+} else if (sscanf(s + n, "%31[^)]))", reason) > 0) {
+n += strlen(reason) + 2;
+} else {
+return -EINVAL;
+}
cookie.type = USER_ACTION_COOKIE_SLOW_PATH;
cookie.slow_path.unused = 0;
-cookie.slow_path.reason = 0;
+cookie.slow_path.reason = string_to_slow_path_reason(reason);
-res = parse_flags(&s[n], slow_path_reason_to_string,
- &cookie.slow_path.reason);
-if (res < 0) {
-return res;
-}
-n += res;
-if (s[n] != ')') {
+if (reason[0] && !cookie.slow_path.reason) {
return -EINVAL;
}
-n++;
odp_put_userspace_action(pid, &cookie, sizeof cookie.slow_path,
actions);
diff --git a/lib/odp-util.h b/lib/odp-util.h
index d48c1a3..baee7a1 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -174,11 +174,11 @@ void odp_put_skb_mark_action(const uint32_t skb_mark,
/* Reasons why a subfacet might not be fast-pathable. */
enum slow_path_reason {
-/* These reasons are mutually exclusive. */
-SLOW_CFM = 1 << 0, /* CFM packets need per-packet processing. */
-SLOW_LACP = 1 << 1, /* LACP packets need per-packet processing. */
-SLOW_STP = 1 << 2, /* STP packets need per-packet processing. */
-SLOW_CONTROLLER = 1 << 3, /* Packets must go to OpenFlow controller. */
+SLOW_CFM = 1, /* CFM packets need per-packet processing. */
+SLOW_LACP, /* LACP packets need per-packet processing. */
+SLOW_STP, /* STP packets need per-packet processing. */
+SLOW_CONTROLLER,/* Packets must go to OpenFlow controller. */
+__
[ovs-dev] [xlate 1.11 7/9] ofproto-dpif: Move odp_actions from subfacet to facet.
Upon close inspection, it appears that it's not possible for
actions to differ between subfacets belonging to a given facet.
Given this fact, it makes sense to move datapath actions from
subfacets to their parent facets. It's both conceptually more
straightforward, and necessary for future threading and megaflow
work.
Co-authored-by: Justin Pettit
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif.c | 408 +++-
1 file changed, 126 insertions(+), 282 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ce39d2d..d1c14a4 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -356,8 +356,6 @@ enum subfacet_path {
SF_SLOW_PATH, /* Send-to-userspace action is installed. */
};
-static const char *subfacet_path_to_string(enum subfacet_path);
-
/* A dpif flow and actions associated with a facet.
*
* See also the large comment on struct facet. */
@@ -377,19 +375,8 @@ struct subfacet {
uint64_t dp_packet_count; /* Last known packet count in the datapath. */
uint64_t dp_byte_count; /* Last known byte count in the datapath. */
-/* Datapath actions.
- *
- * These should be essentially identical for every subfacet in a facet, but
- * may differ in trivial ways due to VLAN splinters. */
-size_t actions_len; /* Number of bytes in actions[]. */
-struct nlattr *actions; /* Datapath actions. */
-
-enum slow_path_reason slow; /* 0 if fast path may be used. */
enum subfacet_path path;/* Installed in datapath? */
-/* Initial values of the packet that may be needed later. */
-struct initial_vals initial_vals;
-
/* Datapath port the packet arrived on. This is needed to remove
* flows for ports that are no longer part of the bridge. Since the
* flow definition only has the OpenFlow port number and the port is
@@ -414,15 +401,11 @@ static void subfacet_reset_dp_stats(struct subfacet *,
static void subfacet_update_time(struct subfacet *, long long int used);
static void subfacet_update_stats(struct subfacet *,
const struct dpif_flow_stats *);
-static void subfacet_make_actions(struct subfacet *,
- const struct ofpbuf *packet);
static int subfacet_install(struct subfacet *,
-const struct nlattr *actions, size_t actions_len,
-struct dpif_flow_stats *, enum slow_path_reason);
+const struct ofpbuf *odp_actions,
+struct dpif_flow_stats *);
static void subfacet_uninstall(struct subfacet *);
-static enum subfacet_path subfacet_want_path(enum slow_path_reason);
-
/* An exact-match instantiation of an OpenFlow flow.
*
* A facet associates a "struct flow", which represents the Open vSwitch
@@ -475,18 +458,21 @@ struct facet {
struct netflow_flow nf_flow; /* Per-flow NetFlow tracking data. */
uint8_t tcp_flags; /* TCP flags seen for this 'rule'. */
-/* Properties of datapath actions.
- *
- * Every subfacet has its own actions because actions can differ slightly
- * between splintered and non-splintered subfacets due to the VLAN tag
- * being initially different (present vs. absent). All of them have these
- * properties in common so we just store one copy of them here. */
bool has_learn; /* Actions include NXAST_LEARN? */
bool has_normal; /* Actions output to OFPP_NORMAL? */
bool has_fin_timeout;/* Actions include NXAST_FIN_TIMEOUT? */
tag_type tags; /* Tags that would require revalidation. */
mirror_mask_t mirrors; /* Bitmap of dependent mirrors. */
+/* Datapath actions. */
+struct ofpbuf odp_actions;
+uint64_t odp_actions_stub[256 / 8];
+
+/* Initial values of the packet that may be needed later. */
+struct initial_vals initial_vals;
+
+enum slow_path_reason slow; /* 0 if fast path may be used. */
+
/* Storage for a single subfacet, to reduce malloc() time and space
* overhead. (A facet always has at least one subfacet and in the common
* case has exactly one subfacet. However, 'one_subfacet' may not
@@ -497,8 +483,7 @@ struct facet {
long long int learn_rl; /* Rate limiter for facet_learn(). */
};
-static struct facet *facet_create(struct rule_dpif *,
- const struct flow *, uint32_t hash);
+static struct facet *facet_create(const struct flow_miss *, uint32_t hash);
static void facet_remove(struct facet *);
static void facet_free(struct facet *);
@@ -518,8 +503,6 @@ static void facet_learn(struct facet *);
static void facet_account(struct facet *);
static void push_all_stats(void);
-static struct subfacet *facet_get_subfacet(struct facet *);
-
static bool facet_is_controller_flow(struct facet *);
struct ofport_dpif {
@
[ovs-dev] [xlate 1.11 2/9] ofproto-dpif: Avoid redundant facet_find() in facet_lookup_valid().
Suggested-by: Ben Pfaff
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e0c88a8..3a1484e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -507,7 +507,7 @@ static struct facet *facet_find(struct ofproto_dpif *,
const struct flow *, uint32_t hash);
static struct facet *facet_lookup_valid(struct ofproto_dpif *,
const struct flow *, uint32_t hash);
-static void facet_revalidate(struct facet *);
+static bool facet_revalidate(struct facet *);
static bool facet_check_consistency(struct facet *);
static void facet_flush_stats(struct facet *);
@@ -4865,10 +4865,8 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const
struct flow *flow,
if (facet
&& (ofproto->backer->need_revalidate
|| tag_set_intersects(&ofproto->backer->revalidate_set,
- facet->tags))) {
-facet_revalidate(facet);
-
-/* facet_revalidate() may have destroyed 'facet'. */
+ facet->tags))
+&& !facet_revalidate(facet)) {
facet = facet_find(ofproto, flow, hash);
}
@@ -5040,8 +5038,10 @@ facet_check_consistency(struct facet *facet)
* where it is and recompiles its actions anyway.
*
* - If any of 'facet''s subfacets correspond to a new flow according to
- * ofproto_receive(), 'facet' is removed. */
-static void
+ * ofproto_receive(), 'facet' is removed.
+ *
+ * Returns true if 'facet' is still valid. False if 'facet' was removed. */
+static bool
facet_revalidate(struct facet *facet)
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
@@ -5076,7 +5076,7 @@ facet_revalidate(struct facet *facet)
|| recv_ofproto != ofproto
|| memcmp(&recv_flow, &facet->flow, sizeof recv_flow)) {
facet_remove(facet);
-return;
+return false;
}
}
@@ -5156,6 +5156,8 @@ facet_revalidate(struct facet *facet)
facet->used = new_rule->up.created;
facet->prev_used = facet->used;
}
+
+return true;
}
/* Updates 'facet''s used time. Caller is responsible for calling
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [xlate 1.11 5/9] ofproto: Ditch SLOW_IN_BAND slow path reason.
Before this patch, when in band control was enabled, every DHCP
packet had to be sent to userspace to calculate it's actions.
Those DHCP packets intended for the local port would have a special
action added to ensure they actually make it there. This
unnecessarily complicates the code, so this patch takes a slightly
different approach. When in-band is enabled, *all* DHCP packets
must be sent to the local port. This guarantees that
xlate_actions() returns the same result every time for a given
flow.
Signed-off-by: Ethan Jackson
---
lib/odp-util.c |2 --
lib/odp-util.h |6 +---
ofproto/connmgr.c | 15 +++--
ofproto/connmgr.h | 10 +++---
ofproto/in-band.c | 31 --
ofproto/in-band.h |2 --
ofproto/ofproto-dpif.c | 81 +---
7 files changed, 37 insertions(+), 110 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 42f4828..1fba981 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -181,8 +181,6 @@ slow_path_reason_to_string(uint32_t data)
return "lacp";
case SLOW_STP:
return "stp";
-case SLOW_IN_BAND:
-return "in_band";
case SLOW_CONTROLLER:
return "controller";
default:
diff --git a/lib/odp-util.h b/lib/odp-util.h
index ea9bf3a..d48c1a3 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -178,11 +178,7 @@ enum slow_path_reason {
SLOW_CFM = 1 << 0, /* CFM packets need per-packet processing. */
SLOW_LACP = 1 << 1, /* LACP packets need per-packet processing. */
SLOW_STP = 1 << 2, /* STP packets need per-packet processing. */
-SLOW_IN_BAND = 1 << 3, /* In-band control needs every packet. */
-
-/* Mutually exclusive with SLOW_CFM, SLOW_LACP, SLOW_STP.
- * Could possibly appear with SLOW_IN_BAND. */
-SLOW_CONTROLLER = 1 << 4, /* Packets must go to OpenFlow controller. */
+SLOW_CONTROLLER = 1 << 3, /* Packets must go to OpenFlow controller. */
};
#endif /* odp-util.h */
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index b8cdfa5..1a1ab6c 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1643,17 +1643,10 @@ any_extras_changed(const struct connmgr *mgr,
/* In-band implementation. */
bool
-connmgr_msg_in_hook(struct connmgr *mgr, const struct flow *flow,
-const struct ofpbuf *packet)
-{
-return mgr->in_band && in_band_msg_in_hook(mgr->in_band, flow, packet);
-}
-
-bool
-connmgr_may_set_up_flow(struct connmgr *mgr, const struct flow *flow,
-uint32_t local_odp_port,
-const struct nlattr *odp_actions,
-size_t actions_len)
+connmgr_must_output_local(struct connmgr *mgr, const struct flow *flow,
+ uint32_t local_odp_port,
+ const struct nlattr *odp_actions,
+ size_t actions_len)
{
return !mgr->in_band || in_band_rule_check(flow, local_odp_port,
odp_actions, actions_len);
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 48b8119..506f9c7 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -156,12 +156,10 @@ void connmgr_set_extra_in_band_remotes(struct connmgr *,
void connmgr_set_in_band_queue(struct connmgr *, int queue_id);
/* In-band implementation. */
-bool connmgr_msg_in_hook(struct connmgr *, const struct flow *,
- const struct ofpbuf *packet);
-bool connmgr_may_set_up_flow(struct connmgr *, const struct flow *,
- uint32_t local_odp_port,
- const struct nlattr *odp_actions,
- size_t actions_len);
+bool connmgr_must_output_local(struct connmgr *, const struct flow *,
+ uint32_t local_odp_port,
+ const struct nlattr *odp_actions,
+ size_t actions_len);
/* Fail-open and in-band implementation. */
void connmgr_flushed(struct connmgr *);
diff --git a/ofproto/in-band.c b/ofproto/in-band.c
index 1a08fcc..ba6fc54 100644
--- a/ofproto/in-band.c
+++ b/ofproto/in-band.c
@@ -222,37 +222,6 @@ refresh_local(struct in_band *ib)
return true;
}
-/* Returns true if 'packet' should be sent to the local port regardless
- * of the flow table. */
-bool
-in_band_msg_in_hook(struct in_band *in_band, const struct flow *flow,
-const struct ofpbuf *packet)
-{
-/* Regardless of how the flow table is configured, we want to be
- * able to see replies to our DHCP requests. */
-if (flow->dl_type == htons(ETH_TYPE_IP)
-&& flow->nw_proto == IPPROTO_UDP
-&& flow->tp_src == htons(DHCP_SERVER_PORT)
-&& flow->tp_dst == htons(DHCP_CLIENT_PORT)
-&& packet->l7) {
-struct dhcp_header *dhcp;
-
-dhcp = ofpbuf_at(packet, (char *)packet->l7 - (cha
[ovs-dev] [xlate 1.11 8/9] ofproto-dpif: Rename action_xlate_ctx.
This patch changes the name of action_xlate_ctx to xlate_ctx. Aside
from being a bit snappier, it fits more cleanly with structures
added in future patches.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif.c | 183
1 file changed, 90 insertions(+), 93 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d1c14a4..d2da4c8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -207,8 +207,8 @@ static int set_stp_port(struct ofport *,
static bool ofbundle_includes_vlan(const struct ofbundle *, uint16_t vlan);
-struct action_xlate_ctx {
-/* action_xlate_ctx_init() initializes these members. */
+struct xlate_ctx {
+/* xlate_ctx_init() initializes these members. */
/* The ofproto. */
struct ofproto_dpif *ofproto;
@@ -250,19 +250,19 @@ struct action_xlate_ctx {
* resubmit or OFPP_TABLE action didn't find a matching rule.
*
* This is normally null so the client has to set it manually after
- * calling action_xlate_ctx_init(). */
-void (*resubmit_hook)(struct action_xlate_ctx *, struct rule_dpif *rule);
+ * calling xlate_ctx_init(). */
+void (*resubmit_hook)(struct xlate_ctx *, struct rule_dpif *rule);
/* If nonnull, flow translation calls this function to report some
* significant decision, e.g. to explain why OFPP_NORMAL translation
* dropped a packet. */
-void (*report_hook)(struct action_xlate_ctx *, const char *s);
+void (*report_hook)(struct xlate_ctx *, const char *s);
/* If nonnull, flow translation credits the specified statistics to each
* rule reached through a resubmit or OFPP_TABLE action.
*
* This is normally null so the client has to set it manually after
- * calling action_xlate_ctx_init(). */
+ * calling xlate_ctx_init(). */
const struct dpif_flow_stats *resubmit_stats;
/* xlate_actions() initializes and uses these members. The client might want
@@ -309,18 +309,17 @@ struct initial_vals {
uint8_t tunnel_ip_tos;
};
-static void action_xlate_ctx_init(struct action_xlate_ctx *,
- struct ofproto_dpif *, const struct flow *,
- const struct initial_vals *initial_vals,
- struct rule_dpif *,
- uint8_t tcp_flags, const struct ofpbuf *);
-static void xlate_actions(struct action_xlate_ctx *,
- const struct ofpact *ofpacts, size_t ofpacts_len,
- struct ofpbuf *odp_actions);
-static void xlate_actions_for_side_effects(struct action_xlate_ctx *,
+static void xlate_ctx_init(struct xlate_ctx *, struct ofproto_dpif *,
+ const struct flow *,
+ const struct initial_vals *initial_vals,
+ struct rule_dpif *, uint8_t tcp_flags,
+ const struct ofpbuf *);
+static void xlate_actions(struct xlate_ctx *, const struct ofpact *ofpacts,
+ size_t ofpacts_len, struct ofpbuf *odp_actions);
+static void xlate_actions_for_side_effects(struct xlate_ctx *,
const struct ofpact *ofpacts,
size_t ofpacts_len);
-static void xlate_table_action(struct action_xlate_ctx *, uint16_t in_port,
+static void xlate_table_action(struct xlate_ctx *, uint16_t in_port,
uint8_t table_id, bool may_packet_in);
static size_t put_userspace_action(const struct ofproto_dpif *,
@@ -335,7 +334,7 @@ static void compose_slow_path(const struct ofproto_dpif *,
const struct flow *,
const struct nlattr **actionsp,
size_t *actions_lenp);
-static void xlate_report(struct action_xlate_ctx *ctx, const char *s);
+static void xlate_report(struct xlate_ctx *ctx, const char *s);
/* A subfacet (see "struct subfacet" below) has three possible installation
* states:
@@ -808,7 +807,7 @@ static size_t compose_sflow_action(const struct
ofproto_dpif *,
static void compose_ipfix_action(const struct ofproto_dpif *,
struct ofpbuf *odp_actions,
const struct flow *);
-static void add_mirror_actions(struct action_xlate_ctx *ctx,
+static void add_mirror_actions(struct xlate_ctx *ctx,
const struct flow *flow);
/* Global variables. */
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
@@ -3608,7 +3607,7 @@ handle_flow_miss_without_facet(struct flow_miss *miss,
{
struct rule_dpif *rule = rule_dpif_lookup(miss->ofproto, &miss->flow);
long long int now = time_msec();
-struct action_xlate_ctx ctx;
+struct xlate_ctx ctx;
struct ofpbuf *packet;
LIST_FOR_EACH (packet, list_node, &miss->packets) {
@@ -3625,9 +3624,8 @@ handle_flow_miss
[ovs-dev] [xlate 1.11 4/9] ofproto-dpif: Ditch SLOW_MATCH slow path reason.
Before this patch, datapath keys with ODP_FIT_TO_LITTLE, would be
assigned subfacets and installed in the kernel with a SLOW_MATCH
slow path reason. This is problematic, because these flow keys
can't be reliable converted into a 'struct flow' thus breaking a
fundamental assumption of ofproto-dpif. This patch circumvents the
issue by skipping facet creation for these flows altogether. This
approach has the added benefit of simplifying the code for future
patches.
Signed-off-by: Ethan Jackson
---
lib/odp-util.c |2 --
lib/odp-util.h |4
ofproto/ofproto-dpif.c | 34 +-
tests/odp.at |1 -
4 files changed, 13 insertions(+), 28 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 1988c63..42f4828 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -185,8 +185,6 @@ slow_path_reason_to_string(uint32_t data)
return "in_band";
case SLOW_CONTROLLER:
return "controller";
-case SLOW_MATCH:
-return "match";
default:
return NULL;
}
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 0b34383..ea9bf3a 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -183,10 +183,6 @@ enum slow_path_reason {
/* Mutually exclusive with SLOW_CFM, SLOW_LACP, SLOW_STP.
* Could possibly appear with SLOW_IN_BAND. */
SLOW_CONTROLLER = 1 << 4, /* Packets must go to OpenFlow controller. */
-
-/* This can appear on its own, or, theoretically at least, along with any
- * other combination of reasons. */
-SLOW_MATCH = 1 << 5,/* Datapath can't match specifically enough. */
};
#endif /* odp-util.h */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3a1484e..6c0aa2d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3764,7 +3764,13 @@ handle_flow_miss(struct flow_miss *miss, struct
flow_miss_op *ops,
if (!facet) {
struct rule_dpif *rule = rule_dpif_lookup(ofproto, &miss->flow);
-if (!flow_miss_should_make_facet(ofproto, miss, hash)) {
+/* There does not exist a bijection between 'struct flow' and datapath
+ * flow keys with fitness ODP_FIT_TO_LITTLE. This breaks a fundamental
+ * assumption used throughout the facet and subfacet handling code.
+ * Since we have to handle these misses in userspace anyway, we simply
+ * skip facet creation, avoiding the problem alltogether. */
+if (miss->key_fitness == ODP_FIT_TOO_LITTLE
+|| !flow_miss_should_make_facet(ofproto, miss, hash)) {
handle_flow_miss_without_facet(miss, rule, ops, n_ops);
return;
}
@@ -5095,19 +5101,16 @@ facet_revalidate(struct facet *facet)
memset(&ctx, 0, sizeof ctx);
ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
-enum slow_path_reason slow;
-
action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
&subfacet->initial_vals, new_rule, 0, NULL);
xlate_actions(&ctx, new_rule->up.ofpacts, new_rule->up.ofpacts_len,
&odp_actions);
-slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
-if (subfacet_should_install(subfacet, slow, &odp_actions)) {
+if (subfacet_should_install(subfacet, ctx.slow, &odp_actions)) {
struct dpif_flow_stats stats;
-subfacet_install(subfacet,
- odp_actions.data, odp_actions.size, &stats, slow);
+subfacet_install(subfacet, odp_actions.data, odp_actions.size,
+ &stats, ctx.slow);
subfacet_update_stats(subfacet, &stats);
if (!new_actions) {
@@ -5137,7 +5140,7 @@ facet_revalidate(struct facet *facet)
i = 0;
LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
-subfacet->slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
+subfacet->slow = ctx.slow;
if (new_actions && new_actions[i].odp_actions) {
free(subfacet->actions);
@@ -5335,9 +5338,7 @@ subfacet_create(struct facet *facet, struct flow_miss
*miss,
subfacet->dp_byte_count = 0;
subfacet->actions_len = 0;
subfacet->actions = NULL;
-subfacet->slow = (subfacet->key_fitness == ODP_FIT_TOO_LITTLE
- ? SLOW_MATCH
- : 0);
+subfacet->slow = 0;
subfacet->path = SF_NOT_INSTALLED;
subfacet->initial_vals = miss->initial_vals;
subfacet->odp_in_port = miss->odp_in_port;
@@ -5432,7 +5433,7 @@ subfacet_make_actions(struct subfacet *subfacet, const
struct ofpbuf *packet,
facet->nf_flow.output_iface = ctx.nf_output_iface;
facet->mirrors = ctx.mirrors;
-subfacet->slow = (subfacet->slow & SLOW_MATCH) | ctx.slow;
+subfacet->slow = ctx.slow;
if (subfacet->actions_len != odp_actions->size
|| memcmp(subfacet->actions, odp_actions-
[ovs-dev] [xlate 1.11 3/9] ofpbuf: New helper ofpbuf_equal().
Used in future commits.
Signed-off-by: Ethan Jackson
---
lib/ofpbuf.h |6 ++
1 file changed, 6 insertions(+)
diff --git a/lib/ofpbuf.h b/lib/ofpbuf.h
index 8b03c7e..0c12162 100644
--- a/lib/ofpbuf.h
+++ b/lib/ofpbuf.h
@@ -107,6 +107,12 @@ static inline struct ofpbuf *ofpbuf_from_list(const struct
list *list)
}
void ofpbuf_list_delete(struct list *);
+static inline bool
+ofpbuf_equal(const struct ofpbuf *a, const struct ofpbuf *b)
+{
+return a->size == b->size && memcmp(a->data, b->data, a->size) == 0;
+}
+
#ifdef __cplusplus
}
#endif
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif.c: Replace vsp_realdev_to_vlandev() with two new functions
In ofproto-dpif.c, function vsp_realdev_to_vlandev() is called with both
OpenFlow port and datapath port number as argument. This patch replaces
the function vsp_realdev_to_vlandev() with vsp_ofp_realdev_to_vlandev() and
vsp_odp_realdev_to_vlandev(), which take OpenFlow port number and datapth
port number respectively.
Signed-off-by: Alex Wang
---
ofproto/ofproto-dpif.c | 61 +---
1 file changed, 42 insertions(+), 19 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 280fd57..193064b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -586,8 +586,12 @@ struct vlan_splinter {
int vid;
};
-static uint32_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
- uint32_t realdev, ovs_be16 vlan_tci);
+static uint16_t vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *,
+ uint16_t realdev,
+ ovs_be16 vlan_tci);
+static uint32_t vsp_odp_realdev_to_vlandev(const struct ofproto_dpif *,
+ uint32_t realdev,
+ ovs_be16 vlan_tci);
static bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *);
static void vsp_remove(struct ofport_dpif *);
static void vsp_add(struct ofport_dpif *, uint16_t realdev_ofp_port, int vid);
@@ -6183,8 +6187,8 @@ compose_output_action__(struct action_xlate_ctx *ctx,
uint16_t ofp_port,
ctx->flow.tunnel = flow_tnl; /* Restore tunnel metadata */
} else {
odp_port = ofport->odp_port;
-out_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
- ctx->flow.vlan_tci);
+out_port = vsp_odp_realdev_to_vlandev(ctx->ofproto, odp_port,
+ ctx->flow.vlan_tci);
if (out_port != odp_port) {
ctx->flow.vlan_tci = htons(0);
}
@@ -8729,33 +8733,52 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int vid)
return hash_2words(realdev_ofp_port, vid);
}
-/* Returns the ODP port number of the Linux VLAN device that corresponds to
- * 'vlan_tci' on the network device with port number 'realdev_odp_port' in
- * 'ofproto'. For example, given 'realdev_odp_port' of eth0 and 'vlan_tci' 9,
- * it would return the port number of eth0.9.
+/* Returns the OFP port number of the Linux VLAN device that corresponds to
+ * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
+ * 'ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
+ * 'vlan_tci' 9, it would return the port number of eth0.9.
*
- * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
- * function just returns its 'realdev_odp_port' argument. */
-static uint32_t
-vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
- uint32_t realdev_odp_port, ovs_be16 vlan_tci)
+ * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
+ * function just returns its 'realdev_ofp_port' argument. */
+static uint16_t
+vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
+ uint16_t realdev_ofp_port, ovs_be16 vlan_tci)
{
if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
-uint16_t realdev_ofp_port;
int vid = vlan_tci_to_vid(vlan_tci);
const struct vlan_splinter *vsp;
-realdev_ofp_port = odp_port_to_ofp_port(ofproto, realdev_odp_port);
HMAP_FOR_EACH_WITH_HASH (vsp, realdev_vid_node,
hash_realdev_vid(realdev_ofp_port, vid),
&ofproto->realdev_vid_map) {
if (vsp->realdev_ofp_port == realdev_ofp_port
&& vsp->vid == vid) {
-return ofp_port_to_odp_port(ofproto, vsp->vlandev_ofp_port);
+return vsp->vlandev_ofp_port;
}
}
}
-return realdev_odp_port;
+return realdev_ofp_port;
+}
+
+/* Returns the ODP port number of the Linux VLAN device that corresponds to
+ * 'vlan_tci' on the network device with datapath port number
+ * 'realdev_odp_port'. For example, given 'realdev_odp_port' of eth0 and
+ * 'vlan_tci' 9, it would return the port number of eth0.9.
+ *
+ * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
+ * function just returns its 'realdev_odp_port' argument. */
+static uint32_t
+vsp_odp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
+ uint32_t realdev_odp_port, ovs_be16 vlan_tci)
+{
+uint16_t realdev_ofp_port;
+uint16_t vlandev_ofp_port;
+
+realdev_ofp_port = odp_port_to_ofp_port(ofproto, realdev_odp_port);
+vlandev_ofp_port = vsp_ofp_realdev_to_vlandev(ofproto, realdev_ofp_port,
+ vlan_tci);
+
+return ofp_port_to_odp_port(ofproto, vlandev_ofp_port);
}
static struct
Re: [ovs-dev] [PATCH net-next v5 1/2] net: Use 16bits for *_headers fields of struct skbuff
Sorry, that was not a great bug report. So, looks like it's the removal of NET_SKBUFF_DATA_USES_OFFSET that does it for me. The devices I've seen it with are with asix usb-ethernet adapters (on Tegra seaboard) and with mv643xx_eth on cubox (dove). I'll try to get a better stacktrace out of it. Simon, maybe you can reproduce on shmobile hardware? On Wed, May 29, 2013 at 2:21 PM, Olof Johansson wrote: > Simon, > > On Thu, May 23, 2013 at 11:51 PM, Simon Horman wrote: >> In order to mitigate ongoing incresase in the size of struct skbuff >> use 16 bit integer offsets rather than pointers for inner_*_headers. >> >> This appears to reduce the size of struct skbuff from 0xd0 to 0xc0 >> bytes on x86_64 with the following all unset. >> >> CONFIG_XFRM >> CONFIG_NF_CONNTRACK >> CONFIG_NF_CONNTRACK_MODULE >> NET_SKBUFF_NF_DEFRAG_NEEDED >> CONFIG_BRIDGE_NETFILTER >> CONFIG_NET_SCHED >> CONFIG_IPV6_NDISC_NODETYPE >> CONFIG_NET_DMA >> CONFIG_NETWORK_SECMARK >> >> Signed-off-by: Simon Horman > > I'm getting crashes in csum_partial() on several ARM platforms that I > bisected down to this patch (and reverting this + "MPLS: > Add limited GSO support" due to conflicts) results in a working kernel. > > The failures started with the 0529 linux-next, didn't exist in 0528. > > Unfortunately I'm not getting a useful stack from the crashes: > > [6.495560] Unable to handle kernel paging request at virtual > address eb00 > [6.502769] pgd = e9084000 > [6.505465] [eb00] *pgd= > [6.509034] Internal error: Oops: 5 [#1] PREEMPT SMP ARM > [6.514329] Modules linked in: > [6.517378] CPU: 0 PID: 0 Comm: swapper/0 Not tainted > 3.10.0-rc2-00483-g1a37e41 #27 > [6.525015] task: c06e1980 ti: c06d6000 task.ti: c06d6000 > [6.530402] PC is at csum_partial+0x40/0x130 > [6.534658] LR is at 0x0 > [6.537181] pc : []lr : [<>]psr: 000f0113 > [6.537181] sp : c06d7d88 ip : e700c060 fp : c06d80c0 > [6.548631] r10: e90e2140 r9 : e9006050 r8 : 0001 > [6.553839] r7 : e9006020 r6 : e9b7e400 r5 : r4 : > [6.560348] r3 : r2 : 73129f9e r1 : e900601c r0 : eb00 > [6.566857] Flags: nzcv IRQs on FIQs on Mode SVC_32 ISA ARM > Segment kernel > [6.574146] Control: 10c5387d Table: 2908404a DAC: 0015 > [6.579874] Process swapper/0 (pid: 0, stack limit = 0xc06d6238) > [6.585862] Stack: (0xc06d7d88 to 0xc06d8000) > [6.590207] 7d80: c0714fc0 e900601c e9006050 > c043a0b8 e92737b0 > [6.598365] 7da0: e9273740 c043c1d4 c0010208 c0f83cc0 c06d4cc0 > c0f83cc0 e9b32000 > [6.606523] 7dc0: c06d4cc0 0001 c06d80c0 e9273758 00989680 > 0001 e9b7aec0 > [6.614680] 7de0: e90e2140 e9b7af30 e9b7e400 e9b7e490 > 0001 c043c5d4 > [6.622838] 7e00: 0001 7cdfa980 0001 e9b7e458 c06d6000 > 0101 c06d6030 c043c414 > [6.630995] 7e20: 00200200 c06d80c0 c002fbe8 0002 > 7c4716e8 e9b7e458 c073fb80 > [6.639153] 7e40: c043c414 e9b7e400 c06d7e68 c002fde8 > c0f82678 c0740394 > [6.647310] 7e60: c0740594 c0740794 c06d7e68 c06d7e68 0006 > 0001 0004 c06d8088 > [6.655468] 7e80: c06d8080 c06d6000 0001 0004 0101 > c002a57c 7c4716e8 0001 > [6.663625] 7ea0: c06d37e0 c073f940 c06d80c0 8d4f > c06e4970 0020 c06d6018 > [6.671783] 7ec0: c02025e8 c06d6028 001d fe000100 > c06d6000 > [6.679940] 7ee0: c06de42c c002a9a4 c06d3fac c000eaa0 fe00010c > c06df074 c06d7f18 c00086f8 > [6.688098] 7f00: c0061288 c032b560 600f0013 c06d7f4c > c000de60 c06d7f60 0006 > [6.696255] 7f20: 7c471300 0001 7c3bbca8 0001 c0f83110 > c06e3cd4 > [6.704413] 7f40: c06d6000 c06de42c 0015 c06d7f60 c0061288 > c032b560 600f0013 > [6.712571] 7f60: 7c471300 0001 c0765ac8 c04fe16c c0f83110 > c0765ac8 c06e3cd4 > [6.720728] 7f80: c032b6b8 4c9c c071ddc7 c06de490 > c04fe16c c06d6000 c071ddc7 > [6.728886] 7fa0: c06d6000 c000edb4 4c9c c0060b3c c04fe5fc > c0f7f9c0 c069f828 > [6.737043] 7fc0: c069f2e8 > c06cbac0 10c5387d > [6.745200] 7fe0: c06de3f4 c06cbabc c06e28e4 406a 411fc090 > 8074 > [6.753359] Code: e0b22003 e0b22004 e0b22005 e0b2200e (e8b04038) > [6.759438] ---[ end trace 40c34f89615c2c53 ]--- > [6.764041] Kernel panic - not syncing: Fatal exception in interrupt > [6.770386] CPU1: stopping > [6.773086] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G D > 3.10.0-rc2-00483-g1a37e41 #27 > > > -Olof ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto/pktbuf.c: Fix a typo in the comment
On Wed, May 29, 2013 at 01:57:22PM -0700, Alex Wang wrote: > This patch fixes a typo in the comment of the pktbuf_retrieve() function. > > Signed-off-by: Alex Wang Looks good, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif.c: Replace vsp_realdev_to_vlandev() with two new functions
On Wed, May 29, 2013 at 03:01:43PM -0700, Alex Wang wrote: > In ofproto-dpif.c, function vsp_realdev_to_vlandev() is called with both > OpenFlow port and datapath port number as argument. This patch replaces > the function vsp_realdev_to_vlandev() with vsp_ofp_realdev_to_vlandev() and > vsp_odp_realdev_to_vlandev(), which take OpenFlow port number and datapth > port number respectively. > > Signed-off-by: Alex Wang Good catch! The existing code was written so that, if there are no VLAN splinters (the common case), then vsp_realdev_to_vlandev() does not incur the overhead of a pair of ODP-to-OFP and OFP-to-ODP translations. With this patch, I believe that we incur this overhead even when there are no VLAN splinters. Can you refactor it to avoid that, and then resubmit? I'll backport this to older branches when you do. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif.c: Replace vsp_realdev_to_vlandev() with two new functions
I haven't looked at this patch carefully, so my next comment may or
may not be worthwhile, but I'm wondering if it would be cleaner if we
kept a single vsp_realdev_to_vlandev, which took an ofp_port as an
argument, and then had compose_output_action__ pass in the openflow
port number instead of the datapath port number to that function?
Ethan
On Wed, May 29, 2013 at 5:01 PM, Alex Wang wrote:
> In ofproto-dpif.c, function vsp_realdev_to_vlandev() is called with both
> OpenFlow port and datapath port number as argument. This patch replaces
> the function vsp_realdev_to_vlandev() with vsp_ofp_realdev_to_vlandev() and
> vsp_odp_realdev_to_vlandev(), which take OpenFlow port number and datapth
> port number respectively.
>
> Signed-off-by: Alex Wang
> ---
> ofproto/ofproto-dpif.c | 61
> +---
> 1 file changed, 42 insertions(+), 19 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 280fd57..193064b 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -586,8 +586,12 @@ struct vlan_splinter {
> int vid;
> };
>
> -static uint32_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
> - uint32_t realdev, ovs_be16 vlan_tci);
> +static uint16_t vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *,
> + uint16_t realdev,
> + ovs_be16 vlan_tci);
> +static uint32_t vsp_odp_realdev_to_vlandev(const struct ofproto_dpif *,
> + uint32_t realdev,
> + ovs_be16 vlan_tci);
> static bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *);
> static void vsp_remove(struct ofport_dpif *);
> static void vsp_add(struct ofport_dpif *, uint16_t realdev_ofp_port, int
> vid);
> @@ -6183,8 +6187,8 @@ compose_output_action__(struct action_xlate_ctx *ctx,
> uint16_t ofp_port,
> ctx->flow.tunnel = flow_tnl; /* Restore tunnel metadata */
> } else {
> odp_port = ofport->odp_port;
> -out_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
> - ctx->flow.vlan_tci);
> +out_port = vsp_odp_realdev_to_vlandev(ctx->ofproto, odp_port,
> + ctx->flow.vlan_tci);
> if (out_port != odp_port) {
> ctx->flow.vlan_tci = htons(0);
> }
> @@ -8729,33 +8733,52 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int vid)
> return hash_2words(realdev_ofp_port, vid);
> }
>
> -/* Returns the ODP port number of the Linux VLAN device that corresponds to
> - * 'vlan_tci' on the network device with port number 'realdev_odp_port' in
> - * 'ofproto'. For example, given 'realdev_odp_port' of eth0 and 'vlan_tci'
> 9,
> - * it would return the port number of eth0.9.
> +/* Returns the OFP port number of the Linux VLAN device that corresponds to
> + * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
> + * 'ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
> + * 'vlan_tci' 9, it would return the port number of eth0.9.
> *
> - * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
> - * function just returns its 'realdev_odp_port' argument. */
> -static uint32_t
> -vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> - uint32_t realdev_odp_port, ovs_be16 vlan_tci)
> + * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
> + * function just returns its 'realdev_ofp_port' argument. */
> +static uint16_t
> +vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> + uint16_t realdev_ofp_port, ovs_be16 vlan_tci)
> {
> if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
> -uint16_t realdev_ofp_port;
> int vid = vlan_tci_to_vid(vlan_tci);
> const struct vlan_splinter *vsp;
>
> -realdev_ofp_port = odp_port_to_ofp_port(ofproto, realdev_odp_port);
> HMAP_FOR_EACH_WITH_HASH (vsp, realdev_vid_node,
> hash_realdev_vid(realdev_ofp_port, vid),
> &ofproto->realdev_vid_map) {
> if (vsp->realdev_ofp_port == realdev_ofp_port
> && vsp->vid == vid) {
> -return ofp_port_to_odp_port(ofproto, vsp->vlandev_ofp_port);
> +return vsp->vlandev_ofp_port;
> }
> }
> }
> -return realdev_odp_port;
> +return realdev_ofp_port;
> +}
> +
> +/* Returns the ODP port number of the Linux VLAN device that corresponds to
> + * 'vlan_tci' on the network device with datapath port number
> + * 'realdev_odp_port'. For example, given 'realdev_odp_port' of eth0 and
> + * 'vlan_tci' 9, it would return the port number of eth0.9.
> + *
> + * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
Re: [ovs-dev] [PATCH] ofproto-dpif.c: Replace vsp_realdev_to_vlandev() with two new functions
Thanks Ben and Ethan,
I'm okay with both ways.
I'm not sure how likely the function "vsp_realdev_to_vlandev()" will be
called with odp_port as argument in the future. I'll go with the "two
functions" implementation first.
Hope to hear more comments from you.
Thanks
On Wed, May 29, 2013 at 3:25 PM, Ethan Jackson wrote:
> I haven't looked at this patch carefully, so my next comment may or
> may not be worthwhile, but I'm wondering if it would be cleaner if we
> kept a single vsp_realdev_to_vlandev, which took an ofp_port as an
> argument, and then had compose_output_action__ pass in the openflow
> port number instead of the datapath port number to that function?
>
> Ethan
>
> On Wed, May 29, 2013 at 5:01 PM, Alex Wang wrote:
> > In ofproto-dpif.c, function vsp_realdev_to_vlandev() is called with both
> > OpenFlow port and datapath port number as argument. This patch replaces
> > the function vsp_realdev_to_vlandev() with vsp_ofp_realdev_to_vlandev()
> and
> > vsp_odp_realdev_to_vlandev(), which take OpenFlow port number and datapth
> > port number respectively.
> >
> > Signed-off-by: Alex Wang
> > ---
> > ofproto/ofproto-dpif.c | 61
> +---
> > 1 file changed, 42 insertions(+), 19 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 280fd57..193064b 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -586,8 +586,12 @@ struct vlan_splinter {
> > int vid;
> > };
> >
> > -static uint32_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
> > - uint32_t realdev, ovs_be16
> vlan_tci);
> > +static uint16_t vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *,
> > + uint16_t realdev,
> > + ovs_be16 vlan_tci);
> > +static uint32_t vsp_odp_realdev_to_vlandev(const struct ofproto_dpif *,
> > + uint32_t realdev,
> > + ovs_be16 vlan_tci);
> > static bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *);
> > static void vsp_remove(struct ofport_dpif *);
> > static void vsp_add(struct ofport_dpif *, uint16_t realdev_ofp_port,
> int vid);
> > @@ -6183,8 +6187,8 @@ compose_output_action__(struct action_xlate_ctx
> *ctx, uint16_t ofp_port,
> > ctx->flow.tunnel = flow_tnl; /* Restore tunnel metadata */
> > } else {
> > odp_port = ofport->odp_port;
> > -out_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
> > - ctx->flow.vlan_tci);
> > +out_port = vsp_odp_realdev_to_vlandev(ctx->ofproto, odp_port,
> > + ctx->flow.vlan_tci);
> > if (out_port != odp_port) {
> > ctx->flow.vlan_tci = htons(0);
> > }
> > @@ -8729,33 +8733,52 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int
> vid)
> > return hash_2words(realdev_ofp_port, vid);
> > }
> >
> > -/* Returns the ODP port number of the Linux VLAN device that
> corresponds to
> > - * 'vlan_tci' on the network device with port number 'realdev_odp_port'
> in
> > - * 'ofproto'. For example, given 'realdev_odp_port' of eth0 and
> 'vlan_tci' 9,
> > - * it would return the port number of eth0.9.
> > +/* Returns the OFP port number of the Linux VLAN device that
> corresponds to
> > + * 'vlan_tci' on the network device with port number 'realdev_ofp_port'
> in
> > + * 'ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
> > + * 'vlan_tci' 9, it would return the port number of eth0.9.
> > *
> > - * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
> > - * function just returns its 'realdev_odp_port' argument. */
> > -static uint32_t
> > -vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> > - uint32_t realdev_odp_port, ovs_be16 vlan_tci)
> > + * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
> > + * function just returns its 'realdev_ofp_port' argument. */
> > +static uint16_t
> > +vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> > + uint16_t realdev_ofp_port, ovs_be16 vlan_tci)
> > {
> > if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
> > -uint16_t realdev_ofp_port;
> > int vid = vlan_tci_to_vid(vlan_tci);
> > const struct vlan_splinter *vsp;
> >
> > -realdev_ofp_port = odp_port_to_ofp_port(ofproto,
> realdev_odp_port);
> > HMAP_FOR_EACH_WITH_HASH (vsp, realdev_vid_node,
> > hash_realdev_vid(realdev_ofp_port,
> vid),
> > &ofproto->realdev_vid_map) {
> > if (vsp->realdev_ofp_port == realdev_ofp_port
> > && vsp->vid == vid) {
> > -return ofp_port_to_odp_port(ofproto,
> vsp->vlandev_ofp_port);
> > +
Re: [ovs-dev] [PATCH] ofproto-dpif.c: Replace vsp_realdev_to_vlandev() with two new functions
That would avoid an ofp->odp translation since compose_output_action__()
already has the ofp_port. It's probably a good idea. Alex, do you want
to do it that way?
On Wed, May 29, 2013 at 05:25:09PM -0500, Ethan Jackson wrote:
> I haven't looked at this patch carefully, so my next comment may or
> may not be worthwhile, but I'm wondering if it would be cleaner if we
> kept a single vsp_realdev_to_vlandev, which took an ofp_port as an
> argument, and then had compose_output_action__ pass in the openflow
> port number instead of the datapath port number to that function?
>
> Ethan
>
> On Wed, May 29, 2013 at 5:01 PM, Alex Wang wrote:
> > In ofproto-dpif.c, function vsp_realdev_to_vlandev() is called with both
> > OpenFlow port and datapath port number as argument. This patch replaces
> > the function vsp_realdev_to_vlandev() with vsp_ofp_realdev_to_vlandev() and
> > vsp_odp_realdev_to_vlandev(), which take OpenFlow port number and datapth
> > port number respectively.
> >
> > Signed-off-by: Alex Wang
> > ---
> > ofproto/ofproto-dpif.c | 61
> > +---
> > 1 file changed, 42 insertions(+), 19 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 280fd57..193064b 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -586,8 +586,12 @@ struct vlan_splinter {
> > int vid;
> > };
> >
> > -static uint32_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
> > - uint32_t realdev, ovs_be16
> > vlan_tci);
> > +static uint16_t vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *,
> > + uint16_t realdev,
> > + ovs_be16 vlan_tci);
> > +static uint32_t vsp_odp_realdev_to_vlandev(const struct ofproto_dpif *,
> > + uint32_t realdev,
> > + ovs_be16 vlan_tci);
> > static bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *);
> > static void vsp_remove(struct ofport_dpif *);
> > static void vsp_add(struct ofport_dpif *, uint16_t realdev_ofp_port, int
> > vid);
> > @@ -6183,8 +6187,8 @@ compose_output_action__(struct action_xlate_ctx *ctx,
> > uint16_t ofp_port,
> > ctx->flow.tunnel = flow_tnl; /* Restore tunnel metadata */
> > } else {
> > odp_port = ofport->odp_port;
> > -out_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
> > - ctx->flow.vlan_tci);
> > +out_port = vsp_odp_realdev_to_vlandev(ctx->ofproto, odp_port,
> > + ctx->flow.vlan_tci);
> > if (out_port != odp_port) {
> > ctx->flow.vlan_tci = htons(0);
> > }
> > @@ -8729,33 +8733,52 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int vid)
> > return hash_2words(realdev_ofp_port, vid);
> > }
> >
> > -/* Returns the ODP port number of the Linux VLAN device that corresponds to
> > - * 'vlan_tci' on the network device with port number 'realdev_odp_port' in
> > - * 'ofproto'. For example, given 'realdev_odp_port' of eth0 and
> > 'vlan_tci' 9,
> > - * it would return the port number of eth0.9.
> > +/* Returns the OFP port number of the Linux VLAN device that corresponds to
> > + * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
> > + * 'ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
> > + * 'vlan_tci' 9, it would return the port number of eth0.9.
> > *
> > - * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
> > - * function just returns its 'realdev_odp_port' argument. */
> > -static uint32_t
> > -vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> > - uint32_t realdev_odp_port, ovs_be16 vlan_tci)
> > + * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
> > + * function just returns its 'realdev_ofp_port' argument. */
> > +static uint16_t
> > +vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
> > + uint16_t realdev_ofp_port, ovs_be16 vlan_tci)
> > {
> > if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
> > -uint16_t realdev_ofp_port;
> > int vid = vlan_tci_to_vid(vlan_tci);
> > const struct vlan_splinter *vsp;
> >
> > -realdev_ofp_port = odp_port_to_ofp_port(ofproto, realdev_odp_port);
> > HMAP_FOR_EACH_WITH_HASH (vsp, realdev_vid_node,
> > hash_realdev_vid(realdev_ofp_port, vid),
> > &ofproto->realdev_vid_map) {
> > if (vsp->realdev_ofp_port == realdev_ofp_port
> > && vsp->vid == vid) {
> > -return ofp_port_to_odp_port(ofproto,
> > vsp->vlandev_ofp_port);
> > +return vsp->vlandev_ofp_port;
> > }
> > }
> > }
> > -
Re: [ovs-dev] [PATCH] ofproto-dpif.c: Replace vsp_realdev_to_vlandev() with two new functions
I saw you comment after sending the previous email. I'll have some time and
consider both. And I'll resubmit the patch later.
Thanks
On Wed, May 29, 2013 at 3:37 PM, Alex Wang wrote:
> Thanks Ben and Ethan,
>
> I'm okay with both ways.
>
> I'm not sure how likely the function "vsp_realdev_to_vlandev()" will be
> called with odp_port as argument in the future. I'll go with the "two
> functions" implementation first.
>
> Hope to hear more comments from you.
>
> Thanks
>
>
>
> On Wed, May 29, 2013 at 3:25 PM, Ethan Jackson wrote:
>
>> I haven't looked at this patch carefully, so my next comment may or
>> may not be worthwhile, but I'm wondering if it would be cleaner if we
>> kept a single vsp_realdev_to_vlandev, which took an ofp_port as an
>> argument, and then had compose_output_action__ pass in the openflow
>> port number instead of the datapath port number to that function?
>>
>> Ethan
>>
>> On Wed, May 29, 2013 at 5:01 PM, Alex Wang wrote:
>> > In ofproto-dpif.c, function vsp_realdev_to_vlandev() is called with both
>> > OpenFlow port and datapath port number as argument. This patch replaces
>> > the function vsp_realdev_to_vlandev() with vsp_ofp_realdev_to_vlandev()
>> and
>> > vsp_odp_realdev_to_vlandev(), which take OpenFlow port number and
>> datapth
>> > port number respectively.
>> >
>> > Signed-off-by: Alex Wang
>> > ---
>> > ofproto/ofproto-dpif.c | 61
>> +---
>> > 1 file changed, 42 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> > index 280fd57..193064b 100644
>> > --- a/ofproto/ofproto-dpif.c
>> > +++ b/ofproto/ofproto-dpif.c
>> > @@ -586,8 +586,12 @@ struct vlan_splinter {
>> > int vid;
>> > };
>> >
>> > -static uint32_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
>> > - uint32_t realdev, ovs_be16
>> vlan_tci);
>> > +static uint16_t vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *,
>> > + uint16_t realdev,
>> > + ovs_be16 vlan_tci);
>> > +static uint32_t vsp_odp_realdev_to_vlandev(const struct ofproto_dpif *,
>> > + uint32_t realdev,
>> > + ovs_be16 vlan_tci);
>> > static bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow
>> *);
>> > static void vsp_remove(struct ofport_dpif *);
>> > static void vsp_add(struct ofport_dpif *, uint16_t realdev_ofp_port,
>> int vid);
>> > @@ -6183,8 +6187,8 @@ compose_output_action__(struct action_xlate_ctx
>> *ctx, uint16_t ofp_port,
>> > ctx->flow.tunnel = flow_tnl; /* Restore tunnel metadata */
>> > } else {
>> > odp_port = ofport->odp_port;
>> > -out_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
>> > - ctx->flow.vlan_tci);
>> > +out_port = vsp_odp_realdev_to_vlandev(ctx->ofproto, odp_port,
>> > + ctx->flow.vlan_tci);
>> > if (out_port != odp_port) {
>> > ctx->flow.vlan_tci = htons(0);
>> > }
>> > @@ -8729,33 +8733,52 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int
>> vid)
>> > return hash_2words(realdev_ofp_port, vid);
>> > }
>> >
>> > -/* Returns the ODP port number of the Linux VLAN device that
>> corresponds to
>> > - * 'vlan_tci' on the network device with port number
>> 'realdev_odp_port' in
>> > - * 'ofproto'. For example, given 'realdev_odp_port' of eth0 and
>> 'vlan_tci' 9,
>> > - * it would return the port number of eth0.9.
>> > +/* Returns the OFP port number of the Linux VLAN device that
>> corresponds to
>> > + * 'vlan_tci' on the network device with port number
>> 'realdev_ofp_port' in
>> > + * 'ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
>> > + * 'vlan_tci' 9, it would return the port number of eth0.9.
>> > *
>> > - * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
>> > - * function just returns its 'realdev_odp_port' argument. */
>> > -static uint32_t
>> > -vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
>> > - uint32_t realdev_odp_port, ovs_be16 vlan_tci)
>> > + * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
>> > + * function just returns its 'realdev_ofp_port' argument. */
>> > +static uint16_t
>> > +vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
>> > + uint16_t realdev_ofp_port, ovs_be16
>> vlan_tci)
>> > {
>> > if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
>> > -uint16_t realdev_ofp_port;
>> > int vid = vlan_tci_to_vid(vlan_tci);
>> > const struct vlan_splinter *vsp;
>> >
>> > -realdev_ofp_port = odp_port_to_ofp_port(ofproto,
>> realdev_odp_port);
>> > HMAP_FOR_EACH_WITH_HASH (vsp, realdev_vid_node,
>> >
[ovs-dev] [PATCH] ofproto-dpif: Consolidate facet stat logic.
The logic for updating statistics at the facet level had been
spread through ofproto-dpif in a rather confusing manner. This
patch consolidates as much of this logic as is reasonable into
facet_push_stats().
On a side note, I'd expect this patch to have a marginal positive
performance impact when using learning (though I haven't bothered
to measure it). It combines facet_learn() and facet_push_stats()
into one step allowing us to avoid a redundant xlate_actions().
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif.c | 114 +---
1 file changed, 40 insertions(+), 74 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 386434d..40be3c8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -121,7 +121,6 @@ static struct rule_dpif *rule_dpif_miss_rule(struct
ofproto_dpif *ofproto,
static void rule_credit_stats(struct rule_dpif *,
const struct dpif_flow_stats *);
-static void flow_push_stats(struct facet *, const struct dpif_flow_stats *);
static tag_type rule_calculate_tag(const struct flow *,
const struct minimask *, uint32_t basis);
static void rule_invalidate(const struct rule_dpif *);
@@ -420,7 +419,6 @@ static void subfacet_destroy_batch(struct ofproto_dpif *,
struct subfacet **, int n);
static void subfacet_reset_dp_stats(struct subfacet *,
struct dpif_flow_stats *);
-static void subfacet_update_time(struct subfacet *, long long int used);
static void subfacet_update_stats(struct subfacet *,
const struct dpif_flow_stats *);
static int subfacet_install(struct subfacet *,
@@ -508,9 +506,8 @@ static bool facet_check_consistency(struct facet *);
static void facet_flush_stats(struct facet *);
-static void facet_update_time(struct facet *, long long int used);
static void facet_reset_counters(struct facet *);
-static void facet_push_stats(struct facet *);
+static void facet_push_stats(struct facet *, bool may_learn);
static void facet_learn(struct facet *);
static void facet_account(struct facet *);
static void push_all_stats(void);
@@ -4312,26 +4309,29 @@ update_subfacet_stats(struct subfacet *subfacet,
const struct dpif_flow_stats *stats)
{
struct facet *facet = subfacet->facet;
+struct dpif_flow_stats diff;
+
+diff.tcp_flags = stats->tcp_flags;
+diff.used = stats->used;
if (stats->n_packets >= subfacet->dp_packet_count) {
-uint64_t extra = stats->n_packets - subfacet->dp_packet_count;
-facet->packet_count += extra;
+diff.n_packets = stats->n_packets - subfacet->dp_packet_count;
} else {
VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
+diff.n_packets = 0;
}
if (stats->n_bytes >= subfacet->dp_byte_count) {
-facet->byte_count += stats->n_bytes - subfacet->dp_byte_count;
+diff.n_bytes = stats->n_bytes - subfacet->dp_byte_count;
} else {
VLOG_WARN_RL(&rl, "unexpected byte count from datapath");
+diff.n_bytes = 0;
}
subfacet->dp_packet_count = stats->n_packets;
subfacet->dp_byte_count = stats->n_bytes;
+subfacet_update_stats(subfacet, &diff);
-facet->tcp_flags |= stats->tcp_flags;
-
-subfacet_update_time(subfacet, stats->used);
if (facet->accounted_bytes < facet->byte_count) {
facet_learn(facet);
facet_account(facet);
@@ -4387,7 +4387,6 @@ update_stats(struct dpif_backer *backer)
while (dpif_flow_dump_next(&dump, &key, &key_len, NULL, NULL, &stats)) {
struct flow flow;
struct subfacet *subfacet;
-struct ofport_dpif *ofport;
uint32_t key_hash;
if (ofproto_receive(backer, NULL, key, key_len, &flow, NULL, &ofproto,
@@ -4398,11 +4397,6 @@ update_stats(struct dpif_backer *backer)
ofproto->total_subfacet_count += hmap_count(&ofproto->subfacets);
ofproto->n_update_stats++;
-ofport = get_ofp_port(ofproto, flow.in_port);
-if (ofport && ofport->tnl_port) {
-netdev_vport_inc_rx(ofport->up.netdev, stats);
-}
-
key_hash = odp_flow_key_hash(key, key_len);
subfacet = subfacet_find(ofproto, key, key_len, key_hash);
switch (subfacet ? subfacet->path : SF_NOT_INSTALLED) {
@@ -4713,9 +4707,7 @@ facet_remove(struct facet *facet)
static void
facet_learn(struct facet *facet)
{
-struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
long long int now = time_msec();
-struct xlate_in xin;
if (!facet->xout.has_fin_timeout && now < facet->learn_rl) {
return;
@@ -4730,10 +4722,7 @@ facet_learn(struct facet *facet)
return;
}
-xlate_in_init(&xin, ofproto, &facet->flow, &facet->initial_vals,
- facet->rule, facet->tcp_flags, NULL);
-
Re: [ovs-dev] [PATCH] ofproto-dpif: Consolidate facet stat logic.
Btw, this version is based on master, and is ready for review.
Thanks,
Ethan
On Wed, May 29, 2013 at 5:43 PM, Ethan Jackson wrote:
> The logic for updating statistics at the facet level had been
> spread through ofproto-dpif in a rather confusing manner. This
> patch consolidates as much of this logic as is reasonable into
> facet_push_stats().
>
> On a side note, I'd expect this patch to have a marginal positive
> performance impact when using learning (though I haven't bothered
> to measure it). It combines facet_learn() and facet_push_stats()
> into one step allowing us to avoid a redundant xlate_actions().
>
> Signed-off-by: Ethan Jackson
> ---
> ofproto/ofproto-dpif.c | 114
> +---
> 1 file changed, 40 insertions(+), 74 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 386434d..40be3c8 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -121,7 +121,6 @@ static struct rule_dpif *rule_dpif_miss_rule(struct
> ofproto_dpif *ofproto,
>
> static void rule_credit_stats(struct rule_dpif *,
>const struct dpif_flow_stats *);
> -static void flow_push_stats(struct facet *, const struct dpif_flow_stats *);
> static tag_type rule_calculate_tag(const struct flow *,
> const struct minimask *, uint32_t basis);
> static void rule_invalidate(const struct rule_dpif *);
> @@ -420,7 +419,6 @@ static void subfacet_destroy_batch(struct ofproto_dpif *,
> struct subfacet **, int n);
> static void subfacet_reset_dp_stats(struct subfacet *,
> struct dpif_flow_stats *);
> -static void subfacet_update_time(struct subfacet *, long long int used);
> static void subfacet_update_stats(struct subfacet *,
>const struct dpif_flow_stats *);
> static int subfacet_install(struct subfacet *,
> @@ -508,9 +506,8 @@ static bool facet_check_consistency(struct facet *);
>
> static void facet_flush_stats(struct facet *);
>
> -static void facet_update_time(struct facet *, long long int used);
> static void facet_reset_counters(struct facet *);
> -static void facet_push_stats(struct facet *);
> +static void facet_push_stats(struct facet *, bool may_learn);
> static void facet_learn(struct facet *);
> static void facet_account(struct facet *);
> static void push_all_stats(void);
> @@ -4312,26 +4309,29 @@ update_subfacet_stats(struct subfacet *subfacet,
>const struct dpif_flow_stats *stats)
> {
> struct facet *facet = subfacet->facet;
> +struct dpif_flow_stats diff;
> +
> +diff.tcp_flags = stats->tcp_flags;
> +diff.used = stats->used;
>
> if (stats->n_packets >= subfacet->dp_packet_count) {
> -uint64_t extra = stats->n_packets - subfacet->dp_packet_count;
> -facet->packet_count += extra;
> +diff.n_packets = stats->n_packets - subfacet->dp_packet_count;
> } else {
> VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
> +diff.n_packets = 0;
> }
>
> if (stats->n_bytes >= subfacet->dp_byte_count) {
> -facet->byte_count += stats->n_bytes - subfacet->dp_byte_count;
> +diff.n_bytes = stats->n_bytes - subfacet->dp_byte_count;
> } else {
> VLOG_WARN_RL(&rl, "unexpected byte count from datapath");
> +diff.n_bytes = 0;
> }
>
> subfacet->dp_packet_count = stats->n_packets;
> subfacet->dp_byte_count = stats->n_bytes;
> +subfacet_update_stats(subfacet, &diff);
>
> -facet->tcp_flags |= stats->tcp_flags;
> -
> -subfacet_update_time(subfacet, stats->used);
> if (facet->accounted_bytes < facet->byte_count) {
> facet_learn(facet);
> facet_account(facet);
> @@ -4387,7 +4387,6 @@ update_stats(struct dpif_backer *backer)
> while (dpif_flow_dump_next(&dump, &key, &key_len, NULL, NULL, &stats)) {
> struct flow flow;
> struct subfacet *subfacet;
> -struct ofport_dpif *ofport;
> uint32_t key_hash;
>
> if (ofproto_receive(backer, NULL, key, key_len, &flow, NULL,
> &ofproto,
> @@ -4398,11 +4397,6 @@ update_stats(struct dpif_backer *backer)
> ofproto->total_subfacet_count += hmap_count(&ofproto->subfacets);
> ofproto->n_update_stats++;
>
> -ofport = get_ofp_port(ofproto, flow.in_port);
> -if (ofport && ofport->tnl_port) {
> -netdev_vport_inc_rx(ofport->up.netdev, stats);
> -}
> -
> key_hash = odp_flow_key_hash(key, key_len);
> subfacet = subfacet_find(ofproto, key, key_len, key_hash);
> switch (subfacet ? subfacet->path : SF_NOT_INSTALLED) {
> @@ -4713,9 +4707,7 @@ facet_remove(struct facet *facet)
> static void
> facet_learn(struct facet *facet)
> {
> -struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> long long int
Re: [ovs-dev] [PATCH] ofproto-dpif.c: Replace vsp_realdev_to_vlandev() with two new functions
> I'm not sure how likely the function "vsp_realdev_to_vlandev()" will be
> called with odp_port as argument in the future. I'll go with the "two
> functions" implementation first.
FWIW I suspect this is unlikely, and even if it wasn't. We can always
change it in future when we have more information about what's really
necessary.
Ethan
> Hope to hear more comments from you.
>
> Thanks
>
>
>
> On Wed, May 29, 2013 at 3:25 PM, Ethan Jackson wrote:
>>
>> I haven't looked at this patch carefully, so my next comment may or
>> may not be worthwhile, but I'm wondering if it would be cleaner if we
>> kept a single vsp_realdev_to_vlandev, which took an ofp_port as an
>> argument, and then had compose_output_action__ pass in the openflow
>> port number instead of the datapath port number to that function?
>>
>> Ethan
>>
>> On Wed, May 29, 2013 at 5:01 PM, Alex Wang wrote:
>> > In ofproto-dpif.c, function vsp_realdev_to_vlandev() is called with both
>> > OpenFlow port and datapath port number as argument. This patch replaces
>> > the function vsp_realdev_to_vlandev() with vsp_ofp_realdev_to_vlandev()
>> > and
>> > vsp_odp_realdev_to_vlandev(), which take OpenFlow port number and
>> > datapth
>> > port number respectively.
>> >
>> > Signed-off-by: Alex Wang
>> > ---
>> > ofproto/ofproto-dpif.c | 61
>> > +---
>> > 1 file changed, 42 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> > index 280fd57..193064b 100644
>> > --- a/ofproto/ofproto-dpif.c
>> > +++ b/ofproto/ofproto-dpif.c
>> > @@ -586,8 +586,12 @@ struct vlan_splinter {
>> > int vid;
>> > };
>> >
>> > -static uint32_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
>> > - uint32_t realdev, ovs_be16
>> > vlan_tci);
>> > +static uint16_t vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *,
>> > + uint16_t realdev,
>> > + ovs_be16 vlan_tci);
>> > +static uint32_t vsp_odp_realdev_to_vlandev(const struct ofproto_dpif *,
>> > + uint32_t realdev,
>> > + ovs_be16 vlan_tci);
>> > static bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow
>> > *);
>> > static void vsp_remove(struct ofport_dpif *);
>> > static void vsp_add(struct ofport_dpif *, uint16_t realdev_ofp_port,
>> > int vid);
>> > @@ -6183,8 +6187,8 @@ compose_output_action__(struct action_xlate_ctx
>> > *ctx, uint16_t ofp_port,
>> > ctx->flow.tunnel = flow_tnl; /* Restore tunnel metadata */
>> > } else {
>> > odp_port = ofport->odp_port;
>> > -out_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
>> > - ctx->flow.vlan_tci);
>> > +out_port = vsp_odp_realdev_to_vlandev(ctx->ofproto, odp_port,
>> > + ctx->flow.vlan_tci);
>> > if (out_port != odp_port) {
>> > ctx->flow.vlan_tci = htons(0);
>> > }
>> > @@ -8729,33 +8733,52 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int
>> > vid)
>> > return hash_2words(realdev_ofp_port, vid);
>> > }
>> >
>> > -/* Returns the ODP port number of the Linux VLAN device that
>> > corresponds to
>> > - * 'vlan_tci' on the network device with port number 'realdev_odp_port'
>> > in
>> > - * 'ofproto'. For example, given 'realdev_odp_port' of eth0 and
>> > 'vlan_tci' 9,
>> > - * it would return the port number of eth0.9.
>> > +/* Returns the OFP port number of the Linux VLAN device that
>> > corresponds to
>> > + * 'vlan_tci' on the network device with port number 'realdev_ofp_port'
>> > in
>> > + * 'ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
>> > + * 'vlan_tci' 9, it would return the port number of eth0.9.
>> > *
>> > - * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
>> > - * function just returns its 'realdev_odp_port' argument. */
>> > -static uint32_t
>> > -vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
>> > - uint32_t realdev_odp_port, ovs_be16 vlan_tci)
>> > + * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
>> > + * function just returns its 'realdev_ofp_port' argument. */
>> > +static uint16_t
>> > +vsp_ofp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
>> > + uint16_t realdev_ofp_port, ovs_be16
>> > vlan_tci)
>> > {
>> > if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
>> > -uint16_t realdev_ofp_port;
>> > int vid = vlan_tci_to_vid(vlan_tci);
>> > const struct vlan_splinter *vsp;
>> >
>> > -realdev_ofp_port = odp_port_to_ofp_port(ofproto,
>> > realdev_odp_port);
>> > HMAP_FOR_EACH_WITH_HASH (vsp, realdev_vid_node,
>> > hash_realdev_vid(realdev_ofp_port,
>> > vid),
Re: [ovs-dev] [PATCH net-next v5 1/2] net: Use 16bits for *_headers fields of struct skbuff
On Wed, May 29, 2013 at 02:46:05PM -0700, Olof Johansson wrote:
> Sorry, that was not a great bug report.
>
> So, looks like it's the removal of NET_SKBUFF_DATA_USES_OFFSET that
> does it for me.
>
> The devices I've seen it with are with asix usb-ethernet adapters (on
> Tegra seaboard) and with mv643xx_eth on cubox (dove).
>
> I'll try to get a better stacktrace out of it. Simon, maybe you can
> reproduce on shmobile hardware?
Sure, I will try and reproduce it.
I did subsequently post some fixes for fallout resulting from
the patch listed below. Those changes are now net-next.
Are you seeing the crash with those patches?
The fix-up patches are:
net,ipv4,ipv6: Correct assignment of skb->network_header to skb->tail
sctp: Correct access to skb->{network,transport}_header
ipv4: Correct comparisons and calculations using skb->tail and
skb-transport_header
ipv6: Correct comparisons and calculations using skb->tail and
skb-transport_header
net: Correct comparisons and calculations using skb->tail and
skb-transport_header
cxgb3: Correct comparisons and calculations using skb->tail and
skb-transport_header
isdn: Correct comparison of skb->tail and skb-transport_header
net: Fix build warnings after mac_header and transport_header became __u16.
>
>
>
> On Wed, May 29, 2013 at 2:21 PM, Olof Johansson wrote:
> > Simon,
> >
> > On Thu, May 23, 2013 at 11:51 PM, Simon Horman wrote:
> >> In order to mitigate ongoing incresase in the size of struct skbuff
> >> use 16 bit integer offsets rather than pointers for inner_*_headers.
> >>
> >> This appears to reduce the size of struct skbuff from 0xd0 to 0xc0
> >> bytes on x86_64 with the following all unset.
> >>
> >> CONFIG_XFRM
> >> CONFIG_NF_CONNTRACK
> >> CONFIG_NF_CONNTRACK_MODULE
> >> NET_SKBUFF_NF_DEFRAG_NEEDED
> >> CONFIG_BRIDGE_NETFILTER
> >> CONFIG_NET_SCHED
> >> CONFIG_IPV6_NDISC_NODETYPE
> >> CONFIG_NET_DMA
> >> CONFIG_NETWORK_SECMARK
> >>
> >> Signed-off-by: Simon Horman
> >
> > I'm getting crashes in csum_partial() on several ARM platforms that I
> > bisected down to this patch (and reverting this + "MPLS:
> > Add limited GSO support" due to conflicts) results in a working kernel.
> >
> > The failures started with the 0529 linux-next, didn't exist in 0528.
> >
> > Unfortunately I'm not getting a useful stack from the crashes:
> >
> > [6.495560] Unable to handle kernel paging request at virtual
> > address eb00
> > [6.502769] pgd = e9084000
> > [6.505465] [eb00] *pgd=
> > [6.509034] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> > [6.514329] Modules linked in:
> > [6.517378] CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> > 3.10.0-rc2-00483-g1a37e41 #27
> > [6.525015] task: c06e1980 ti: c06d6000 task.ti: c06d6000
> > [6.530402] PC is at csum_partial+0x40/0x130
> > [6.534658] LR is at 0x0
> > [6.537181] pc : []lr : [<>]psr: 000f0113
> > [6.537181] sp : c06d7d88 ip : e700c060 fp : c06d80c0
> > [6.548631] r10: e90e2140 r9 : e9006050 r8 : 0001
> > [6.553839] r7 : e9006020 r6 : e9b7e400 r5 : r4 :
> > [6.560348] r3 : r2 : 73129f9e r1 : e900601c r0 : eb00
> > [6.566857] Flags: nzcv IRQs on FIQs on Mode SVC_32 ISA ARM
> > Segment kernel
> > [6.574146] Control: 10c5387d Table: 2908404a DAC: 0015
> > [6.579874] Process swapper/0 (pid: 0, stack limit = 0xc06d6238)
> > [6.585862] Stack: (0xc06d7d88 to 0xc06d8000)
> > [6.590207] 7d80: c0714fc0 e900601c e9006050
> > c043a0b8 e92737b0
> > [6.598365] 7da0: e9273740 c043c1d4 c0010208 c0f83cc0 c06d4cc0
> > c0f83cc0 e9b32000
> > [6.606523] 7dc0: c06d4cc0 0001 c06d80c0 e9273758 00989680
> > 0001 e9b7aec0
> > [6.614680] 7de0: e90e2140 e9b7af30 e9b7e400 e9b7e490
> > 0001 c043c5d4
> > [6.622838] 7e00: 0001 7cdfa980 0001 e9b7e458 c06d6000
> > 0101 c06d6030 c043c414
> > [6.630995] 7e20: 00200200 c06d80c0 c002fbe8 0002
> > 7c4716e8 e9b7e458 c073fb80
> > [6.639153] 7e40: c043c414 e9b7e400 c06d7e68 c002fde8
> > c0f82678 c0740394
> > [6.647310] 7e60: c0740594 c0740794 c06d7e68 c06d7e68 0006
> > 0001 0004 c06d8088
> > [6.655468] 7e80: c06d8080 c06d6000 0001 0004 0101
> > c002a57c 7c4716e8 0001
> > [6.663625] 7ea0: c06d37e0 c073f940 c06d80c0 8d4f
> > c06e4970 0020 c06d6018
> > [6.671783] 7ec0: c02025e8 c06d6028 001d fe000100
> > c06d6000
> > [6.679940] 7ee0: c06de42c c002a9a4 c06d3fac c000eaa0 fe00010c
> > c06df074 c06d7f18 c00086f8
> > [6.688098] 7f00: c0061288 c032b560 600f0013 c06d7f4c
> > c000de60 c06d7f60 0006
> > [6.696255] 7f20: 7c471300 0001 7c3bbca8 0001 c0f83110
> > c06e3cd4
> > [6.704413] 7f40: c06d6000 c
Re: [ovs-dev] [PATCH net-next v5 1/2] net: Use 16bits for *_headers fields of struct skbuff
Hi,
On Wed, May 29, 2013 at 3:54 PM, Simon Horman wrote:
> On Wed, May 29, 2013 at 02:46:05PM -0700, Olof Johansson wrote:
>> Sorry, that was not a great bug report.
>>
>> So, looks like it's the removal of NET_SKBUFF_DATA_USES_OFFSET that
>> does it for me.
>>
>> The devices I've seen it with are with asix usb-ethernet adapters (on
>> Tegra seaboard) and with mv643xx_eth on cubox (dove).
>>
>> I'll try to get a better stacktrace out of it. Simon, maybe you can
>> reproduce on shmobile hardware?
>
> Sure, I will try and reproduce it.
>
> I did subsequently post some fixes for fallout resulting from
> the patch listed below. Those changes are now net-next.
> Are you seeing the crash with those patches?
>
> The fix-up patches are:
>
> net,ipv4,ipv6: Correct assignment of skb->network_header to skb->tail
> sctp: Correct access to skb->{network,transport}_header
> ipv4: Correct comparisons and calculations using skb->tail and
> skb-transport_header
> ipv6: Correct comparisons and calculations using skb->tail and
> skb-transport_header
> net: Correct comparisons and calculations using skb->tail and
> skb-transport_header
> cxgb3: Correct comparisons and calculations using skb->tail and
> skb-transport_header
> isdn: Correct comparison of skb->tail and skb-transport_header
> net: Fix build warnings after mac_header and transport_header became __u16.
Ah, no, looks like those didn't make it into the 0529 linux-next.
I've merged net-next on top and retested, and I no longer see the panics.
Thanks!
-Olof
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: Remove useless odp_in_port from subfacet.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 40be3c8..580044e 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -397,13 +397,6 @@ struct subfacet {
uint64_t dp_byte_count; /* Last known byte count in the datapath. */
enum subfacet_path path;/* Installed in datapath? */
-
-/* Datapath port the packet arrived on. This is needed to remove
- * flows for ports that are no longer part of the bridge. Since the
- * flow definition only has the OpenFlow port number and the port is
- * no longer part of the bridge, we can't determine the datapath port
- * number needed to delete the flow from the datapath. */
-uint32_t odp_in_port;
};
#define SUBFACET_DESTROY_MAX_BATCH 50
@@ -3490,7 +3483,6 @@ struct flow_miss {
struct initial_vals initial_vals;
struct list packets;
enum dpif_upcall_type upcall_type;
-uint32_t odp_in_port;
};
struct flow_miss_op {
@@ -4022,7 +4014,6 @@ handle_miss_upcalls(struct dpif_backer *backer, struct
dpif_upcall *upcalls,
miss->key = upcall->key;
miss->key_len = upcall->key_len;
miss->upcall_type = upcall->type;
-miss->odp_in_port = odp_in_port;
list_init(&miss->packets);
n_misses++;
@@ -5208,7 +5199,6 @@ subfacet_create(struct facet *facet, struct flow_miss
*miss,
subfacet->dp_packet_count = 0;
subfacet->dp_byte_count = 0;
subfacet->path = SF_NOT_INSTALLED;
-subfacet->odp_in_port = miss->odp_in_port;
ofproto->subfacet_add_count++;
return subfacet;
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Don't count misses in OpenFlow table stats.
On Wed, May 29, 2013 at 6:20 AM, Ben Pfaff wrote: > On Fri, May 24, 2013 at 05:01:34PM -0700, Jesse Gross wrote: >> Originally no rule existed for packets that did not match an >> OpenFlow flow and therefore every packet with a rule could be >> counted as a hit. However, newer versions of OVS have hidden >> miss rules so this is no longer true. To return the correct >> table stats, this subtracts packets that hit the miss rule >> from the total and removes the separate counter. >> >> Reported-by: love you >> Signed-off-by: Jesse Gross > > Looks good, thanks. Thanks, applied. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: Fix facet_lookup_valid().
facet_lookup_valid() attempted to re-revalidate its facet after
it had been removed.
Found by inspection (by Justin).
Reported-by: Justin Pettit
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif.c |5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 386434d..8448e1b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4883,10 +4883,7 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const
struct flow *flow,
|| tag_set_intersects(&ofproto->backer->revalidate_set,
facet->xout.tags))
&& !facet_revalidate(facet)) {
-facet_revalidate(facet);
-
-/* facet_revalidate() may have destroyed 'facet'. */
-facet = facet_find(ofproto, flow, hash);
+return NULL;
}
return facet;
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif.c: Modify vsp_realdev_to_vlandev() function
This patch modifies the vsp_realdev_to_vlandev() function. Instead of taking
and returning datapath port number, the new implementation takes and returns
OpenFlow port number.
Signed-off-by: Alex Wang
---
ofproto/ofproto-dpif.c | 34 +-
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 280fd57..0c865cb 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -586,8 +586,9 @@ struct vlan_splinter {
int vid;
};
-static uint32_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
- uint32_t realdev, ovs_be16 vlan_tci);
+static uint16_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
+ uint16_t realdev_ofp_port,
+ ovs_be16 vlan_tci);
static bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *);
static void vsp_remove(struct ofport_dpif *);
static void vsp_add(struct ofport_dpif *, uint16_t realdev_ofp_port, int vid);
@@ -6183,9 +6184,10 @@ compose_output_action__(struct action_xlate_ctx *ctx,
uint16_t ofp_port,
ctx->flow.tunnel = flow_tnl; /* Restore tunnel metadata */
} else {
odp_port = ofport->odp_port;
-out_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
- ctx->flow.vlan_tci);
-if (out_port != odp_port) {
+out_port = ofp_port_to_odp_port(ctx->ofproto,
+ vsp_realdev_to_vlandev(ctx->ofproto, ofp_port,
+ ctx->flow.vlan_tci));
+ if (out_port != odp_port) {
ctx->flow.vlan_tci = htons(0);
}
ctx->flow.skb_mark &= ~IPSEC_MARK;
@@ -8729,33 +8731,31 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int vid)
return hash_2words(realdev_ofp_port, vid);
}
-/* Returns the ODP port number of the Linux VLAN device that corresponds to
- * 'vlan_tci' on the network device with port number 'realdev_odp_port' in
- * 'ofproto'. For example, given 'realdev_odp_port' of eth0 and 'vlan_tci' 9,
- * it would return the port number of eth0.9.
+/* Returns the OFP port number of the Linux VLAN device that corresponds to
+ * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
+ * 'struct ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
+ * 'vlan_tci' 9, it would return the port number of eth0.9.
*
- * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
- * function just returns its 'realdev_odp_port' argument. */
-static uint32_t
+ * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
+ * function just returns its 'realdev_ofp_port' argument. */
+static uint16_t
vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
- uint32_t realdev_odp_port, ovs_be16 vlan_tci)
+ uint16_t realdev_ofp_port, ovs_be16 vlan_tci)
{
if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
-uint16_t realdev_ofp_port;
int vid = vlan_tci_to_vid(vlan_tci);
const struct vlan_splinter *vsp;
-realdev_ofp_port = odp_port_to_ofp_port(ofproto, realdev_odp_port);
HMAP_FOR_EACH_WITH_HASH (vsp, realdev_vid_node,
hash_realdev_vid(realdev_ofp_port, vid),
&ofproto->realdev_vid_map) {
if (vsp->realdev_ofp_port == realdev_ofp_port
&& vsp->vid == vid) {
-return ofp_port_to_odp_port(ofproto, vsp->vlandev_ofp_port);
+return vsp->vlandev_ofp_port;
}
}
}
-return realdev_odp_port;
+return realdev_ofp_port;
}
static struct vlan_splinter *
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/3] ovs-vswitchd: An option to wait for userspace flow restore to complete.
On Tue, May 28, 2013 at 02:02:17PM +, Gurucharan Shetty wrote:
> While upgrading openvswitch, it helps to restore openflow flows before
> starting packet processing. Typically we want to restart openvswitch,
> add the openflow flows and then start packet processing.
>
> To do this, we look for the other_config:flow-restore-wait column
> in the Open_vSwitch table during startup. If set as true, we disable
> receiving packets from the datapath, expiring or flushing flows and
> running any periodic ofproto activities. This option does not prevent
> the addition and deletion of ports. Once this option is set to false,
> we return to normal processing.
>
> An upcoming commit will use this feature in Open vSwitch startup scripts.
>
> Bug #16086.
> Signed-off-by: Gurucharan Shetty
The dependency here is the opposite of what we usually do. That is,
ordinarily, nothing in ofproto depends on anything in bridge; rather,
bridge is a client of the ofproto library. It would be best to maintain
that client relationship here. One natural way would be for ofproto to
export a function to set this new flow-restore-wait value, instead of
bridge to export a variable. Another alternative would be to make
ofproto export the variable instead of bridge, but hiding a global
behind a function may be cleaner.
"backer->recv_set_enable = !flow_restore_wait;" is simpler than this:
> +if (flow_restore_wait) {
> +backer->recv_set_enable = false;
> +} else {
> +backer->recv_set_enable = true;
> +}
The description in vswitch.xml is accurate, but I think that users would
find it easier to understand if it stepped back a little and explained
the larger issue. How about this, instead:
When ovs-vswitchd starts up, it has an empty flow table
and therefore it handles all arriving packets in its default fashion
according to its configuration, by dropping them or sending them to
an OpenFlow controller or switching them as a standalone switch.
This behavior is ordinarily desirable. However, if
ovs-vswitchd is restarting as part of a
``hot-upgrade,'' then this leads to a relatively long period during
which packets are mishandled.
This option allows for improvement. When ovs-vswitchd
starts with this value set as true, it will neither
flush or expire previously set datapath flows nor will it send and
receive any packets to or from the datapath. When this value is
later set to false, ovs-vswitchd will
start receiving packets from the datapath and re-setup the flows.
Thus, with this option, the procedure for a hot-upgrade of
ovs-vswitchd becomes roughly the following:
Stop ovs-vswitchd.
Set
to true.
Start ovs-vswitchd.
Use ovs-ofctl (or some other program, such as an
OpenFlow controller) to restore the OpenFlow flow table
to the desired state.
Set
to false (or remove it entirely from the database).
In the later patch where it is true, it would be nice to also note that
restart does all of the above.
It might also be nice to mention this in the top-level INSTALL, or at
least provide a pointer.
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif.c: Modify vsp_realdev_to_vlandev() function
On Wed, May 29, 2013 at 04:57:11PM -0700, Alex Wang wrote:
> This patch modifies the vsp_realdev_to_vlandev() function. Instead of taking
> and returning datapath port number, the new implementation takes and returns
> OpenFlow port number.
>
> Signed-off-by: Alex Wang
This needs a re-spin against current master, which has changed slightly
due to the refactoring of xlate_action_ctx.
I would prefer to avoid the ofp_port_to_odp_port() call in the common
case. How about this:
uint16_t vlandev_port;
odp_port = ofport->odp_port;
vlandev_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
ctx->xin->flow.vlan_tci);
if (vlandev_port == ofport->up.ofp_port) {
out_port = odp_port;
} else {
out_port = ofp_port_to_odp_port(vlandev_port);
ctx->xin->flow.vlan_tci = htons(0);
}
ctx->xin->flow.skb_mark &= ~IPSEC_MARK;
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [xlate 1.11 9/9] ofproto-dpif: Revamp xlate_actions() interface.
On Wed, May 29, 2013 at 02:24:41PM -0700, Ethan Jackson wrote: > This patch implements a new interface to xlate_actions which, aside > from being simpler and more intuitive, achieves several goals. It > pulls all of xlate_actions() results into a single structure which > will be easier to cache and pass around in future. And it shields > xlate_ctx from the rest of the code, making it possible to factor > xlate_actions() in the future (hopefully). > > Signed-off-by: Ethan Jackson Before I really review this, in patch 0/9 of this series, you said: I'd a appreciate a review of xlate_in_init(), xlate_out_uninit(), and xlate_actions() in the final patch of the series. Specifically focusing on how the tunnel_ip_tos field of initial_vals is handled. Everything else backported trivially. Once that's reviewed, assuming there are no objections, I'll push this. but the handling of tunnel_ip_tos looks really simple and straightforward. As far as I can see, it just gets copied around. Is there something tricky here? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif.c: Modify vsp_realdev_to_vlandev() function
Thanks Ben for the suggestion, I'll adjust accordingly.
On Wed, May 29, 2013 at 4:58 PM, Ben Pfaff wrote:
> On Wed, May 29, 2013 at 04:57:11PM -0700, Alex Wang wrote:
> > This patch modifies the vsp_realdev_to_vlandev() function. Instead of
> taking
> > and returning datapath port number, the new implementation takes and
> returns
> > OpenFlow port number.
> >
> > Signed-off-by: Alex Wang
>
> This needs a re-spin against current master, which has changed slightly
> due to the refactoring of xlate_action_ctx.
>
> I would prefer to avoid the ofp_port_to_odp_port() call in the common
> case. How about this:
>
> uint16_t vlandev_port;
>
> odp_port = ofport->odp_port;
> vlandev_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
> ctx->xin->flow.vlan_tci);
> if (vlandev_port == ofport->up.ofp_port) {
> out_port = odp_port;
> } else {
> out_port = ofp_port_to_odp_port(vlandev_port);
> ctx->xin->flow.vlan_tci = htons(0);
> }
> ctx->xin->flow.skb_mark &= ~IPSEC_MARK;
>
> Thanks,
>
> Ben.
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Bug#710125: Bug#710125: openvswitch: unable to compile kernel module with linux-3.8
On Tue, May 28, 2013 at 10:56 PM, Maciej Galkiewicz wrote: > Package: openvswitch > Version: 1.4.2+git20120612-9 > Severity: important > > # dkms install -m openvswitch -v 1.4.2+git20120612 -k 3.8-2-amd64 Open vSwitch 1.4 was released almost a year before Linux 3.8 so it is not surprising that this kernel version is not supported. The current LTS version of Open vSwitch is 1.9, which does support Linux 3.8. I believe the current plan is upload that version to Debian, which is the best way to address this. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH V2] ofproto-dpif.c: Modify vsp_realdev_to_vlandev() function
This patch modifies the vsp_realdev_to_vlandev() function. Instead of taking
and returning datapath port number, the new implementation takes and returns
OpenFlow port number.
Signed-off-by: Alex Wang
---
ofproto/ofproto-dpif.c | 38 +-
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 280fd57..b4308aa 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -586,8 +586,9 @@ struct vlan_splinter {
int vid;
};
-static uint32_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
- uint32_t realdev, ovs_be16 vlan_tci);
+static uint16_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
+ uint16_t realdev_ofp_port,
+ ovs_be16 vlan_tci);
static bool vsp_adjust_flow(const struct ofproto_dpif *, struct flow *);
static void vsp_remove(struct ofport_dpif *);
static void vsp_add(struct ofport_dpif *, uint16_t realdev_ofp_port, int vid);
@@ -6182,10 +6183,15 @@ compose_output_action__(struct action_xlate_ctx *ctx,
uint16_t ofp_port,
ctx->odp_actions);
ctx->flow.tunnel = flow_tnl; /* Restore tunnel metadata */
} else {
+uint16_t vlandev_port;
+vlandev_port = vsp_realdev_to_vlandev(ctx->ofproto, ofp_port,
+ ctx->flow.vlan_tci);
odp_port = ofport->odp_port;
-out_port = vsp_realdev_to_vlandev(ctx->ofproto, odp_port,
- ctx->flow.vlan_tci);
-if (out_port != odp_port) {
+
+if (vlandev_port == ofp_port) {
+out_port = odp_port;
+} else {
+out_port = ofp_port_to_odp_port(ctx->ofproto, vlandev_port);
ctx->flow.vlan_tci = htons(0);
}
ctx->flow.skb_mark &= ~IPSEC_MARK;
@@ -8729,33 +8735,31 @@ hash_realdev_vid(uint16_t realdev_ofp_port, int vid)
return hash_2words(realdev_ofp_port, vid);
}
-/* Returns the ODP port number of the Linux VLAN device that corresponds to
- * 'vlan_tci' on the network device with port number 'realdev_odp_port' in
- * 'ofproto'. For example, given 'realdev_odp_port' of eth0 and 'vlan_tci' 9,
- * it would return the port number of eth0.9.
+/* Returns the OFP port number of the Linux VLAN device that corresponds to
+ * 'vlan_tci' on the network device with port number 'realdev_ofp_port' in
+ * 'struct ofport_dpif'. For example, given 'realdev_ofp_port' of eth0 and
+ * 'vlan_tci' 9, it would return the port number of eth0.9.
*
- * Unless VLAN splinters are enabled for port 'realdev_odp_port', this
- * function just returns its 'realdev_odp_port' argument. */
-static uint32_t
+ * Unless VLAN splinters are enabled for port 'realdev_ofp_port', this
+ * function just returns its 'realdev_ofp_port' argument. */
+static uint16_t
vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
- uint32_t realdev_odp_port, ovs_be16 vlan_tci)
+ uint16_t realdev_ofp_port, ovs_be16 vlan_tci)
{
if (!hmap_is_empty(&ofproto->realdev_vid_map)) {
-uint16_t realdev_ofp_port;
int vid = vlan_tci_to_vid(vlan_tci);
const struct vlan_splinter *vsp;
-realdev_ofp_port = odp_port_to_ofp_port(ofproto, realdev_odp_port);
HMAP_FOR_EACH_WITH_HASH (vsp, realdev_vid_node,
hash_realdev_vid(realdev_ofp_port, vid),
&ofproto->realdev_vid_map) {
if (vsp->realdev_ofp_port == realdev_ofp_port
&& vsp->vid == vid) {
-return ofp_port_to_odp_port(ofproto, vsp->vlandev_ofp_port);
+return vsp->vlandev_ofp_port;
}
}
}
-return realdev_odp_port;
+return realdev_ofp_port;
}
static struct vlan_splinter *
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2 1/3] ovs-vswitchd: An option to wait for userspace > flow restore to complete. (Gurucharan Shetty)
Date: Wed, 29 May 2013 13:46:05 -0700
Subject: Re: [ovs-dev] [PATCH v2 1/3] ovs-vswitchd: An option to wait for
userspace > flow restore to complete. (Gurucharan Shetty)
From: shet...@nicira.com
To: ai_jing2...@hotmail.com
CC: dev@openvswitch.org
On Wed, May 29, 2013 at 1:23 PM, Jing Ai wrote:
> While upgrading openvswitch, it helps to restore openflow flows before
> starting packet processing. Typically we want to restart openvswitch,
> add the openflow flows and then start packet processing.
>
> To do this, we look for the other_config:flow-restore-wait column
> in the Open_vSwitch table during startup. If set as true, we disable
> receiving packets from the datapath, expiring or flushing flows and
> running any periodic ofproto activities. This option does not prevent
> the addition and deletion of ports. Once this option is set to false,
> we return to normal processing.
>
> An upcoming commit will use this feature in Open vSwitch startup scripts.
Will this patch leave ongoing traffic not disturbed while upgrading OVS?Yes.
That is one intent for just a "restart --save-flows=yes". The other intent is
to not setup new flows till the userspace flows are restored. For
force-reload-kmod (when kernel module is replaced), the intent is not to setup
new flows without having all the userspace flows replaced.
I am asking since I notice that the existing ports, which keep packet flowing,
in the fast datapath (i.e., kernel) will be garbage collected in
open_dpif_backer.
I don't think that is true. The goal is to keep the datapath's ports to be the
same as seen in OVSDB. A OVS upgrade can result in either just a "restart", in
which case only userspace is restarted or a 'force-reload-kmod', in which case
the kernel module is replaced too (in this case there will be traffic
disruption for a few seconds). In both cases the expectation is that the kernel
will have the ports for the interfaces in OVSDB. If not, we will add it.
Yes, I took another look of the codes and you are right, existing ports will be
preserved.
One more question, there are two flavors of OVS restart by sending either TERM
signal or KILL signal (refer to stop_daemon() in ovs-lib). For the TERM signal
case, it seems would trigger a graceful shutdown of ovs-vswitchd and thereby
all kernel flows and vports would be deleted. If it is the case, existing
traffic would be impacted.
Tahnks,Guru
Best,Jing
>
> Bug #16086.
> Signed-off-by: Gurucharan Shetty
> ---
> ofproto/ofproto-dpif.c | 55
> +---
> vswitchd/bridge.c | 11 ++
> vswitchd/vswitch.xml | 17 +++
> 3 files changed, 80 insertions(+), 3 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index c4f7d25..31f04f3 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -72,6 +72,8 @@ COVERAGE_DEFINE(facet_suppress);
> * flow translation. */
> #define MAX_RESUBMIT_RECURSION 64
>
> +extern bool flow_restore_wait;
> +
> /* Number of implemented OpenFlow tables. */
> enum { N_TABLES = 255 };
> enum { TBL_INTERNAL = N_TABLES - 1 };/* Used for internal hidden rules.
> */
> @@ -671,6 +673,7 @@ struct dpif_backer {
> struct tag_set revalidate_set; /* Revalidate only matching facets. */
>
> struct hmap drop_keys; /* Set of dropped odp keys. */
> +bool recv_set_enable; /* Enables or disables receiving packets. */
> };
>
> /* All existing ofproto_backer instances, indexed by ofproto->up.type. */
> @@ -947,6 +950,21 @@ type_run(const char *type)
> push_all_stats();
> }
>
> +/* If vswitchd started with other_config:flow_restore_wait set as "true",
> + * and the configuration has now changed to "false", enable receiving
> + * packets from the datapath. */
> +if (!backer->recv_set_enable && !flow_restore_wait) {
> +backer->recv_set_enable = true;
> +
> +error = dpif_recv_set(backer->dpif, backer->recv_set_enable);
> +if (error) {
> +VLOG_ERR("Failed to enable receiving packets in dpif.");
> +return error;
> +}
> +dpif_flow_flush(backer->dpif);
> +backer->need_revalidate = REV_RECONFIGURE;
> +}
> +
> if (backer->need_revalidate
> || !tag_set_is_empty(&backer->revalidate_set)) {
> struct tag_set revalidate_set = backer->revalidate_set;
> @@ -1040,7 +1058,10 @@ type_run(const char *type)
> }
> }
>
> -if (timer_expired(&backer->next_expiration)) {
> +if (!backer->recv_set_enable) {
> +/* A minimum of 1000 ms delay. */
> +timer_set_duration(&backer->next_expiration, 1000);
> +} else if (timer_expired(&backer->next_expiration)) {
> int delay = expire(backer);
> timer_set_duration(&backer->next_expiration, delay);
> }
> @@ -1106,6 +1127,11 @@ dpif_backer_run_fast(struct dpif_backer *backer, int
> max_batch)
> {
>
Re: [ovs-dev] [PATCH net-next v5 1/2] net: Use 16bits for *_headers fields of struct skbuff
On Wed, May 29, 2013 at 04:02:12PM -0700, Olof Johansson wrote:
> Hi,
>
> On Wed, May 29, 2013 at 3:54 PM, Simon Horman wrote:
> > On Wed, May 29, 2013 at 02:46:05PM -0700, Olof Johansson wrote:
> >> Sorry, that was not a great bug report.
> >>
> >> So, looks like it's the removal of NET_SKBUFF_DATA_USES_OFFSET that
> >> does it for me.
> >>
> >> The devices I've seen it with are with asix usb-ethernet adapters (on
> >> Tegra seaboard) and with mv643xx_eth on cubox (dove).
> >>
> >> I'll try to get a better stacktrace out of it. Simon, maybe you can
> >> reproduce on shmobile hardware?
> >
> > Sure, I will try and reproduce it.
> >
> > I did subsequently post some fixes for fallout resulting from
> > the patch listed below. Those changes are now net-next.
> > Are you seeing the crash with those patches?
> >
> > The fix-up patches are:
> >
> > net,ipv4,ipv6: Correct assignment of skb->network_header to skb->tail
> > sctp: Correct access to skb->{network,transport}_header
> > ipv4: Correct comparisons and calculations using skb->tail and
> > skb-transport_header
> > ipv6: Correct comparisons and calculations using skb->tail and
> > skb-transport_header
> > net: Correct comparisons and calculations using skb->tail and
> > skb-transport_header
> > cxgb3: Correct comparisons and calculations using skb->tail and
> > skb-transport_header
> > isdn: Correct comparison of skb->tail and skb-transport_header
> > net: Fix build warnings after mac_header and transport_header became __u16.
>
> Ah, no, looks like those didn't make it into the 0529 linux-next.
>
> I've merged net-next on top and retested, and I no longer see the panics.
Great news, sorry for the mess.
FWIW I did not have any luck reproducing the problem (without the fix-ups)
on the mackerel board.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] effective user isolation
Hello. I have some nodes with virtual machines and want that each owner can view traffic from only owning virtual machines. For example: user1 have vm1_1 vm1_2 vm1_3 user2 have vm2_1 vm2_2 vm2_3 I want that user1 from vm1_1 and vm2_1 can view only traffic from this three servers and can't see vm2_*/, for user2 too. As i understand i can use vlans for each user - vlan1 for user1 vlan2 for user2... and so. But vlan numbers limits to 4096, if i have more than 4096 clients ..? -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru jabber: v...@selfip.ru ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
