[ovs-dev] [PATCH v2 2/2] datapath: sample action without side effects
The sample action is rather generic, allowing arbitrary actions to be
executed based on a probability. However its use, within the Open vSwitch
code-base is limited: only a single user-space action is ever nested.
A consequence of the current implementation of sample actions is that
depending on weather the sample action executed (due to its probability)
any side-effects of nested actions may or may not be present before
executing subsequent actions. This has the potential to complicate
verification of valid actions by the (kernel) datapath. And indeed adding
support for push and pop MPLS actions inside sample actions is one case
where such case.
In order to allow all supported actions to be continue to be nested inside
sample actions without the potential need for complex verification code
this patch changes the implementation of the sample action in the kernel
datapath so that sample actions are more like a function call and any side
effects of nested actions are not present when executing subsequent
actions.
With the above in mind the motivation for this change is twofold:
* To contain side-effects the sample action in the hope of making it
easier to deal with in the future and;
* To avoid some rather complex verification code introduced in the MPLS
datapath patch.
Some notes about the implementation:
* This patch silently changes the behaviour of sample actions whose nested
actions have side-effects. There are no known users of such sample
actions.
* sample() does not clone the skb for the only known use-case of the sample
action: a single nested userspace action. In such a case a clone is not
needed as the userspace action has no side effects.
Given that there are no known users of other nested actions and in order
to avoid the complexity of predicting if other sequences of actions have
side-effects in such cases the skb is cloned.
* As sample() provides a cloned skb in the unlikely case where there are
nested actions other than a single userspace action it is no longer
necessary to clone the skb in do_execute_actions() when executing a
recirculation action just because the keep_skb parameter is set: this
parameter was only set when processing the nested actions of a sample
action. Moreover it is possible to remove the keep_skb parameter of
do_execute_actions entirely.
---
v2
* As suggested by Jesse Gross
- Do not add attributes to noisily break compatibility with user-space
- (Try to) streamline cloning of skbs by sample and recirculate actions
---
datapath/actions.c | 48 ++--
1 file changed, 34 insertions(+), 14 deletions(-)
diff --git a/datapath/actions.c b/datapath/actions.c
index 56d..4dc7c3e 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -39,7 +39,7 @@
#include "vport.h"
static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
- const struct nlattr *attr, int len, bool
keep_skb);
+ const struct nlattr *attr, int len);
static int make_writable(struct sk_buff *skb, int write_len)
{
@@ -435,12 +435,18 @@ static int output_userspace(struct datapath *dp, struct
sk_buff *skb,
return ovs_dp_upcall(dp, skb, &upcall);
}
+static bool last_action(const struct nlattr *a, int rem)
+{
+ return a->nla_len == rem;
+}
+
static int sample(struct datapath *dp, struct sk_buff *skb,
const struct nlattr *attr)
{
const struct nlattr *acts_list = NULL;
const struct nlattr *a;
- int rem;
+ struct sk_buff *sample_skb;
+ int rem, err;
for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
a = nla_next(a, &rem)) {
@@ -456,8 +462,27 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
}
}
- return do_execute_actions(dp, skb, nla_data(acts_list),
- nla_len(acts_list), true);
+ rem = nla_len(acts_list);
+ a = nla_data(acts_list);
+
+ /* Actions list is either empty or only contains a single user-space
+* action, the latter being a special case as it is the only known
+* usage of the sample action.
+* In these special cases don't clone the skb as there are no
+* side-effects in the nested actions.
+* Otherwise, clone in case the nested actions have side effects. */
+ if (likely(rem == 0 ||
+ (nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
+ last_action(a, rem
+ sample_skb = skb;
+ else
+ sample_skb = skb_clone(skb, GFP_ATOMIC);
+
+ err = do_execute_actions(dp, sample_skb, a, rem);
+ if (unlikely(err && sample_skb != skb))
+ kfree_skb(skb); /* sample_skb but not skb already freed. */
+ return err;
+
}
static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
@@ -543,7 +568,7 @@ static int
[ovs-dev] [PATCH v2 0/2] datapath: sample action without side effects
Hi Jessee, as discussed elsewhere this series alters the sample action so that nested-actions do not have side effects. The motivation for this is to potentially simplify verification of new actions. And in particular to simplify the verification of MPLS actions (not included in this patchset). Simon Horman (2): datapath: Free skb(s) on recirculation error datapath: sample action without side effects datapath/actions.c | 52 1 file changed, 36 insertions(+), 16 deletions(-) -- 1.8.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 4/9] ofproto-dpif: Add recirc_id field to struct rule_dpif
On Wed, May 07, 2014 at 04:58:59PM +1200, Simon Horman wrote:
> Hi Ben,
>
> On Fri, May 02, 2014 at 07:34:58AM -0700, Ben Pfaff wrote:
> > On Fri, May 02, 2014 at 05:41:35PM +0900, Simon Horman wrote:
> > > This is to allow a recirculation id to be associated with a rule
> > > in the case that its actions cause recirculation.
> > >
> > > In such a case if the recirc_id field is non-zero then that value should
> > > be
> > > used, otherwise a value should be obtained using
> > > ofproto_dpif_alloc_recirc_id and saved in recirc_id field.
> > >
> > > When destructing the rule if the recirc_id field is non-zero then
> > > the associated internal flow should be deleted.
> > >
> > > This is in preparation for using the same helper as part of support
> > > for using recirculation in conjunction series of actions including
> > > with MPLS actions that are currently not able to be translated.
> > >
> > > Signed-off-by: Simon Horman
> >
> > Has this been tested? It appears that rule_dpif_get_recirc_id()
> > requires rule->up.mutex:
>
> I have exercised the code that uses this code.
> But not specifically the locking portions.
> I will look into this.
Sorry for the delay, I have been on vacation.
> > > +uint32_t
> > > +rule_dpif_get_recirc_id(struct rule_dpif *rule)
> > > +OVS_REQUIRES(rule->up.mutex)
> > > +{
> > > +if (!rule->recirc_id) {
> > > +struct ofproto_dpif *ofproto =
> > > ofproto_dpif_cast(rule->up.ofproto);
> > > +
> > > +rule_dpif_set_recirc_id(rule,
> > > ofproto_dpif_alloc_recirc_id(ofproto));
> > > +}
> > > +return rule->recirc_id;
> > > +}
> >
> > but that rule_dpif_get_recirc_id() sometimes calls rule_set_recirc_id(),
> > which attempts to acquire rule->up.mutex:
rule_set_recirc_id() attempts to acquire rule->up.mutex.
However, (the confusingly named) rule_dpif_set_recirc_id()
requires rather than attempting to acquire that lock.
/* Sets 'rule''s recirculation id. */
static void
rule_dpif_set_recirc_id(struct rule_dpif *rule, uint32_t id)
OVS_REQUIRES(rule->up.mutex)
{
ovs_assert(!rule->recirc_id);
rule->recirc_id = id;
}
> >
> > > +/* Sets 'rule''s recirculation id. */
> > > +void
> > > +rule_set_recirc_id(struct rule *rule_, uint32_t id)
> > > +{
> > > +struct rule_dpif *rule = rule_dpif_cast(rule_);
> > > +
> > > +ovs_mutex_lock(&rule->up.mutex);
> > > +rule_dpif_set_recirc_id(rule, id);
> > > +ovs_mutex_unlock(&rule->up.mutex);
> > > +}
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 1/2] datapath: Free skb(s) on recirculation error
It is my understanding that on error execute_recirc() does not free the skb passed to it. Assuming this is true then on error skb should always be freed if an error occurs in execute_recirc(). And if recirc_skb differs from skb, because it is a clone of skb, then it should also be freed. Signed-off-by: Simon Horman --- datapath/actions.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/datapath/actions.c b/datapath/actions.c index 7fe2f54..56d 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -596,8 +596,8 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb, err = execute_recirc(dp, recirc_skb, a); - if (last_action || err) - return err; + if (unlikely(err && recirc_skb != skb)) + kfree_skb(recirc_skb); break; } -- 1.8.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2.56] datapath: Add basic MPLS support to kernel
On Thu, May 01, 2014 at 09:45:12AM -0700, Jesse Gross wrote: > On Thu, May 1, 2014 at 1:54 AM, Simon Horman wrote: > > On Wed, Apr 30, 2014 at 03:56:44PM -0700, Jesse Gross wrote: > >> On Tue, Apr 29, 2014 at 10:58 PM, Simon Horman wrote: > >> > On Tue, Apr 29, 2014 at 11:41:57AM -0700, Jesse Gross wrote: > >> >> On Mon, Apr 28, 2014 at 5:13 PM, Simon Horman > >> >> wrote: > >> >> > On Mon, Apr 28, 2014 at 02:37:47PM -0700, Jesse Gross wrote: > >> >> >> On Mon, Apr 28, 2014 at 12:00 AM, Simon Horman > >> >> >> wrote: > >> >> >> > On Fri, Apr 25, 2014 at 12:57:20PM -0700, Jesse Gross wrote: > >> >> >> >> On Fri, Apr 25, 2014 at 1:06 AM, YAMAMOTO Takashi > >> >> >> >> wrote: > >> >> >> >> >> On Thu, Apr 24, 2014 at 05:57:29PM +0900, YAMAMOTO Takashi > >> >> >> >> >> wrote: > >> >> >> >> >>> hi, > >> >> >> >> >>> > >> >> >> >> >>> > + * Due to the sample action there may be multiple possible > >> >> >> >> >>> > eth types. > >> >> >> >> >>> > + * In order to correctly validate actions all possible > >> >> >> >> >>> > types are tracked > >> >> >> >> >>> > + * and verified. This is done using struct eth_types. > >> >> >> >> >>> > >> >> >> >> >>> is there any real-world use cases of these actions inside a > >> >> >> >> >>> sample? > >> >> >> >> >>> otherwise, how about just rejecting such combinations? > >> >> >> >> >>> it doesn't seem to worth the code complexity to me. > >> >> >> >> >>> (sorry if it has been already discussed. it's the first time > >> >> >> >> >>> for me > >> >> >> >> >>> to seriously read this long-lived patch.) > >> >> >> >> >> > >> >> >> >> >> Good point, the code is rather complex. > >> >> >> >> >> > >> >> >> >> >> My understanding is that it comes into effect in the case > >> >> >> >> >> of sflow or ipfix being configured on the bridge. I tend > >> >> >> >> >> to think these are real-world use-cases, though that thinking > >> >> >> >> >> is by no means fixed. > >> >> >> >> >> > >> >> >> >> >> My reading of the code is that in the case of sflow and ipfix > >> >> >> >> >> a single > >> >> >> >> >> sample rule appears at the beginning of the flow. And that it > >> >> >> >> >> may be > >> >> >> >> >> possible to replace the code that you are referring to with > >> >> >> >> >> something > >> >> >> >> >> simpler to handle these cases. > >> >> >> >> > > >> >> >> >> > it seems that they put only a userland action inside a sample. > >> >> >> >> > it's what i expected from its name "sample". > >> >> >> >> > >> >> >> >> Yes, that's the only current use case. In theory, this could be > >> >> >> >> used > >> >> >> >> more generally although nothing has come up yet. > >> >> >> >> > >> >> >> >> In retrospect, I regret the design of the sample action - not the > >> >> >> >> part > >> >> >> >> that allows it to handle different actions but the fact that the > >> >> >> >> results can affect things outside of the sample action list. I > >> >> >> >> think > >> >> >> >> that if we had made it more like a subroutine then that would have > >> >> >> >> retained all of the functionality but none of the complexity here. > >> >> >> >> Perhaps if we can find a clean way to restructure it without > >> >> >> >> breaking > >> >> >> >> compatibility then that would simplify the validation here. > >> >> >> > > >> >> >> > I have not thought deeply about this but it seems to me that it > >> >> >> > should be > >> >> >> > easy enough to provide compatibility for a new user-space to work > >> >> >> > with both > >> >> >> > new and old datapaths. But it is not clear to me how to achieve > >> >> >> > the > >> >> >> > reverse: allowing a new datapath to work with both new and old > >> >> >> > user-spaces. > >> >> >> > I assume that we care about such compatibility. > >> >> >> > >> >> >> Generally, I would say yes although there is potentially some room > >> >> >> for > >> >> >> debate here. No version of OVS userspace has ever put an action other > >> >> >> than userspace in the sample field. I know that other people have > >> >> >> talked about writing different userspaces that run on the OVS kernel > >> >> >> module but I highly doubt that they use this action or would do so > >> >> >> differently. I can't prove that but it might be OK to bite the > >> >> >> bullet. > >> >> > > >> >> > I am also concerned about the sample() action which is exposed via > >> >> > OpenFlow > >> >> > (as a Nicira extension) and in turn ovs-ofctl. Thus it seems to me > >> >> > that > >> >> > there could be users adding flows with sample actions whose behaviour > >> >> > would > >> >> > either no longer be supported or would be changed. But I believe > >> >> > that we > >> >> > should reason about this case the same way that you reason about > >> >> > alternate > >> >> > user-spaces above. > >> >> > >> >> The sample action exposed through OpenFlow is a little different. It > >> >> allows you to specify where in the action list to do sampling but it > >> >> doesn't allow arbitrary actions to be embedded. As a result,
Re: [ovs-dev] [PATCH] [resend] net: get rid of SET_ETHTOOL_OPS
Am Sun, May 11, 2014 at 01:13:47PM +0530 schrieb Anish Khurana: > SET_ETHTOOL_OPS is equivalent to : > #define SET_ETHTOOL_OPS(netdev,ops) \ > ( (netdev)->ethtool_ops = (ops) ) > > how it makes difference removing this code and replacing with the > code mentioned ? It doesn't change anything in the resulting binaries. The whole point is to remove the macro, which is something Dave wants to happen. Kind regards, Wilfried ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [of1.5 v2 07/20] ofp-util: Implement OpenFlow 1.4 port status and port desc reply messages.
On Sat, May 10, 2014 at 5:40 AM, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff
>
> diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
> index df7569e..b548f6b 100644
> --- a/lib/ofp-msgs.h
> +++ b/lib/ofp-msgs.h
> @@ -162,8 +162,10 @@ enum ofpraw {
>
> /* OFPT 1.0 (12): struct ofp_port_status, struct ofp10_phy_port. */
> OFPRAW_OFPT10_PORT_STATUS,
> -/* OFPT 1.1+ (12): struct ofp_port_status, struct ofp11_port. */
> +/* OFPT 1.1-1.3 (12): struct ofp_port_status, struct ofp11_port. */
> OFPRAW_OFPT11_PORT_STATUS,
> +/* OFPT 1.4+ (12): struct ofp_port_status, struct ofp14_port,
> uint8_t[8][]. */
> +OFPRAW_OFPT14_PORT_STATUS,
>
> /* OFPT 1.0 (13): struct ofp10_packet_out, uint8_t[]. */
> OFPRAW_OFPT10_PACKET_OUT,
> @@ -360,8 +362,10 @@ enum ofpraw {
>
> /* OFPST 1.0 (13): struct ofp10_phy_port[]. */
> OFPRAW_OFPST10_PORT_DESC_REPLY,
> -/* OFPST 1.1+ (13): struct ofp11_port[]. */
> +/* OFPST 1.1-1.3 (13): struct ofp11_port[]. */
> OFPRAW_OFPST11_PORT_DESC_REPLY,
> +/* OFPST 1.4+ (13): uint8_t[8][]. */
> +OFPRAW_OFPST14_PORT_DESC_REPLY,
>
> /* Nicira extension messages.
> *
> @@ -471,7 +475,8 @@ enum ofptype {
>* OFPRAW_OFPT11_FLOW_REMOVED.
>* OFPRAW_NXT_FLOW_REMOVED. */
> OFPTYPE_PORT_STATUS, /* OFPRAW_OFPT10_PORT_STATUS.
> - * OFPRAW_OFPT11_PORT_STATUS. */
> + * OFPRAW_OFPT11_PORT_STATUS.
> + * OFPRAW_OFPT14_PORT_STATUS. */
>
Now that there is OFPRAW_OFPT14_PORT_STATUS, encode_port_status()
should use it for OF1.4 instead of OFPRAW_OFPT11_PORT_STATUS. Otherwise,
now we get an abort.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] Fix a typo in lib/vconn-provider.h.
This trivial patch fixes a typo in lib/vconn-provider.h.
Signed-off-by: Rami Rosen
---
lib/vconn-provider.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/vconn-provider.h b/lib/vconn-provider.h
index b05bb45..0225a58 100644
--- a/lib/vconn-provider.h
+++ b/lib/vconn-provider.h
@@ -60,7 +60,7 @@ struct vconn_class {
* connection name provided by the user, e.g. "tcp:1.2.3.4". This name is
* useful for error messages but must not be modified.
*
- * 'allowed_verions' is the OpenFlow versions that may be
+ * 'allowed_versions' is the OpenFlow versions that may be
* negotiated for a connection.
*
* 'suffix' is a copy of 'name' following the colon and may be modified.
--
1.9.0
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v5] ofproto: per-table statistics
Add per-table counters. This resolves some short-comings
in the data provided in a table stats reply message.
* Lookups and matches are calculated based on table
accesses rather than datapath flow hits and misses.
* Lookups and matches are credited to the table where they
occurred rather than all being credited to table 0.
These problems were observed when running make check-ryu
and this patch allows many of its tester.py match checks
to pass.
Reviewed-by: YAMAMOTO Takashi
Signed-off-by: Simon Horman
--
v5
* As suggested by Ben Pfaff
- Use atomic_ulong instead of atomic_uint64_t as the later may
not be portable.
v4
* Rebase
v3
* Add: Reviewed-by: YAMAMOTO Takashi
* Rebase
v2
* Use NULL instead of 0 as NULL pointer argument
* Rebase
---
ofproto/ofproto-dpif-xlate.c | 8 +-
ofproto/ofproto-dpif.c | 39 -
ofproto/ofproto-dpif.h | 5 +-
ofproto/ofproto-provider.h | 3 +
ofproto/ofproto.c| 2 +
tests/ofproto-dpif.at| 195 +++
6 files changed, 228 insertions(+), 24 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index da538b7..84554f4 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2053,7 +2053,8 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t
in_port, uint8_t table_id,
? &ctx->xout->wc : NULL,
honor_table_miss,
&ctx->table_id, &rule,
- ctx->xin->xcache != NULL);
+ ctx->xin->xcache != NULL,
+ ctx->xin->resubmit_stats);
ctx->xin->flow.in_port.ofp_port = old_in_port;
if (ctx->xin->resubmit_hook) {
@@ -2662,7 +2663,7 @@ xlate_learn_action(struct xlate_ctx *ctx,
/* Lookup the learned rule, taking a reference on it. The reference
* is released when this cache entry is deleted. */
rule_dpif_lookup(ctx->xbridge->ofproto, &ctx->xin->flow, NULL,
- &entry->u.learn.rule, true);
+ &entry->u.learn.rule, true, NULL);
}
}
@@ -3273,7 +3274,8 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
*xout)
if (!xin->ofpacts && !ctx.rule) {
ctx.table_id = rule_dpif_lookup(ctx.xbridge->ofproto, flow,
!xin->skip_wildcards ? wc : NULL,
-&rule, ctx.xin->xcache != NULL);
+&rule, ctx.xin->xcache != NULL,
+ctx.xin->resubmit_stats);
if (ctx.xin->resubmit_stats) {
rule_dpif_credit_stats(rule, ctx.xin->resubmit_stats);
}
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index e50b4fe..b96ad1b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1527,25 +1527,20 @@ get_features(struct ofproto *ofproto_ OVS_UNUSED,
}
static void
-get_tables(struct ofproto *ofproto_, struct ofp12_table_stats *ots)
+get_tables(struct ofproto *ofproto, struct ofp12_table_stats *ots)
{
-struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
-struct dpif_dp_stats s;
-uint64_t n_miss, n_no_pkt_in, n_bytes, n_dropped_frags;
-uint64_t n_lookup;
-long long int used;
+int i;
strcpy(ots->name, "classifier");
-dpif_get_dp_stats(ofproto->backer->dpif, &s);
-rule_get_stats(&ofproto->miss_rule->up, &n_miss, &n_bytes, &used);
-rule_get_stats(&ofproto->no_packet_in_rule->up, &n_no_pkt_in, &n_bytes,
- &used);
-rule_get_stats(&ofproto->drop_frags_rule->up, &n_dropped_frags, &n_bytes,
- &used);
-n_lookup = s.n_hit + s.n_missed - n_dropped_frags;
-ots->lookup_count = htonll(n_lookup);
-ots->matched_count = htonll(n_lookup - n_miss - n_no_pkt_in);
+for (i = 0; i < ofproto->n_tables; i++) {
+unsigned long missed, matched;
+
+atomic_read(&ofproto->tables[i].n_matched, &matched);
+ots[i].matched_count = htonll(matched);
+atomic_read(&ofproto->tables[i].n_missed, &missed);
+ots[i].lookup_count = htonll(matched + missed);
+}
}
static struct ofport *
@@ -3204,7 +3199,7 @@ rule_dpif_get_actions(const struct rule_dpif *rule)
uint8_t
rule_dpif_lookup(struct ofproto_dpif *ofproto, struct flow *flow,
struct flow_wildcards *wc, struct rule_dpif **rule,
- bool take_ref)
+ bool take_ref, const struct dpif_flow_stats *stats)
{
enum rule_dpif_lookup_verdict verdict;
enum ofputil_port_config config = 0;
@@ -3231,7 +3226,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct
flow *flow,
}
verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, true,
-
[ovs-dev] [PATCH 2/2] ovs-atomic: Remove atomic_{u,}int64_t
Some concern has been raised by Ben Pfaff that atomic_uint64_t may not be portable. In particular on 32bit platforms that do not have atomic 64bit integers. Now that there are no longer any users of atomic_uint64_t remove it entirely. Also remove atomic_int64_t which has no users. Cc: YAMAMOTO Takashi Signed-off-by: Simon Horman --- tests/test-atomic.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test-atomic.c b/tests/test-atomic.c index b1a5d9d..24456d8 100644 --- a/tests/test-atomic.c +++ b/tests/test-atomic.c @@ -99,8 +99,6 @@ test_atomic_main(int argc OVS_UNUSED, char *argv[] OVS_UNUSED) TEST_ATOMIC_TYPE(atomic_int16_t, int16_t); TEST_ATOMIC_TYPE(atomic_uint32_t, uint32_t); TEST_ATOMIC_TYPE(atomic_int32_t, int32_t); -TEST_ATOMIC_TYPE(atomic_uint64_t, uint64_t); -TEST_ATOMIC_TYPE(atomic_int64_t, int64_t); test_atomic_flag(); } -- 1.8.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: Use atomic_long in struct udpif
Some concern has been raised by Ben Pfaff that atomic_uint64_t may not
be portable. Accordingly, use atomic_ulong instead of atomic_uint64_t
in struct ofproto.
This is in preparation for removing atomic_uint64_t entirely.
Cc: YAMAMOTO Takashi
Signed-off-by: Simon Horman
---
ofproto/ofproto-dpif-upcall.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index e1117ba..a5047ec 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -125,7 +125,7 @@ struct udpif {
atomic_uint flow_limit;/* Datapath flow hard limit. */
/* n_flows_mutex prevents multiple threads updating these concurrently. */
-atomic_uint64_t n_flows; /* Number of flows in the datapath. */
+atomic_ulong n_flows; /* Number of flows in the datapath. */
atomic_llong n_flows_timestamp;/* Last time n_flows was updated. */
struct ovs_mutex n_flows_mutex;
};
@@ -484,11 +484,11 @@ udpif_flush_all_datapaths(void)
}
-static uint64_t
+static unsigned long
udpif_get_n_flows(struct udpif *udpif)
{
long long int time, now;
-uint64_t flow_count;
+unsigned long flow_count;
now = time_msec();
atomic_read(&udpif->n_flows_timestamp, &time);
@@ -1599,7 +1599,7 @@ upcall_unixctl_show(struct unixctl_conn *conn, int argc
OVS_UNUSED,
atomic_read(&udpif->flow_limit, &flow_limit);
ds_put_format(&ds, "%s:\n", dpif_name(udpif->dpif));
-ds_put_format(&ds, "\tflows : (current %"PRIu64")"
+ds_put_format(&ds, "\tflows : (current %lu)"
" (avg %u) (max %u) (limit %u)\n", udpif_get_n_flows(udpif),
udpif->avg_n_flows, udpif->max_n_flows, flow_limit);
ds_put_format(&ds, "\tdump duration : %lldms\n", udpif->dump_duration);
--
1.8.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 0/2] ovs-atomic: Remove atomic_{u,}int64_t
Some concern has been raised by Ben Pfaff that atomic_uint64_t may not
be portable. In particular on 32bit platforms that do not have atomic
64bit integers.
This series removes the users of atomic_uint64_t.
It then removes atomic_uint64_t and atomic_int64_t themselves.
Simon Horman (2):
ofproto-dpif-upcall: Use atomic_long in struct udpif
ovs-atomic: Remove atomic_{u,}int64_t
ofproto/ofproto-dpif-upcall.c | 8
tests/test-atomic.c | 2 --
2 files changed, 4 insertions(+), 6 deletions(-)
--
1.8.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] dpif-netdev: Always serialise recirc_id to upcall key.
On 10 May 2014 03:14, Ben Pfaff wrote: > On Fri, May 09, 2014 at 02:35:44PM +1200, Joe Stringer wrote: > > This was causing a mismatch between the odp flow key in an upcall > > compared to the odp flow key that is serialised upon flow_dump. > > > > Signed-off-by: Joe Stringer > > I agree that the flow keys that appear in the flow dump and in upcalls > should match. This change implies that flow dumps always include a > recirc_id. But why does that happen? I don't see obvious code in > dpif-netdev.c that does that. > To be more precise, this is my understanding: * Upcall from dpif-netdev does not contain recirc_id in the odp key. * handle_upcalls() does xlate_actions() which generates a mask that exact-matches recirc_id. * handle_upcalls sends down the original odp flow key (missing recirc_id) with the flow mask (exact matching it). * dpif-netdev creates the flow based on this. * flow_dump_next(), when re-serialising the flow, puts the recirc_id as part of the odp flow key, because the flow masks it. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2] lib/classifier: Add subtable cache diagnostics.
Assert failures that should not happen have been reported on some
(non-OVS) test cases. This patch adds diagnostics to analyze what
goes wrong.
None of the OVS unit tests trigger these, so there is no performance
penalty.
This could be moved to test-classifier once it has served it's
purpose.
Signed-off-by: Jarno Rajahalme
---
v2: abort() after dignostics so this will be harder to dismiss.
lib/classifier.c | 134 +-
1 file changed, 132 insertions(+), 2 deletions(-)
diff --git a/lib/classifier.c b/lib/classifier.c
index 2646996..a9046cd 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -262,6 +262,126 @@ cls_subtable_cache_remove(struct cls_subtable_cache
*array,
ITER > (ARRAY)->subtables \
&& OVS_LIKELY(SUBTABLE = (--ITER)->subtable);)
+static void
+cls_subtable_cache_verify(struct cls_subtable_cache *array)
+{
+struct cls_subtable *table;
+struct cls_subtable_entry *iter;
+unsigned int priority = 0;
+
+CLS_SUBTABLE_CACHE_FOR_EACH_REVERSE (table, iter, array) {
+if (iter->max_priority != table->max_priority) {
+VLOG_WARN("Subtable %p has mismatching priority in cache (%u !=
%u)",
+ table, iter->max_priority, table->max_priority);
+}
+if (iter->max_priority < priority) {
+VLOG_WARN("Subtable cache is out of order (%u < %u)",
+ iter->max_priority, priority);
+}
+priority = iter->max_priority;
+}
+}
+
+static void
+cls_subtable_cache_reset(struct cls_classifier *cls)
+{
+struct cls_subtable_cache old = cls->subtables_priority;
+struct cls_subtable *subtable;
+
+VLOG_WARN("Resetting subtable cache.");
+
+cls_subtable_cache_verify(&cls->subtables_priority);
+
+cls_subtable_cache_init(&cls->subtables_priority);
+
+HMAP_FOR_EACH (subtable, hmap_node, &cls->subtables) {
+struct cls_match *head;
+struct cls_subtable_entry elem;
+struct cls_subtable *table;
+struct cls_subtable_entry *iter, *subtable_iter = NULL;
+unsigned int new_max = 0;
+unsigned int max_count = 0;
+bool found;
+
+/* Verify max_priority. */
+HMAP_FOR_EACH (head, hmap_node, &subtable->rules) {
+if (head->priority > new_max) {
+new_max = head->priority;
+max_count = 1;
+} else if (head->priority == new_max) {
+max_count++;
+}
+}
+if (new_max != subtable->max_priority ||
+max_count != subtable->max_count) {
+VLOG_WARN("subtable %p (%u rules) has mismatching max_priority "
+ "(%u) or max_count (%u). Highest priority found was %u, "
+ "count: %u",
+ subtable, subtable->n_rules, subtable->max_priority,
+ subtable->max_count, new_max, max_count);
+subtable->max_priority = new_max;
+subtable->max_count = max_count;
+}
+
+/* Locate the subtable from the old cache. */
+found = false;
+CLS_SUBTABLE_CACHE_FOR_EACH (table, iter, &old) {
+if (table == subtable) {
+if (iter->max_priority != new_max) {
+VLOG_WARN("Subtable %p has wrong max priority (%u != %u) "
+ "in the old cache.",
+ subtable, iter->max_priority, new_max);
+}
+if (found) {
+VLOG_WARN("Subtable %p duplicated in the old cache.",
+ subtable);
+}
+found = true;
+}
+}
+if (!found) {
+VLOG_WARN("Subtable %p not found from the old cache.", subtable);
+}
+
+elem.subtable = subtable;
+elem.tag = subtable->tag;
+elem.max_priority = subtable->max_priority;
+cls_subtable_cache_push_back(&cls->subtables_priority, elem);
+
+/* Possibly move 'subtable' earlier in the priority list. If we break
+ * out of the loop, then 'subtable_iter' should be moved just before
+ * 'iter'. If the loop terminates normally, then 'iter' will be the
+ * first list element and we'll move subtable just before that
+ * (e.g. to the front of the list). */
+CLS_SUBTABLE_CACHE_FOR_EACH_REVERSE (table, iter,
+ &cls->subtables_priority) {
+if (table == subtable) {
+subtable_iter = iter; /* Locate the subtable as we go. */
+} else if (table->max_priority >= new_max) {
+ovs_assert(subtable_iter != NULL);
+iter++;
+break;
+}
+}
+
+/* Move 'subtable' just before 'iter' (unless it's already there). */
+if (iter != subtable_iter) {
Re: [ovs-dev] [PATCH v2] lib/classifier: Add subtable cache diagnostics.
I assume that's the only change. If so, it sounds good. I hope we can catch
the bug quickly.
On May 11, 2014 8:34 PM, "Jarno Rajahalme" wrote:
> Assert failures that should not happen have been reported on some
> (non-OVS) test cases. This patch adds diagnostics to analyze what
> goes wrong.
>
> None of the OVS unit tests trigger these, so there is no performance
> penalty.
>
> This could be moved to test-classifier once it has served it's
> purpose.
>
> Signed-off-by: Jarno Rajahalme
> ---
> v2: abort() after dignostics so this will be harder to dismiss.
>
> lib/classifier.c | 134
> +-
> 1 file changed, 132 insertions(+), 2 deletions(-)
>
> diff --git a/lib/classifier.c b/lib/classifier.c
> index 2646996..a9046cd 100644
> --- a/lib/classifier.c
> +++ b/lib/classifier.c
> @@ -262,6 +262,126 @@ cls_subtable_cache_remove(struct cls_subtable_cache
> *array,
> ITER > (ARRAY)->subtables \
> && OVS_LIKELY(SUBTABLE = (--ITER)->subtable);)
>
> +static void
> +cls_subtable_cache_verify(struct cls_subtable_cache *array)
> +{
> +struct cls_subtable *table;
> +struct cls_subtable_entry *iter;
> +unsigned int priority = 0;
> +
> +CLS_SUBTABLE_CACHE_FOR_EACH_REVERSE (table, iter, array) {
> +if (iter->max_priority != table->max_priority) {
> +VLOG_WARN("Subtable %p has mismatching priority in cache (%u
> != %u)",
> + table, iter->max_priority, table->max_priority);
> +}
> +if (iter->max_priority < priority) {
> +VLOG_WARN("Subtable cache is out of order (%u < %u)",
> + iter->max_priority, priority);
> +}
> +priority = iter->max_priority;
> +}
> +}
> +
> +static void
> +cls_subtable_cache_reset(struct cls_classifier *cls)
> +{
> +struct cls_subtable_cache old = cls->subtables_priority;
> +struct cls_subtable *subtable;
> +
> +VLOG_WARN("Resetting subtable cache.");
> +
> +cls_subtable_cache_verify(&cls->subtables_priority);
> +
> +cls_subtable_cache_init(&cls->subtables_priority);
> +
> +HMAP_FOR_EACH (subtable, hmap_node, &cls->subtables) {
> +struct cls_match *head;
> +struct cls_subtable_entry elem;
> +struct cls_subtable *table;
> +struct cls_subtable_entry *iter, *subtable_iter = NULL;
> +unsigned int new_max = 0;
> +unsigned int max_count = 0;
> +bool found;
> +
> +/* Verify max_priority. */
> +HMAP_FOR_EACH (head, hmap_node, &subtable->rules) {
> +if (head->priority > new_max) {
> +new_max = head->priority;
> +max_count = 1;
> +} else if (head->priority == new_max) {
> +max_count++;
> +}
> +}
> +if (new_max != subtable->max_priority ||
> +max_count != subtable->max_count) {
> +VLOG_WARN("subtable %p (%u rules) has mismatching
> max_priority "
> + "(%u) or max_count (%u). Highest priority found was
> %u, "
> + "count: %u",
> + subtable, subtable->n_rules, subtable->max_priority,
> + subtable->max_count, new_max, max_count);
> +subtable->max_priority = new_max;
> +subtable->max_count = max_count;
> +}
> +
> +/* Locate the subtable from the old cache. */
> +found = false;
> +CLS_SUBTABLE_CACHE_FOR_EACH (table, iter, &old) {
> +if (table == subtable) {
> +if (iter->max_priority != new_max) {
> +VLOG_WARN("Subtable %p has wrong max priority (%u !=
> %u) "
> + "in the old cache.",
> + subtable, iter->max_priority, new_max);
> +}
> +if (found) {
> +VLOG_WARN("Subtable %p duplicated in the old cache.",
> + subtable);
> +}
> +found = true;
> +}
> +}
> +if (!found) {
> +VLOG_WARN("Subtable %p not found from the old cache.",
> subtable);
> +}
> +
> +elem.subtable = subtable;
> +elem.tag = subtable->tag;
> +elem.max_priority = subtable->max_priority;
> +cls_subtable_cache_push_back(&cls->subtables_priority, elem);
> +
> +/* Possibly move 'subtable' earlier in the priority list. If we
> break
> + * out of the loop, then 'subtable_iter' should be moved just
> before
> + * 'iter'. If the loop terminates normally, then 'iter' will be
> the
> + * first list element and we'll move subtable just before that
> + * (e.g. to the front of the list). */
> +CLS_SUBTABLE_CACHE_FOR_EACH_REVERSE (table, iter,
> + &cls->subtables_priority) {
> +if (table
[ovs-dev] [PATCH] lib/classifier: Fix array splicing.
Array splicing was broken when multiple elements were being moved,
resulting in the priority order being mixed. This came up when the
highest priority rule from a subtable was removed and the subtable
needed to be moved down the priority list by more than one position.
Signed-off-by: Jarno Rajahalme
---
lib/classifier.c |4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lib/classifier.c b/lib/classifier.c
index a9046cd..aef57bb 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -224,12 +224,14 @@ cls_subtable_cache_splice(struct cls_subtable_entry *to,
to = start; start = end; end = temp;
}
if (to < start) {
+/* Move elements [start, end) to (to) one by one. */
while (start != end) {
struct cls_subtable_entry temp = *start;
+/* Shift array by one, making space for the element at 'temp'. */
memmove(to + 1, to, (start - to) * sizeof *to);
*to = temp;
-start++;
+start++; to++;
}
} /* Else nothing to be done. */
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
