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().
>
> And now the cli we implement is very crude. Maybe it could
> display better.
>
> Signed-off-by: Alexander Wu <alexander...@huawei.com>
> ---
> 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?
> +
> +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.
> +
> +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.
> + int i = 0;
> + int olen = 0;
> +
> + struct ofputil_instruction *inst = NULL;
> + int element_size = sizeof(struct ofputil_instruction);
> +
> + olen = OFPIT13_END * element_size;
> +
> + inst = (struct ofputil_instruction *)xmalloc(olen);
> + memset(inst, 0, olen);
> +
> + tf->props[type].data = (uint8_t*)inst;
> + for(i = 0 ; i < OFPIT13_END ; i++){
> + /* ofp11(3)_instruction_type */
> + inst[i].type = i + 1;
> + inst[i].len = element_size;
> + }
> +
> + tf->props[type].type = type;
> + tf->props[type].length = olen + sizeof(struct
> ofp13_table_feature_prop_header);
This line is greater than 80 columns wide. I'm unsure if it matters.
> +
> + tf->n_property++;
> +}
> +
> +static void
> +table_features_INSTRUCTIONS_init(struct ofputil_table_features *tf)
> +{
> + table_instructions_init(tf, OFPUTIL_INSTRUCTIONS);
> +}
> +
> +static void
> +table_features_INSTRUCTIONS_MISS_init(struct ofputil_table_features *tf)
> +{
> + table_instructions_init(tf, OFPUTIL_INSTRUCTIONS_MISS);
> +
> +}
I'm unsure that there is much value in these wrapper functions.
Are they use as callbacks or as pointers in a function table?
If not I suggest just open-coding the one-line that they encapsulate.
If you really need these, and the numerous similar ones below,
then perhaps they could at all be grouped together. And automatically
generated using a macro.
> +
> +static void
> +table_next_table_init(struct ofputil_table_features *tf,enum
> ofputil_table_feature_prop_type type)
I believe that the following is more in keeping with the prevailing coding
style:
static void
table_next_table_init(struct ofputil_table_features *tf,
enum ofputil_table_feature_prop_type type)
> +{
> + const int MAX_TABLE = OFTABLE_NUM;
See comments regarding OFPIT13_END above.
> +
> + int olen = 0;
olen is re-initialised immediately below.
I would either initialise it here to the value set below or
not initialise it at all here.
> + uint8_t i = 0;
> + uint8_t cursor = 0;
> + uint8_t *next_table = NULL;
> +
> + olen = (MAX_TABLE - tf->table_id - 1) * sizeof(uint8_t);
> +
> + /* last table */
> + if (0 == olen)
> + return ;
The following would be more in keeping with the prevailing coding style.
if (olen == 0) {
return;
}
> +
> + next_table = xmalloc(olen);
> + memset(next_table, 0, olen);
Does next_table really need to be zeroed?
If so you can replace the above two lines with:
next_table = xzalloc(olen);
> +
> + tf->props[type].data = next_table;
> +
> + for(i = tf->table_id; i < MAX_TABLE - 1; i++)
> + next_table[cursor++] = i + 1;
Please add a space after for. And use { }
for (i = tf->table_id; i < MAX_TABLE - 1; i++) {
next_table[cursor++] = i + 1;
}
> +
> + tf->props[type].type = OFPUTIL_NEXT_TABLES;
> + tf->props[type].length = olen + sizeof(struct
> ofp13_table_feature_prop_header);
The line above is more than 80 columns wide.
> + tf->n_property++;
> +}
> +
> +static void
> +table_features_NEXT_TABLES_init(struct ofputil_table_features *tf)
> +{
> + table_next_table_init(tf, OFPUTIL_NEXT_TABLES);
> +}
> +static void
> +table_features_NEXT_TABLES_MISS_init(struct ofputil_table_features *tf)
> +{
> + table_next_table_init(tf, OFPUTIL_NEXT_TABLES_MISS);
> +}
See comment regarding table_features_INSTRUCTIONS_MISS_init() and
table_features_INSTRUCTIONS_init().
> +
> +static void
> +table_action_init(struct ofputil_table_features *tf,
> + enum ofputil_table_feature_prop_type type)
> +{
> + int i = 0 ,olen = 0;
> + struct ofp_action_header *action = NULL;
> + const int ACTION_NUM = OFPAT13_POP_PBB + 1;
> +
> + olen = ACTION_NUM * sizeof(struct ofp_action_header);
> +
> + action = (struct ofp_action_header *)xzalloc(olen);
> +
> + tf->props[type].data = (uint8_t*)action;
> +
> + for(i = 0 ; i < ACTION_NUM; i++){
Please add a space before '(' and after ')'
for (i = 0 ; i < ACTION_NUM; i++) {
> + action[i].type = i;
The indentation of the above line is inconsistent with the one below.
> + action[i].len = 8;
> + }
> +
> + tf->props[type].length = olen + sizeof(struct
> ofp13_table_feature_prop_header);
The line above is more than 80 columns wide.
> + tf->props[type].type = type;
> + tf->n_property++;
> }
>
> +
> +static void
> +table_features_WRITE_ACTIONS_init(struct ofputil_table_features *tf)
> +{
> + table_action_init(tf, OFPUTIL_WRITE_ACTIONS);
> +}
> +
> +static void
> +table_features_WRITE_ACTIONS_MISS_init(struct ofputil_table_features *tf)
> +{
> + table_action_init(tf, OFPUTIL_WRITE_ACTIONS_MISS);
> +}
> +
> +static void
> +table_features_APPLY_ACTIONS_init(struct ofputil_table_features *tf)
> +{
> + table_action_init(tf, OFPUTIL_APPLY_ACTIONS);
> +}
> +
> +static void
> +table_features_APPLY_ACTIONS_MISS_init(struct ofputil_table_features *tf)
> +{
> + table_action_init(tf, OFPUTIL_APPLY_ACTIONS_MISS);
> +}
See comment regarding table_features_INSTRUCTIONS_MISS_init() and
table_features_INSTRUCTIONS_init().
> +
> +static void
> +table_oxm_init(struct ofputil_table_features *tf,
> + enum ofputil_table_feature_prop_type type)
I think there is an extra space before enum, it should be aligned
to start in the column after the '(' in the line above.
static void
table_oxm_init(struct ofputil_table_features *tf,
enum ofputil_table_feature_prop_type type)
> +{
> + int i = 0 ,olen = 0;
The space should come after the , not before it.
But I seem to recall that when initialising variable in their declaration
Ben prefers them on individual lines.
Also olen is set to a different value immediately below.
int i = 0;
int olen = 0;
Or re-arnage things so you can set olen to its final value.
> + const int OXM_NUM = get_oxm_num();
I don't think this should const or that the variable name should
not be capitalised.
> + uint32_t *oxm_ids = NULL;
> +
> + olen = sizeof(uint32_t) * OXM_NUM;
> + oxm_ids = xzalloc(olen);
> +
> + tf->props[type].data = (uint8_t*)oxm_ids;
> +
> + for (i = 0; i < OXM_NUM; i++) {
> + oxm_ids[i] = oxm_variables[i].data;
> + }
> +
> + tf->props[type].length = olen + sizeof(struct
> ofp13_table_feature_prop_header);
The line above is more than 80 columns wide.
> + tf->props[type].type = type;
> + tf->n_property++;
> +}
> +
> +static void
> +table_features_MATCH_init(struct ofputil_table_features *tf)
> +{
> + table_oxm_init(tf, OFPUTIL_MATCH);
> + return ;
> +}
> +
> +
> +static void
> +table_features_WILDCARDS_init(struct ofputil_table_features *tf)
> +{
> + table_oxm_init(tf, OFPUTIL_WILDCARDS);
> + return;
> +}
> +
> +static void
> +table_features_WRITE_SETFIELD_init(struct ofputil_table_features *tf)
> +{
> + table_oxm_init(tf, OFPUTIL_WRITE_SETFIELD);
> + return ;
> +}
> +
> +static void
> +table_features_WRITE_SETFIELD_MISS_init(struct ofputil_table_features *tf)
> +{
> + table_oxm_init(tf, OFPUTIL_WRITE_SETFIELD_MISS);
> + return ;
> +}
> +
> +static void
> +table_features_APPLY_SETFIELD_init(struct ofputil_table_features *tf)
> +{
> + table_oxm_init(tf, OFPUTIL_APPLY_SETFIELD);
> + return ;
> +}
> +
> +static void
> +table_features_APPLY_SETFIELD_MISS_init(struct ofputil_table_features *tf)
> +{
> + table_oxm_init(tf, OFPUTIL_APPLY_SETFIELD_MISS);
> + return;
> +}
> +
> +static void
> +table_features_EXPERIMENTER_init(
> + struct ofputil_table_features *tf OVS_UNUSED)
> +{
> + return;
> +}
> +
> +static void
> +table_features_EXPERIMENTER_MISS_init(
> + struct ofputil_table_features *tf OVS_UNUSED)
> +{
> + return;
> +}
See comment regarding table_features_INSTRUCTIONS_MISS_init() and
table_features_INSTRUCTIONS_init().
> +
> +static void
> +table_features_init(struct ofputil_table_features *tf, int table_id)
> +{
> + tf->table_id = table_id;
> + snprintf(tf->name, OFP_MAX_TABLE_NAME_LEN, "%s%d", "table", table_id);
> +
> + tf->metadata_match = OVS_BE64_MAX;
> + tf->metadata_write = OVS_BE64_MAX;
metadata_match and metadata_write are host endian (uint64_t) but
here you are initialising them as big endian (OVS_BE64_MAX). Either
their type of their initialisation is incorrect.
> + tf->config = OFPTC11_TABLE_MISS_MASK;
> + tf->max_entries = 1000000;
I'm curuious, why 1000000?
If it is an abitrary limit I would add a comment saying so.
Also, I would be tempted to make the limit the maximum possible value.
I.e. UINT32_MAX
> +
> + tf->length = 0; /* useless now */
Why is it useless?
> + tf->n_property = 0; /* update when needed */
> +
> + /* CHECK the props are inited to 0. */
> + memset(tf->props, 0, sizeof(tf->props));
> +
> + /* NOTE now the implement use ofp13_*, change it to ofputil_* later. */
> + table_features_INSTRUCTIONS_init(tf);
> + table_features_INSTRUCTIONS_MISS_init(tf);
> + table_features_NEXT_TABLES_init(tf);
> + table_features_NEXT_TABLES_MISS_init(tf);
> + table_features_WRITE_ACTIONS_init(tf);
> + table_features_WRITE_ACTIONS_MISS_init(tf);
> + table_features_APPLY_ACTIONS_init(tf);
> + table_features_APPLY_ACTIONS_MISS_init(tf);
> + table_features_MATCH_init(tf);
> + table_features_WILDCARDS_init(tf);
> + table_features_WRITE_SETFIELD_init(tf);
> + table_features_WRITE_SETFIELD_MISS_init(tf);
> + table_features_APPLY_SETFIELD_init(tf);
> + table_features_APPLY_SETFIELD_MISS_init(tf);
> + table_features_EXPERIMENTER_init(tf);
> + table_features_EXPERIMENTER_MISS_init(tf);
> +
> + /* FIXME Now we search the array at last. */
Is this FIXME still valid? Is the code usable or at least harmless as-is?
> + tf->n_property = 0xff;
> +}
> +
> +static void
> +table_features_destroy(struct ofputil_table_features *tf)
> +{
> + int i = 0;
> + int n = tf->n_property;
> + struct ofputil_table_feature_prop_header *props = tf->props;
> +
> + for (i = 0; i < n; i++) {
> + if (props[i].data) {
> + free(props[i].data);
> + props[i].data = NULL;
> + }
> + }
> +}
> +
> +
> /* To be optionally called (only) by an ofproto implementation in its
> * constructor function. See the large comment on 'construct' in struct
> * ofproto_class for details.
> @@ -1216,6 +1521,7 @@ static void
> ofproto_destroy__(struct ofproto *ofproto)
> OVS_EXCLUDED(ofproto_mutex)
> {
> + int i;
> struct oftable *table;
>
> ovs_assert(list_is_empty(&ofproto->pending));
> @@ -1242,6 +1548,10 @@ ofproto_destroy__(struct ofproto *ofproto)
> shash_destroy(&ofproto->port_by_name);
> simap_destroy(&ofproto->ofp_requests);
>
> + for (i = 0; i < ofproto->n_tables; i++) {
> + table_features_destroy(&ofproto->otf[i]);
> + }
> +
> OFPROTO_FOR_EACH_TABLE (table, ofproto) {
> oftable_destroy(table);
> }
> @@ -5648,6 +5958,74 @@ handle_group_mod(struct ofconn *ofconn, const struct
> ofp_header *oh)
> }
> }
>
> +static void table_features_get_by_id(struct ofproto *ofproto,
> + int id, struct ofputil_table_features *features)
static void
table_features_get_by_id(struct ofproto *ofproto, int id,
struct ofputil_table_features *features)
> +{
> + /* CHECK if it doesn't need copy */
Does this comment still apply?
> + memcpy(features, &ofproto->tables[id].tf, sizeof(*features));
I think "sizeof *features" is preferable to sizeof(*features)
as sizeof is not a function or a macro.
> +}
> +
> +
> +static void table_features_set(struct ofproto *ofproto,
> + struct ofputil_table_features *features)
static void
table_features_set(struct ofproto *ofproto,
struct ofputil_table_features *features)
> +{
> + uint8_t table_id = features->table_id;
> + struct ofputil_table_features *tf = &ofproto->tables[table_id].tf;
> +
> + /*
> + * NOTE the props' pointers have been copied, so we free the olds
> + * Currently we use table_features_destroy func, FIXME with a specified
> + */
Does this comment still apply?
> + table_features_destroy(tf);
> + memcpy(tf, features, sizeof(*features));
> +
> + return ;
> +}
> +
> +
One blank line should be sufficient here.
> +static enum ofperr
> +handle_table_features_stats_request(struct ofconn *ofconn,
> + const struct ofp_header *oh)
> +{
> +
> + struct ofproto *ofproto = ofconn_get_ofproto(ofconn);
> + struct ofputil_table_features request_features[ofproto->n_tables];
> + struct ofputil_table_features reply_features[ofproto->n_tables];
> + int features_num;
> + uint32_t request_flag = 0;
> + int error;
> + int i;
> +
> + struct list replies;
> +
> + /* decode request */
> + error = ofputil_decode_table_features_request(oh, &features_num,
> request_features,
The line above is greater than 80 columns wide.
> + &request_flag);
> + if (error) {
> + return error;
> + }
> +
> + switch (request_flag) {
> + case OTF_GET: {
> + ofpmp_init(&replies, oh);
> + for (i = 0; i < ofproto->n_tables; i++) {
> + table_features_get_by_id(ofproto, i, &reply_features[0]);
> + ofputil_append_table_features_reply(reply_features, &replies);
> + }
> + ofconn_send_replies(ofconn, &replies);
> + break;
> + }
> + case OTF_SET: {
> + for (i = 0; i < features_num; i++) {
> + table_features_set(ofproto, &request_features[i]);
> + }
> + break;
> + }
There is no need for the outer-most { } in the OTF_SET case unless you
wish to declare i there and in the OTF_GET case.
> + }
> +
> + return 0;
> +}
> +
> static enum ofperr
> handle_table_mod(struct ofconn *ofconn, const struct ofp_header *oh)
> {
> @@ -5680,6 +6058,7 @@ handle_openflow__(struct ofconn *ofconn, const struct
> ofpbuf *msg)
> if (error) {
> return error;
> }
> +
This change seems unrelated to the rest of the patch.
Please remove it.
> if (oh->version >= OFP13_VERSION && ofpmsg_is_stat_request(oh)
> && ofpmp_more(oh)) {
> /* We have no buffer implementation for multipart requests.
> @@ -5797,9 +6176,11 @@ handle_openflow__(struct ofconn *ofconn, const struct
> ofpbuf *msg)
> case OFPTYPE_GROUP_FEATURES_STATS_REQUEST:
> return handle_group_features_stats_request(ofconn, oh);
>
> + case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
> + return handle_table_features_stats_request(ofconn, oh);
> +
> /* FIXME: Change the following once they are implemented: */
> case OFPTYPE_QUEUE_GET_CONFIG_REQUEST:
> - case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
> /* fallthrough */
>
> case OFPTYPE_HELLO:
> @@ -6512,6 +6893,7 @@ oftable_destroy(struct oftable *table)
> oftable_disable_eviction(table);
> classifier_destroy(&table->cls);
> free(table->name);
> + table_features_destroy(&table->tf);
> }
>
> /* Changes the name of 'table' to 'name'. If 'name' is NULL or the empty
Its not a big deal. But it seems to me that the ovs-ofctl.c changes
below would comfortably go into a separate patch to be applied after this
one.
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 1a1d423..5970731 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -281,6 +281,7 @@ usage(void)
> " show SWITCH show OpenFlow information\n"
> " dump-desc SWITCH print switch description\n"
> " dump-tables SWITCH print table stats\n"
> + " dump-table-features SWITCH print table features\n"
> " mod-port SWITCH IFACE ACT modify port behavior\n"
> " mod-table SWITCH MOD modify flow table behavior\n"
> " get-frags SWITCH print fragment handling behavior\n"
> @@ -649,6 +650,21 @@ ofctl_dump_tables(int argc OVS_UNUSED, char *argv[])
> dump_trivial_stats_transaction(argv[1], OFPRAW_OFPST_TABLE_REQUEST);
> }
>
> +static void
> +ofctl_dump_table_features(int argc OVS_UNUSED, char *argv[])
> +{
> + struct ofpbuf *request;
> + struct vconn *vconn;
> +
> + open_vconn(argv[1], &vconn);
Do you need to check argc to make sure accessing argv[1] is valid?
> + request =
> ofputil_encode_table_features_request(vconn_get_version(vconn));
> + if (request) {
> + dump_stats_transaction(vconn, request);
> + }
> +
> + vconn_close(vconn);
> +}
> +
> static bool
> fetch_port_by_features(const char *vconn_name,
> const char *port_name, ofp_port_t port_no,
> @@ -3323,6 +3339,7 @@ static const struct command all_commands[] = {
> { "snoop", 1, 1, ofctl_snoop },
> { "dump-desc", 1, 1, ofctl_dump_desc },
> { "dump-tables", 1, 1, ofctl_dump_tables },
> + { "dump-table-features", 1, 1, ofctl_dump_table_features },
> { "dump-flows", 1, 2, ofctl_dump_flows },
> { "dump-aggregate", 1, 2, ofctl_dump_aggregate },
> { "queue-stats", 1, 3, ofctl_queue_stats },
> --
> 1.7.3.1.msysgit.0
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
