[dpdk-dev] [PATCH] pipeline: improve handling of learner table action arguments

[dpdk-dev] [PATCH] pipeline: improve handling of learner table action arguments

[Cristian Dumitrescu]

The arguments of actions that are learned are now specified as part of
the learn instruction as opposed to being statically specified as part
of the learner table configuration.

Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
---
Depends-on: series-18878 ("[V3,01/24] pipeline: move data structures to
internal header file")

 examples/pipeline/examples/learner.spec  |  6 +--
 lib/pipeline/rte_swx_pipeline.c          | 55 +++++++++---------------
 lib/pipeline/rte_swx_pipeline.h          | 10 -----
 lib/pipeline/rte_swx_pipeline_internal.h |  8 ++--
 lib/pipeline/rte_swx_pipeline_spec.c     | 35 ++-------------
 5 files changed, 31 insertions(+), 83 deletions(-)

diff --git a/examples/pipeline/examples/learner.spec b/examples/pipeline/examples/learner.spec
index d635422282..4ee52da7ac 100644
--- a/examples/pipeline/examples/learner.spec
+++ b/examples/pipeline/examples/learner.spec
@@ -84,7 +84,7 @@ action learn_action args none {
 	// Add the current lookup key to the table with fwd_action as the key action. The action
 	// arguments are read from the packet meta-data (the m.fwd_action_arg_port_out field). These
 	// packet meta-data fields have to be written before the "learn" instruction is invoked.
-	learn fwd_action
+	learn fwd_action m.fwd_action_arg_port_out
 
 	// Send the current packet to the same output port.
 	mov m.port_out m.fwd_action_arg_port_out
@@ -101,9 +101,9 @@ learner fwd_table {
 	}
 
 	actions {
-		fwd_action args m.fwd_action_arg_port_out
+		fwd_action
 
-		learn_action args none
+		learn_action
 	}
 
 	default_action learn_action args none
diff --git a/lib/pipeline/rte_swx_pipeline.c b/lib/pipeline/rte_swx_pipeline.c
index 31f0029404..1cd09a4b44 100644
--- a/lib/pipeline/rte_swx_pipeline.c
+++ b/lib/pipeline/rte_swx_pipeline.c
@@ -2359,6 +2359,9 @@ action_find(struct rte_swx_pipeline *p, const char *name);
 static int
 action_has_nbo_args(struct action *a);
 
+static int
+learner_action_args_check(struct rte_swx_pipeline *p, struct action *a, const char *mf_name);
+
 static int
 instr_learn_translate(struct rte_swx_pipeline *p,
 		      struct action *action,
@@ -2368,16 +2371,31 @@ instr_learn_translate(struct rte_swx_pipeline *p,
 		      struct instruction_data *data __rte_unused)
 {
 	struct action *a;
+	const char *mf_name;
+	uint32_t mf_offset = 0;
 
 	CHECK(action, EINVAL);
-	CHECK(n_tokens == 2, EINVAL);
+	CHECK((n_tokens == 2) || (n_tokens == 3), EINVAL);
 
 	a = action_find(p, tokens[1]);
 	CHECK(a, EINVAL);
 	CHECK(!action_has_nbo_args(a), EINVAL);
 
+	mf_name = (n_tokens > 2) ? tokens[2] : NULL;
+	CHECK(!learner_action_args_check(p, a, mf_name), EINVAL);
+
+	if (mf_name) {
+		struct field *mf;
+
+		mf = metadata_field_parse(p, mf_name);
+		CHECK(mf, EINVAL);
+
+		mf_offset = mf->offset / 8;
+	}
+
 	instr->type = INSTR_LEARNER_LEARN;
 	instr->learn.action_id = a->id;
+	instr->learn.mf_offset = mf_offset;
 
 	return 0;
 }
@@ -8165,7 +8183,6 @@ rte_swx_pipeline_learner_config(struct rte_swx_pipeline *p,
 	CHECK(params->action_names, EINVAL);
 	for (i = 0; i < params->n_actions; i++) {
 		const char *action_name = params->action_names[i];
-		const char *action_field_name = params->action_field_names[i];
 		struct action *a;
 		uint32_t action_data_size;
 
@@ -8174,10 +8191,6 @@ rte_swx_pipeline_learner_config(struct rte_swx_pipeline *p,
 		a = action_find(p, action_name);
 		CHECK(a, EINVAL);
 
-		status = learner_action_args_check(p, a, action_field_name);
-		if (status)
-			return status;
-
 		status = learner_action_learning_check(p,
 						       a,
 						       params->action_names,
@@ -8218,10 +8231,6 @@ rte_swx_pipeline_learner_config(struct rte_swx_pipeline *p,
 	if (!l->actions)
 		goto nomem;
 
-	l->action_arg = calloc(params->n_actions, sizeof(struct field *));
-	if (!l->action_arg)
-		goto nomem;
-
 	if (action_data_size_max) {
 		l->default_action_data = calloc(1, action_data_size_max);
 		if (!l->default_action_data)
@@ -8243,14 +8252,9 @@ rte_swx_pipeline_learner_config(struct rte_swx_pipeline *p,
 
 	l->header = header;
 
-	for (i = 0; i < params->n_actions; i++) {
-		const char *mf_name = params->action_field_names[i];
-
+	for (i = 0; i < params->n_actions; i++)
 		l->actions[i] = action_find(p, params->action_names[i]);
 
-		l->action_arg[i] = mf_name ? metadata_field_parse(p, mf_name) : NULL;
-	}
-
 	l->default_action = default_action;
 
 	if (default_action->st)
@@ -8280,7 +8284,6 @@ rte_swx_pipeline_learner_config(struct rte_swx_pipeline *p,
 	if (!l)
 		return -ENOMEM;
 
-	free(l->action_arg);
 	free(l->actions);
 	free(l->fields);
 	free(l);
@@ -8375,7 +8378,6 @@ learner_build_free(struct rte_swx_pipeline *p)
 			struct learner_runtime *r = &t->learners[j];
 
 			free(r->mailbox);
-			free(r->action_data);
 		}
 
 		free(t->learners);
@@ -8419,7 +8421,6 @@ learner_build(struct rte_swx_pipeline *p)
 		TAILQ_FOREACH(l, &p->learners, node) {
 			struct learner_runtime *r = &t->learners[l->id];
 			uint64_t size;
-			uint32_t j;
 
 			/* r->mailbox. */
 			size = rte_swx_table_learner_mailbox_size_get();
@@ -8435,21 +8436,6 @@ learner_build(struct rte_swx_pipeline *p)
 			r->key = l->header ?
 				&t->structs[l->header->struct_id] :
 				&t->structs[p->metadata_struct_id];
-
-			/* r->action_data. */
-			r->action_data = calloc(p->n_actions, sizeof(uint8_t *));
-			if (!r->action_data) {
-				status = -ENOMEM;
-				goto error;
-			}
-
-			for (j = 0; j < l->n_actions; j++) {
-				struct action *a = l->actions[j];
-				struct field *mf = l->action_arg[j];
-				uint8_t *m = t->structs[p->metadata_struct_id];
-
-				r->action_data[a->id] = mf ? &m[mf->offset / 8] : NULL;
-			}
 		}
 	}
 
@@ -8476,7 +8462,6 @@ learner_free(struct rte_swx_pipeline *p)
 		TAILQ_REMOVE(&p->learners, l, node);
 		free(l->fields);
 		free(l->actions);
-		free(l->action_arg);
 		free(l->default_action_data);
 		free(l);
 	}
diff --git a/lib/pipeline/rte_swx_pipeline.h b/lib/pipeline/rte_swx_pipeline.h
index 2f18a820b9..490ff60c0d 100644
--- a/lib/pipeline/rte_swx_pipeline.h
+++ b/lib/pipeline/rte_swx_pipeline.h
@@ -696,16 +696,6 @@ struct rte_swx_pipeline_learner_params {
 	 */
 	uint32_t n_actions;
 
-	/** This table type allows adding the latest lookup key (typically done
-	 * only in the case of lookup miss) to the table with a given action.
-	 * The action arguments are picked up from the packet meta-data: for
-	 * each action, a set of successive meta-data fields (with the name of
-	 * the first such field provided here) is 1:1 mapped to the action
-	 * arguments. These meta-data fields must be set with the actual values
-	 * of the action arguments before the key add operation.
-	 */
-	const char **action_field_names;
-
 	/** The default table action that gets executed on lookup miss. Must be
 	 * one of the table actions included in the *action_names*.
 	 */
diff --git a/lib/pipeline/rte_swx_pipeline_internal.h b/lib/pipeline/rte_swx_pipeline_internal.h
index 3baae55737..4361c535a0 100644
--- a/lib/pipeline/rte_swx_pipeline_internal.h
+++ b/lib/pipeline/rte_swx_pipeline_internal.h
@@ -448,7 +448,7 @@ enum instruction_type {
 	INSTR_LEARNER,
 	INSTR_LEARNER_AF,
 
-	/* learn LEARNER ACTION_NAME */
+	/* learn LEARNER ACTION_NAME [ m.action_first_arg ] */
 	INSTR_LEARNER_LEARN,
 
 	/* forget */
@@ -583,6 +583,7 @@ struct instr_table {
 
 struct instr_learn {
 	uint8_t action_id;
+	uint8_t mf_offset;
 };
 
 struct instr_extern_obj {
@@ -809,7 +810,6 @@ struct learner {
 
 	/* Action. */
 	struct action **actions;
-	struct field **action_arg;
 	struct action *default_action;
 	uint8_t *default_action_data;
 	uint32_t n_actions;
@@ -826,7 +826,6 @@ TAILQ_HEAD(learner_tailq, learner);
 struct learner_runtime {
 	void *mailbox;
 	uint8_t **key;
-	uint8_t **action_data;
 };
 
 struct learner_statistics {
@@ -2017,6 +2016,7 @@ __instr_learn_exec(struct rte_swx_pipeline *p,
 		   const struct instruction *ip)
 {
 	uint64_t action_id = ip->learn.action_id;
+	uint32_t mf_offset = ip->learn.mf_offset;
 	uint32_t learner_id = t->learner_id;
 	struct rte_swx_table_state *ts = &t->table_state[p->n_tables +
 		p->n_selectors + learner_id];
@@ -2029,7 +2029,7 @@ __instr_learn_exec(struct rte_swx_pipeline *p,
 					   l->mailbox,
 					   t->time,
 					   action_id,
-					   l->action_data[action_id]);
+					   &t->metadata[mf_offset]);
 
 	TRACE("[Thread %2u] learner %u learn %s\n",
 	      p->thread_id,
diff --git a/lib/pipeline/rte_swx_pipeline_spec.c b/lib/pipeline/rte_swx_pipeline_spec.c
index d9cd1d0595..5c21a7ae4d 100644
--- a/lib/pipeline/rte_swx_pipeline_spec.c
+++ b/lib/pipeline/rte_swx_pipeline_spec.c
@@ -1293,7 +1293,7 @@ selector_block_parse(struct selector_spec *s,
  *		...
  *	}
  *	actions {
- *		ACTION_NAME args METADATA_FIELD_NAME
+ *		ACTION_NAME
  *		...
  *	}
  *	default_action ACTION_NAME args none | ARGS_BYTE_ARRAY [ const ]
@@ -1340,15 +1340,6 @@ learner_spec_free(struct learner_spec *s)
 	free(s->params.action_names);
 	s->params.action_names = NULL;
 
-	for (i = 0; i < s->params.n_actions; i++) {
-		uintptr_t name = (uintptr_t)s->params.action_field_names[i];
-
-		free((void *)name);
-	}
-
-	free(s->params.action_field_names);
-	s->params.action_field_names = NULL;
-
 	s->params.n_actions = 0;
 
 	default_action_name = (uintptr_t)s->params.default_action_name;
@@ -1468,9 +1459,7 @@ learner_actions_block_parse(struct learner_spec *s,
 			    const char **err_msg)
 {
 	const char **new_action_names = NULL;
-	const char **new_action_field_names = NULL;
-	char *action_name = NULL, *action_field_name = NULL;
-	int has_args = 1;
+	char *action_name = NULL;
 
 	/* Handle end of block. */
 	if ((n_tokens == 1) && !strcmp(tokens[0], "}")) {
@@ -1479,7 +1468,7 @@ learner_actions_block_parse(struct learner_spec *s,
 	}
 
 	/* Check input arguments. */
-	if ((n_tokens != 3) || strcmp(tokens[1], "args")) {
+	if (n_tokens != 1) {
 		if (err_line)
 			*err_line = n_lines;
 		if (err_msg)
@@ -1487,28 +1476,14 @@ learner_actions_block_parse(struct learner_spec *s,
 		return -EINVAL;
 	}
 
-	if (!strcmp(tokens[2], "none"))
-		has_args = 0;
-
 	action_name = strdup(tokens[0]);
 
-	if (has_args)
-		action_field_name = strdup(tokens[2]);
-
 	new_action_names = realloc(s->params.action_names,
 				   (s->params.n_actions + 1) * sizeof(char *));
 
-	new_action_field_names = realloc(s->params.action_field_names,
-					 (s->params.n_actions + 1) * sizeof(char *));
-
-	if (!action_name ||
-	    (has_args && !action_field_name) ||
-	    !new_action_names ||
-	    !new_action_field_names) {
+	if (!action_name || !new_action_names) {
 		free(action_name);
-		free(action_field_name);
 		free(new_action_names);
-		free(new_action_field_names);
 
 		if (err_line)
 			*err_line = n_lines;
@@ -1519,8 +1494,6 @@ learner_actions_block_parse(struct learner_spec *s,
 
 	s->params.action_names = new_action_names;
 	s->params.action_names[s->params.n_actions] = action_name;
-	s->params.action_field_names = new_action_field_names;
-	s->params.action_field_names[s->params.n_actions] = action_field_name;
 	s->params.n_actions++;
 
 	return 0;
-- 
2.17.1

Re: [dpdk-dev] [PATCH] pipeline: improve handling of learner table action arguments

[Thomas Monjalon]

14/09/2021 21:00, Cristian Dumitrescu:
> The arguments of actions that are learned are now specified as part of
> the learn instruction as opposed to being statically specified as part
> of the learner table configuration.
> 
> Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>

Applied, thanks.