Re: [ovs-dev] [PATCH] OPENFLOW-1.1+: note on table-miss
On Fri, Nov 01, 2013 at 03:23:10PM +0900, YAMAMOTO Takashi wrote: > > On Tue, Oct 15, 2013 at 05:12:22PM +0900, YAMAMOTO Takashi wrote: > >> Signed-off-by: YAMAMOTO Takashi > >> --- > >> OPENFLOW-1.1+ | 7 +++ > >> 1 file changed, 7 insertions(+) > >> > >> diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+ > >> index 07b2660..4f30520 100644 > >> --- a/OPENFLOW-1.1+ > >> +++ b/OPENFLOW-1.1+ > >> @@ -121,6 +121,13 @@ didn't compare the specs carefully yet.) > >>[optional for OF1.3+] > >> > >> * More flexible table miss support. > >> + This requires the following. > >> + - Change the default table-miss action (in the absense of table-miss > >> +entry) from packet_in to drop for OF1.3+. Decide what to do if > >> +a switch is configured to support multiple OF versions. > > > > I'm wondering what is a good approach to take here. > > > > It is possible to configure Open vSwitch (ovs-vswtichd) to only accept Open > > Flow 1.3+ connections. In which case it should be possible to select the > > default behaviour described above. However it is also possible for Open > > vSwitch (ovs-vswtichd) to be configured to accept a connections for Open > > Flow versions prior to 1.3, and 1.3+. > > > > This is complicated by the fact that OpenFlow 1.3 conveniently deprecates > > all the TABLE_MOD bits that allow configuration of this behaviour. Though I > > assume deprecated doesn't mean not allowed. > > > > With the constraints describe above and making the bold assumption that I'm > > not missing any further constraints I propose the following: > > > > A: > > 1. If Open vSwtich is configured to only accept connections > > for Open Flow 1.3+ then default to drop. > > > > 2. Otherwise use the current default, packet_in. > > > > Is this a good idea? It may be to subtle to be useful in practice. > > it sounds difficult to handle for controller-side programmers. > > > > > B: > > Implement TABLE_MOD to allow it to be used to control the behaviour > > of each table's miss behaviour. > > > > We could even go so far as to encourage people to use it, > > even if they are using Open Flow 1.3+, to ensure that the > > behaviour is what they expect. > > this leaves the question what should be the default. I meant to do both A and B. But yes, it does side-step the issue to some extent. > > C: > decide what to do (packet-in or drop) per ofconn basis, > depending on OF versions. > (ofconn_receives_async_msg can take care of this.) That is fine, so long as there is one connection. And in the case where there is no controller then I think the OpenFlow 1.0 behaviour degrades to drop anyway. But I think we need to consider the case of multiple controllers. In particular two, both ROLE_EQUAL, that use OpenFlow versions with different defaults in this regards. I believe this is supported as of OpenFlow 1.2. > optionally, if a switch is configured to accept OF 1.3+ only, > drop it in kernel as an optimization. I guess it depends how cleanly it could be implemented. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] OPENFLOW-1.1+: note on table-miss
> On Fri, Nov 01, 2013 at 03:23:10PM +0900, YAMAMOTO Takashi wrote: >> > On Tue, Oct 15, 2013 at 05:12:22PM +0900, YAMAMOTO Takashi wrote: >> >> Signed-off-by: YAMAMOTO Takashi >> >> --- >> >> OPENFLOW-1.1+ | 7 +++ >> >> 1 file changed, 7 insertions(+) >> >> >> >> diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+ >> >> index 07b2660..4f30520 100644 >> >> --- a/OPENFLOW-1.1+ >> >> +++ b/OPENFLOW-1.1+ >> >> @@ -121,6 +121,13 @@ didn't compare the specs carefully yet.) >> >>[optional for OF1.3+] >> >> >> >> * More flexible table miss support. >> >> + This requires the following. >> >> + - Change the default table-miss action (in the absense of >> >> table-miss >> >> +entry) from packet_in to drop for OF1.3+. Decide what to do if >> >> +a switch is configured to support multiple OF versions. >> > >> > I'm wondering what is a good approach to take here. >> > >> > It is possible to configure Open vSwitch (ovs-vswtichd) to only accept Open >> > Flow 1.3+ connections. In which case it should be possible to select the >> > default behaviour described above. However it is also possible for Open >> > vSwitch (ovs-vswtichd) to be configured to accept a connections for Open >> > Flow versions prior to 1.3, and 1.3+. >> > >> > This is complicated by the fact that OpenFlow 1.3 conveniently deprecates >> > all the TABLE_MOD bits that allow configuration of this behaviour. Though I >> > assume deprecated doesn't mean not allowed. >> > >> > With the constraints describe above and making the bold assumption that I'm >> > not missing any further constraints I propose the following: >> > >> > A: >> > 1. If Open vSwtich is configured to only accept connections >> > for Open Flow 1.3+ then default to drop. >> > >> > 2. Otherwise use the current default, packet_in. >> > >> > Is this a good idea? It may be to subtle to be useful in practice. >> >> it sounds difficult to handle for controller-side programmers. >> >> > >> > B: >> > Implement TABLE_MOD to allow it to be used to control the behaviour >> > of each table's miss behaviour. >> > >> > We could even go so far as to encourage people to use it, >> > even if they are using Open Flow 1.3+, to ensure that the >> > behaviour is what they expect. >> >> this leaves the question what should be the default. > > I meant to do both A and B. > But yes, it does side-step the issue to some extent. > >> >> C: >> decide what to do (packet-in or drop) per ofconn basis, >> depending on OF versions. >> (ofconn_receives_async_msg can take care of this.) > > That is fine, so long as there is one connection. > And in the case where there is no controller then I think > the OpenFlow 1.0 behaviour degrades to drop anyway. > > But I think we need to consider the case of multiple controllers. > In particular two, both ROLE_EQUAL, that use OpenFlow versions > with different defaults in this regards. I believe this is supported > as of OpenFlow 1.2. i meant, in that case, send packet-in only to connections using prior OF versions. YAMAMOTO Takashi > >> optionally, if a switch is configured to accept OF 1.3+ only, >> drop it in kernel as an optimization. > > I guess it depends how cleanly it could be implemented. > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] OPENFLOW-1.1+: note on table-miss
On Fri, Nov 01, 2013 at 04:25:25PM +0900, YAMAMOTO Takashi wrote: > > On Fri, Nov 01, 2013 at 03:23:10PM +0900, YAMAMOTO Takashi wrote: > >> > On Tue, Oct 15, 2013 at 05:12:22PM +0900, YAMAMOTO Takashi wrote: > >> >> Signed-off-by: YAMAMOTO Takashi > >> >> --- > >> >> OPENFLOW-1.1+ | 7 +++ > >> >> 1 file changed, 7 insertions(+) > >> >> > >> >> diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+ > >> >> index 07b2660..4f30520 100644 > >> >> --- a/OPENFLOW-1.1+ > >> >> +++ b/OPENFLOW-1.1+ > >> >> @@ -121,6 +121,13 @@ didn't compare the specs carefully yet.) > >> >>[optional for OF1.3+] > >> >> > >> >> * More flexible table miss support. > >> >> + This requires the following. > >> >> + - Change the default table-miss action (in the absense of > >> >> table-miss > >> >> +entry) from packet_in to drop for OF1.3+. Decide what to do if > >> >> +a switch is configured to support multiple OF versions. > >> > > >> > I'm wondering what is a good approach to take here. > >> > > >> > It is possible to configure Open vSwitch (ovs-vswtichd) to only accept > >> > Open > >> > Flow 1.3+ connections. In which case it should be possible to select the > >> > default behaviour described above. However it is also possible for Open > >> > vSwitch (ovs-vswtichd) to be configured to accept a connections for Open > >> > Flow versions prior to 1.3, and 1.3+. > >> > > >> > This is complicated by the fact that OpenFlow 1.3 conveniently deprecates > >> > all the TABLE_MOD bits that allow configuration of this behaviour. > >> > Though I > >> > assume deprecated doesn't mean not allowed. > >> > > >> > With the constraints describe above and making the bold assumption that > >> > I'm > >> > not missing any further constraints I propose the following: > >> > > >> > A: > >> > 1. If Open vSwtich is configured to only accept connections > >> > for Open Flow 1.3+ then default to drop. > >> > > >> > 2. Otherwise use the current default, packet_in. > >> > > >> > Is this a good idea? It may be to subtle to be useful in practice. > >> > >> it sounds difficult to handle for controller-side programmers. > >> > >> > > >> > B: > >> > Implement TABLE_MOD to allow it to be used to control the behaviour > >> > of each table's miss behaviour. > >> > > >> > We could even go so far as to encourage people to use it, > >> > even if they are using Open Flow 1.3+, to ensure that the > >> > behaviour is what they expect. > >> > >> this leaves the question what should be the default. > > > > I meant to do both A and B. > > But yes, it does side-step the issue to some extent. > > > >> > >> C: > >> decide what to do (packet-in or drop) per ofconn basis, > >> depending on OF versions. > >> (ofconn_receives_async_msg can take care of this.) > > > > That is fine, so long as there is one connection. > > And in the case where there is no controller then I think > > the OpenFlow 1.0 behaviour degrades to drop anyway. > > > > But I think we need to consider the case of multiple controllers. > > In particular two, both ROLE_EQUAL, that use OpenFlow versions > > with different defaults in this regards. I believe this is supported > > as of OpenFlow 1.2. > > i meant, in that case, send packet-in only to connections using > prior OF versions. Oh, I see. Yes, I think that could work. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] debian: Option to create tunnel through 'interfaces'.
Is that possible to add ability to bring up ovs interfaces in case it
defined not in /etc/network/interfaces but using source xxx, for
example i have
/etc/network/interfaces:
source /etc/network/interfaces.d/*
and in /etc/network/interfaces.d/XXX i'm configure each interface.
2013/10/31 Gurucharan Shetty :
> On Thu, Oct 31, 2013 at 10:24 AM, Ben Pfaff wrote:
>> On Thu, Oct 31, 2013 at 09:24:08AM -0700, Gurucharan Shetty wrote:
>>> This is a port of commit 7b75828bf5654c (rhel: Option to create tunnel
>>> through ifcfg scripts.) from rhel to debian's ifupdown script.
>>>
>>> Signed-off-by: Gurucharan Shetty
>>
>> It looks like the first and later lines of this command are all indented
>> the same amount, which makes it harder to pick it out visually as a
>> single command:
> I added the correct indentation.
>>> +ovs_vsctl -- --may-exist add-port "${IF_OVS_BRIDGE}"\
>>> +"${IFACE}" ${IF_OVS_OPTIONS} -- set Interface "${IFACE}" \
>>> +type=${IF_OVS_TUNNEL_TYPE} ${IF_OVS_TUNNEL_OPTIONS} \
>>> +${OVS_EXTRA+-- $OVS_EXTRA}
>>> +;;
>>
>> Otherwise:
>> Acked-by: Ben Pfaff
> Thanks. I pushed this to master.
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
--
Vasiliy Tolstov,
e-mail: v.tols...@selfip.ru
jabber: v...@selfip.ru
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/4] Implement the encode/decode Table Features functions
Thanks for your reply!
On 01/11/2013 12:29, Simon Horman wrote:
On Sat, Oct 26, 2013 at 06:14:28PM +0800, Alexander Wu wrote:
Implement the encode/decode table features msgs function, and
NOTE that we implement the decode functions *_raw, maybe we
should change it the ofpbuf_pull?
Signed-off-by: Alexander Wu
---
lib/ofp-print.c | 128 +++-
lib/ofp-util.c | 646 +++
2 files changed, 772 insertions(+), 2 deletions(-)
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index e4d0303..418f918 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2381,6 +2381,127 @@ ofp_print_group_mod(struct ds *s, const struct
ofp_header *oh)
ofp_print_group(s, gm.group_id, gm.type, &gm.buckets);
}
+/* Appends a string representation of 'prop' to 's'. */
+static void
+table_feature_prop_format(const struct ofputil_table_feature_prop_header *prop,
+struct ds *s)
static void
table_feature_prop_format(const struct ofputil_table_feature_prop_header *prop,
struct ds *s)
I'll fix it, thanks.
+{
+int i = 0;
+int n = 0;
+int element_size = (int)get_prop_length(prop->type);
Blank line here please.
I'll fix it, thanks.
+if (!element_size) {
+/* FIXME LOG SOMETHING */
This should be fixed. I can think of three ways:
* Please add a (rate limited?) log message here
* Return an error and handle it in the callers
* NOT_REACHED(), though in that case you could remove the check
all together as a division by zero should alert the user that
there is a bug somewhere.
The last option seems easiest. Is it a bug for element_size to be zero?
Currently if the type is not in OFPTFPT13_*, get_prop_length
would return 0. So I think it should return OFPERR_OFPTFFC_BAD_TYPE.
Thanks!
+return;
+} else {
+n = (prop->length - 4) / element_size;
+}
+
+ds_put_format(s, "%s: ", get_prop_name(prop->type));
+
+switch (prop->type) {
+case OFPTFPT13_INSTRUCTIONS:
+case OFPTFPT13_INSTRUCTIONS_MISS: {
+struct ofp11_instruction *inst = (struct ofp11_instruction
*)prop->data;
+
+/* FIXME ofpacts_format */
Is the comment above still valid?
If so, could you fix it?
Yes, I'm trying to fix it. But there are 2 ways to fix it.
A. Print act/inst/oxm bitmap in console.
B. Print human-readable texts in console.
Which one do you think is better?
A -> human-unreadable
B -> lots of texts in console
+for (i = 0; i < n; i++) {
+ds_put_format(s, "%"PRIu16, inst[i].type);
+if (i != n - 1)
+ds_put_format(s, ",");
+}
+break;
+}
+case OFPTFPT13_NEXT_TABLES:
+case OFPTFPT13_NEXT_TABLES_MISS: {
+uint8_t *ntables = prop->data;
+for (i = 0; i < n; i++) {
+ds_put_format(s, "%"PRIu8, ntables[i]);
+if (i != n - 1)
+ds_put_format(s, ",");
+}
+break;
+}
+case OFPTFPT13_WRITE_ACTIONS:
+case OFPTFPT13_WRITE_ACTIONS_MISS:
+case OFPTFPT13_APPLY_ACTIONS:
+case OFPTFPT13_APPLY_ACTIONS_MISS: {
+struct ofp_action_header *acts =(struct ofp_action_header *)prop->data;
+
+/* FIXME ofpacts_format */
Ditto.
Same.
+for (i = 0; i < n; i++) {
+ds_put_format(s, "%"PRIu16, acts[i].type);
+if (i != n - 1)
+ds_put_format(s, ",");
+}
+break;
+}
+case OFPTFPT13_MATCH:
+case OFPTFPT13_WILDCARDS:
+case OFPTFPT13_WRITE_SETFIELD:
+case OFPTFPT13_WRITE_SETFIELD_MISS:
+case OFPTFPT13_APPLY_SETFIELD:
+case OFPTFPT13_APPLY_SETFIELD_MISS: {
+uint32_t *oxm = (uint32_t *)prop->data;
+
+for (i = 0; i < n; i++) {
+ds_put_format(s, "%s", get_oxm_name(oxm[i]));
+if (i != n - 1)
+ds_put_format(s, ",");
+}
+break;
+}
+case OFPTFPT13_EXPERIMENTER:
+case OFPTFPT13_EXPERIMENTER_MISS:
+ds_put_format(s, "experimenter");
+if (i != n - 1)
+ds_put_format(s, ",");
+break;
+default:
+ds_put_format(s, "unknown(%u)", prop->type);
+if (i != n - 1)
+ds_put_format(s, ",");
+break;
+}
+}
+
+
+static void
+ofp_print_table_features_stats_single(struct ds *s,
+const struct ofputil_table_features *tf)
+{
+int i;
+ds_put_format(s, "\n %"PRIu8":", tf->table_id);
+ds_put_format(s, " name:%s", tf->name);
+ds_put_format(s, " metadata_match:%"PRIx64, tf->metadata_match);
+ds_put_format(s, " metadata_write:%"PRIx64, tf->metadata_write);
+ds_put_format(s, " config:%"PRIx32, tf->config);
+ds_put_format(s, " max_entries:%"PRIu32, tf->max_entries);
+
+ds_put_format(s, "\nProperties:");
+for (i = 0; i < tf->n_property; i++) {
+if (tf->props[i].data == NULL || tf->props[i].length == 0)
+con
Re: [ovs-dev] [PATCH 1/4] Add/Modify headers for Multipart - Table Features.
On 01/11/2013 12:32, Simon Horman wrote:
On Sat, Oct 26, 2013 at 06:12:27PM +0800, Alexander Wu wrote:
Add headers and function prototype for table features.
And modify the limits of mp-table-features msg
Signed-off-by: Alexander Wu
---
lib/ofp-msgs.h |4 +-
lib/ofp-util.h | 159
ofproto/ofproto-provider.h |3 +
3 files changed, 164 insertions(+), 2 deletions(-)
diff --git a/lib/ofp-msgs.h b/lib/ofp-msgs.h
index aa19fe3..9a679f4 100644
--- a/lib/ofp-msgs.h
+++ b/lib/ofp-msgs.h
@@ -336,10 +336,10 @@ enum ofpraw {
/* OFPST 1.3+ (11): struct ofp13_meter_features. */
OFPRAW_OFPST13_METER_FEATURES_REPLY,
-/* OFPST 1.3+ (12): struct ofp13_table_features[]. */
+/* OFPST 1.3+ (12): void. */
OFPRAW_OFPST13_TABLE_FEATURES_REQUEST,
-/* OFPST 1.3+ (12): struct ofp13_table_features[]. */
+/* OFPST 1.3+ (12): struct ofp13_table_features, uint8_t[8][]. */
OFPRAW_OFPST13_TABLE_FEATURES_REPLY,
/* OFPST 1.0+ (13): void. */
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 1f77808..a33ddec 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -598,6 +598,165 @@ enum ofperr ofputil_decode_table_mod(const struct
ofp_header *,
struct ofpbuf *ofputil_encode_table_mod(const struct ofputil_table_mod *,
enum ofputil_protocol);
+struct ofputil_table_feature_prop_header {
+uint16_t type; /* One of OFPTFPT_*. */
+uint16_t length; /* Length in bytes of this property. */
+uint8_t *data;
+};
+
+#define OTF_GET (1 << 0)
+#define OTF_SET (1 << 1)
Perhaps an enum is appropriate for these values.
I will check it, thanks.
+#define OFTABLE_NUM 0xff
+
+/* Abstract ofp13_table_features */
+struct ofputil_table_features {
+uint16_t length; /* Length is padded to 64 bits. */
+uint8_t table_id; /* Identifier of table. Lower numbered tables
+ are consulted first. */
+char name[OFP_MAX_TABLE_NAME_LEN];
+uint64_t metadata_match; /* Bits of metadata table can match. */
+uint64_t metadata_write; /* Bits of metadata table can write. */
+uint32_t config; /* Bitmap of OFPTC_* values */
+uint32_t max_entries; /* Max number of entries supported. */
+
+struct ofputil_table_feature_prop_header props[0xff];
+uint16_t n_property;
+};
+
+
I think that one blank line is sufficient here.
Thanks, I'll remove the blank line.
+struct oxm_variable {
+uint32_t data;
+char *name;
+};
+
+extern struct oxm_variable oxm_variables[];
+int get_oxm_num(void);
+char *get_oxm_name(uint32_t type);
oxm_variables, get_oxm_num and get_oxm_name are added in the next patch not
this one. So its declaration should go there too.
Yes, I'll merge it to next patch.
+
+/* Table Feature property types.
+ * Low order bit cleared indicates a property for a regular Flow Entry.
+ * Low order bit set indicates a property for the Table-Miss Flow Entry. */
+enum ofputil_table_feature_prop_type {
+OFPUTIL_INSTRUCTIONS = 0, /* Instructions property. */
+OFPUTIL_INSTRUCTIONS_MISS= 1, /* Instructions for table-miss. */
+OFPUTIL_NEXT_TABLES = 2, /* Next Table property. */
+OFPUTIL_NEXT_TABLES_MISS = 3, /* Next Table for table-miss. */
+OFPUTIL_WRITE_ACTIONS= 4, /* Write Actions property. */
+OFPUTIL_WRITE_ACTIONS_MISS = 5, /* Write Actions for table-miss. */
+OFPUTIL_APPLY_ACTIONS= 6, /* Apply Actions property. */
+OFPUTIL_APPLY_ACTIONS_MISS = 7, /* Apply Actions for table-miss. */
+OFPUTIL_MATCH= 8, /* Match property. */
+OFPUTIL_WILDCARDS= 10, /* Wildcards property. */
+OFPUTIL_WRITE_SETFIELD = 12, /* Write Set-Field property. */
+OFPUTIL_WRITE_SETFIELD_MISS = 13, /* Write Set-Field for table-miss. */
+OFPUTIL_APPLY_SETFIELD = 14, /* Apply Set-Field property. */
+OFPUTIL_APPLY_SETFIELD_MISS = 15, /* Apply Set-Field for table-miss. */
+OFPUTIL_EXPERIMENTER = 0xFFFE, /* Experimenter property. */
+OFPUTIL_EXPERIMENTER_MISS= 0x, /* Experimenter for table-miss. */
+};
This seems to duplicate ofp13_table_feature_prop_type.
I wonder if it is necessary.
Currently it seems not necessary, but I'm rewriting the big structure
'static_props' to switch-case like this:
DEFINE_TFPROP(ENUM, STRUCT, EXTENSIBLE, NAME)
It will be used in next version.
+
+struct ofputil_instruction {
+uint16_t type; /* Instruction type */
+uint16_t len; /* Length of this struct in bytes. */
+uint8_t pad[4];
+};
+
+/* Instructions property */
+struct ofputil_table_feature_prop_instructions {
+uint16_ttype;/* One of OFPUTIL_INSTRUCTIONS,
+OFPUTIL_INSTRUCTIONS_MISS. */
+uint16_tlength; /* Length in bytes of this property. */
+uint8_t table_id;
+uint8_t
Re: [ovs-dev] [PATCH 3/4] Add init/destroy funcs and handle Table Features msgs.
On 01/11/2013 12:32, Simon Horman wrote:
On Sat, Oct 26, 2013 at 06:15:30PM +0800, Alexander Wu wrote:
Add some functions to init table features(use ofp13_* struct
currently, change it to ofputil later).
Use the encode/decode functions to handle table features request.
Currently we just implement GET table feature.
SET table feature may be a hard job, we'll do it later.
I'm confused, the patchset seems to implement SET though
not expose it to ovs-ofctl.
If the SET implementation is not complete perhaps it is better
to remove it. If it is complete perhaps it should be exposed to ovs-ofctl
along with GET.
If the SET implementation is not complete and unlikely to be complete in
the forseeable future then it seems to me that the implementation could be
greatly simplified by just algorithmically returning the values that are
currently initialised by table_features_init() and not actually allocating
the features at all in ofproto_init_tables().
It's not done because I think:
controller/user SET table features, and then switch should
be limited to the features, like this:
controller says: you could only use OUTPUT,xxx,yyy action.
switch: ok.(silently)
Is it right? I'm not sure because I don't find the definition in spec.
Agreed to you suggesstion, I'll split the SET table-features code.
And now the cli we implement is very crude. Maybe it could
display better.
Signed-off-by: Alexander Wu
---
lib/rconn.c |4 +-
ofproto/ofproto.c | 384 -
utilities/ovs-ofctl.c | 17 +++
3 files changed, 402 insertions(+), 3 deletions(-)
diff --git a/lib/rconn.c b/lib/rconn.c
index 96b3579..c441962 100644
--- a/lib/rconn.c
+++ b/lib/rconn.c
@@ -1381,8 +1381,6 @@ is_admitted_msg(const struct ofpbuf *b)
case OFPTYPE_GROUP_DESC_STATS_REPLY:
case OFPTYPE_GROUP_FEATURES_STATS_REQUEST:
case OFPTYPE_GROUP_FEATURES_STATS_REPLY:
-case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
-case OFPTYPE_TABLE_FEATURES_STATS_REPLY:
return false;
case OFPTYPE_PACKET_IN:
@@ -1429,6 +1427,8 @@ is_admitted_msg(const struct ofpbuf *b)
case OFPTYPE_FLOW_MONITOR_CANCEL:
case OFPTYPE_FLOW_MONITOR_PAUSED:
case OFPTYPE_FLOW_MONITOR_RESUMED:
+case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
+case OFPTYPE_TABLE_FEATURES_STATS_REPLY:
default:
return true;
}
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 91a39ef..03f6e14 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -559,6 +559,45 @@ ofproto_create(const char *datapath_name, const char
*datapath_type,
return 0;
}
+static void
+table_features_init(struct ofputil_table_features *tf, int table_id);
Is it possible to avoid this forward declaration by re-ordering the
functions?
Possible, I'll rethink it, thanks.
+
+static void
+ofproto_init_max_table_features(struct ofproto *ofproto)
+{
+int i;
+struct ofputil_table_features *tf;
+
+for (i = 0; i < ofproto->n_tables; i++) {
+tf = &ofproto->otf[i];
+memset(tf, 0, sizeof(*tf));
+table_features_init(tf, i);
+}
+}
+
+static void
+ofproto_init_real_table_features(struct ofproto *ofproto)
+{
+int i;
+struct ofputil_table_features *tf;
+
+for (i = 0; i < ofproto->n_tables; i++) {
+tf = &ofproto->tables[i].tf;
+memset(tf, 0, sizeof(*tf));
+table_features_init(tf, i);
+}
+}
It seems to me that you could refactor ofproto_init_max_table_features()
and ofproto_init_real_table_features() into a single function paramatised
over the array of ofputil_table_features to use.
Yes, I do this before, but I think split them could improve the performance.
The ofproto->otf[0-255] are compact.
+
+static void
+ofproto_init_table_features(struct ofproto *ofproto)
+{
+/* init the max table features for get */
+ofproto_init_max_table_features(ofproto);
+
+/* set the real one */
+ofproto_init_real_table_features(ofproto);
+}
+
/* Must be called (only) by an ofproto implementation in its constructor
* function. See the large comment on 'construct' in struct ofproto_class for
* details. */
@@ -575,8 +614,274 @@ ofproto_init_tables(struct ofproto *ofproto, int n_tables)
OFPROTO_FOR_EACH_TABLE (table, ofproto) {
oftable_init(table);
}
+ofproto_init_table_features(ofproto);
+}
+
+static void
+table_instructions_init(struct ofputil_table_features *tf,
+enum ofputil_table_feature_prop_type type)
+{
+const int OFPIT13_END = OFPIT13_METER;
Can you just use OFPIT13_METER directly?
If not, I don't think its necessary or in keeping with the coding style
to make it const or capitalise the variable name.
Maybe OFPIT13_END is more meaningful?
I'll rethink it, thanks.
+int i = 0;
+int olen = 0;
+
+struct ofputil_instruction *inst = NULL;
+int element_size = sizeof(struct ofputil_instruction);
+
+olen = O
Re: [ovs-dev] [PATCH] debian: Option to create tunnel through 'interfaces'.
On Fri, Nov 1, 2013 at 3:13 AM, Vasiliy Tolstov wrote:
> Is that possible to add ability to bring up ovs interfaces in case it
> defined not in /etc/network/interfaces but using source xxx, for
> example i have
> /etc/network/interfaces:
> source /etc/network/interfaces.d/*
>
> and in /etc/network/interfaces.d/XXX i'm configure each interface.
It is not possible with the current code. It only looks at
/etc/network/interfaces
>
> 2013/10/31 Gurucharan Shetty :
>> On Thu, Oct 31, 2013 at 10:24 AM, Ben Pfaff wrote:
>>> On Thu, Oct 31, 2013 at 09:24:08AM -0700, Gurucharan Shetty wrote:
This is a port of commit 7b75828bf5654c (rhel: Option to create tunnel
through ifcfg scripts.) from rhel to debian's ifupdown script.
Signed-off-by: Gurucharan Shetty
>>>
>>> It looks like the first and later lines of this command are all indented
>>> the same amount, which makes it harder to pick it out visually as a
>>> single command:
>> I added the correct indentation.
+ovs_vsctl -- --may-exist add-port "${IF_OVS_BRIDGE}"\
+"${IFACE}" ${IF_OVS_OPTIONS} -- set Interface "${IFACE}" \
+type=${IF_OVS_TUNNEL_TYPE} ${IF_OVS_TUNNEL_OPTIONS} \
+${OVS_EXTRA+-- $OVS_EXTRA}
+;;
>>>
>>> Otherwise:
>>> Acked-by: Ben Pfaff
>> Thanks. I pushed this to master.
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>
>
> --
> Vasiliy Tolstov,
> e-mail: v.tols...@selfip.ru
> jabber: v...@selfip.ru
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/3] ofp-util: Enforce OpenFlow 1.1+ table_id requirements in flow_mod messages.
Signed-off-by: Ben Pfaff
---
lib/ofp-util.c | 21 +-
tests/learn.at |4 +--
tests/ofp-print.at | 12
tests/ovs-ofctl.at | 48 +++
utilities/ovs-ofctl.8.in | 71 +++---
5 files changed, 106 insertions(+), 50 deletions(-)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 3d9efab..e6a9494 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1530,7 +1530,19 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
}
fm->modify_cookie = false;
fm->command = ofm->command;
+
+/* Get table ID.
+ *
+ * OF1.1 entirely forbids table_id == 255.
+ * OF1.2+ allows table_id == 255 only for deletes. */
fm->table_id = ofm->table_id;
+if (fm->table_id == 0xff
+&& (oh->version == OFP11_VERSION
+|| (ofm->command != OFPFC_DELETE &&
+ofm->command != OFPFC_DELETE_STRICT))) {
+return OFPERR_OFPFMFC_BAD_TABLE_ID;
+}
+
fm->idle_timeout = ntohs(ofm->idle_timeout);
fm->hard_timeout = ntohs(ofm->hard_timeout);
fm->buffer_id = ntohl(ofm->buffer_id);
@@ -2056,7 +2068,14 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod
*fm,
ofm->cookie = fm->cookie;
}
ofm->cookie_mask = fm->cookie_mask;
-ofm->table_id = fm->table_id;
+if (fm->table_id != 255
+|| (protocol != OFPUTIL_P_OF11_STD
+&& (fm->command == OFPFC_DELETE ||
+fm->command == OFPFC_DELETE_STRICT))) {
+ofm->table_id = fm->table_id;
+} else {
+ofm->table_id = 0;
+}
ofm->command = fm->command;
ofm->idle_timeout = htons(fm->idle_timeout);
ofm->hard_timeout = htons(fm->hard_timeout);
diff --git a/tests/learn.at b/tests/learn.at
index 7a4bda5..491cd5e 100644
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -32,8 +32,8 @@ actions=learn(table=1, in_port=1,
load:OXM_OF_IN_PORT[]->NXM_NX_REG1[], load:0xf
AT_CHECK([ovs-ofctl -O OpenFlow12 parse-flows flows.txt], [0],
[[usable protocols: any
chosen protocol: OXM-OpenFlow12
-OFPT_FLOW_MOD (OF1.2) (xid=0x1): ADD table:255
actions=learn(table=1,output:OXM_OF_IN_PORT[])
-OFPT_FLOW_MOD (OF1.2) (xid=0x2): ADD table:255
actions=learn(table=1,in_port=1,load:OXM_OF_IN_PORT[]->NXM_NX_REG1[],load:0xfffe->OXM_OF_IN_PORT[])
+OFPT_FLOW_MOD (OF1.2) (xid=0x1): ADD
actions=learn(table=1,output:OXM_OF_IN_PORT[])
+OFPT_FLOW_MOD (OF1.2) (xid=0x2): ADD
actions=learn(table=1,in_port=1,load:OXM_OF_IN_PORT[]->NXM_NX_REG1[],load:0xfffe->OXM_OF_IN_PORT[])
]])
AT_CLEANUP
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index 52e6fef..8701f54 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -724,7 +724,7 @@ AT_SETUP([OFPT_FLOW_MOD - OF1.2 - low verbosity])
AT_KEYWORDS([ofp-print])
AT_CHECK([ovs-ofctl '-vPATTERN:console:%c|%p|%m' ofp-print "\
03 0e 00 90 00 00 00 02 00 00 00 00 00 00 00 00 \
-00 00 00 00 00 00 00 00 ff 00 00 00 00 00 ff ff \
+00 00 00 00 00 00 00 00 01 00 00 00 00 00 ff ff \
ff ff ff ff ff ff ff ff ff ff ff ff 00 00 00 00 \
00 01 00 42 80 00 00 04 00 00 00 01 80 00 08 06 \
50 54 00 00 00 06 80 00 06 06 50 54 00 00 00 05 \
@@ -733,7 +733,7 @@ ff ff ff ff ff ff ff ff ff ff ff ff 00 00 00 00 \
00 01 00 00 00 00 00 00 00 04 00 18 00 00 00 00 \
00 00 00 10 00 00 00 03 00 00 00 00 00 00 00 00 \
" 2], [0], [dnl
-OFPT_FLOW_MOD (OF1.2) (xid=0x2): ADD table:255
priority=65535,arp,in_port=1,vlan_tci=0x/0x1fff,dl_src=50:54:00:00:00:06,dl_dst=50:54:00:00:00:05,arp_spa=192.168.0.2,arp_tpa=192.168.0.1,arp_op=2
actions=output:3
+OFPT_FLOW_MOD (OF1.2) (xid=0x2): ADD table:1
priority=65535,arp,in_port=1,vlan_tci=0x/0x1fff,dl_src=50:54:00:00:00:06,dl_dst=50:54:00:00:00:05,arp_spa=192.168.0.2,arp_tpa=192.168.0.1,arp_op=2
actions=output:3
], [dnl
])
AT_CLEANUP
@@ -762,7 +762,7 @@ AT_SETUP([OFPT_FLOW_MOD - OF1.2 - low verbosity])
AT_KEYWORDS([ofp-print])
AT_CHECK([ovs-ofctl '-vPATTERN:console:%c|%p|%m' ofp-print "\
03 0e 00 90 00 00 00 02 00 00 00 00 00 00 00 00 \
-00 00 00 00 00 00 00 00 ff 00 00 00 00 00 ff ff \
+00 00 00 00 00 00 00 00 01 00 00 00 00 00 ff ff \
ff ff ff ff ff ff ff ff ff ff ff ff 00 00 00 00 \
00 01 00 42 80 00 00 04 00 00 00 01 80 00 08 06 \
50 54 00 00 00 06 80 00 06 06 50 54 00 00 00 05 \
@@ -771,7 +771,7 @@ ff ff ff ff ff ff ff ff ff ff ff ff 00 00 00 00 \
00 01 00 00 00 00 00 00 00 04 00 18 00 00 00 00 \
00 00 00 10 00 00 00 03 00 00 00 00 00 00 00 00 \
" 2], [0], [dnl
-OFPT_FLOW_MOD (OF1.2) (xid=0x2): ADD table:255
priority=65535,arp,in_port=1,vlan_tci=0x/0x1fff,dl_src=50:54:00:00:00:06,dl_dst=50:54:00:00:00:05,arp_spa=192.168.0.2,arp_tpa=192.168.0.1,arp_op=2
actions=output:3
+OFPT_FLOW_MOD (OF1.2) (xid=0x2): ADD table:1
priority=65535,arp,in_port=1,vlan_tci=0x/0x1fff,dl_src=50:54:00:00:00:06,dl_dst=50:54:00:00:00
[ovs-dev] [PATCH 3/3] ofproto: Centralize action checking, doing it at decode time.
Jarno pointed out that modify_flows__() didn't really need to check every
instance of the flow separately. After some further investigation I
decided that this was even more of an improvement.
CC: Jarno Rajahalme
Signed-off-by: Ben Pfaff
---
lib/ofp-actions.c | 23 ++-
lib/ofp-actions.h |5 +++--
lib/ofp-parse.c |5 +++--
lib/ofp-print.c |3 ++-
lib/ofp-util.c|7 +--
lib/ofp-util.h|4 +++-
ofproto/ofproto.c | 50 +++--
utilities/ovs-ofctl.c |5 +++--
8 files changed, 44 insertions(+), 58 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 6fa80a2..65e0437 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1565,8 +1565,9 @@ ofpact_check_output_port(ofp_port_t port, ofp_port_t
max_ports)
/* May modify flow->dl_type, caller must restore it. */
static enum ofperr
-ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
- uint8_t table_id, bool enforce_consistency)
+ofpact_check__(const struct ofpact *a, struct flow *flow,
+ bool enforce_consistency, ofp_port_t max_ports,
+ uint8_t table_id, uint8_t n_tables)
{
const struct ofpact_enqueue *enqueue;
@@ -1700,7 +1701,7 @@ ofpact_check__(const struct ofpact *a, struct flow *flow,
ofp_port_t max_ports,
case OFPACT_WRITE_ACTIONS: {
struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
- flow, max_ports, table_id, false);
+ flow, false, max_ports, table_id, n_tables);
}
case OFPACT_WRITE_METADATA:
@@ -1714,11 +1715,14 @@ ofpact_check__(const struct ofpact *a, struct flow
*flow, ofp_port_t max_ports,
return 0;
}
-case OFPACT_GOTO_TABLE:
-if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) {
+case OFPACT_GOTO_TABLE: {
+uint8_t goto_table = ofpact_get_GOTO_TABLE(a)->table_id;
+if ((table_id != 255 && goto_table <= table_id)
+|| (n_tables != 255 && goto_table >= n_tables)) {
return OFPERR_OFPBRC_BAD_TABLE_ID;
}
return 0;
+}
case OFPACT_GROUP:
return 0;
@@ -1741,16 +1745,17 @@ ofpact_check__(const struct ofpact *a, struct flow
*flow, ofp_port_t max_ports,
* May temporarily modify 'flow', but restores the changes before returning. */
enum ofperr
ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
- struct flow *flow, ofp_port_t max_ports, uint8_t table_id,
- bool enforce_consistency)
+ struct flow *flow, bool enforce_consistency,
+ ofp_port_t max_ports,
+ uint8_t table_id, uint8_t n_tables)
{
const struct ofpact *a;
ovs_be16 dl_type = flow->dl_type;
enum ofperr error = 0;
OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
-error = ofpact_check__(a, flow, max_ports, table_id,
- enforce_consistency);
+error = ofpact_check__(a, flow, enforce_consistency,
+ max_ports, table_id, n_tables);
if (error) {
break;
}
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index dd8f35f..2312121 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -564,8 +564,9 @@ enum ofperr ofpacts_pull_openflow11_instructions(struct
ofpbuf *openflow,
unsigned int instructions_len,
struct ofpbuf *ofpacts);
enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
- struct flow *, ofp_port_t max_ports,
- uint8_t table_id, bool enforce_consistency);
+ struct flow *, bool enforce_consistency,
+ ofp_port_t max_ports,
+ uint8_t table_id, uint8_t n_tables);
enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);
enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports);
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 757b971..0abd068 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1361,7 +1361,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command,
char *string,
enum ofperr err;
err = ofpacts_check(ofpacts.data, ofpacts.size, &fm->match.flow,
-OFPP_MAX, 0, true);
+true, OFPP_MAX, fm->table_id, 255);
if (err) {
if (!enforce_consistency &&
err == OFPERR_OFPBAC_MATCH_INCONSISTENT) {
@@ -1370,7 +1370,8 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command,
char *string,
/* Try again, allowing for inconsistency.
*
[ovs-dev] [PATCH 1/3] ofp-util: Move ofputil_check_output_port() to ofp-actions, rename.
This function is related to actions to ofp-actions seems like a logical
place for it.
Signed-off-by: Ben Pfaff
---
lib/bundle.c |2 +-
lib/ofp-actions.c | 33 +
lib/ofp-actions.h |1 +
lib/ofp-util.c| 25 -
lib/ofp-util.h|2 --
5 files changed, 31 insertions(+), 32 deletions(-)
diff --git a/lib/bundle.c b/lib/bundle.c
index dcabaaa..7d00f87 100644
--- a/lib/bundle.c
+++ b/lib/bundle.c
@@ -204,7 +204,7 @@ bundle_check(const struct ofpact_bundle *bundle, ofp_port_t
max_ports,
ofp_port_t ofp_port = bundle->slaves[i];
enum ofperr error;
-error = ofputil_check_output_port(ofp_port, max_ports);
+error = ofpact_check_output_port(ofp_port, max_ports);
if (error) {
VLOG_WARN_RL(&rl, "invalid slave %"PRIu16, ofp_port);
return error;
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index cccd6b1..6fa80a2 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -89,7 +89,7 @@ output_from_openflow10(const struct ofp10_action_output *oao,
output->port = u16_to_ofp(ntohs(oao->port));
output->max_len = ntohs(oao->max_len);
-return ofputil_check_output_port(output->port, OFPP_MAX);
+return ofpact_check_output_port(output->port, OFPP_MAX);
}
static enum ofperr
@@ -766,7 +766,7 @@ output_from_openflow11(const struct ofp11_action_output
*oao,
return error;
}
-return ofputil_check_output_port(output->port, OFPP_MAX);
+return ofpact_check_output_port(output->port, OFPP_MAX);
}
static enum ofperr
@@ -1538,6 +1538,31 @@ exit:
return error;
}
+/* Checks that 'port' is a valid output port for OFPACT_OUTPUT, given that the
+ * switch will never have more than 'max_ports' ports. Returns 0 if 'port' is
+ * valid, otherwise an OpenFlow error code. */
+enum ofperr
+ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports)
+{
+switch (port) {
+case OFPP_IN_PORT:
+case OFPP_TABLE:
+case OFPP_NORMAL:
+case OFPP_FLOOD:
+case OFPP_ALL:
+case OFPP_CONTROLLER:
+case OFPP_NONE:
+case OFPP_LOCAL:
+return 0;
+
+default:
+if (ofp_to_u16(port) < ofp_to_u16(max_ports)) {
+return 0;
+}
+return OFPERR_OFPBAC_BAD_OUT_PORT;
+}
+}
+
/* May modify flow->dl_type, caller must restore it. */
static enum ofperr
ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
@@ -1547,8 +1572,8 @@ ofpact_check__(const struct ofpact *a, struct flow *flow,
ofp_port_t max_ports,
switch (a->type) {
case OFPACT_OUTPUT:
-return ofputil_check_output_port(ofpact_get_OUTPUT(a)->port,
- max_ports);
+return ofpact_check_output_port(ofpact_get_OUTPUT(a)->port,
+max_ports);
case OFPACT_CONTROLLER:
return 0;
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index e097468..dd8f35f 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -567,6 +567,7 @@ enum ofperr ofpacts_check(const struct ofpact[], size_t
ofpacts_len,
struct flow *, ofp_port_t max_ports,
uint8_t table_id, bool enforce_consistency);
enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);
+enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports);
/* Converting ofpacts to OpenFlow. */
void ofpacts_put_openflow10(const struct ofpact[], size_t ofpacts_len,
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index d435e99..3d9efab 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4833,31 +4833,6 @@ ofputil_port_to_ofp11(ofp_port_t ofp10_port)
: ofp_to_u16(ofp10_port) + OFPP11_OFFSET);
}
-/* Checks that 'port' is a valid output port for the OFPAT10_OUTPUT action,
given
- * that the switch will never have more than 'max_ports' ports. Returns 0 if
- * 'port' is valid, otherwise an OpenFlow return code. */
-enum ofperr
-ofputil_check_output_port(ofp_port_t port, ofp_port_t max_ports)
-{
-switch (port) {
-case OFPP_IN_PORT:
-case OFPP_TABLE:
-case OFPP_NORMAL:
-case OFPP_FLOOD:
-case OFPP_ALL:
-case OFPP_CONTROLLER:
-case OFPP_NONE:
-case OFPP_LOCAL:
-return 0;
-
-default:
-if (ofp_to_u16(port) < ofp_to_u16(max_ports)) {
-return 0;
-}
-return OFPERR_OFPBAC_BAD_OUT_PORT;
-}
-}
-
#define OFPUTIL_NAMED_PORTS \
OFPUTIL_NAMED_PORT(IN_PORT) \
OFPUTIL_NAMED_PORT(TABLE) \
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index c37ab2b..a77c301 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -37,8 +37,6 @@ enum ofperr ofputil_port_from_ofp11(ovs_be32 ofp11_port,
ofp_port_t *ofp10_port);
ovs_be32 ofputil_port_to_ofp11(ofp_port_t ofp10_port);
-enum ofperr ofputil
Re: [ovs-dev] [PATCH] ofproto: Avoid abandoning an ofopgroup without committing it.
On Fri, Oct 25, 2013 at 02:25:10PM -0700, Ben Pfaff wrote:
> On Fri, Oct 25, 2013 at 02:04:44PM -0700, Jarno Rajahalme wrote:
> >
> > On Oct 25, 2013, at 1:56 PM, Ben Pfaff wrote:
> >
> > > On Fri, Oct 25, 2013 at 01:44:17PM -0700, Jarno Rajahalme wrote:
> > >>
> > >> On Oct 25, 2013, at 1:32 PM, Ben Pfaff wrote:
> > >>
> > >>> On Thu, Oct 24, 2013 at 02:08:24PM -0700, Jarno Rajahalme wrote:
> >
> >
> > On Oct 21, 2013, at 3:52 PM, Ben Pfaff wrote:
> >
> > > Commit e3b5693319c (Fix table checking for goto table instruction.)
> > > moved
> > > action checking into modify_flows__(), for good reason, but as a side
> > > effect made modify_flows__() abandon and never commit the ofopgroup
> > > that it
> > > started, if action checking failed. This commit fixes the problem.
> > >
> > > The following commands, run under "make sandbox", illustrate the
> > > problem.
> > > Without this change, the final command hangs because the barrier
> > > request
> > > that ovs-ofctl sends never gets a response (because barriers wait for
> > > all
> > > ofopgroups to complete, which never happens). With this commit, the
> > > commands complete quickly:
> > >
> > > ovs-vsctl add-br br0
> > > ovs-vsctl set bridge br0
> > > protocols=OpenFlow10,OpenFlow11,OpenFlow12,OpenFlow13
> > > ovs-ofctl add-flow -O OpenFlow11 br0
> > > table=1,action=mod_tp_dst:79,goto_table:2
> > > ovs-ofctl add-flow -O OpenFlow11 br0
> > > table=1,action=mod_tp_dst:79,goto_table:1
> > >
> > > Reported-by: Jarno Rajahalme
> > > Signed-off-by: Ben Pfaff
> > > ---
> > > ofproto/ofproto.c | 19 ---
> > > tests/ofproto.at | 26 ++
> > > 2 files changed, 38 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > > index f67e1fb..8dba732 100644
> > > --- a/ofproto/ofproto.c
> > > +++ b/ofproto/ofproto.c
> > > @@ -4041,6 +4041,18 @@ modify_flows__(struct ofproto *ofproto, struct
> > > ofconn *ofconn,
> > > enum ofperr error;
> > > size_t i;
> > >
> > > +/* Verify actions before we start to modify any rules, to avoid
> > > partial
> > > + * flow table modifications. */
> > > +for (i = 0; i < rules->n; i++) {
> > > +struct rule *rule = rules->rules[i];
> > > +
> > > +error = ofpacts_check(fm->ofpacts, fm->ofpacts_len,
> > > &fm->match.flow,
> > > + u16_to_ofp(ofproto->max_ports),
> > > rule->table_id);
> > > +if (error) {
> > > +return error;
> > > +}
> > > +}
> > > +
> >
> > This fixes the problem I had, thank you!
> >
> > While we are at this, we should use ofproto_check_ofpacts() instead
> > and maybe avoid repeating the same check over and over again. How
> > about this incremental:
> > >>>
> > >>> Can we really avoid repeating the check? Since I proposed this
> > >>> change, ofpacts_check() now checks consistency of the flow and the
> > >>> actions, and since the flows vary among the rules that we are
> > >>> checking, I imagine that some of them could be inconsistent within a
> > >>> single table, even if others are not.
> > >>>
> > >>
> > >> It seems to me that we are checking the new actions against the new flow
> > >> (both from the new flow mod) in the context of the old rule's table_id,
> > >> i.e.
> > >> the check calls do not really vary by the rule (other than rule's table
> > >> id) at all.
> > >
> > > I don't understand yet.
> > >
> > > Let me provide an example. Suppose we do a flow_mod that changes all
> > > of the actions in table 0 from whatever they were previously to
> > > "mod_tp_src:80". If the first rule whose change we validate in that
> > > table satisfies the prerequisites for mod_tp_src, but other rules in
> > > the table do not satisfy the prerequisites, then I think that we would
> > > allow the flow_mod to go through without noticing the problem.
> >
> > But the old rule is never passed for the check, see:
> >
> >error = ofpacts_check(fm->ofpacts, fm->ofpacts_len, &fm->match.flow,
> > u16_to_ofp(ofproto->max_ports),
> > rule->table_id);
> >
> > The flow mod comes in with the flow (&fm->match.flow) so the exact
> > same validation is being repeated over and over again, if the
> > rule->table_id remains the same.
>
> OK, I understand now and agree.
>
> I have further thoughts here that I'm going to investigate before I
> apply your incremental (as a new patch).
I finally finished polishing my alternative version. Please take a look
at this series when you get a chance:
http://openvswitch.org/pipermail/dev/2013-November/033478.html
http://openvswitch.org/pipermail/dev/2013-November/033479.html
[ovs-dev] [PATCH] ovsdb: Do not leak error in ovsdb_server_compact()
Signed-off-by: Thomas Graf --- ovsdb/ovsdb-server.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c index ab44b3a..f64122e 100644 --- a/ovsdb/ovsdb-server.c +++ b/ovsdb/ovsdb-server.c @@ -983,6 +983,7 @@ ovsdb_server_compact(struct unixctl_conn *conn, int argc, char *s = ovsdb_error_to_string(error); ds_put_format(&reply, "%s\n", s); free(s); +ovsdb_error_destroy(error); } n++; -- 1.8.3.1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v8 00/16] Enhanced Group Support
On Wed, Oct 30, 2013 at 06:17:04PM +0900, Simon Horman wrote: > With these patches in place it is possible to use all group types > defined in OpenFlow1.3. I applied patches 1-4 to master. I haven't had a chance to look at the rest yet. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovsdb: Do not leak error in ovsdb_server_compact()
On Fri, Nov 01, 2013 at 05:44:52PM +0100, Thomas Graf wrote: > Signed-off-by: Thomas Graf Thanks, applied to master, branch-2.0, branch-1.11, branch-1.10, branch-1.9. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] vtep: add fault notifications to physical switch and ports
Add a "switch_fault" and "port_fault" column to the appropriate
tables in the VTEP schema.
Signed-off-by: Bruce Davie
---
vtep/vtep.ovsschema | 16
vtep/vtep.xml | 48 ++--
2 files changed, 58 insertions(+), 6 deletions(-)
diff --git a/vtep/vtep.ovsschema b/vtep/vtep.ovsschema
index d03d96d..017183a 100644
--- a/vtep/vtep.ovsschema
+++ b/vtep/vtep.ovsschema
@@ -1,6 +1,6 @@
{
"name": "hardware_vtep",
- "cksum": "825115144 5318",
+ "cksum": "1365749839 5604",
"tables": {
"Global": {
"columns": {
@@ -24,7 +24,11 @@
"management_ips": {
"type": {"key": {"type": "string"}, "min": 0, "max": "unlimited"}},
"tunnel_ips": {
- "type": {"key": {"type": "string"}, "min": 0, "max": "unlimited"}}},
+ "type": {"key": {"type": "string"}, "min": 0, "max": "unlimited"}},
+"switch_fault_status": {
+ "type": {
+"key": "string", "min": 0, "max": "unlimited"},
+ "ephemeral": true}},
"indexes": [["name"]]},
"Physical_Port": {
"columns": {
@@ -40,7 +44,11 @@
"minInteger": 0, "maxInteger": 4095},
"value": {"type": "uuid",
"refTable": "Logical_Binding_Stats"},
- "min": 0, "max": "unlimited",
+ "min": 0, "max": "unlimited"}},
+"port_fault_status": {
+ "type": {
+"key": "string", "min": 0, "max": "unlimited"},
+ "ephemeral": true}}},
"Logical_Binding_Stats": {
"columns": {
"bytes_from_local": {"type": "integer"},
@@ -154,4 +162,4 @@
"ephemeral": true}},
"indexes": [["target"]],
"isRoot": false}},
- "version": "1.0.0"}
+ "version": "1.1.0"}
diff --git a/vtep/vtep.xml b/vtep/vtep.xml
index 3940479..fbf68ca 100644
--- a/vtep/vtep.xml
+++ b/vtep/vtep.xml
@@ -274,6 +274,29 @@
banner.
+
+
+ An entry in this column indicates to the NVC that this switch
+ has encountered a fault. The switch must clear this column
+ when the fault has been cleared.
+
+
+
+Indicates that the switch has been unable to process MAC
+entries requested by the NVC due to lack of table resources.
+
+
+
+Indicates that the switch has been unable to create tunnels
+requested by the NVC due to lack of resources.
+
+
+
+Indicates that an error has occurred in the switch but that no
+more specific information is available.
+
+
+
@@ -306,6 +329,27 @@
An extended description for the port.
+
+
+ An entry in this column indicates to the NVC that the physical port has
+ encountered a fault. The switch must clear this column when the errror
+ has been cleared.
+
+
+
+ Indicates that a VLAN-to-logical-switch mapping requested by
+ the controller could not be instantiated by the switch
+ because of a conflict with local configuration.
+
+
+
+
+ Indicates that an error has occurred on the port but that no
+ more specific information is available.
+
+
+
+
@@ -453,7 +497,7 @@
- A MAC address that has been learned by the NSC.
+ A MAC address that has been learned by the NVC.
@@ -526,7 +570,7 @@
- A MAC address that has been learned by the NSC.
+ A MAC address that has been learned by the NVC.
The keyword unknown-dst is used as a special
--
1.8.0.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v1 1/6] dpif-netdev: Change a variable name.
'struct dp_netdev_flow' is currently being instantiated as 'flow'.
An upcoming commit introduces a classifier to dpif-netdev
which uses 'struct flow' at a few places and that can cause
confusion while reading code.
Signed-off-by: Gurucharan Shetty
---
lib/dpif-netdev.c | 135 -
1 file changed, 72 insertions(+), 63 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index d3ac370..1b0039c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -624,20 +624,20 @@ dpif_netdev_get_max_ports(const struct dpif *dpif
OVS_UNUSED)
}
static void
-dp_netdev_free_flow(struct dp_netdev *dp, struct dp_netdev_flow *flow)
+dp_netdev_free_flow(struct dp_netdev *dp, struct dp_netdev_flow *netdev_flow)
{
-hmap_remove(&dp->flow_table, &flow->node);
-free(flow->actions);
-free(flow);
+hmap_remove(&dp->flow_table, &netdev_flow->node);
+free(netdev_flow->actions);
+free(netdev_flow);
}
static void
dp_netdev_flow_flush(struct dp_netdev *dp)
{
-struct dp_netdev_flow *flow, *next;
+struct dp_netdev_flow *netdev_flow, *next;
-HMAP_FOR_EACH_SAFE (flow, next, node, &dp->flow_table) {
-dp_netdev_free_flow(dp, flow);
+HMAP_FOR_EACH_SAFE (netdev_flow, next, node, &dp->flow_table) {
+dp_netdev_free_flow(dp, netdev_flow);
}
}
@@ -736,23 +736,25 @@ dpif_netdev_port_poll_wait(const struct dpif *dpif_)
static struct dp_netdev_flow *
dp_netdev_lookup_flow(const struct dp_netdev *dp, const struct flow *key)
{
-struct dp_netdev_flow *flow;
+struct dp_netdev_flow *netdev_flow;
-HMAP_FOR_EACH_WITH_HASH (flow, node, flow_hash(key, 0), &dp->flow_table) {
-if (flow_equal(&flow->key, key)) {
-return flow;
+HMAP_FOR_EACH_WITH_HASH (netdev_flow, node, flow_hash(key, 0),
+ &dp->flow_table) {
+if (flow_equal(&netdev_flow->key, key)) {
+return netdev_flow;
}
}
return NULL;
}
static void
-get_dpif_flow_stats(struct dp_netdev_flow *flow, struct dpif_flow_stats *stats)
+get_dpif_flow_stats(struct dp_netdev_flow *netdev_flow,
+struct dpif_flow_stats *stats)
{
-stats->n_packets = flow->packet_count;
-stats->n_bytes = flow->byte_count;
-stats->used = flow->used;
-stats->tcp_flags = flow->tcp_flags;
+stats->n_packets = netdev_flow->packet_count;
+stats->n_bytes = netdev_flow->byte_count;
+stats->used = netdev_flow->used;
+stats->tcp_flags = netdev_flow->tcp_flags;
}
static int
@@ -794,7 +796,7 @@ dpif_netdev_flow_get(const struct dpif *dpif,
struct ofpbuf **actionsp, struct dpif_flow_stats *stats)
{
struct dp_netdev *dp = get_dp_netdev(dpif);
-struct dp_netdev_flow *flow;
+struct dp_netdev_flow *netdev_flow;
struct flow key;
int error;
@@ -804,13 +806,14 @@ dpif_netdev_flow_get(const struct dpif *dpif,
}
ovs_mutex_lock(&dp_netdev_mutex);
-flow = dp_netdev_lookup_flow(dp, &key);
-if (flow) {
+netdev_flow = dp_netdev_lookup_flow(dp, &key);
+if (netdev_flow) {
if (stats) {
-get_dpif_flow_stats(flow, stats);
+get_dpif_flow_stats(netdev_flow, stats);
}
if (actionsp) {
-*actionsp = ofpbuf_clone_data(flow->actions, flow->actions_len);
+*actionsp = ofpbuf_clone_data(netdev_flow->actions,
+ netdev_flow->actions_len);
}
} else {
error = ENOENT;
@@ -821,12 +824,12 @@ dpif_netdev_flow_get(const struct dpif *dpif,
}
static int
-set_flow_actions(struct dp_netdev_flow *flow,
+set_flow_actions(struct dp_netdev_flow *netdev_flow,
const struct nlattr *actions, size_t actions_len)
{
-flow->actions = xrealloc(flow->actions, actions_len);
-flow->actions_len = actions_len;
-memcpy(flow->actions, actions, actions_len);
+netdev_flow->actions = xrealloc(netdev_flow->actions, actions_len);
+netdev_flow->actions_len = actions_len;
+memcpy(netdev_flow->actions, actions, actions_len);
return 0;
}
@@ -834,36 +837,37 @@ static int
dp_netdev_flow_add(struct dp_netdev *dp, const struct flow *key,
const struct nlattr *actions, size_t actions_len)
{
-struct dp_netdev_flow *flow;
+struct dp_netdev_flow *netdev_flow;
int error;
-flow = xzalloc(sizeof *flow);
-flow->key = *key;
+netdev_flow = xzalloc(sizeof *netdev_flow);
+netdev_flow->key = *key;
-error = set_flow_actions(flow, actions, actions_len);
+error = set_flow_actions(netdev_flow, actions, actions_len);
if (error) {
-free(flow);
+free(netdev_flow);
return error;
}
-hmap_insert(&dp->flow_table, &flow->node, flow_hash(&flow->key, 0));
+hmap_insert(&dp->flow_table, &netdev_flow->node,
+flow_hash(&netdev_flow->key, 0));
return 0;
}
static
[ovs-dev] [PATCH v1 2/6] dpif-netdev: Introduce a classifier in userspace datapath.
Instead of an exact match flow table, we introduce a classifier.
This enables mega-flows in userspace datapath.
Signed-off-by: Gurucharan Shetty
---
lib/dpif-netdev.c | 87 ++
tests/ofproto-dpif.at | 142 +
2 files changed, 136 insertions(+), 93 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1b0039c..ea8a6d3 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -31,6 +31,7 @@
#include
#include
+#include "classifier.h"
#include "csum.h"
#include "dpif.h"
#include "dpif-provider.h"
@@ -59,6 +60,9 @@
VLOG_DEFINE_THIS_MODULE(dpif_netdev);
+/* By default, choose a priority in the middle. */
+#define NETDEV_DEFAULT_PRIORITY 0x8000
+
/* Configuration parameters. */
enum { MAX_PORTS = 256 }; /* Maximum number of ports. */
enum { MAX_FLOWS = 65536 }; /* Maximum number of flows in flow table. */
@@ -92,6 +96,7 @@ struct dp_netdev {
int max_mtu;/* Maximum MTU of any port added so far. */
struct dp_netdev_queue queues[N_QUEUES];
+struct classifier cls; /* Classifier. */
struct hmap flow_table; /* Flow table. */
struct seq *queue_seq; /* Incremented whenever a packet is queued. */
@@ -119,8 +124,10 @@ struct dp_netdev_port {
/* A flow in dp_netdev's 'flow_table'. */
struct dp_netdev_flow {
struct hmap_node node; /* Element in dp_netdev's 'flow_table'. */
-struct flow key;
-
+struct flow flow; /* The flow that created this entry. */
+struct flow mask; /* The mask accompanying the 'flow'. */
+struct cls_rule cr; /* The rule created for the 'flow' and inserted
+ into dp_netdev's classifier. */
/* Statistics. */
long long int used; /* Last used time, in monotonic msecs. */
long long int packet_count; /* Number of packets matched. */
@@ -283,6 +290,7 @@ create_dp_netdev(const char *name, const struct dpif_class
*class,
dp->queues[i].head = dp->queues[i].tail = 0;
}
dp->queue_seq = seq_create();
+classifier_init(&dp->cls);
hmap_init(&dp->flow_table);
list_init(&dp->port_list);
dp->port_seq = seq_create();
@@ -349,6 +357,7 @@ dp_netdev_free(struct dp_netdev *dp)
}
dp_netdev_purge_queues(dp);
seq_destroy(dp->queue_seq);
+classifier_destroy(&dp->cls);
hmap_destroy(&dp->flow_table);
seq_destroy(dp->port_seq);
free(dp->name);
@@ -626,6 +635,7 @@ dpif_netdev_get_max_ports(const struct dpif *dpif
OVS_UNUSED)
static void
dp_netdev_free_flow(struct dp_netdev *dp, struct dp_netdev_flow *netdev_flow)
{
+classifier_remove(&dp->cls, &netdev_flow->cr);
hmap_remove(&dp->flow_table, &netdev_flow->node);
free(netdev_flow->actions);
free(netdev_flow);
@@ -734,13 +744,19 @@ dpif_netdev_port_poll_wait(const struct dpif *dpif_)
}
static struct dp_netdev_flow *
-dp_netdev_lookup_flow(const struct dp_netdev *dp, const struct flow *key)
+dp_netdev_lookup_flow(const struct dp_netdev *dp, const struct flow *flow)
{
struct dp_netdev_flow *netdev_flow;
+struct cls_rule *cr;
+
+cr = classifier_lookup(&dp->cls, flow, NULL);
+if (!cr) {
+return NULL;
+}
-HMAP_FOR_EACH_WITH_HASH (netdev_flow, node, flow_hash(key, 0),
+HMAP_FOR_EACH_WITH_HASH (netdev_flow, node, cls_rule_hash(cr, 0),
&dp->flow_table) {
-if (flow_equal(&netdev_flow->key, key)) {
+if (cls_rule_equal(&netdev_flow->cr, cr)) {
return netdev_flow;
}
}
@@ -758,23 +774,28 @@ get_dpif_flow_stats(struct dp_netdev_flow *netdev_flow,
}
static int
-dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
- struct flow *flow)
+dpif_netdev_flow_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
+ const struct nlattr *mask_key,
+ uint32_t mask_key_len, struct flow *flow,
+ struct flow *mask)
{
odp_port_t in_port;
-if (odp_flow_key_to_flow(key, key_len, flow) != ODP_FIT_PERFECT) {
+if (odp_flow_key_to_flow(key, key_len, flow) || (mask_key &&
+odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) {
/* This should not happen: it indicates that odp_flow_key_from_flow()
- * and odp_flow_key_to_flow() disagree on the acceptable form of a
- * flow. Log the problem as an error, with enough details to enable
- * debugging. */
+ * and odp_flow_key_to_flow() disagree on the acceptable form of a flow
+ * or odp_flow_key_from_mask() and odp_flow_key_to_mask() disagree on
+ * the acceptable form of a mask. Log the problem as an error, with
+ * enough details to enable debugging. */
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5)
[ovs-dev] [PATCH v1 4/6] ofproto-dpif: Unixctl command to dump netdev flows.
Right now, when we want to look at the flows of netdev
datapath, we use the dpif/dump-flows command which provides
it without querying the netdev datapath but rather through the
facets and subfacets.
This commit adds the ability to directly query the netdev
datapath through the dpif interface.
Signed-off-by: Gurucharan Shetty
---
ofproto/ofproto-dpif.c | 42 ++
vswitchd/ovs-vswitchd.8.in |5 +
2 files changed, 47 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ff1c74d..1a53766 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5713,6 +5713,46 @@ ofproto_unixctl_dpif_del_flows(struct unixctl_conn *conn,
}
static void
+ofproto_unixctl_netdev_dump_flows(struct unixctl_conn *conn,
+ int argc OVS_UNUSED,
+ const char *argv[] OVS_UNUSED,
+ void *aux OVS_UNUSED)
+{
+struct dpif *dpif;
+struct dpif_flow_dump flow_dump;
+const struct dpif_flow_stats *stats;
+const struct nlattr *key, *mask, *actions;
+size_t key_len, mask_len, actions_len;
+struct ds ds;
+int error;
+
+/* We only have one backer for dpif-netdev. So just use it. */
+error = dpif_open("ovs-netdev", "netdev", &dpif);
+if (error) {
+unixctl_command_reply_error(conn, "No netdev backer found.");
+return;
+}
+
+ds_init(&ds);
+dpif_flow_dump_start(&flow_dump, dpif);
+while (dpif_flow_dump_next(&flow_dump, &key, &key_len,
+ &mask, &mask_len,
+ &actions, &actions_len, &stats)) {
+odp_flow_format(key, key_len, mask, mask_len, NULL, &ds, true);
+ds_put_cstr(&ds, ", ");
+
+dpif_flow_stats_format(stats, &ds);
+ds_put_cstr(&ds, ", actions:");
+format_odp_actions(&ds, actions, actions_len);
+ds_put_cstr(&ds, "\n");
+}
+dpif_flow_dump_done(&flow_dump);
+dpif_close(dpif);
+unixctl_command_reply(conn, ds_cstr(&ds));
+ds_destroy(&ds);
+}
+
+static void
ofproto_dpif_unixctl_init(void)
{
static bool registered;
@@ -5745,6 +5785,8 @@ ofproto_dpif_unixctl_init(void)
ofproto_unixctl_dpif_disable_megaflows, NULL);
unixctl_command_register("dpif/enable-megaflows", "", 0, 0,
ofproto_unixctl_dpif_enable_megaflows, NULL);
+unixctl_command_register("netdev/dump-flows", "", 0, 0,
+ ofproto_unixctl_netdev_dump_flows, NULL);
}
/* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
index 4fb3ae3..c72093f 100644
--- a/vswitchd/ovs-vswitchd.8.in
+++ b/vswitchd/ovs-vswitchd.8.in
@@ -212,6 +212,11 @@ information, and partner information. If \fIport\fR is
not specified,
then displays detailed information about all interfaces with CFM
enabled.
.
+.SS "NETDEV COMMANDS"
+These commands manage the userspace datapath.
+.IP "\fBnetdev/dump-flows\fR"
+Prints to the console all flow entries in the userspace datapath's flow table.
+.
.so ofproto/ofproto-dpif-unixctl.man
.so ofproto/ofproto-unixctl.man
.so lib/vlog-unixctl.man
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v1 3/6] dpif-netdev: Flow dump function should populate masks.
Signed-off-by: Gurucharan Shetty
---
lib/dpif-netdev.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index ea8a6d3..ae1fecd 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -987,6 +987,7 @@ struct dp_netdev_flow_state {
uint32_t offset;
struct nlattr *actions;
struct odputil_keybuf keybuf;
+struct odputil_keybuf maskbuf;
struct dpif_flow_stats stats;
};
@@ -1034,9 +1035,15 @@ dpif_netdev_flow_dump_next(const struct dpif *dpif, void
*state_,
*key_len = buf.size;
}
-if (mask) {
-*mask = NULL;
-*mask_len = 0;
+if (key && mask) {
+struct ofpbuf buf;
+
+ofpbuf_use_stack(&buf, &state->maskbuf, sizeof state->maskbuf);
+odp_flow_key_from_mask(&buf, &netdev_flow->mask, &netdev_flow->flow,
+ UINT32_MAX);
+
+*mask = buf.data;
+*mask_len = buf.size;
}
if (actions) {
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v1 6/6] ofproto-dpif: Unixctl command to print userspace datapath summary.
Signed-off-by: Gurucharan Shetty
---
ofproto/ofproto-dpif.c | 26 ++
vswitchd/ovs-vswitchd.8.in |7 ++-
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 1a53766..2d6ea61 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5713,6 +5713,30 @@ ofproto_unixctl_dpif_del_flows(struct unixctl_conn *conn,
}
static void
+ofproto_unixctl_netdev_show(struct unixctl_conn *conn,
+int argc OVS_UNUSED,
+const char *argv[] OVS_UNUSED,
+void *aux OVS_UNUSED)
+{
+struct dpif *dpif;
+struct ds ds;
+int error;
+
+/* We only have one backer for dpif-netdev. So just use it. */
+error = dpif_open("ovs-netdev", "netdev", &dpif);
+if (error) {
+unixctl_command_reply_error(conn, "No netdev backer found.");
+return;
+}
+
+ds_init(&ds);
+dpif_show(dpif, &ds, false);
+
+unixctl_command_reply(conn, ds_cstr(&ds));
+ds_destroy(&ds);
+}
+
+static void
ofproto_unixctl_netdev_dump_flows(struct unixctl_conn *conn,
int argc OVS_UNUSED,
const char *argv[] OVS_UNUSED,
@@ -5785,6 +5809,8 @@ ofproto_dpif_unixctl_init(void)
ofproto_unixctl_dpif_disable_megaflows, NULL);
unixctl_command_register("dpif/enable-megaflows", "", 0, 0,
ofproto_unixctl_dpif_enable_megaflows, NULL);
+unixctl_command_register("netdev/show", "", 0, 0,
+ ofproto_unixctl_netdev_show, NULL);
unixctl_command_register("netdev/dump-flows", "", 0, 0,
ofproto_unixctl_netdev_dump_flows, NULL);
}
diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
index c72093f..a370dd9 100644
--- a/vswitchd/ovs-vswitchd.8.in
+++ b/vswitchd/ovs-vswitchd.8.in
@@ -215,7 +215,12 @@ enabled.
.SS "NETDEV COMMANDS"
These commands manage the userspace datapath.
.IP "\fBnetdev/dump-flows\fR"
-Prints to the console all flow entries in the userspace datapath's flow table.
+Prints to the console all the flow entries in the userspace datapath's flow
+table.
+.
+.IP "\fBnetdev/show\fR"
+Prints to the console a summary of the configured datapath, flow lookup stats
+and a list of ports connected to the datapath.
.
.so ofproto/ofproto-dpif-unixctl.man
.so ofproto/ofproto-unixctl.man
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v1 5/6] ovs-dpctl: Make 'show_dpif' more general.
Currently, with 'ovs-dpctl show', we call show_dpif().
Move this functionality to lib/dpif.c so that it can
be used by an upcoming commit.
Signed-off-by: Gurucharan Shetty
---
lib/dpif.c| 136 +++
lib/dpif.h|1 +
utilities/ovs-dpctl.c | 141 +++--
3 files changed, 145 insertions(+), 133 deletions(-)
diff --git a/lib/dpif.c b/lib/dpif.c
index b284e13..6275570 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -37,6 +37,7 @@
#include "packets.h"
#include "poll-loop.h"
#include "shash.h"
+#include "smap.h"
#include "sset.h"
#include "timeval.h"
#include "util.h"
@@ -474,6 +475,141 @@ dpif_get_dp_stats(const struct dpif *dpif, struct
dpif_dp_stats *stats)
return error;
}
+static void
+print_stat(struct ds *ds, const char *leader, uint64_t value)
+{
+ds_put_cstr(ds, leader);
+if (value != UINT64_MAX) {
+ds_put_format(ds, "%"PRIu64, value);
+} else {
+ds_put_cstr(ds, "?");
+}
+}
+
+static void
+print_human_size(struct ds *ds, uint64_t value)
+{
+if (value == UINT64_MAX) {
+/* Nothing to do. */
+} else if (value >= 1024ULL * 1024 * 1024 * 1024) {
+ds_put_format(ds, " (%.1f TiB)",
+ value / (1024.0 * 1024 * 1024 * 1024));
+} else if (value >= 1024ULL * 1024 * 1024) {
+ds_put_format(ds, " (%.1f GiB)", value / (1024.0 * 1024 * 1024));
+} else if (value >= 1024ULL * 1024) {
+ds_put_format(ds, " (%.1f MiB)", value / (1024.0 * 1024));
+} else if (value >= 1024) {
+ds_put_format(ds, " (%.1f KiB)", value / 1024.0);
+}
+}
+
+void
+dpif_show(struct dpif *dpif, struct ds *ds, bool print_statistics)
+{
+struct dpif_port_dump dump;
+struct dpif_port dpif_port;
+struct dpif_dp_stats stats;
+struct netdev *netdev;
+
+ds_put_format(ds, "%s:\n", dpif_name(dpif));
+if (!dpif_get_dp_stats(dpif, &stats)) {
+ds_put_format(ds, "\tlookups: hit:%"PRIu64" missed:%"PRIu64""
+ " lost:%"PRIu64"\n\tflows: %"PRIu64"\n",
+ stats.n_hit, stats.n_missed, stats.n_lost,
+ stats.n_flows);
+if (stats.n_masks != UINT64_MAX) {
+uint64_t n_pkts = stats.n_hit + stats.n_missed;
+double avg = n_pkts ? (double) stats.n_mask_hit / n_pkts : 0.0;
+
+ds_put_format(ds, "\tmasks: hit:%"PRIu64" total:%"PRIu64" "
+ "hit/pkt:%.2f\n",
+ stats.n_mask_hit, stats.n_masks, avg);
+}
+}
+
+DPIF_PORT_FOR_EACH (&dpif_port, &dump, dpif) {
+ds_put_format(ds, "\tport %u: %s", dpif_port.port_no, dpif_port.name);
+
+if (strcmp(dpif_port.type, "system")) {
+int error;
+
+ds_put_format(ds, " (%s", dpif_port.type);
+
+error = netdev_open(dpif_port.name, dpif_port.type, &netdev);
+if (!error) {
+struct smap config;
+
+smap_init(&config);
+error = netdev_get_config(netdev, &config);
+if (!error) {
+const struct smap_node **nodes;
+size_t i;
+
+nodes = smap_sort(&config);
+for (i = 0; i < smap_count(&config); i++) {
+const struct smap_node *node = nodes[i];
+ds_put_format(ds, "%c %s=%s", i ? ',' : ':',
+ node->key, node->value);
+}
+free(nodes);
+} else {
+ds_put_format(ds, ", could not retrieve configuration"
+ " (%s)", ovs_strerror(error));
+}
+smap_destroy(&config);
+
+netdev_close(netdev);
+} else {
+ds_put_format(ds, ": open failed (%s)", ovs_strerror(error));
+}
+ds_put_cstr(ds, ")");
+}
+ds_put_cstr(ds, "\n");
+
+if (print_statistics) {
+struct netdev_stats s;
+int error;
+
+error = netdev_open(dpif_port.name, dpif_port.type, &netdev);
+if (error) {
+ds_put_format(ds, ", open failed (%s)", ovs_strerror(error));
+continue;
+}
+error = netdev_get_stats(netdev, &s);
+if (error) {
+ds_put_format(ds, ", could not retrieve stats (%s)",
+ ovs_strerror(error));
+continue;
+}
+
+netdev_close(netdev);
+print_stat(ds, "\t\tRX packets:", s.rx_packets);
+print_stat(ds, " errors:", s.rx_errors);
+print_stat(ds, " dropped:", s.rx_dropped);
+print_stat(ds, " overruns:", s.rx_over_errors);
+print_stat(ds, " frame:", s.rx_frame_errors);
+ds_put_cstr(ds, "\n");
+
+
Re: [ovs-dev] [PATCH v1 1/6] dpif-netdev: Change a variable name.
On Fri, Nov 01, 2013 at 02:15:12AM -0700, Gurucharan Shetty wrote: > 'struct dp_netdev_flow' is currently being instantiated as 'flow'. > An upcoming commit introduces a classifier to dpif-netdev > which uses 'struct flow' at a few places and that can cause > confusion while reading code. > > Signed-off-by: Gurucharan Shetty Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] netdev-linux: Skip miimon execution when disabled
When dealing with a large number of ports, one of the performance
bottlenecks is that we loop through all netdevs in the main loop. Miimon
is a contributor to this, executing even if it is not enabled on any
devices.
This patch introduces a counter for the number of netdevs with miimon
configured. If this is 0, then we skip miimon_run() and miimon_wait().
In a test environment of 5000 internal ports and 50 tunnel ports with
bfd, this reduces CPU usage from about 50% to about 45%.
Signed-off-by: Joe Stringer
---
lib/netdev-linux.c | 26 --
vswitchd/bridge.c |2 ++
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 2752623..82e1358 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -61,6 +61,7 @@
#include "netlink.h"
#include "ofpbuf.h"
#include "openflow/openflow.h"
+#include "ovs-atomic.h"
#include "packets.h"
#include "poll-loop.h"
#include "rtnetlink-link.h"
@@ -402,6 +403,11 @@ struct netdev_rx_linux {
* additional log messages. */
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+/* Polling miimon status for all ports causes performance degradation when
+ * handling a large number of ports. If there are no devices using miimon, then
+ * we skip netdev_linux_miimon_run() and netdev_linux_miimon_wait(). */
+static atomic_int miimon_cnt = ATOMIC_VAR_INIT(0);
+
static void netdev_linux_run(void);
static int netdev_linux_do_ethtool(const char *name, struct ethtool_cmd *,
@@ -490,8 +496,12 @@ netdev_linux_run(void)
{
struct nl_sock *sock;
int error;
+int miimon;
-netdev_linux_miimon_run();
+atomic_read(&miimon_cnt, &miimon);
+if (miimon) {
+netdev_linux_miimon_run();
+}
sock = netdev_linux_notify_sock();
if (!sock) {
@@ -552,8 +562,12 @@ static void
netdev_linux_wait(void)
{
struct nl_sock *sock;
+int miimon;
-netdev_linux_miimon_wait();
+atomic_read(&miimon_cnt, &miimon);
+if (miimon) {
+netdev_linux_miimon_wait();
+}
sock = netdev_linux_notify_sock();
if (sock) {
nl_sock_wait(sock, POLLIN);
@@ -1222,6 +1236,14 @@ netdev_linux_set_miimon_interval(struct netdev *netdev_,
ovs_mutex_lock(&netdev->mutex);
interval = interval > 0 ? MAX(interval, 100) : 0;
if (netdev->miimon_interval != interval) {
+int junk;
+
+if (interval && !netdev->miimon_interval) {
+atomic_add(&miimon_cnt, 1, &junk);
+} else if (!interval && netdev->miimon_interval) {
+atomic_sub(&miimon_cnt, 1, &junk);
+}
+
netdev->miimon_interval = interval;
timer_set_expired(&netdev->miimon_timer);
}
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index ec3633c..1e1ef10 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -3470,6 +3470,8 @@ iface_destroy(struct iface *iface)
list_remove(&iface->port_elem);
hmap_remove(&br->iface_by_name, &iface->name_node);
+/* Ensure that miimon netdevs are counted correctly. */
+netdev_set_miimon_interval(iface->netdev, 0);
netdev_close(iface->netdev);
free(iface->name);
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v1 2/6] dpif-netdev: Introduce a classifier in userspace datapath.
On Fri, Nov 01, 2013 at 02:15:13AM -0700, Gurucharan Shetty wrote:
> Instead of an exact match flow table, we introduce a classifier.
> This enables mega-flows in userspace datapath.
>
> Signed-off-by: Gurucharan Shetty
Clang says:
../lib/dpif-netdev.c:638:5: error: calling function 'classifier_remove'
requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
classifier_remove(&dp->cls, &netdev_flow->cr);
^
../lib/dpif-netdev.c:752:10: error: calling function 'classifier_lookup'
requires shared lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
cr = classifier_lookup(&dp->cls, flow, NULL);
^
../lib/dpif-netdev.c:882:5: error: calling function 'classifier_insert'
requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
classifier_insert(&dp->cls, &netdev_flow->cr);
^
../lib/dpif-netdev.c:886:9: error: calling function 'classifier_remove'
requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
classifier_remove(&dp->cls, &netdev_flow->cr);
^
All the flows in the classifier will have the same priority, right? I
think so. The "default" here, to my eyes, implies that the default
can be overridden:
> +/* By default, choose a priority in the middle. */
> +#define NETDEV_DEFAULT_PRIORITY 0x8000
I know why struct dp_netdev_flow needs 'flow' in addition to a
cls_rule: because a cls_rule always zeros out the bits in the flow,
and dpif-netdev needs to be able to report the original values of
those bits. But why does struct dp_netdev_flow need another copy of
the mask?
I don't really understand dp_netdev_lookup_flow(). It is being called
in two different and conflicting contexts. dp_netdev_port_input()
uses it to determine how to handle an incoming packet, which correctly
would just call classifier_lookup(). The other functions that call
dp_netdev_lookup_flow() should just call
classifier_find_rule_exactly(). (Or am I missing something
important?)
Also in dpif_netdev_flow_mask_from_nlattrs(), should we now only check
that the in_port is valid, if the in_port is not wildcarded?
The wrapper in the conditional here looks odd to me, because the
indentation does not reflect the logical structure of the test:
if (odp_flow_key_to_flow(key, key_len, flow) || (mask_key &&
odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) {
I would write it as, say:
if (odp_flow_key_to_flow(key, key_len, flow)
|| (mask_key
&& odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) {
although || and && at the ends of lines instead of at the beginning is
fine too if you prefer.
In dp_netdev_flow_add(), copying the mask into a flow_wildcards
structure seems wasteful. I think that the caller can easily provide
the mask in that form, without copying.
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] netdev-linux: Skip miimon execution when disabled
On Fri, Nov 01, 2013 at 01:06:39PM -0700, Joe Stringer wrote: > When dealing with a large number of ports, one of the performance > bottlenecks is that we loop through all netdevs in the main loop. Miimon > is a contributor to this, executing even if it is not enabled on any > devices. > > This patch introduces a counter for the number of netdevs with miimon > configured. If this is 0, then we skip miimon_run() and miimon_wait(). > In a test environment of 5000 internal ports and 50 tunnel ports with > bfd, this reduces CPU usage from about 50% to about 45%. > > Signed-off-by: Joe Stringer I think that netdev-linux should be responsible for dropping miimon_cnt whenever a netdev_linux is destroyed. Probably netdev_linux_destruct() is the right place. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v1 3/6] dpif-netdev: Flow dump function should populate masks.
On Fri, Nov 01, 2013 at 02:15:14AM -0700, Gurucharan Shetty wrote: > Signed-off-by: Gurucharan Shetty Any reason this isn't part of patch 2? It seems to be needed. Why is the in_port mask hard-coded to UINT32_MAX? I don't remember anything in patch 2 that always un-wildcarded the in_port. (Maybe there should be?) Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v1 4/6] ofproto-dpif: Unixctl command to dump netdev flows.
On Fri, Nov 01, 2013 at 02:15:15AM -0700, Gurucharan Shetty wrote: > Right now, when we want to look at the flows of netdev > datapath, we use the dpif/dump-flows command which provides > it without querying the netdev datapath but rather through the > facets and subfacets. > > This commit adds the ability to directly query the netdev > datapath through the dpif interface. > > Signed-off-by: Gurucharan Shetty This seems useful but why not implement it in lib/dpif.c? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v1 5/6] ovs-dpctl: Make 'show_dpif' more general.
On Fri, Nov 01, 2013 at 02:15:16AM -0700, Gurucharan Shetty wrote: > Currently, with 'ovs-dpctl show', we call show_dpif(). > Move this functionality to lib/dpif.c so that it can > be used by an upcoming commit. > > Signed-off-by: Gurucharan Shetty Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v1 6/6] ofproto-dpif: Unixctl command to print userspace datapath summary.
On Fri, Nov 01, 2013 at 02:15:17AM -0700, Gurucharan Shetty wrote: > Signed-off-by: Gurucharan Shetty Again this looks useful but why not put it in lib/dpif.c? Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] vtep: add fault notifications to physical switch and ports
On Fri, Nov 01, 2013 at 10:53:23AM -0700, Bruce Davie wrote: > Add a "switch_fault" and "port_fault" column to the appropriate > tables in the VTEP schema. > > Signed-off-by: Bruce Davie Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 01/10] OF 1.1 set vlan vid/pcp compatibility.
On Thu, Oct 24, 2013 at 01:19:25PM -0700, Jarno Rajahalme wrote: > OpenFlow 1.1 set vlan actions only modify existing vlan > headers, while OF 1.0 actions push a new vlan header if one > does not exist already. > > Signed-off-by: Jarno Rajahalme Applied. I folded in a couple of minor improvements: diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c index c7322f6..ca26038 100644 --- a/lib/ofp-actions.c +++ b/lib/ofp-actions.c @@ -1554,7 +1554,10 @@ exit: return error; } -/* May modify flow->dl_type and flow->vlan_tci, caller must restore them. */ +/* May modify flow->dl_type and flow->vlan_tci, caller must restore them. + * + * Modifies some actions, filling in fields that could not be properly set + * without context. */ static enum ofperr ofpact_check__(struct ofpact *a, struct flow *flow, ofp_port_t max_ports, uint8_t table_id, bool enforce_consistency) @@ -2754,17 +2757,17 @@ ofpact_format(const struct ofpact *a, struct ds *s) case OFPACT_SET_VLAN_VID: ds_put_format(s, "%s:%"PRIu16, - ofpact_get_SET_VLAN_VID(a)->ofpact.compat - == OFPUTIL_OFPAT11_SET_VLAN_VID ? - "set_vlan_vid" : "mod_vlan_vid", + (a->compat == OFPUTIL_OFPAT11_SET_VLAN_VID + ? "set_vlan_vid" + : "mod_vlan_vid"), ofpact_get_SET_VLAN_VID(a)->vlan_vid); break; case OFPACT_SET_VLAN_PCP: ds_put_format(s, "%s:%"PRIu8, - ofpact_get_SET_VLAN_PCP(a)->ofpact.compat - == OFPUTIL_OFPAT11_SET_VLAN_PCP ? - "set_vlan_pcp" : "mod_vlan_pcp", + (a->compat == OFPUTIL_OFPAT11_SET_VLAN_PCP + ? "set_vlan_pcp" + : "mod_vlan_pcp"), ofpact_get_SET_VLAN_PCP(a)->vlan_pcp); break; ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif-xlate: Remove obsolete comment.
Nothing about the existing code in this function cares whether Goto-Table
is the last action.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto-dpif-xlate.c |1 -
1 file changed, 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f19dbf1..f5bc12c 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2529,7 +2529,6 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
break;
case OFPACT_GOTO_TABLE: {
-/* It is assumed that goto-table is the last action. */
struct ofpact_goto_table *ogt = ofpact_get_GOTO_TABLE(a);
ovs_assert(ctx->table_id < ogt->table_id);
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 02/10] OF 1.1 pop vlan compatibility.
On Thu, Oct 24, 2013 at 01:19:26PM -0700, Jarno Rajahalme wrote: > Store the original action code with the strip vlan action, > so that it can be printed back properly. > > Signed-off-by: Jarno Rajahalme Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 03/10] Inline mf_from_id().
On Thu, Oct 24, 2013 at 01:19:27PM -0700, Jarno Rajahalme wrote: > mf_from_id accesses a static table, so the compiler should be able to > completely optimize it away. > > Also use OVS_PACKED_ENUM to waste less space. > > Signed-off-by: Jarno Rajahalme Applied, thanks. I added a "meta-flow: " prefix to the commit message. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove obsolete comment.
LGTM,
On Fri, Nov 1, 2013 at 2:08 PM, Ben Pfaff wrote:
> Nothing about the existing code in this function cares whether Goto-Table
> is the last action.
>
> Signed-off-by: Ben Pfaff
> ---
> ofproto/ofproto-dpif-xlate.c |1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index f19dbf1..f5bc12c 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2529,7 +2529,6 @@ do_xlate_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
> break;
>
> case OFPACT_GOTO_TABLE: {
> -/* It is assumed that goto-table is the last action. */
> struct ofpact_goto_table *ogt = ofpact_get_GOTO_TABLE(a);
>
> ovs_assert(ctx->table_id < ogt->table_id);
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovsdb-tool: replace tabs with spaces
Signed-off-by: Alexandru Copot
---
ovsdb/ovsdb-tool.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ovsdb/ovsdb-tool.c b/ovsdb/ovsdb-tool.c
index 11e61e6..5e2b71b 100644
--- a/ovsdb/ovsdb-tool.c
+++ b/ovsdb/ovsdb-tool.c
@@ -524,7 +524,7 @@ do_show_log(int argc, char *argv[])
t *= 1000;
}
- s = xastrftime_msec(" %Y-%m-%d %H:%M:%S.###", t, true);
+s = xastrftime_msec(" %Y-%m-%d %H:%M:%S.###", t, true);
fputs(s, stdout);
free(s);
}
--
1.8.4.2
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] FAQ: Elaborate further on how one drops packets with OpenFlow.
Hopefully I got the right thread here ("FAQ: Elaborate further on how
one drops packets with OpenFlow.").
Acked-by: Joe Stringer
On Wed, Oct 9, 2013 at 3:22 PM, Ben Pfaff wrote:
>
> Signed-off-by: Ben Pfaff
> ---
> FAQ | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/FAQ b/FAQ
> index d36495c..14fb1c0 100644
> --- a/FAQ
> +++ b/FAQ
> @@ -1235,9 +1235,11 @@ A: To debug network behavior problems, trace the
> path of a packet,
>
> Q: How do I make a flow drop packets?
>
> -A: An empty set of actions causes a packet to be dropped. You can
> - specify an empty set of actions with "actions=" on the ovs-ofctl
> - command line. For example:
> +A: To drop a packet is to receive it without forwarding it. OpenFlow
> + explicitly specifies forwarding actions. Thus, a flow with an
> + empty set of actions does not forward packets anywhere, causing
> + them to be dropped. You can specify an empty set of actions with
> + "actions=" on the ovs-ofctl command line. For example:
>
> ovs-ofctl add-flow br0 priority=65535,actions=
>
> @@ -1249,6 +1251,9 @@ A: An empty set of actions causes a packet to be
> dropped. You can
>
> ovs-ofctl add-flow br0 priority=65535,actions=drop
>
> + "drop" is not an action, either in OpenFlow or Open vSwitch.
> + Rather, it is only a way to say that there are no actions.
> +
> Q: I added a flow to send packets out the ingress port, like this:
>
> ovs-ofctl add-flow br0 in_port=2,actions=2
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 04/10] OXM inspired match field names.
On Thu, Oct 24, 2013 at 01:19:28PM -0700, Jarno Rajahalme wrote:
> Adds OXM inspired aliases for match fields that don't have them
> already ("ip_proto", "ip_ecn", "ip_dscp", and "tunnel_id").
>
> "ip_dscp" replaces the earlier undocumented "nw_tos_shifted",
> and takes the DSCP value (0-63), which is then shifted
> appropriately when applied to an IP packet.
> The number of bits for this field is fixed from 8 to 6.
>
> Signed-off-by: Jarno Rajahalme
Thanks, I folded in the following to adjust to recent changes in the
repo and applied this to master.
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 4272fb1..01ef8e7 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -70,8 +70,8 @@ for test_case in \
'ipv6,nw_proto=1 NXM,OXM' \
'ip,nw_tos=0xf0 any' \
'ipv6,nw_tos=0xf0NXM,OXM' \
-'ip,nw_tos_shifted=0x3c any' \
-'ipv6,nw_tos_shifted=0x3cNXM,OXM' \
+'ip,ip_dscp=0x3c any' \
+'ipv6,ip_dscp=0x3c NXM,OXM' \
'ip,nw_ecn=1 NXM,OXM' \
'ipv6,nw_ecn=1 NXM,OXM' \
'ip,nw_ttl=5 NXM,OXM' \
@@ -252,8 +252,8 @@ for test_case in \
'ipv6 nw_proto 1/1' \
'ip nw_tos 0xf0/0xf0' \
'ipv6 nw_tos 0xf0/0xf0' \
-'ip nw_tos_shifted 0x3c/0xf0' \
-'ipv6 nw_tos_shifted 0x3c/0xf0' \
+'ip ip_dscp 0x3c/0xf0' \
+'ipv6 ip_dscp 0x3c/0xf0' \
'ip nw_ecn 1/1' \
'ipv6 nw_ecn 1/1' \
'ip nw_ttl 5/1' \
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] netdev-linux: Skip miimon execution when disabled
On 1 November 2013 13:31, Ben Pfaff wrote: > On Fri, Nov 01, 2013 at 01:06:39PM -0700, Joe Stringer wrote: > > When dealing with a large number of ports, one of the performance > > bottlenecks is that we loop through all netdevs in the main loop. Miimon > > is a contributor to this, executing even if it is not enabled on any > > devices. > > > > This patch introduces a counter for the number of netdevs with miimon > > configured. If this is 0, then we skip miimon_run() and miimon_wait(). > > In a test environment of 5000 internal ports and 50 tunnel ports with > > bfd, this reduces CPU usage from about 50% to about 45%. > > > > Signed-off-by: Joe Stringer > > I think that netdev-linux should be responsible for dropping > miimon_cnt whenever a netdev_linux is destroyed. Probably > netdev_linux_destruct() is the right place. > I agree, it looks much tidier there. I'll send an update for it. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove obsolete comment.
Thanks.
On Fri, Nov 01, 2013 at 02:17:11PM -0700, Alex Wang wrote:
> LGTM,
>
>
> On Fri, Nov 1, 2013 at 2:08 PM, Ben Pfaff wrote:
>
> > Nothing about the existing code in this function cares whether Goto-Table
> > is the last action.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > ofproto/ofproto-dpif-xlate.c |1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index f19dbf1..f5bc12c 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -2529,7 +2529,6 @@ do_xlate_actions(const struct ofpact *ofpacts,
> > size_t ofpacts_len,
> > break;
> >
> > case OFPACT_GOTO_TABLE: {
> > -/* It is assumed that goto-table is the last action. */
> > struct ofpact_goto_table *ogt = ofpact_get_GOTO_TABLE(a);
> >
> > ovs_assert(ctx->table_id < ogt->table_id);
> > --
> > 1.7.10.4
> >
> > ___
> > dev mailing list
> > dev@openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovsdb-tool: replace tabs with spaces
On Fri, Nov 01, 2013 at 11:20:56PM +0200, Alexandru Copot wrote: > Signed-off-by: Alexandru Copot Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] daemon: Precisely document signals that cause the monitor to restart.
Acked-by: Joe Stringer
On Fri, Oct 11, 2013 at 4:53 PM, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff
> ---
> lib/daemon.c |2 ++
> lib/daemon.man |7 ---
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/lib/daemon.c b/lib/daemon.c
> index 1c9ebe2..54641d0 100644
> --- a/lib/daemon.c
> +++ b/lib/daemon.c
> @@ -376,6 +376,8 @@ should_restart(int status)
> {
> if (WIFSIGNALED(status)) {
> static const int error_signals[] = {
> +/* This list of signals is documented in daemon.man. If you
> + * change the list, update the documentation too. */
> SIGABRT, SIGALRM, SIGBUS, SIGFPE, SIGILL, SIGPIPE, SIGSEGV,
> SIGXCPU, SIGXFSZ
> };
> diff --git a/lib/daemon.man b/lib/daemon.man
> index 7b07cb8..00de1a3 100644
> --- a/lib/daemon.man
> +++ b/lib/daemon.man
> @@ -26,9 +26,10 @@ run as a background process. \*(DD
> \fB\-\-monitor\fR
> Creates an additional process to monitor the \fB\*(PN\fR daemon. If
> the daemon dies due to a signal that indicates a programming error
> -(e.g. \fBSIGSEGV\fR, \fBSIGABRT\fR), then the monitor process starts a
> -new copy of it. If the daemon die or exits for another reason, the
> -monitor process exits.
> +(\fBSIGABRT\fR, \fBSIGALRM\fR, \fBSIGBUS\fR, \fBSIGFPE\fR,
> +\fBSIGILL\fR, \fBSIGPIPE\fR, \fBSIGSEGV\fR, \fBSIGXCPU\fR, or
> +\fBSIGXFSZ\fR) then the monitor process starts a new copy of it. If
> +the daemon dies or exits for another reason, the monitor process exits.
> .IP
> This option is normally used with \fB\-\-detach\fR, but it also
> functions without it.
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 05/10] Native Set-Field action.
On Thu, Oct 24, 2013 at 01:19:29PM -0700, Jarno Rajahalme wrote:
> Signed-off-by: Jarno Rajahalme
I'm going to fold this in:
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index a9d4340..9f31449 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -793,7 +793,7 @@ set_field_from_openflow(const struct ofp12_action_set_field
*oasf,
return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
}
sf = ofpact_put_SET_FIELD(ofpacts);
-sf->field = mf->id;
+sf->field = mf;
memcpy(&sf->value, oasf + 1, mf->n_bytes);
/* The value must be valid for match and must have the OFPVID_PRESENT bit
@@ -816,17 +816,16 @@ static void
set_field_to_ofast(const struct ofpact_set_field *sf,
struct ofpbuf *openflow)
{
-const struct mf_field *mf = mf_from_id(sf->field);
-uint16_t padded_value_len = ROUND_UP(mf->n_bytes, 8);
+uint16_t padded_value_len = ROUND_UP(sf->field->n_bytes, 8);
struct ofp12_action_set_field *oasf;
char *value;
oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
-oasf->dst = htonl(mf->oxm_header);
+oasf->dst = htonl(sf->field->oxm_header);
oasf->len = htons(ntohs(oasf->len) + padded_value_len);
value = ofpbuf_put_zeros(openflow, padded_value_len);
-memcpy(value, &sf->value, mf->n_bytes);
+memcpy(value, &sf->value, sf->field->n_bytes);
}
static void
@@ -834,16 +833,14 @@ set_field_to_openflow(const struct ofpact_set_field *sf,
struct ofpbuf *openflow)
{
struct ofp_header *oh = (struct ofp_header *)openflow->l2;
+const struct mf_field *mf = sf->field;
struct nx_action_reg_load *narl;
-const struct mf_field *mf;
if (oh->version >= OFP12_VERSION) {
set_field_to_ofast(sf, openflow);
return;
}
-mf = mf_from_id(sf->field);
-
/* Convert to one or two REG_LOADs */
if (mf->n_bits > 64) {
@@ -1780,7 +1777,7 @@ ofpact_check__(struct ofpact *a, struct flow *flow,
ofp_port_t max_ports,
return nxm_reg_load_check(ofpact_get_REG_LOAD(a), flow);
case OFPACT_SET_FIELD:
-mf = mf_from_id(ofpact_get_SET_FIELD(a)->field);
+mf = ofpact_get_SET_FIELD(a)->field;
/* Require OXM_OF_VLAN_VID to have an existing VLAN header. */
if (!mf_are_prereqs_ok(mf, flow) ||
(mf->id == MFF_VLAN_VID && !(flow->vlan_tci & htons(VLAN_CFI {
@@ -2967,7 +2964,7 @@ ofpact_format(const struct ofpact *a, struct ds *s)
case OFPACT_SET_FIELD:
set_field = ofpact_get_SET_FIELD(a);
-mf = mf_from_id(set_field->field);
+mf = set_field->field;
ds_put_format(s, "set_field:");
/* RFC: Print VLAN VID without the OFPVID_PRESENT bit. */
if (mf->id == MFF_VLAN_VID) {
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 1809db0..233a31b 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -378,7 +378,7 @@ enum ofpact_mpls_position {
* Used for OFPAT12_SET_FIELD. */
struct ofpact_set_field {
struct ofpact ofpact;
-enum mf_field_id field;
+const struct mf_field *field;
union mf_value value; /* Most-significant bits are used. */
};
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 7b19dc0..0fb32d5 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -489,7 +489,7 @@ set_field_parse__(char *arg, struct ofpbuf *ofpacts,
if (!mf->writable) {
return xasprintf("%s is read-only", key);
}
-sf->field = mf->id;
+sf->field = mf;
delim[0] = '\0';
error = mf_parse_value(mf, value, &sf->value);
if (error) {
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index f8c4f5f..1b849d2 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2440,7 +2440,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
ofpacts_len,
case OFPACT_SET_FIELD:
set_field = ofpact_get_SET_FIELD(a);
-mf = mf_from_id(set_field->field);
+mf = set_field->field;
mf_mask_field_and_prereqs(mf, &wc->masks);
/* Set field action only ever overwrites packet's outermost
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Transaction!
My name is Mark Carney, I am the new Governor of Bank Of England, There is the sum of 22,400,000.00 Pounds in my Bank, There were no beneficiaries stated concerning these funds which means no one would ever come forward to claim it. Please write me back if interested for further explanation. Reply only to my personal email markj.car...@hotmail.co.uk My friendly greetings Mark.J.Carney ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/3] Classifier: Staged sub-table matching.
v2: Properly finish hashes, more flexible configuration.
Signed-off-by: Jarno Rajahalme
---
lib/classifier.c | 174 +++---
lib/classifier.h | 12 ++-
lib/flow.c | 163 ++-
lib/flow.h | 82 +++-
lib/match.c |2 +-
lib/nx-match.c |2 +-
lib/ofp-util.c |2 +-
ofproto/ofproto-dpif-xlate.c |2 +-
ofproto/ofproto-dpif.c |2 +-
ofproto/ofproto.c|2 +-
tests/classifier.at | 38 +
tests/test-classifier.c | 12 +--
utilities/ovs-ofctl.c|4 +-
13 files changed, 433 insertions(+), 64 deletions(-)
diff --git a/lib/classifier.c b/lib/classifier.c
index 5587141..790bf8d 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -42,7 +42,8 @@ static void update_subtables_after_removal(struct classifier
*,
unsigned int del_priority);
static struct cls_rule *find_match(const struct cls_subtable *,
- const struct flow *);
+ const struct flow *,
+ struct flow_wildcards *);
static struct cls_rule *find_equal(struct cls_subtable *,
const struct miniflow *, uint32_t hash);
static struct cls_rule *insert_rule(struct classifier *,
@@ -148,14 +149,26 @@ cls_rule_is_catchall(const struct cls_rule *rule)
/* Initializes 'cls' as a classifier that initially contains no classification
* rules. */
+
void
-classifier_init(struct classifier *cls)
+classifier_init(struct classifier *cls, const uint8_t *flow_segments)
{
cls->n_rules = 0;
hmap_init(&cls->subtables);
list_init(&cls->subtables_priority);
hmap_init(&cls->partitions);
ovs_rwlock_init(&cls->rwlock);
+cls->n_flow_segments = 0;
+if (flow_segments) {
+while (cls->n_flow_segments < CLS_MAX_INDICES
+ && *flow_segments < FLOW_U32S) {
+cls->flow_segments[cls->n_flow_segments++] = *flow_segments++;
+}
+ovs_assert(cls->n_flow_segments == 3 &&
+ cls->flow_segments[0] == FLOW_SEGMENT_1_ENDS_AT / 4 &&
+ cls->flow_segments[1] == FLOW_SEGMENT_2_ENDS_AT / 4 &&
+ cls->flow_segments[2] == FLOW_SEGMENT_3_ENDS_AT / 4);
+}
}
/* Destroys 'cls'. Rules within 'cls', if any, are not freed; this is the
@@ -224,6 +237,7 @@ create_partition(struct classifier *cls, struct
cls_subtable *subtable,
{
uint32_t hash = hash_metadata(metadata);
struct cls_partition *partition = find_partition(cls, metadata, hash);
+
if (!partition) {
partition = xmalloc(sizeof *partition);
partition->metadata = metadata;
@@ -273,6 +287,7 @@ classifier_replace(struct classifier *cls, struct cls_rule
*rule)
} else {
rule->partition = old_rule->partition;
}
+
return old_rule;
}
@@ -298,8 +313,15 @@ classifier_remove(struct classifier *cls, struct cls_rule
*rule)
struct cls_partition *partition;
struct cls_rule *head;
struct cls_subtable *subtable;
+int i;
subtable = find_subtable(cls, &rule->match.mask);
+
+/* Remove rule node from indices. */
+for (i = 0; i < subtable->n_indices; i++) {
+hindex_remove(&subtable->indices[i], &rule->index_nodes[i]);
+}
+
head = find_equal(subtable, &rule->match.flow, rule->hmap_node.hash);
if (head != rule) {
list_remove(&rule->list);
@@ -380,10 +402,7 @@ classifier_lookup(const struct classifier *cls, const
struct flow *flow,
continue;
}
-rule = find_match(subtable, flow);
-if (wc) {
-flow_wildcards_fold_minimask(wc, &subtable->mask);
-}
+rule = find_match(subtable, flow, wc);
if (rule) {
best = rule;
LIST_FOR_EACH_CONTINUE (subtable, list_node,
@@ -397,10 +416,7 @@ classifier_lookup(const struct classifier *cls, const
struct flow *flow,
continue;
}
-rule = find_match(subtable, flow);
-if (wc) {
-flow_wildcards_fold_minimask(wc, &subtable->mask);
-}
+rule = find_match(subtable, flow, wc);
if (rule && rule->priority > best->priority) {
best = rule;
}
@@ -657,15 +673,47 @@ insert_subtable(struct classifier *cls, const struct
minimask *mask)
{
uint32_t hash = minimask_hash(mask, 0);
struct cls_subtable *subtable;
+int i, index = 0;
+struct flow_wildcards old, new;
+uint8_t prev;
subtable = xzalloc(sizeof *subtable);
hmap_init(&subtable->rules);
minimask_clone(&subtable->mask, mask);
-hmap_insert(&cls->subtables, &subtable->hmap_node, minimask_h
[ovs-dev] [PATCH 0/3] Classifier: Better wildcarding
This series implements two techniques for better wildcarding: 1. Staged sub-table matching: Each sub-table is matched in segments, starting from metadata and lower protocol layer fields, progressing towards higher layers only as needed. That is, if we can determine that there is no match on a specific segment, the higher layer fields need not be looked at, and hence will not need to be unwildcarded. 2. Prefix tree (trie) lookup for IP address fields: Classifier maintains a tree representation of the used address space. The tree is then traversed to find if certain sub-tables can be skipped. If the longest matching prefix on the packet's address is, e.g., /16, then the rest of the address field need not be unwildcarded in most cases. However, if there are flow entries with more specific addresses, enough bits need to be unwildcarded to ensure that future packets that should match the more specific entries do not match the kernel flow being created due to the current packet. Both these techniques stem from ideas by Ethan, but I'm responsible for the bugs most likely luring in there. A thorough review would be welcome. The third patch implements on-demand trie lookup, which delays the trie lookup until the field(s) in question become relevant. This adds some complexity and I was not able to find significant performance difference, so this one should be considered as an item for further discussion. Jarno Rajahalme (3): Classifier: Staged sub-table matching. Classifier: Track IP addresses for more wildcarding. Classifier: On-demand prefix tree lookup. lib/classifier.c | 759 -- lib/classifier.h | 25 +- lib/flow.c | 175 +- lib/flow.h | 82 +++-- lib/match.c |2 +- lib/meta-flow.c | 56 lib/meta-flow.h |4 + lib/nx-match.c |2 +- lib/ofp-util.c |2 +- lib/ofp-util.h |2 +- lib/util.h | 13 + ofproto/ofproto-dpif-xlate.c |2 +- ofproto/ofproto-dpif.c |2 +- ofproto/ofproto.c|2 +- tests/classifier.at | 38 +++ tests/ofproto-dpif.at|4 +- tests/test-classifier.c | 12 +- utilities/ovs-ofctl.c|4 +- 18 files changed, 1116 insertions(+), 70 deletions(-) -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 3/3] Classifier: On-demand prefix tree lookup.
There is a bit more bookkeeping, and preliminary measurements did
not show significant benefit. Results may differ with larger
flow tables, though.
Signed-off-by: Jarno Rajahalme
---
lib/classifier.c | 156 +-
1 file changed, 84 insertions(+), 72 deletions(-)
diff --git a/lib/classifier.c b/lib/classifier.c
index 0c668c4..17f8267 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -43,8 +43,7 @@ static void update_subtables_after_removal(struct classifier
*,
unsigned int del_priority);
static struct cls_rule *find_match(const struct cls_subtable *,
- const struct flow *,
- const struct trie_ctx *,
+ const struct flow *, struct trie_ctx *,
unsigned int n_tries,
struct flow_wildcards *);
static struct cls_rule *find_equal(struct cls_subtable *,
@@ -66,8 +65,8 @@ static struct cls_rule *next_rule_in_list(struct cls_rule *);
static uint8_t minimask_get_prefix_len(const struct minimask *,
const struct mf_field *,
unsigned int *);
-static bool trie_lookup(const struct cls_trie *, const struct flow *,
-struct trie_ctx *);
+static uint8_t trie_lookup(const struct cls_trie *, const struct flow *,
+ uint8_t *checkbits);
static void trie_destroy(struct trie_node *);
static void trie_insert(struct cls_trie *, const struct cls_rule *);
static void trie_remove(struct cls_trie *, const struct cls_rule *);
@@ -404,10 +403,11 @@ classifier_remove(struct classifier *cls, struct cls_rule
*rule)
* 'chekbits' prefix bits should un-wildcarded to quarantee no false matches
* will happen on the wildcarded datapath flow. */
struct trie_ctx {
-uint8_t idx; /* Index of the corresponding trie. */
+const struct cls_trie *trie;
+bool lookup_done; /* Status of the lookup. */
uint8_t u32ofs; /* U32 offset of the field in question. */
uint8_t match_plen; /* Longest prefix than could possibly match. */
-uint8_t checkbits;/* Prefix length needed to avoid false matches. */
+uint8_t nbits;/* Prefix length needed to avoid false matches. */
};
/* Finds and returns the highest-priority rule in 'cls' that matches 'flow'.
@@ -427,7 +427,7 @@ classifier_lookup(const struct classifier *cls, const
struct flow *flow,
struct cls_rule *best;
tag_type tags;
struct trie_ctx trie_ctx[CLS_N_TRIES];
-int i, n_tries = 0;
+int i;
/* Determine 'tags' such that, if 'subtable->tag' doesn't intersect them,
* then 'flow' cannot possibly match in 'subtable':
@@ -455,10 +455,9 @@ classifier_lookup(const struct classifier *cls, const
struct flow *flow,
/* Initialize trie contexts for match_find(). */
for (i = 0; i < cls->n_tries; i++) {
-if (trie_lookup(&cls->tries[i], flow, &trie_ctx[n_tries])) {
-trie_ctx[n_tries].idx = i;
-n_tries++;
-}
+trie_ctx[i].trie = &cls->tries[i];
+trie_ctx[i].u32ofs = cls->tries[i].field->flow_u32ofs;
+trie_ctx[i].lookup_done = false;
}
best = NULL;
LIST_FOR_EACH (subtable, list_node, &cls->subtables_priority) {
@@ -467,7 +466,7 @@ classifier_lookup(const struct classifier *cls, const
struct flow *flow,
if (!tag_intersects(tags, subtable->tag)) {
continue;
}
-rule = find_match(subtable, flow, trie_ctx, n_tries, wc);
+rule = find_match(subtable, flow, trie_ctx, cls->n_tries, wc);
if (rule) {
best = rule;
LIST_FOR_EACH_CONTINUE (subtable, list_node,
@@ -480,7 +479,7 @@ classifier_lookup(const struct classifier *cls, const
struct flow *flow,
if (!tag_intersects(tags, subtable->tag)) {
continue;
}
-rule = find_match(subtable, flow, trie_ctx, n_tries, wc);
+rule = find_match(subtable, flow, trie_ctx, cls->n_tries, wc);
if (rule && rule->priority > best->priority) {
best = rule;
}
@@ -898,45 +897,60 @@ update_subtables_after_removal(struct classifier *cls,
}
}
+struct range {
+uint8_t start;
+uint8_t end;
+};
+
/* Return 'true' if can skip rest of the subtable based on the prefix trie
* lookup results. */
static inline bool
-check_tries(const struct trie_ctx trie_ctx[CLS_N_TRIES], unsigned int n_tries,
-const uint8_t field_plen[CLS_N_TRIES], uint8_t next_u32ofs,
-struct flow_wildcards *wc)
+check_tries(const struct flow *flow,
+struct trie_ctx trie_ctx[CLS_N_TRIES],
+struct range *tries, const uint8_t field_plen[CLS_N_TRIES],
+struct ra
[ovs-dev] [PATCH 2/3] Classifier: Track IP addresses for more wildcarding.
Add a prefix tree (trie) structure for tracking the used
IP address space, enabling skipping classifier tables
containing longer masks than necessary for the given
address. This enables more wildcarding for megaflows in
parts of the address space without host routes.
In fact, the trie lookups results are only ever used when
they could potentially reduce the number of bits that need
to be un-wildcarded.
This implementation creates tries for IPv4 source and
destiantion addresses in any classifier tables those
fields are used. The tries are computed before checking
any other fields, so the same tree has nodes also for
addresses from mutually exclusive rules, making this
implementation sub-optimal in some cases.
It should be noted that this implementation does not
require any constraining of rule priorities, meaning
that this is not limited to longest-prefix match policy.
To mitigate overheads controllers could concentrate IP
address matching into specific tables.
More aggressive table skipping would be possible by
maintaining lists of tables that have prefixes at the
lengths encountered on tree traversal.
Signed-off-by: Jarno Rajahalme
---
lib/classifier.c | 593 -
lib/classifier.h | 13 ++
lib/flow.c| 12 +-
lib/meta-flow.c | 56 +
lib/meta-flow.h |4 +
lib/ofp-util.h|2 +-
lib/util.h| 13 ++
tests/classifier.at |2 +-
tests/ofproto-dpif.at |4 +-
9 files changed, 682 insertions(+), 17 deletions(-)
diff --git a/lib/classifier.c b/lib/classifier.c
index 790bf8d..0c668c4 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -27,6 +27,7 @@
#include "packets.h"
#include "ovs-thread.h"
+struct trie_ctx;
static struct cls_subtable *find_subtable(const struct classifier *,
const struct minimask *);
static struct cls_subtable *insert_subtable(struct classifier *,
@@ -43,6 +44,8 @@ static void update_subtables_after_removal(struct classifier
*,
static struct cls_rule *find_match(const struct cls_subtable *,
const struct flow *,
+ const struct trie_ctx *,
+ unsigned int n_tries,
struct flow_wildcards *);
static struct cls_rule *find_equal(struct cls_subtable *,
const struct miniflow *, uint32_t hash);
@@ -59,6 +62,19 @@ static struct cls_rule *insert_rule(struct classifier *,
static struct cls_rule *next_rule_in_list__(struct cls_rule *);
static struct cls_rule *next_rule_in_list(struct cls_rule *);
+
+static uint8_t minimask_get_prefix_len(const struct minimask *,
+ const struct mf_field *,
+ unsigned int *);
+static bool trie_lookup(const struct cls_trie *, const struct flow *,
+struct trie_ctx *);
+static void trie_destroy(struct trie_node *);
+static void trie_insert(struct cls_trie *, const struct cls_rule *);
+static void trie_remove(struct cls_trie *, const struct cls_rule *);
+static void mask_set_prefix_bits(struct flow_wildcards *, uint8_t u32ofs,
+ uint8_t nbits);
+static bool mask_prefix_bits_set(const struct flow_wildcards *,
+ uint8_t u32ofs, uint8_t nbits);
/* cls_rule. */
@@ -153,6 +169,12 @@ cls_rule_is_catchall(const struct cls_rule *rule)
void
classifier_init(struct classifier *cls, const uint8_t *flow_segments)
{
+int i;
+
+static enum mf_field_id trie_fields[2] = {
+MFF_IPV4_DST, MFF_IPV4_SRC
+};
+
cls->n_rules = 0;
hmap_init(&cls->subtables);
list_init(&cls->subtables_priority);
@@ -169,6 +191,12 @@ classifier_init(struct classifier *cls, const uint8_t
*flow_segments)
cls->flow_segments[1] == FLOW_SEGMENT_2_ENDS_AT / 4 &&
cls->flow_segments[2] == FLOW_SEGMENT_3_ENDS_AT / 4);
}
+for (i = 0; i < sizeof trie_fields / sizeof trie_fields[0]; i++) {
+cls->tries[i].field = mf_from_id(trie_fields[i]);
+ovs_assert(cls->tries[i].field);
+cls->tries[i].root = NULL;
+}
+cls->n_tries = i;
}
/* Destroys 'cls'. Rules within 'cls', if any, are not freed; this is the
@@ -179,6 +207,13 @@ classifier_destroy(struct classifier *cls)
if (cls) {
struct cls_subtable *partition, *next_partition;
struct cls_subtable *subtable, *next_subtable;
+int i;
+
+for (i = 0; i < cls->n_tries; i++) {
+if (cls->tries[i].root) {
+trie_destroy(cls->tries[i].root);
+}
+}
HMAP_FOR_EACH_SAFE (subtable, next_subtable, hmap_node,
&cls->subtables) {
@@ -275,6 +310,8 @@ classifier_replace(struct classifier *cls, struct cls_rule
*rule)
old_rule = in
Re: [ovs-dev] [PATCH v4 05/10] Native Set-Field action.
Fair enough ;-)
Jarno
On Nov 1, 2013, at 3:02 PM, Ben Pfaff wrote:
> On Thu, Oct 24, 2013 at 01:19:29PM -0700, Jarno Rajahalme wrote:
>> Signed-off-by: Jarno Rajahalme
>
> I'm going to fold this in:
>
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index a9d4340..9f31449 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -793,7 +793,7 @@ set_field_from_openflow(const struct
> ofp12_action_set_field *oasf,
> return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
> }
> sf = ofpact_put_SET_FIELD(ofpacts);
> -sf->field = mf->id;
> +sf->field = mf;
> memcpy(&sf->value, oasf + 1, mf->n_bytes);
>
> /* The value must be valid for match and must have the OFPVID_PRESENT bit
> @@ -816,17 +816,16 @@ static void
> set_field_to_ofast(const struct ofpact_set_field *sf,
>struct ofpbuf *openflow)
> {
> -const struct mf_field *mf = mf_from_id(sf->field);
> -uint16_t padded_value_len = ROUND_UP(mf->n_bytes, 8);
> +uint16_t padded_value_len = ROUND_UP(sf->field->n_bytes, 8);
> struct ofp12_action_set_field *oasf;
> char *value;
>
> oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
> -oasf->dst = htonl(mf->oxm_header);
> +oasf->dst = htonl(sf->field->oxm_header);
> oasf->len = htons(ntohs(oasf->len) + padded_value_len);
>
> value = ofpbuf_put_zeros(openflow, padded_value_len);
> -memcpy(value, &sf->value, mf->n_bytes);
> +memcpy(value, &sf->value, sf->field->n_bytes);
> }
>
> static void
> @@ -834,16 +833,14 @@ set_field_to_openflow(const struct ofpact_set_field *sf,
> struct ofpbuf *openflow)
> {
> struct ofp_header *oh = (struct ofp_header *)openflow->l2;
> +const struct mf_field *mf = sf->field;
> struct nx_action_reg_load *narl;
> -const struct mf_field *mf;
>
> if (oh->version >= OFP12_VERSION) {
> set_field_to_ofast(sf, openflow);
> return;
> }
>
> -mf = mf_from_id(sf->field);
> -
> /* Convert to one or two REG_LOADs */
>
> if (mf->n_bits > 64) {
> @@ -1780,7 +1777,7 @@ ofpact_check__(struct ofpact *a, struct flow *flow,
> ofp_port_t max_ports,
> return nxm_reg_load_check(ofpact_get_REG_LOAD(a), flow);
>
> case OFPACT_SET_FIELD:
> -mf = mf_from_id(ofpact_get_SET_FIELD(a)->field);
> +mf = ofpact_get_SET_FIELD(a)->field;
> /* Require OXM_OF_VLAN_VID to have an existing VLAN header. */
> if (!mf_are_prereqs_ok(mf, flow) ||
> (mf->id == MFF_VLAN_VID && !(flow->vlan_tci & htons(VLAN_CFI {
> @@ -2967,7 +2964,7 @@ ofpact_format(const struct ofpact *a, struct ds *s)
>
> case OFPACT_SET_FIELD:
> set_field = ofpact_get_SET_FIELD(a);
> -mf = mf_from_id(set_field->field);
> +mf = set_field->field;
> ds_put_format(s, "set_field:");
> /* RFC: Print VLAN VID without the OFPVID_PRESENT bit. */
> if (mf->id == MFF_VLAN_VID) {
> diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> index 1809db0..233a31b 100644
> --- a/lib/ofp-actions.h
> +++ b/lib/ofp-actions.h
> @@ -378,7 +378,7 @@ enum ofpact_mpls_position {
> * Used for OFPAT12_SET_FIELD. */
> struct ofpact_set_field {
> struct ofpact ofpact;
> -enum mf_field_id field;
> +const struct mf_field *field;
> union mf_value value; /* Most-significant bits are used. */
> };
>
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 7b19dc0..0fb32d5 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -489,7 +489,7 @@ set_field_parse__(char *arg, struct ofpbuf *ofpacts,
> if (!mf->writable) {
> return xasprintf("%s is read-only", key);
> }
> -sf->field = mf->id;
> +sf->field = mf;
> delim[0] = '\0';
> error = mf_parse_value(mf, value, &sf->value);
> if (error) {
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index f8c4f5f..1b849d2 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2440,7 +2440,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
>
> case OFPACT_SET_FIELD:
> set_field = ofpact_get_SET_FIELD(a);
> -mf = mf_from_id(set_field->field);
> +mf = set_field->field;
> mf_mask_field_and_prereqs(mf, &wc->masks);
>
> /* Set field action only ever overwrites packet's outermost
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] ofp-util: Move ofputil_check_output_port() to ofp-actions, rename.
Acked-by: Jarno Rajahalme
On Nov 1, 2013, at 9:43 AM, Ben Pfaff wrote:
> This function is related to actions to ofp-actions seems like a logical
> place for it.
>
> Signed-off-by: Ben Pfaff
> ---
> lib/bundle.c |2 +-
> lib/ofp-actions.c | 33 +
> lib/ofp-actions.h |1 +
> lib/ofp-util.c| 25 -
> lib/ofp-util.h|2 --
> 5 files changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/lib/bundle.c b/lib/bundle.c
> index dcabaaa..7d00f87 100644
> --- a/lib/bundle.c
> +++ b/lib/bundle.c
> @@ -204,7 +204,7 @@ bundle_check(const struct ofpact_bundle *bundle,
> ofp_port_t max_ports,
> ofp_port_t ofp_port = bundle->slaves[i];
> enum ofperr error;
>
> -error = ofputil_check_output_port(ofp_port, max_ports);
> +error = ofpact_check_output_port(ofp_port, max_ports);
> if (error) {
> VLOG_WARN_RL(&rl, "invalid slave %"PRIu16, ofp_port);
> return error;
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index cccd6b1..6fa80a2 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -89,7 +89,7 @@ output_from_openflow10(const struct ofp10_action_output
> *oao,
> output->port = u16_to_ofp(ntohs(oao->port));
> output->max_len = ntohs(oao->max_len);
>
> -return ofputil_check_output_port(output->port, OFPP_MAX);
> +return ofpact_check_output_port(output->port, OFPP_MAX);
> }
>
> static enum ofperr
> @@ -766,7 +766,7 @@ output_from_openflow11(const struct ofp11_action_output
> *oao,
> return error;
> }
>
> -return ofputil_check_output_port(output->port, OFPP_MAX);
> +return ofpact_check_output_port(output->port, OFPP_MAX);
> }
>
> static enum ofperr
> @@ -1538,6 +1538,31 @@ exit:
> return error;
> }
>
> +/* Checks that 'port' is a valid output port for OFPACT_OUTPUT, given that
> the
> + * switch will never have more than 'max_ports' ports. Returns 0 if 'port'
> is
> + * valid, otherwise an OpenFlow error code. */
> +enum ofperr
> +ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports)
> +{
> +switch (port) {
> +case OFPP_IN_PORT:
> +case OFPP_TABLE:
> +case OFPP_NORMAL:
> +case OFPP_FLOOD:
> +case OFPP_ALL:
> +case OFPP_CONTROLLER:
> +case OFPP_NONE:
> +case OFPP_LOCAL:
> +return 0;
> +
> +default:
> +if (ofp_to_u16(port) < ofp_to_u16(max_ports)) {
> +return 0;
> +}
> +return OFPERR_OFPBAC_BAD_OUT_PORT;
> +}
> +}
> +
> /* May modify flow->dl_type, caller must restore it. */
> static enum ofperr
> ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t
> max_ports,
> @@ -1547,8 +1572,8 @@ ofpact_check__(const struct ofpact *a, struct flow
> *flow, ofp_port_t max_ports,
>
> switch (a->type) {
> case OFPACT_OUTPUT:
> -return ofputil_check_output_port(ofpact_get_OUTPUT(a)->port,
> - max_ports);
> +return ofpact_check_output_port(ofpact_get_OUTPUT(a)->port,
> +max_ports);
>
> case OFPACT_CONTROLLER:
> return 0;
> diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> index e097468..dd8f35f 100644
> --- a/lib/ofp-actions.h
> +++ b/lib/ofp-actions.h
> @@ -567,6 +567,7 @@ enum ofperr ofpacts_check(const struct ofpact[], size_t
> ofpacts_len,
> struct flow *, ofp_port_t max_ports,
> uint8_t table_id, bool enforce_consistency);
> enum ofperr ofpacts_verify(const struct ofpact ofpacts[], size_t ofpacts_len);
> +enum ofperr ofpact_check_output_port(ofp_port_t port, ofp_port_t max_ports);
>
> /* Converting ofpacts to OpenFlow. */
> void ofpacts_put_openflow10(const struct ofpact[], size_t ofpacts_len,
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index d435e99..3d9efab 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -4833,31 +4833,6 @@ ofputil_port_to_ofp11(ofp_port_t ofp10_port)
> : ofp_to_u16(ofp10_port) + OFPP11_OFFSET);
> }
>
> -/* Checks that 'port' is a valid output port for the OFPAT10_OUTPUT action,
> given
> - * that the switch will never have more than 'max_ports' ports. Returns 0 if
> - * 'port' is valid, otherwise an OpenFlow return code. */
> -enum ofperr
> -ofputil_check_output_port(ofp_port_t port, ofp_port_t max_ports)
> -{
> -switch (port) {
> -case OFPP_IN_PORT:
> -case OFPP_TABLE:
> -case OFPP_NORMAL:
> -case OFPP_FLOOD:
> -case OFPP_ALL:
> -case OFPP_CONTROLLER:
> -case OFPP_NONE:
> -case OFPP_LOCAL:
> -return 0;
> -
> -default:
> -if (ofp_to_u16(port) < ofp_to_u16(max_ports)) {
> -return 0;
> -}
> -return OFPERR_OFPBAC_BAD_OUT_PORT;
> -}
> -}
> -
> #define OFPUTIL_NAMED_PORTS \
> OFPUTIL_NAMED_PORT(IN_PORT) \
> OFPUTIL_NAMED_PORT(TABLE) \
> diff -
[ovs-dev] [RFC] ofproto-dpif-upcall: Improve upcall dispatching fairness.
This commit improves the upcall dispatching fairness by introduing
a 2-stage scheme. At the first stage, the dispatcher thread will
try receiving upcalls from dpif and hashing the upcalls to the local
queues based on the L2 header information. When either there is no
upcall to receive or one of the dispatcher queues is full, the
disptacher will go to the second stage. At the second stage, the
dispatcher will iterate over its queues in Round Robin fashion.
Each time, it will try inserting the upcall at the front of queue
into the corresponding upcall handler thread's queue.
Signed-off-by: Alex Wang
---
ofproto/ofproto-dpif-upcall.c | 137 ++---
1 file changed, 114 insertions(+), 23 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 24a5729..874bfbe 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -37,6 +37,7 @@
#include "vlog.h"
#define MAX_QUEUE_LENGTH 512
+#define UPCALL_QUEUE_LENGTH 256
VLOG_DEFINE_THIS_MODULE(ofproto_dpif_upcall);
@@ -79,6 +80,9 @@ struct udpif {
uint32_t secret; /* Random seed for upcall hash. */
pthread_t dispatcher; /* Dispatcher thread ID. */
+/* Queues upcalls based on L2 header. */
+struct list upcall_queues[UPCALL_QUEUE_LENGTH];
+uint16_t upcall_queue_lens[UPCALL_QUEUE_LENGTH];
struct handler *handlers; /* Upcall handlers. */
size_t n_handlers;
@@ -95,6 +99,22 @@ struct udpif {
struct latch exit_latch; /* Tells child threads to exit. */
};
+/* An entry in one of the upcall_queues. */
+struct upcall_queue_entry {
+struct list list_node; /* In one of upcall_queues. */
+
+struct upcall *upcall; /* The received upcall. */
+uint32_t hash_1; /* Level one distribution hash. */
+uint32_t hash_2; /* Level two distribution hash. */
+};
+
+/* Used to store the value, which indicates the index
+ * of a non-empty queue in "udpif"'s upcall_queues. */
+struct non_empty_entry {
+struct list list_node;
+int value;
+};
+
enum upcall_type {
BAD_UPCALL, /* Some kind of bug somewhere. */
MISS_UPCALL,/* A flow miss. */
@@ -127,6 +147,7 @@ struct udpif *
udpif_create(struct dpif_backer *backer, struct dpif *dpif)
{
struct udpif *udpif = xzalloc(sizeof *udpif);
+size_t i;
udpif->dpif = dpif;
udpif->backer = backer;
@@ -136,6 +157,9 @@ udpif_create(struct dpif_backer *backer, struct dpif *dpif)
guarded_list_init(&udpif->drop_keys);
guarded_list_init(&udpif->fmbs);
atomic_init(&udpif->reval_seq, 0);
+for (i = 0; i < UPCALL_QUEUE_LENGTH; i++) {
+list_init(udpif->upcall_queues + i);
+}
return udpif;
}
@@ -490,35 +514,55 @@ classify_upcall(const struct upcall *upcall)
static void
recv_upcalls(struct udpif *udpif)
{
+struct list non_empty_list;
+/* Indicates if the upcall_queues index has already been recorded in
+ * the non_empty_list. */
+int qmap[UPCALL_QUEUE_LENGTH] = {0};
int n;
+list_init(&non_empty_list);
+
+/* Stage-1: recv upcalls from dpif and put them into the upcall_queues. */
for (;;) {
-uint32_t hash = udpif->secret;
-struct handler *handler;
-struct upcall *upcall;
+struct upcall_queue_entry *q_entry;
+struct list *queue;
size_t n_bytes, left;
struct nlattr *nla;
-int error;
-
-upcall = xmalloc(sizeof *upcall);
-ofpbuf_use_stub(&upcall->upcall_buf, upcall->upcall_stub,
-sizeof upcall->upcall_stub);
-error = dpif_recv(udpif->dpif, &upcall->dpif_upcall,
- &upcall->upcall_buf);
+int idx, error;
+
+q_entry = xmalloc(sizeof *q_entry);
+q_entry->upcall = xmalloc(sizeof *q_entry->upcall);
+q_entry->hash_1 = udpif->secret;
+q_entry->hash_2 = udpif->secret;
+ofpbuf_use_stub(&q_entry->upcall->upcall_buf,
+q_entry->upcall->upcall_stub,
+sizeof q_entry->upcall->upcall_stub);
+
+error = dpif_recv(udpif->dpif, &q_entry->upcall->dpif_upcall,
+ &q_entry->upcall->upcall_buf);
if (error) {
-upcall_destroy(upcall);
+upcall_destroy(q_entry->upcall);
break;
}
n_bytes = 0;
-NL_ATTR_FOR_EACH (nla, left, upcall->dpif_upcall.key,
- upcall->dpif_upcall.key_len) {
+NL_ATTR_FOR_EACH (nla, left, q_entry->upcall->dpif_upcall.key,
+ q_entry->upcall->dpif_upcall.key_len) {
enum ovs_key_attr type = nl_attr_type(nla);
-if (type == OVS_KEY_ATTR_IN_PORT
-|| type == OVS_KEY_ATTR_TCP
-|| type == OVS_KEY_ATTR_UDP) {
+if (type == OVS_KEY_ATTR_ETHERNET) {
+if (nl_a
Re: [ovs-dev] [RFC] ofproto-dpif-upcall: Improve upcall dispatching fairness.
Hey Ethan, This is the implementation of the 2-stage scheme. My experiment shows that it does help reduce both the loss rate and handling latency. I'll format my test result and attach it to this thread later. Thanks, ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] ofp-util: Enforce OpenFlow 1.1+ table_id requirements in flow_mod messages.
On Nov 1, 2013, at 9:43 AM, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff
> ---
> lib/ofp-util.c | 21 +-
> tests/learn.at |4 +--
> tests/ofp-print.at | 12
> tests/ovs-ofctl.at | 48 +++
> utilities/ovs-ofctl.8.in | 71 +++---
> 5 files changed, 106 insertions(+), 50 deletions(-)
>
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 3d9efab..e6a9494 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -1530,7 +1530,19 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
> }
> fm->modify_cookie = false;
> fm->command = ofm->command;
> +
> +/* Get table ID.
> + *
> + * OF1.1 entirely forbids table_id == 255.
> + * OF1.2+ allows table_id == 255 only for deletes. */
> fm->table_id = ofm->table_id;
> +if (fm->table_id == 0xff
I’d rather see “255” here as well to match the comments.
> +&& (oh->version == OFP11_VERSION
> +|| (ofm->command != OFPFC_DELETE &&
> +ofm->command != OFPFC_DELETE_STRICT))) {
> +return OFPERR_OFPFMFC_BAD_TABLE_ID;
> +}
> +
…
> +For flow dump commands, limits the flows dumped to those in the table
> +with the given \fInumber\fR between 0 and 254. If not specified (or if
> +255 is specified as \fInumber\fR), then flows in all tables are
> +dumped.
> +.
> +.IP
> +For flow table modification commands, behavior varies based on the
> +OpenFlow version used to connect to the switch:
> +.
> +.RS
> +.IP "OpenFlow 1.0"
> +OpenFlow 1.0 does not support \fBtable\fR for modifying flows.
> +\fBovs\-ofctl\fR will exit with an error if \fBtable\fR (other than
> +\fBtable=255\fR) is specified for a switch that only supports OpenFlow
> +1.0.
> +.IP
> +In OpenFlow 1.0, the switch chooses the table into which to insert a
> +new flow. The Open vSwitch software switch always chooses table 0.
> +Other Open vSwitch datapaths and other OpenFlow implementations may
> +choose different tables.
IMO any implementation should pick the “input table”, which by definition is 0,
or no?
> +.IP
> +The OpenFlow 1.0 behavior in Open vSwitch for modifying or removing
> +flows depends on whether \fB\-\-strict\fR is used. Without
> +\fB\-\-strict\fR, the command applies to matching flows in all tables.
> +With \fB\-\-strict\fR, the command will operate on any single matching
> +flow in any table; it will do nothing if there are matches in more
> +than one table. (The distinction between these behaviors only matters
> +if non-OpenFlow 1.0 commands were also used, because OpenFlow 1.0
> +alone cannot add flows with the same matching criteria to multiple
> +tables.)
> +.
…
Jarno
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 05/10] Native Set-Field action.
On Fri, Nov 01, 2013 at 03:22:34PM -0700, Jarno Rajahalme wrote:
> Fair enough ;-)
Thanks.
I'm folding in the following additional changes, and then I'll apply
this to master.
The only really notable part of the changes that I made is that I'm
feeling timid about silently adding and removing the OFPVID_PRESENT
bit. It will be a convenience to some users, but I think that it will
be confusing to others, especially to anyone who thinks that the
string format of actions accurately represents what is in the binary
form. On that balance, at the moment I prefer to do what requires
fewer special cases, so I've removed the code marked RFC. If you feel
differently, then let's talk about it.
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 9f31449..761cea5 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -788,8 +788,7 @@ set_field_from_openflow(const struct ofp12_action_set_field
*oasf,
ovs_assert(mf->n_bytes == oxm_length);
/* oxm_length is now validated to be compatible with mf_value. */
if (!mf->writable) {
-VLOG_WARN_RL(&rl, "destination field %s is not writable",
- mf->name);
+VLOG_WARN_RL(&rl, "destination field %s is not writable", mf->name);
return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
}
sf = ofpact_put_SET_FIELD(ofpacts);
@@ -813,8 +812,8 @@ set_field_from_openflow(const struct ofp12_action_set_field
*oasf,
}
static void
-set_field_to_ofast(const struct ofpact_set_field *sf,
- struct ofpbuf *openflow)
+set_field_to_openflow12(const struct ofpact_set_field *sf,
+struct ofpbuf *openflow)
{
uint16_t padded_value_len = ROUND_UP(sf->field->n_bytes, 8);
struct ofp12_action_set_field *oasf;
@@ -822,27 +821,19 @@ set_field_to_ofast(const struct ofpact_set_field *sf,
oasf = ofputil_put_OFPAT12_SET_FIELD(openflow);
oasf->dst = htonl(sf->field->oxm_header);
-oasf->len = htons(ntohs(oasf->len) + padded_value_len);
+oasf->len = htons(sizeof *oasf + padded_value_len);
value = ofpbuf_put_zeros(openflow, padded_value_len);
memcpy(value, &sf->value, sf->field->n_bytes);
}
+/* Convert 'sf' to one or two REG_LOADs. */
static void
-set_field_to_openflow(const struct ofpact_set_field *sf,
- struct ofpbuf *openflow)
+set_field_to_nxast(const struct ofpact_set_field *sf, struct ofpbuf *openflow)
{
-struct ofp_header *oh = (struct ofp_header *)openflow->l2;
const struct mf_field *mf = sf->field;
struct nx_action_reg_load *narl;
-if (oh->version >= OFP12_VERSION) {
-set_field_to_ofast(sf, openflow);
-return;
-}
-
-/* Convert to one or two REG_LOADs */
-
if (mf->n_bits > 64) {
ovs_assert(mf->n_bytes == 16); /* IPv6 addr. */
/* Split into 64bit chunks */
@@ -866,6 +857,19 @@ set_field_to_openflow(const struct ofpact_set_field *sf,
}
}
+static void
+set_field_to_openflow(const struct ofpact_set_field *sf,
+ struct ofpbuf *openflow)
+{
+struct ofp_header *oh = (struct ofp_header *)openflow->l2;
+
+if (oh->version >= OFP12_VERSION) {
+set_field_to_openflow12(sf, openflow);
+} else {
+set_field_to_nxast(sf, openflow);
+}
+}
+
static enum ofperr
output_from_openflow11(const struct ofp11_action_output *oao,
struct ofpbuf *out)
@@ -2827,8 +2831,6 @@ ofpact_format(const struct ofpact *a, struct ds *s)
const struct ofpact_sample *sample;
const struct ofpact_set_field *set_field;
const struct mf_field *mf;
-const union mf_value *value;
-union mf_value temp;
ofp_port_t port;
switch (a->type) {
@@ -2966,14 +2968,7 @@ ofpact_format(const struct ofpact *a, struct ds *s)
set_field = ofpact_get_SET_FIELD(a);
mf = set_field->field;
ds_put_format(s, "set_field:");
-/* RFC: Print VLAN VID without the OFPVID_PRESENT bit. */
-if (mf->id == MFF_VLAN_VID) {
-temp.be16 = set_field->value.be16 & htons(VLAN_VID_MASK);
-value = &temp;
-} else {
-value = &set_field->value;
-}
-mf_format(mf, value, NULL, s);
+mf_format(mf, &set_field->value, NULL, s);
ds_put_format(s, "->%s", mf->name);
break;
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 233a31b..0478a9b 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -379,7 +379,7 @@ enum ofpact_mpls_position {
struct ofpact_set_field {
struct ofpact ofpact;
const struct mf_field *field;
-union mf_value value; /* Most-significant bits are used. */
+union mf_value value;
};
/* OFPACT_PUSH_VLAN/MPLS/PBB
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 0fb32d5..83413c2 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -499,10 +499,6 @@ set_field_parse__(char *arg, struct ofpbuf *ofpacts,
if (!mf_is_value_valid(mf, &sf->value)) {
return xasprintf("%s
Re: [ovs-dev] [PATCH 3/3] ofproto: Centralize action checking, doing it at decode time.
On Nov 1, 2013, at 9:43 AM, Ben Pfaff wrote:
> Jarno pointed out that modify_flows__() didn't really need to check every
> instance of the flow separately. After some further investigation I
> decided that this was even more of an improvement.
>
Indeed! Check the actions right after they are decoded rather than multiple
places afterwards.
One question below, otherwise:
Acked-by: Jarno Rajahalme
> CC: Jarno Rajahalme
> Signed-off-by: Ben Pfaff
> —
…
> @@ -1700,7 +1701,7 @@ ofpact_check__(const struct ofpact *a, struct flow
> *flow, ofp_port_t max_ports,
> case OFPACT_WRITE_ACTIONS: {
> struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
> return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
> - flow, max_ports, table_id, false);
> + flow, false, max_ports, table_id, n_tables);
> }
>
> case OFPACT_WRITE_METADATA:
> @@ -1714,11 +1715,14 @@ ofpact_check__(const struct ofpact *a, struct flow
> *flow, ofp_port_t max_ports,
> return 0;
> }
>
> -case OFPACT_GOTO_TABLE:
> -if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) {
> +case OFPACT_GOTO_TABLE: {
> +uint8_t goto_table = ofpact_get_GOTO_TABLE(a)->table_id;
> +if ((table_id != 255 && goto_table <= table_id)
Can the table id really be 255 at this point? I just learned that 255 is not
allowed in OF 1.1,
and in OF1.2(+) it is allowed only on deletes, which should have no actions.
> +|| (n_tables != 255 && goto_table >= n_tables)) {
> return OFPERR_OFPBRC_BAD_TABLE_ID;
> }
> return 0;
> +}
>
...
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovs-controller: Rename test-controller and do not install or package.
This needs a rebase. The deletion of the content in 'manpages.mk' is surprising; the file states that it is automatically generated, the commit message doesn't mention it, and the file isn't actually deleted, only emptied. Was this intentional? There seems to be another reference to "openvswitch-controller" in ./utilities/bugtool/ovs-bugtool.in which this patch doesn't address. Otherwise, it looks pretty good overall; I haven't actually built and installed packages for debian/rhel from this though. On Fri, Oct 11, 2013 at 4:27 PM, Ben Pfaff wrote: >> >> Too many users have incorrectly assumed that ovs-controller is a necessary >> or desirable part of an Open vSwitch deployment. This commit should fix >> the problem by renaming it test-controller and removing it from the >> default install and from packaging. >> >> Signed-off-by: Ben Pfaff >> --- >> FAQ|7 +- >> INSTALL.SSL|2 +- >> INSTALL.XenServer |2 +- >> NEWS |4 + >> README |2 - >> debian/automake.mk |8 - >> debian/changelog |5 +- >> debian/openvswitch-controller.README.Debian|7 - >> debian/openvswitch-controller.default | 29 -- >> debian/openvswitch-controller.dirs |1 - >> debian/openvswitch-controller.init | 278 >> >> debian/openvswitch-controller.install |1 - >> debian/openvswitch-controller.manpages |1 - >> debian/openvswitch-controller.postinst | 60 - >> debian/openvswitch-controller.postrm | 44 >> manpages.mk| 255 >> -- >> rhel/openvswitch-fedora.spec.in|2 - >> rhel/openvswitch.spec.in |2 - >> tests/.gitignore |2 + >> tests/automake.mk |7 + >> .../test-controller.8.in | 45 ++-- >> .../ovs-controller.c => tests/test-controller.c|0 >> utilities/.gitignore |2 - >> utilities/automake.mk |7 - >> xenserver/openvswitch-xen.spec.in |2 - >> 25 files changed, 51 insertions(+), 724 deletions(-) >> delete mode 100644 debian/openvswitch-controller.README.Debian >> delete mode 100644 debian/openvswitch-controller.default >> delete mode 100644 debian/openvswitch-controller.dirs >> delete mode 100755 debian/openvswitch-controller.init >> delete mode 100644 debian/openvswitch-controller.install >> delete mode 100644 debian/openvswitch-controller.manpages >> delete mode 100755 debian/openvswitch-controller.postinst >> delete mode 100755 debian/openvswitch-controller.postrm >> rename utilities/ovs-controller.8.in => tests/test-controller.8.in (77%) >> rename utilities/ovs-controller.c => tests/test-controller.c (100%) >> >> diff --git a/FAQ b/FAQ >> index d36495c..f7bf78a 100644 >> --- a/FAQ >> +++ b/FAQ >> @@ -89,10 +89,9 @@ A: Distributed vswitch applications (e.g., VMware >> vNetwork distributed >> environments: OpenFlow, which exposes flow-based forwarding state, >> and the OVSDB management protocol, which exposes switch port state. >> In addition to the switch implementation itself, Open vSwitch >> - includes tools (ovs-controller, ovs-ofctl, ovs-vsctl) that developers >> - can script and extend to provide distributed vswitch capabilities >> - that are closely integrated with their virtualization management >> - platform. >> + includes tools (ovs-ofctl, ovs-vsctl) that developers can script and >> + extend to provide distributed vswitch capabilities that are closely >> + integrated with their virtualization management platform. >> >> Q: Why doesn't Open vSwitch support distribution? >> >> diff --git a/INSTALL.SSL b/INSTALL.SSL >> index 8eb0c49..061af97 100644 >> --- a/INSTALL.SSL >> +++ b/INSTALL.SSL >> @@ -115,7 +115,7 @@ that contains the PKI structure: >>% ovs-pki req+sign ctl controller >> >> ctl-privkey.pem and ctl-cert.pem would need to be copied to the >> -controller for its use at runtime. If you were to use ovs-controller, >> +controller for its use at runtime. If you were to use test-controller, >> the simple OpenFlow controller included with Open vSwitch, then the >> --private-key and --certificate options, respectively, would point to >> these files. >> diff --git a/INSTALL.XenServer b/INSTALL.XenServer >> index e31788a..d6f5816 100644 >> --- a/INSTALL.XenServer >> +++ b/INSTALL.XenServer >> @@ -167,7 +167,7 @@ controller on XenServer and, as a consequence of the >> step above that >> deletes all of the bridge
Re: [ovs-dev] [PATCH v4 06/10] lib/ofp-actions: Set field OF 1.0/1.1 compatibility.
On Thu, Oct 24, 2013 at 01:19:30PM -0700, Jarno Rajahalme wrote:
> Output set field actions as standard OF1.0/1.1 set actions or to
> reg_load instructions, when a compatible set action(s) do not exist.
>
> Signed-off-by: Jarno Rajahalme
Thanks.
I broke set_field_to_openflow() into a couple of helpers, so that
set_field_to_openflow() is now just:
static void
set_field_to_openflow(const struct ofpact_set_field *sf,
struct ofpbuf *openflow)
{
struct ofp_header *oh = (struct ofp_header *)openflow->l2;
if (oh->version >= OFP12_VERSION) {
set_field_to_openflow12(sf, openflow);
} else if (oh->version == OFP11_VERSION) {
set_field_to_openflow11(sf, openflow);
} else if (oh->version == OFP10_VERSION) {
set_field_to_openflow10(sf, openflow);
} else {
NOT_REACHED();
}
}
I felt that this made the code more readable.
Applied, with that change.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 06/10] lib/ofp-actions: Set field OF 1.0/1.1 compatibility.
On Nov 1, 2013, at 4:04 PM, Ben Pfaff wrote:
> On Thu, Oct 24, 2013 at 01:19:30PM -0700, Jarno Rajahalme wrote:
>> Output set field actions as standard OF1.0/1.1 set actions or to
>> reg_load instructions, when a compatible set action(s) do not exist.
>>
>> Signed-off-by: Jarno Rajahalme
>
> Thanks.
>
> I broke set_field_to_openflow() into a couple of helpers, so that
> set_field_to_openflow() is now just:
>
> static void
> set_field_to_openflow(const struct ofpact_set_field *sf,
> struct ofpbuf *openflow)
> {
>struct ofp_header *oh = (struct ofp_header *)openflow->l2;
>
>if (oh->version >= OFP12_VERSION) {
>set_field_to_openflow12(sf, openflow);
>} else if (oh->version == OFP11_VERSION) {
>set_field_to_openflow11(sf, openflow);
>} else if (oh->version == OFP10_VERSION) {
>set_field_to_openflow10(sf, openflow);
>} else {
>NOT_REACHED();
>}
> }
>
> I felt that this made the code more readable.
>
> Applied, with that change.
Thanks Ben! I’m all for clarity here,
Jarno
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 07/10] lib/ofp-action: Simplify interface and internal structure.
On Thu, Oct 24, 2013 at 01:19:31PM -0700, Jarno Rajahalme wrote: > This makes later changes simpler. > > Signed-off-by: Jarno Rajahalme Thanks! Applied to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 08/10] ofp-actions: Send deprecated actions as set fields.
On Thu, Oct 24, 2013 at 01:19:32PM -0700, Jarno Rajahalme wrote: > Signed-off-by: Jarno Rajahalme Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Handle oversized actions more gracefully.
This looks good in principle.
Test 706 (ofproto-dpif - too many output actions) fails on this, as it
expects the old VLOG_ERR in the logs.
There is also an error from ofproto_dpif_xlate in this case,
complaining about resubmits yielding >64kB of actions. I assume this
is intended to remain as a hint in the logs that something's a bit
strange, even though we should now handle this case.
On Wed, Oct 9, 2013 at 6:09 PM, Ben Pfaff wrote:
>>
>> If the datapath actions exceed the maximum size of a Netlink attribute
>> (about 64 kB), then previously we would assert-fail (before commit
>> 542024c4c3d36 "ofproto-dpif-xlate: Suppress oversize datapath actions.")
>> or just drop all of them (after that commit). This commit makes OVS cope
>> by slow-pathing the flow and executing all of its actions in userspace.
>>
>> Signed-off-by: Ben Pfaff
>> ---
>> lib/dpif.c |4 +++-
>> ofproto/ofproto-dpif-xlate.c | 11 ++-
>> 2 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index b284e13..ed84f5f 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1183,6 +1183,8 @@ dpif_execute__(struct dpif *dpif, const struct
>> dpif_execute *execute)
>> * OVS_ACTION_ATTR_USERSPACE actions it passes the packet through to the
>> dpif
>> * implementation.
>> *
>> + * This works even if 'actions_len' is too long for a Netlink attribute.
>> + *
>> * Returns 0 if successful, otherwise a positive errno value. */
>> int
>> dpif_execute(struct dpif *dpif,
>> @@ -1198,7 +1200,7 @@ dpif_execute(struct dpif *dpif,
>> execute.actions = actions;
>> execute.actions_len = actions_len;
>> execute.packet = buf;
>> -execute.needs_help = needs_help;
>> +execute.needs_help = needs_help || nl_attr_oversized(actions_len);
>> return dpif_execute__(dpif, &execute);
>> }
>>
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 4fb0d5e..ec83db7 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -49,6 +49,7 @@
>> #include "vlog.h"
>>
>> COVERAGE_DEFINE(xlate_actions);
>> +COVERAGE_DEFINE(xlate_actions_oversize);
>>
>> VLOG_DEFINE_THIS_MODULE(ofproto_dpif_xlate);
>>
>> @@ -2832,11 +2833,11 @@ xlate_actions__(struct xlate_in *xin, struct
>> xlate_out *xout)
>>
>> if (nl_attr_oversized(ctx.xout->odp_actions.size)) {
>> /* These datapath actions are too big for a Netlink attribute, so
>> we
>> - * can't execute them. */
>> -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> -
>> -VLOG_ERR_RL(&rl, "discarding oversize datapath actions");
>> -ofpbuf_clear(&ctx.xout->odp_actions);
>> + * can't hand them to the kernel directly. dpif_execute() can
>> execute
>> + * them one by one with help, so just mark the result as
>> SLOW_ACTION to
>> + * prevent the flow from being installed. */
>> +COVERAGE_INC(xlate_actions_oversize);
>> +ctx.xout->slow |= SLOW_ACTION;
>> }
>>
>> ofpbuf_uninit(&ctx.stack);
>> --
>> 1.7.10.4
>>
>> ___
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] netdev-linux: Skip miimon execution when disabled
When dealing with a large number of ports, one of the performance
bottlenecks is that we loop through all netdevs in the main loop. Miimon
is a contributor to this, checking all devices even if it has never been
enabled.
This patch introduces a counter for the number of netdevs with miimon
configured. If this is 0, then we skip miimon_run() and miimon_wait().
In a test environment of 5000 internal ports and 50 tunnel ports with
bfd, this reduces CPU usage from about 50% to about 45%.
Signed-off-by: Joe Stringer
---
v2: Shift netdev cleanup code from bridge.c to netdev-linux.c
Collapse "miimon enabled" check into a new function
---
lib/netdev-linux.c | 37 +++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 2752623..7e75144 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -61,6 +61,7 @@
#include "netlink.h"
#include "ofpbuf.h"
#include "openflow/openflow.h"
+#include "ovs-atomic.h"
#include "packets.h"
#include "poll-loop.h"
#include "rtnetlink-link.h"
@@ -402,6 +403,11 @@ struct netdev_rx_linux {
* additional log messages. */
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+/* Polling miimon status for all ports causes performance degradation when
+ * handling a large number of ports. If there are no devices using miimon, then
+ * we skip netdev_linux_miimon_run() and netdev_linux_miimon_wait(). */
+static atomic_int miimon_cnt = ATOMIC_VAR_INIT(0);
+
static void netdev_linux_run(void);
static int netdev_linux_do_ethtool(const char *name, struct ethtool_cmd *,
@@ -423,6 +429,7 @@ static int set_etheraddr(const char *netdev_name, const
uint8_t[ETH_ADDR_LEN]);
static int get_stats_via_netlink(int ifindex, struct netdev_stats *stats);
static int get_stats_via_proc(const char *netdev_name, struct netdev_stats
*stats);
static int af_packet_sock(void);
+static bool netdev_linux_miimon_enabled(void);
static void netdev_linux_miimon_run(void);
static void netdev_linux_miimon_wait(void);
@@ -485,13 +492,24 @@ netdev_linux_notify_sock(void)
return sock;
}
+static bool
+netdev_linux_miimon_enabled(void)
+{
+int miimon;
+
+atomic_read(&miimon_cnt, &miimon);
+return miimon > 0;
+}
+
static void
netdev_linux_run(void)
{
struct nl_sock *sock;
int error;
-netdev_linux_miimon_run();
+if (netdev_linux_miimon_enabled()) {
+netdev_linux_miimon_run();
+}
sock = netdev_linux_notify_sock();
if (!sock) {
@@ -553,7 +571,9 @@ netdev_linux_wait(void)
{
struct nl_sock *sock;
-netdev_linux_miimon_wait();
+if (netdev_linux_miimon_enabled()) {
+netdev_linux_miimon_wait();
+}
sock = netdev_linux_notify_sock();
if (sock) {
nl_sock_wait(sock, POLLIN);
@@ -711,6 +731,11 @@ netdev_linux_destruct(struct netdev *netdev_)
close(netdev->tap_fd);
}
+if (netdev->miimon_interval > 0) {
+int junk;
+atomic_sub(&miimon_cnt, 1, &junk);
+}
+
ovs_mutex_destroy(&netdev->mutex);
}
@@ -1222,6 +1247,14 @@ netdev_linux_set_miimon_interval(struct netdev *netdev_,
ovs_mutex_lock(&netdev->mutex);
interval = interval > 0 ? MAX(interval, 100) : 0;
if (netdev->miimon_interval != interval) {
+int junk;
+
+if (interval && !netdev->miimon_interval) {
+atomic_add(&miimon_cnt, 1, &junk);
+} else if (!interval && netdev->miimon_interval) {
+atomic_sub(&miimon_cnt, 1, &junk);
+}
+
netdev->miimon_interval = interval;
timer_set_expired(&netdev->miimon_timer);
}
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] nicira-ext: Update comment.
Acked-by: Joe Stringer On Thu, Oct 17, 2013 at 3:29 PM, Ben Pfaff wrote: > The ARP headers have been acceptable as NXAST_REG_MOVE destinations since > commit f6c8a6b163 (Add software switch support for modifying ARP headers > in OpenFlow.) > > Reported-by: Anupam Chanda > Signed-off-by: Ben Pfaff > --- > include/openflow/nicira-ext.h |5 + > 1 file changed, 5 insertions(+) > > diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h > index ca272fd..644885b 100644 > --- a/include/openflow/nicira-ext.h > +++ b/include/openflow/nicira-ext.h > @@ -492,6 +492,11 @@ OFP_ASSERT(sizeof(struct nx_action_pop_queue) == 16); > * - NXM_OF_TCP_DST > * - NXM_OF_UDP_SRC > * - NXM_OF_UDP_DST > + * - NXM_NX_ARP_SHA > + * - NXM_NX_ARP_THA > + * - NXM_OF_ARP_OP > + * - NXM_OF_ARP_SPA > + * - NXM_OF_ARP_TPA > * Modifying any of the above fields changes the corresponding packet > * header. > * > -- > 1.7.10.4 > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] ofp-util: Enforce OpenFlow 1.1+ table_id requirements in flow_mod messages.
On Fri, Nov 01, 2013 at 03:42:30PM -0700, Jarno Rajahalme wrote:
>
> On Nov 1, 2013, at 9:43 AM, Ben Pfaff wrote:
>
> > Signed-off-by: Ben Pfaff
> > ---
> > lib/ofp-util.c | 21 +-
> > tests/learn.at |4 +--
> > tests/ofp-print.at | 12
> > tests/ovs-ofctl.at | 48 +++
> > utilities/ovs-ofctl.8.in | 71
> > +++---
> > 5 files changed, 106 insertions(+), 50 deletions(-)
> >
> > diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> > index 3d9efab..e6a9494 100644
> > --- a/lib/ofp-util.c
> > +++ b/lib/ofp-util.c
> > @@ -1530,7 +1530,19 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
> > }
> > fm->modify_cookie = false;
> > fm->command = ofm->command;
> > +
> > +/* Get table ID.
> > + *
> > + * OF1.1 entirely forbids table_id == 255.
> > + * OF1.2+ allows table_id == 255 only for deletes. */
> > fm->table_id = ofm->table_id;
> > +if (fm->table_id == 0xff
>
> I?d rather see ?255? here as well to match the comments.
OK, I made that change.
> > +&& (oh->version == OFP11_VERSION
> > +|| (ofm->command != OFPFC_DELETE &&
> > +ofm->command != OFPFC_DELETE_STRICT))) {
> > +return OFPERR_OFPFMFC_BAD_TABLE_ID;
> > +}
> > +
>
> > +For flow dump commands, limits the flows dumped to those in the table
> > +with the given \fInumber\fR between 0 and 254. If not specified (or if
> > +255 is specified as \fInumber\fR), then flows in all tables are
> > +dumped.
> > +.
> > +.IP
> > +For flow table modification commands, behavior varies based on the
> > +OpenFlow version used to connect to the switch:
> > +.
> > +.RS
> > +.IP "OpenFlow 1.0"
> > +OpenFlow 1.0 does not support \fBtable\fR for modifying flows.
> > +\fBovs\-ofctl\fR will exit with an error if \fBtable\fR (other than
> > +\fBtable=255\fR) is specified for a switch that only supports OpenFlow
> > +1.0.
> > +.IP
> > +In OpenFlow 1.0, the switch chooses the table into which to insert a
> > +new flow. The Open vSwitch software switch always chooses table 0.
> > +Other Open vSwitch datapaths and other OpenFlow implementations may
> > +choose different tables.
>
> IMO any implementation should pick the ?input table?, which by
> definition is 0, or no?
The design of the pipeline in OF1.0 was motivated quite differently from
the design in OF1.1 and later versions. In OF1.0, multiple flow tables
existed only to illustrate the structure of the switch to the
controller. In particular, at Nicira we had a notion that it would be
useful, for optimization in a software switch, to expose three tables:
one that handled only exact match on all fields (happening first), one
that handled matches on all fields except for the ephemeral port in a
TCP connection (Martin thought this was important but I didn't really
understand the idea so this never got implemented), and one that
implemented wildcards via linear search. Major changes happened later
that got rid of this idea (among them, we realized that exact match only
would never perform adequately, we figured out a better classifier than
linear, and we found uses for explicitly designated multiple flow
tables) and so the OF1.0 pipeline design pretty much died. But an OF1.0
switch is still technically allowed to put a flow in whichever flow
table it likes.
I'm going to commit this soon. We can make the documentation clearer if
you think it's not clear yet.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 3/3] ofproto: Centralize action checking, doing it at decode time.
On Fri, Nov 01, 2013 at 03:58:52PM -0700, Jarno Rajahalme wrote:
>
> On Nov 1, 2013, at 9:43 AM, Ben Pfaff wrote:
>
> > Jarno pointed out that modify_flows__() didn't really need to check every
> > instance of the flow separately. After some further investigation I
> > decided that this was even more of an improvement.
> >
>
> Indeed! Check the actions right after they are decoded rather than
> multiple places afterwards.
>
> One question below, otherwise:
>
> Acked-by: Jarno Rajahalme
Thanks!
> > @@ -1700,7 +1701,7 @@ ofpact_check__(const struct ofpact *a, struct flow
> > *flow, ofp_port_t max_ports,
> > case OFPACT_WRITE_ACTIONS: {
> > struct ofpact_nest *on = ofpact_get_WRITE_ACTIONS(a);
> > return ofpacts_check(on->actions, ofpact_nest_get_action_len(on),
> > - flow, max_ports, table_id, false);
> > + flow, false, max_ports, table_id, n_tables);
> > }
> >
> > case OFPACT_WRITE_METADATA:
> > @@ -1714,11 +1715,14 @@ ofpact_check__(const struct ofpact *a, struct flow
> > *flow, ofp_port_t max_ports,
> > return 0;
> > }
> >
> > -case OFPACT_GOTO_TABLE:
> > -if (ofpact_get_GOTO_TABLE(a)->table_id <= table_id) {
> > +case OFPACT_GOTO_TABLE: {
> > +uint8_t goto_table = ofpact_get_GOTO_TABLE(a)->table_id;
> > +if ((table_id != 255 && goto_table <= table_id)
>
> Can the table id really be 255 at this point? I just learned that 255
> is not allowed in OF 1.1, and in OF1.2(+) it is allowed only on
> deletes, which should have no actions.
I think there's a bug elsewhere that might allow it to squeak through.
When parse_ofp_str__() parses a flow it can't always tell what version
of OpenFlow is going to be used in advance, so it can provide table ==
255 even with a flow that contains a Goto-Table.
(ofputil_encode_flow_mod() will later squash 255 into 0 for
transmission.) I guess we can or should fix that.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 09/10] lib/ofp-actions: Warn on deprecated actions.
On Thu, Oct 24, 2013 at 01:19:33PM -0700, Jarno Rajahalme wrote: > Signed-off-by: Jarno Rajahalme Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 10/10] Add OF11 SET MPLS LABEL and SET MPLS TC actions.
On Thu, Oct 24, 2013 at 01:19:34PM -0700, Jarno Rajahalme wrote: > Signed-off-by: Jarno Rajahalme Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2.47 0/4] MPLS actions and matches
On Thu, Oct 31, 2013 at 04:16:28PM +0900, Simon Horman wrote: > This series implements MPLS actions and matches based on work by > Ravi K, Leo Alterman, Yamahata-san and Joe Stringer. Would you mind rebasing this one more time? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] debian: Option to create tunnel through 'interfaces'.
2013/11/1 Gurucharan Shetty : > It is not possible with the current code. It only looks at > /etc/network/interfaces But if for example cat all files to buffer in case of source and operates not on interfaces but on buffered contents of all files? -- Vasiliy Tolstov, e-mail: v.tols...@selfip.ru jabber: v...@selfip.ru ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v8 05/16] ofproto-dpif: Implement group callbacks
On Wed, Oct 30, 2013 at 06:17:09PM +0900, Simon Horman wrote: > This is a first step towards implementing the dpif side of groups. > > In order to be useful the action translation code needs > to be taught about groups. > > Signed-off-by: Simon Horman Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v8 06/16] ofproto: Verify compatibility of liveness of groups with their type
On Wed, Oct 30, 2013 at 06:17:10PM +0900, Simon Horman wrote: > Require liveness for fast-failover groups as it is mandated by OpenFlow1.3. > Allow livness for select groups which is in keeping with OpenFlow1.3. > Disallow liveness it for other group types. > > Signed-off-by: Simon Horman Applied, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
