[ovs-dev] [PATCH] ovs-ofctl : Setting of importance parameter
Rishi Bamba
rishi.bamba at tcs.com
Mon Oct 27 11:51:42 UTC 2014
Hi Ben,
Taking care of the points mentioned in the last review , we are submitting this patch which ensures the following :
- Importance is set everywhere where it is meant to be.Also we have identified two structures i.e ofputil_flow_removed & ofputil_flow_update where changes related to importance are not done.Can you please verify the same.
- Formatting is in accordance with the coding guidelines.
Regarding “OFP_FLOW_PERMANENT” flag for importance parameter, we have intentionally used the same flag, as in similarity to the hard_timeout & idle_timeout if the value is not provided,the flow will be considered permanent else if set than evictable which defaults to the Zero value only.
Also attached is the file which contains all the 5 patches for your reference.
Thank You
Regards
Rishi Bamba
----- Original Message -----
From: "Rishi Bamba" <rishi.bamba at tcsin.com>
To: "Ben Pfaff" <blp at nicira.com>
Cc: dev at openvswitch.org, "partha datta" <partha.datta at tcs.com>, "deepankar gupta" <deepankar.gupta at tcs.com>, "Rishi Bamba" <rishi.bamba at tcs.com>
Sent: Monday, October 27, 2014 5:17:36 PM
Subject: [PATCH] ovs-ofctl : Setting of importance parameter
This patch ensures setting of importance parameter of a flow
everywhere it should be. This also correct formatting issues
as per the coding guidelines.
Signed-off-by: Rishi Bamba <rishi.bamba at tcs.com>
---
include/openflow/openflow-1.1.h | 4 ++--
lib/learn.c | 1 +
lib/learning-switch.c | 1 +
lib/ofp-parse.c | 2 +-
lib/ofp-print.c | 5 ++++-
lib/ofp-util.h | 3 +--
ofproto/ofproto-dpif.c | 1 +
ofproto/ofproto.c | 2 ++
8 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
index d881192..d951dc7 100644
--- a/include/openflow/openflow-1.1.h
+++ b/include/openflow/openflow-1.1.h
@@ -340,7 +340,7 @@ struct ofp11_flow_mod {
output group. A value of OFPG11_ANY
indicates no restriction. */
ovs_be16 flags; /* One of OFPFF_*. */
- ovs_be16 importance; /* Eviction Precedence */
+ ovs_be16 importance; /* Eviction Precedence */
/* Followed by an ofp11_match structure. */
/* Followed by an instruction set. */
};
@@ -451,7 +451,7 @@ struct ofp11_flow_stats {
ovs_be16 idle_timeout; /* Number of seconds idle before expiration. */
ovs_be16 hard_timeout; /* Number of seconds before expiration. */
ovs_be16 flags; /* OF 1.3: Set of OFPFF*. */
- ovs_be16 importance; /* Eviction Precedence */
+ ovs_be16 importance; /* Eviction Precedence */
uint8_t pad2[2]; /* Align to 64-bits. */
ovs_be64 cookie; /* Opaque controller-issued identifier. */
ovs_be64 packet_count; /* Number of packets in flow. */
diff --git a/lib/learn.c b/lib/learn.c
index e1c73cb..e4f4a44 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -101,6 +101,7 @@ learn_execute(const struct ofpact_learn *learn, const struct flow *flow,
fm->command = OFPFC_MODIFY_STRICT;
fm->idle_timeout = learn->idle_timeout;
fm->hard_timeout = learn->hard_timeout;
+ fm->importance = 0;
fm->buffer_id = UINT32_MAX;
fm->out_port = OFPP_NONE;
fm->flags = 0;
diff --git a/lib/learning-switch.c b/lib/learning-switch.c
index e3b48d5..66dc907 100644
--- a/lib/learning-switch.c
+++ b/lib/learning-switch.c
@@ -202,6 +202,7 @@ lswitch_handshake(struct lswitch *sw)
fm.command = OFPFC_ADD;
fm.idle_timeout = 0;
fm.hard_timeout = 0;
+ fm.importance = 0;
fm.buffer_id = UINT32_MAX;
fm.out_port = OFPP_NONE;
fm.out_group = OFPG_ANY;
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 23626cb..a74ce40 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -248,7 +248,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
enum {
F_OUT_PORT = 1 << 0,
F_ACTIONS = 1 << 1,
- F_IMPORTANCE=1 << 2,
+ F_IMPORTANCE = 1 << 2,
F_TIMEOUT = 1 << 3,
F_PRIORITY = 1 << 4,
F_FLAGS = 1 << 5,
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 7f45858..3ca2cae 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -807,6 +807,9 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header *oh, int verbosity)
if (fm.hard_timeout != OFP_FLOW_PERMANENT) {
ds_put_format(s, "hard:%"PRIu16" ", fm.hard_timeout);
}
+ if (fm.importance != OFP_FLOW_PERMANENT) {
+ ds_put_format(s, "importance:%"PRIu16" ", fm.importance);
+ }
if (fm.priority != OFP_DEFAULT_PRIORITY && need_priority) {
ds_put_format(s, "pri:%"PRIu16" ", fm.priority);
}
@@ -1431,7 +1434,7 @@ ofp_print_flow_stats(struct ds *string, struct ofputil_flow_stats *fs)
if (fs->flags) {
ofp_print_flow_flags(string, fs->flags);
}
- if (fs->importance != OFP_FLOW_PERMANENT) {
+ if (fs->importance != OFP_FLOW_PERMANENT) {
ds_put_format(string, "importance=%"PRIu16", ", fs->importance);
}
if (fs->idle_age >= 0) {
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 3173ab4..d50aa4e 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -306,7 +306,7 @@ struct ofputil_flow_mod {
ofp_port_t out_port;
uint32_t out_group;
enum ofputil_flow_mod_flags flags;
- uint16_t importance; /* Eviction Precedence */
+ uint16_t importance; /* Eviction Precedence */
struct ofpact *ofpacts; /* Series of "struct ofpact"s. */
size_t ofpacts_len; /* Length of ofpacts, in bytes. */
@@ -393,7 +393,6 @@ struct ofputil_flow_removed {
uint16_t hard_timeout;
uint64_t packet_count; /* Packet count, UINT64_MAX if unknown. */
uint64_t byte_count; /* Byte count, UINT64_MAX if unknown. */
- uint16_t importance; /* Eviction Precedence */
};
enum ofperr ofputil_decode_flow_removed(struct ofputil_flow_removed *,
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d965d38..5b5614d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5313,6 +5313,7 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
fm.command = OFPFC_ADD;
fm.idle_timeout = idle_timeout;
fm.hard_timeout = 0;
+ fm.importance = 0;
fm.buffer_id = 0;
fm.out_port = 0;
fm.flags = OFPUTIL_FF_HIDDEN_FIELDS | OFPUTIL_FF_NO_READONLY;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 63338aa..6f1c781 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1886,6 +1886,7 @@ flow_mod_init(struct ofputil_flow_mod *fm,
fm->command = command;
fm->idle_timeout = 0;
fm->hard_timeout = 0;
+ fm->importance = 0;
fm->buffer_id = UINT32_MAX;
fm->out_port = OFPP_ANY;
fm->out_group = OFPG_ANY;
@@ -1982,6 +1983,7 @@ ofproto_flow_mod(struct ofproto *ofproto, struct ofputil_flow_mod *fm)
actions = rule_get_actions(rule);
if (rule->idle_timeout == fm->idle_timeout
&& rule->hard_timeout == fm->hard_timeout
+ && rule->importance == fm->importance
&& rule->flags == (fm->flags & OFPUTIL_FF_STATE)
&& (!fm->modify_cookie || (fm->new_cookie == rule->flow_cookie))
&& ofpacts_equal(fm->ofpacts, fm->ofpacts_len,
--
2.1.1
Thank You
Regards
Rishi Bamba
----- Original Message -----
From: "Ben Pfaff" <blp at nicira.com>
To: "Rishi Bamba" <rishi.bamba at tcs.com>
Cc: dev at openvswitch.org, "partha datta" <partha.datta at tcs.com>, "deepankar gupta" <deepankar.gupta at tcs.com>
Sent: Saturday, October 11, 2014 2:01:51 AM
Subject: Re: [PATCH 1/4] ovs-ofctl:To set importance of a rule for eviction(OF14)
On Fri, Oct 10, 2014 at 07:11:45PM +0530, Rishi Bamba wrote:
> Hi Ben,
>
> # Thank you for reviewing the patch.As per the comments received ,all the changes are made and incorporated for this patch i.e Clang
> # compiler doesn't report any error now and all the test cases are successful including the one added by us for the same.Also along
> # with this patch sending three other patches related to the addition of importance in a rule which includes the changes as asked by
> # you plus replace-flows and diff-flows CLI enhancement after the addition of "importance" parameter in a rule as per OF14.
> ---
> This patch enables a user to set importance for a new rule via add-flow
> OF1.1+ in the OVS and display the same via dump-flows command OF1.1+ .
> The changes are made in accordance with OpenFlow 1.4 specs to implement
> Eviction on the basis of "importance".
Thanks for the patch. I have some comments.
I'd appreciate it if you would be more careful about formatting your
patch emails. The commit message should be at the top, and any
additional commentary should be below the ---. Only the part before
--- actually goes into the commit message, so this ensures that the
commit message has the content that you intend.
In this hunk, please follow the same formatting pattern as the other
lines:
> @@ -248,6 +248,7 @@ parse_ofp_str__(struct ofputil_flow_mod *fm, int command, char *string,
> enum {
> F_OUT_PORT = 1 << 0,
> F_ACTIONS = 1 << 1,
> + F_IMPORTANCE=1 << 2,
> F_TIMEOUT = 1 << 3,
> F_PRIORITY = 1 << 4,
> F_FLAGS = 1 << 5,
OFP_FLOW_PERMANENT is meant to be used for hard_timeout and
idle_timeout. I don't think that we should reuse it for importance
also (please just write 0):
> + if (fs->importance != OFP_FLOW_PERMANENT) {
It looks like some places where 'importance' should be set were
missed. When I grep for ofputil_flow_mod, I see that, for example,
learn_execute() needs to set 'importance'. Please do your own grep
and make sure that importance is set everywhere it should be.
After you fix those problems, I think that this patch will be ready.
Thanks,
Ben.
=====-----=====-----=====
Notice: The information contained in this e-mail
message and/or attachments to it may contain
confidential or privileged information. If you are
not the intended recipient, any dissemination, use,
review, distribution, printing or copying of the
information contained in this e-mail message
and/or attachments to it are strictly prohibited. If
you have received this communication in error,
please notify us by reply e-mail or telephone and
immediately and permanently delete the message
and any attachments. Thank you
More information about the dev
mailing list