On Wed, Nov 06, 2013 at 10:46:04PM +0800, Alexander Wu wrote:
>
> V2:
> 1. fix function align, align to 79 bytes, fix BE64 to UINT64, etc.
> 2. fix n_tables_miss init type.
> 3. Change calls from ofputil.
>
> Simon Horman suggestions:
> 1. Fix CodingStyle, some coding error.
> 2. Fix the init functions to macros. - I'll fix it later.
>
> V1:
> 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.
>
> Signed-off-by: Alexander Wu <[email protected]>
> ---
> ofproto/ofproto.c | 388
> ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 387 insertions(+), 1 deletions(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 0c66a59..9494925 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -590,6 +590,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);
> +
> +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));
I think that you should either remove this memset or move
it into table_features_init(). Likewise for
ofproto_init_real_table_features()
> + 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);
> + }
> +}
> +
> +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. */
> @@ -606,7 +645,277 @@ 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)
> +{
> + int i;
> + int olen;
> +
> + int OFPIT13_END = OFPIT13_METER;
I still don't like OFPIT13_END. Can you either remove it
or make it lowercase?
> + 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);
sizeof is an operator. You probably don't need the ().
Likewise elsewhere in this patch.
> +
> + 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);
> +
> +}
> +
> +static void
> +table_next_table_init(struct ofputil_table_features *tf,
> + enum ofputil_table_feature_prop_type type)
> +{
> + int olen;
> + uint8_t i;
> + uint8_t *next_table;
> + uint8_t cursor = 0;
> +
> + olen = (OFTABLE_NUM - tf->table_id - 1) * sizeof(uint8_t);
> +
> + /* last table */
> + if (olen == 0)
> + return ;
if (olen == 0) {
return;
}
> +
> + next_table = xzalloc(olen);
> +
> + tf->props[type].data = next_table;
> +
> + for(i = tf->table_id; i < OFTABLE_NUM - 1; i++) {
> + next_table[cursor++] = i + 1;
> + }
> +
> + tf->props[type].type = type;
> + tf->props[type].length = olen
> + + sizeof(struct ofp13_table_feature_prop_header);
> + 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);
> +}
> +
> +static void
> +table_action_init(struct ofputil_table_features *tf,
> + enum ofputil_table_feature_prop_type type)
> +{
> + int i;
> + int olen;
> + struct ofp_action_header *action;
> + const int ACTION_NUM = OFPAT13_POP_PBB + 1;
Could you make ACTION_NUM lowercase?
> +
> + 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++) {
> + action[i].type = i;
> + action[i].len = 8;
> + }
> +
> + tf->props[type].length = olen
> + + sizeof(struct ofp13_table_feature_prop_header);
> + 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);
> +}
> +
> +static void
> +table_oxm_init(struct ofputil_table_features *tf,
> + enum ofputil_table_feature_prop_type type)
> +{
> + int i = 0 ,olen = 0;
> + const int OXM_NUM = get_oxm_num();
Could you make OXM_NUM lowercase?
> + 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_info[i].type;
> + }
> +
> + tf->props[type].length = olen
> + + sizeof(struct ofp13_table_feature_prop_header);
> + 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;
> +}
> +
> +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 = UINT64_MAX;
> + tf->metadata_write = UINT64_MAX;
> + tf->config = OFPTC11_TABLE_MISS_MASK;
> + tf->max_entries = 1000000;
> +
> + tf->length = 0; /* useless now */
> + tf->n_property = 0; /* update when needed */
Please enhance or remove the comments above.
> +
> + /* 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. */
Ditto.
> + 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
> @@ -1247,6 +1556,7 @@ static void
> ofproto_destroy__(struct ofproto *ofproto)
> OVS_EXCLUDED(ofproto_mutex)
> {
> + int i;
> struct oftable *table;
>
> ovs_assert(list_is_empty(&ofproto->pending));
> @@ -1273,6 +1583,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);
> }
> @@ -5766,6 +6080,73 @@ 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)
> +{
> + /* CHECK if it doesn't need copy */
> + memcpy(features, &ofproto->tables[id].tf, sizeof(*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;
I think that you need to implement some locking to ensure exclusive access
to tf here.
> +
> + /*
> + * NOTE the props' pointers have been copied, so we free the olds
> + * Currently we use table_features_destroy func, FIXME with a specified
> + */
> + table_features_destroy(tf);
> + memcpy(tf, features, sizeof *features);
> +
> + return ;
> +}
> +
> +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;
> +
> + error = ofputil_pull_table_features(oh, &features_num,
> + request_features, &request_flag);
> + if (error) {
> + return error;
> + }
> +
> + switch (request_flag) {
> + case OVS_TF_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 OVS_TF_SET: {
> + /* NOT IMPLEMENT */
I think you should expand the comment above to explain
how much is implemented and what is not implemented.
I think that the basic situation is that set updates ofproto
but nothing uses the settings: which I think is quite reasonable.
> + for (i = 0; i < features_num; i++) {
> + table_features_set(ofproto, &request_features[i]);
> + }
> + break;
> + }
> + }
> +
> + return 0;
> +}
> +
> static enum ofperr
> handle_table_mod(struct ofconn *ofconn, const struct ofp_header *oh)
> {
> @@ -5915,8 +6296,13 @@ 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:
> return handle_queue_get_config_request(ofconn, oh);
> + /* fallthrough */
But it doesn't fall-through. It returns.
>
> case OFPTYPE_HELLO:
> case OFPTYPE_ERROR:
> @@ -5945,7 +6331,6 @@ handle_openflow__(struct ofconn *ofconn, const struct
> ofpbuf *msg)
> case OFPTYPE_METER_STATS_REPLY:
> case OFPTYPE_METER_CONFIG_STATS_REPLY:
> case OFPTYPE_METER_FEATURES_STATS_REPLY:
> - case OFPTYPE_TABLE_FEATURES_STATS_REQUEST:
> case OFPTYPE_TABLE_FEATURES_STATS_REPLY:
> case OFPTYPE_ROLE_STATUS:
> default:
> @@ -6630,6 +7015,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
> --
> 1.7.3.1.msysgit.0
>
>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
