RE: [PATCH v2 2/2] examples/l3fwd: add config file support for EM

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Sean,

Few comments below.

> Add support to define ipv4 and ipv6 forwarding tables
> from reading from a config file for EM with a format
> similar to l3fwd-acl one.
> 
> With the removal of the hardcoded route tables for IPv4
> and IPv6 from 'l3fwd_em', these routes have been moved
> to a separate default config file for use with EM.
> 
> Related l3fwd docs have been updated to relfect these
> changes.
> 
> Signed-off-by: Sean Morrissey <sean.morrissey@intel.com>
> Signed-off-by: Ravi Kerur <rkerur@gmail.com>
> ---
>  doc/guides/sample_app_ug/l3_forward.rst |  89 +++--
>  examples/l3fwd/em_default_v4.cfg        |  17 +
>  examples/l3fwd/em_default_v6.cfg        |  17 +
>  examples/l3fwd/l3fwd_em.c               | 473 ++++++++++++++----------
>  examples/l3fwd/l3fwd_route.h            |  38 +-
>  5 files changed, 412 insertions(+), 222 deletions(-)
>  create mode 100644 examples/l3fwd/em_default_v4.cfg
>  create mode 100644 examples/l3fwd/em_default_v6.cfg
> 

...

> diff --git a/examples/l3fwd/l3fwd_em.c b/examples/l3fwd/l3fwd_em.c

....

> @@ -346,6 +278,178 @@ em_get_ipv6_dst_port(void *ipv6_hdr, uint16_t portid, void *lookup_struct)
>  #include "l3fwd_em.h"
>  #endif
> 
> +static int
> +em_parse_v6_net(const char *in, uint32_t *v)
> +{
> +	int32_t rc;
> +
> +	/* get address. */
> +	rc = inet_pton(AF_INET6, in, &v);

Why '&v'?
I believe it should be:
rc = inet_pton(AF_INET6, in, v);
here.

> +	if (rc != 1)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int
> +em_parse_v6_rule(char *str, struct em_rule *v)
> +{
> +	int i, rc;
> +	uint32_t ip[4];
> +	char *s, *sp, *in[CB_FLD_MAX];
> +	static const char *dlm = " \t\n";
> +	int dim = CB_FLD_MAX;
> +	s = str;
> +
> +	for (i = 0; i != dim; i++, s = NULL) {
> +		in[i] = strtok_r(s, dlm, &sp);
> +		if (in[i] == NULL)
> +			return -EINVAL;
> +	}
> +
> +	ip[0] = *(v->v6_key.ip32_dst);
> +	rc = em_parse_v6_net(in[CB_FLD_DST_ADDR], ip);

Doesn't look right to me.
I think it should be just:
rc = em_parse_v6_net(in[CB_FLD_DST_ADDR], v->v6_key.ip32_dst);
and we don't need local 'ip[]' at all. 


> +	if (rc != 0)
> +		return rc;
> +	ip[0] = *(v->v6_key.ip32_src);
> +	rc = em_parse_v6_net(in[CB_FLD_SRC_ADDR], ip);

Same:
rc = em_parse_v6_net(in[CB_FLD_SRC_ADDR], v->v6_key.ip32_src);

> +	if (rc != 0)
> +		return rc;
> +
> +
> +	/* source port. */
> +	GET_CB_FIELD(in[CB_FLD_SRC_PORT], v->v6_key.port_src, 0, UINT16_MAX, 0);
> +	/* destination port. */
> +	GET_CB_FIELD(in[CB_FLD_DST_PORT], v->v6_key.port_dst, 0, UINT16_MAX, 0);
> +	/* protocol. */
> +	GET_CB_FIELD(in[CB_FLD_PROTO], v->v6_key.proto, 0, UINT8_MAX, 0);
> +	/* out interface. */
> +	GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0);
> +
> +	return 0;
> +}
> +
> +static int
> +em_parse_v4_rule(char *str, struct em_rule *v)
> +{
> +	int i, rc;
> +	char *s, *sp, *in[CB_FLD_MAX];
> +	static const char *dlm = " \t\n";
> +	int dim = CB_FLD_MAX;
> +	s = str;
> +
> +	for (i = 0; i != dim; i++, s = NULL) {
> +		in[i] = strtok_r(s, dlm, &sp);
> +		if (in[i] == NULL)
> +			return -EINVAL;
> +	}
> +
> +	rc = inet_pton(AF_INET, in[CB_FLD_DST_ADDR], &(v->v4_key.ip_dst));
> +	v->v4_key.ip_dst = ntohl(v->v4_key.ip_dst);
> +	if (rc != 1)
> +		return rc;
> +
> +	rc = inet_pton(AF_INET, in[CB_FLD_SRC_ADDR], &(v->v4_key.ip_src));
> +	v->v4_key.ip_src = ntohl(v->v4_key.ip_src);
> +	if (rc != 1)
> +		return rc;
> +
> +	/* source port. */
> +	GET_CB_FIELD(in[CB_FLD_SRC_PORT], v->v4_key.port_src, 0, UINT16_MAX, 0);
> +	/* destination port. */
> +	GET_CB_FIELD(in[CB_FLD_DST_PORT], v->v4_key.port_dst, 0, UINT16_MAX, 0);
> +	/* protocol. */
> +	GET_CB_FIELD(in[CB_FLD_PROTO], v->v4_key.proto, 0, UINT8_MAX, 0);
> +	/* out interface. */
> +	GET_CB_FIELD(in[CB_FLD_IF_OUT], v->if_out, 0, UINT8_MAX, 0);
> +
> +	return 0;
> +}
> +
> +static int
> +em_add_rules(const char *rule_path,
> +		struct em_rule **proute_base,
> +		int (*parser)(char *, struct em_rule *))
> +{
> +	struct em_rule *route_rules;
> +	struct em_rule *next;
> +	unsigned int route_num = 0;
> +	unsigned int route_cnt = 0;
> +	char buff[LINE_MAX];
> +	FILE *fh;
> +	unsigned int i = 0, rule_size = sizeof(*next);
> +	int val;
> +
> +	*proute_base = NULL;
> +	fh = fopen(rule_path, "rb");
> +	if (fh == NULL)
> +		return -EINVAL;
> +
> +	while ((fgets(buff, LINE_MAX, fh) != NULL)) {
> +		if (buff[0] == ROUTE_LEAD_CHAR)
> +			route_num++;
> +	}
> +
> +	if (route_num == 0) {
> +		fclose(fh);
> +		return -EINVAL;
> +	}
> +
> +	val = fseek(fh, 0, SEEK_SET);
> +	if (val < 0) {
> +		fclose(fh);
> +		return -EINVAL;
> +	}
> +
> +	route_rules = calloc(route_num, rule_size);
> +
> +	if (route_rules == NULL) {
> +		fclose(fh);
> +		return -EINVAL;
> +	}
> +
> +	i = 0;
> +	while (fgets(buff, LINE_MAX, fh) != NULL) {
> +		i++;
> +		if (is_bypass_line(buff))
> +			continue;
> +
> +		char s = buff[0];
> +
> +		/* Route entry */
> +		if (s == ROUTE_LEAD_CHAR)
> +			next = &route_rules[route_cnt];
> +
> +		/* Illegal line */
> +		else {
> +			RTE_LOG(ERR, L3FWD,
> +				"%s Line %u: should start with leading "
> +				"char %c\n",
> +				rule_path, i, ROUTE_LEAD_CHAR);
> +			fclose(fh);
> +			free(route_rules);
> +			return -EINVAL;
> +		}
> +
> +		if (parser(buff + 1, next) != 0) {
> +			RTE_LOG(ERR, L3FWD,
> +				"%s Line %u: parse rules error\n",
> +				rule_path, i);
> +			fclose(fh);
> +			free(route_rules);
> +			return -EINVAL;
> +		}
> +
> +		route_cnt++;
> +	}
> +
> +	fclose(fh);
> +
> +	*proute_base = route_rules;
> +
> +	return route_cnt;
> +}
> +
>  static void
>  convert_ipv4_5tuple(struct ipv4_5tuple *key1,
>  		union ipv4_5tuple_host *key2)
> @@ -382,122 +486,92 @@ convert_ipv6_5tuple(struct ipv6_5tuple *key1,
>  #define BIT_8_TO_15 0x0000ff00
> 
>  static inline void
> -populate_ipv4_few_flow_into_table(const struct rte_hash *h)
> +populate_ipv4_flow_into_table(const struct rte_hash *h)
>  {
> -	uint32_t i;
> +	int i;
>  	int32_t ret;
> +	struct rte_eth_dev_info dev_info;
> +	char srcbuf[INET6_ADDRSTRLEN];
> +	char dstbuf[INET6_ADDRSTRLEN];
> 
>  	mask0 = (rte_xmm_t){.u32 = {BIT_8_TO_15, ALL_32_BITS,
>  				ALL_32_BITS, ALL_32_BITS} };
> 
> -	for (i = 0; i < IPV4_L3FWD_EM_NUM_ROUTES; i++) {
> -		struct ipv4_l3fwd_em_route  entry;
> +	for (i = 0; i < route_num_v4; i++) {
> +		struct em_rule *entry;
>  		union ipv4_5tuple_host newkey;
> +		struct in_addr src;
> +		struct in_addr dst;
> 
> -		entry = ipv4_l3fwd_em_route_array[i];
> -		convert_ipv4_5tuple(&entry.key, &newkey);
> +		if ((1 << em_route_base_v4[i].if_out &
> +				enabled_port_mask) == 0)
> +			continue;
> +
> +		entry = &em_route_base_v4[i];
> +		convert_ipv4_5tuple(&(entry->v4_key), &newkey);
>  		ret = rte_hash_add_key(h, (void *) &newkey);
>  		if (ret < 0) {
>  			rte_exit(EXIT_FAILURE, "Unable to add entry %" PRIu32
>  				" to the l3fwd hash.\n", i);
>  		}
> -		ipv4_l3fwd_out_if[ret] = entry.if_out;
> +		ipv4_l3fwd_out_if[ret] = entry->if_out;
> +		rte_eth_dev_info_get(em_route_base_v4[i].if_out,
> +				     &dev_info);
> +
> +		src.s_addr = htonl(em_route_base_v4[i].v4_key.ip_src);
> +		dst.s_addr = htonl(em_route_base_v4[i].v4_key.ip_dst);
> +		printf("EM: Adding route %s %s (%d) [%s]\n",
> +			   inet_ntop(AF_INET, &dst, dstbuf, sizeof(dstbuf)),
> +		       inet_ntop(AF_INET, &src, srcbuf, sizeof(srcbuf)),
> +		       em_route_base_v4[i].if_out, dev_info.device->name);
>  	}
>  	printf("Hash: Adding 0x%" PRIx64 " keys\n",
> -		(uint64_t)IPV4_L3FWD_EM_NUM_ROUTES);
> +		(uint64_t)route_num_v4);
>  }
> 
>  #define BIT_16_TO_23 0x00ff0000
>  static inline void
> -populate_ipv6_few_flow_into_table(const struct rte_hash *h)
> +populate_ipv6_flow_into_table(const struct rte_hash *h)
>  {
> -	uint32_t i;
> +	int i;
>  	int32_t ret;
> +	struct rte_eth_dev_info dev_info;
> +	char srcbuf[INET6_ADDRSTRLEN];
> +	char dstbuf[INET6_ADDRSTRLEN];
> 
>  	mask1 = (rte_xmm_t){.u32 = {BIT_16_TO_23, ALL_32_BITS,
>  				ALL_32_BITS, ALL_32_BITS} };
> 
>  	mask2 = (rte_xmm_t){.u32 = {ALL_32_BITS, ALL_32_BITS, 0, 0} };
> 
> -	for (i = 0; i < IPV6_L3FWD_EM_NUM_ROUTES; i++) {
> -		struct ipv6_l3fwd_em_route entry;
> +	for (i = 0; i < route_num_v6; i++) {
> +		struct em_rule *entry;
>  		union ipv6_5tuple_host newkey;
> 
> -		entry = ipv6_l3fwd_em_route_array[i];
> -		convert_ipv6_5tuple(&entry.key, &newkey);
> +		if ((1 << em_route_base_v6[i].if_out &
> +				enabled_port_mask) == 0)
> +			continue;
> +
> +		entry = &em_route_base_v6[i];
> +		convert_ipv6_5tuple(&(entry->v6_key), &newkey);
>  		ret = rte_hash_add_key(h, (void *) &newkey);
>  		if (ret < 0) {
>  			rte_exit(EXIT_FAILURE, "Unable to add entry %" PRIu32
>  				" to the l3fwd hash.\n", i);
>  		}
> -		ipv6_l3fwd_out_if[ret] = entry.if_out;
> +		ipv6_l3fwd_out_if[ret] = entry->if_out;
> +		rte_eth_dev_info_get(em_route_base_v6[i].if_out,
> +				     &dev_info);
> +
> +		printf("EM: Adding route %s %s (%d) [%s]\n",
> +			   inet_ntop(AF_INET6, em_route_base_v6[i].v6_key.ip32_dst,
> +			   dstbuf, sizeof(dstbuf)),
> +		       inet_ntop(AF_INET6, em_route_base_v6[i].v6_key.ip32_src,
> +			   srcbuf, sizeof(srcbuf)),
> +		       em_route_base_v6[i].if_out, dev_info.device->name);

I think we need to print full key here: <dest_ip,src_ip, dst_port, src_port, proro>.
Otherwise it is sort of misleading.
Same for ipv4.


>  	}
>  	printf("Hash: Adding 0x%" PRIx64 "keys\n",
> -		(uint64_t)IPV6_L3FWD_EM_NUM_ROUTES);
> -}
> -
> -#define NUMBER_PORT_USED 16
> -static inline void
> -populate_ipv4_many_flow_into_table(const struct rte_hash *h,
> -		unsigned int nr_flow)
> -{
> -	unsigned i;
> -
> -	mask0 = (rte_xmm_t){.u32 = {BIT_8_TO_15, ALL_32_BITS,
> -				ALL_32_BITS, ALL_32_BITS} };
> -
> -	for (i = 0; i < nr_flow; i++) {
> -		uint8_t port = i % NUMBER_PORT_USED;
> -		struct ipv4_l3fwd_em_route entry;
> -		union ipv4_5tuple_host newkey;
> -
> -		uint8_t a = (uint8_t)((port + 1) % BYTE_VALUE_MAX);
> -
> -		/* Create the ipv4 exact match flow */
> -		memset(&entry, 0, sizeof(entry));
> -		entry = ipv4_l3fwd_em_route_array[port];
> -		entry.key.ip_dst = RTE_IPV4(198, 18, port, a);
> -		convert_ipv4_5tuple(&entry.key, &newkey);
> -		int32_t ret = rte_hash_add_key(h, (void *) &newkey);
> -
> -		if (ret < 0)
> -			rte_exit(EXIT_FAILURE, "Unable to add entry %u\n", i);
> -
> -		ipv4_l3fwd_out_if[ret] = (uint8_t) entry.if_out;
> -
> -	}
> -	printf("Hash: Adding 0x%x keys\n", nr_flow);
> -}
> -
> -static inline void
> -populate_ipv6_many_flow_into_table(const struct rte_hash *h,
> -		unsigned int nr_flow)
> -{
> -	unsigned i;
> -
> -	mask1 = (rte_xmm_t){.u32 = {BIT_16_TO_23, ALL_32_BITS,
> -				ALL_32_BITS, ALL_32_BITS} };
> -	mask2 = (rte_xmm_t){.u32 = {ALL_32_BITS, ALL_32_BITS, 0, 0} };
> -
> -	for (i = 0; i < nr_flow; i++) {
> -		uint8_t port = i % NUMBER_PORT_USED;
> -		struct ipv6_l3fwd_em_route entry;
> -		union ipv6_5tuple_host newkey;
> -
> -		/* Create the ipv6 exact match flow */
> -		memset(&entry, 0, sizeof(entry));
> -		entry = ipv6_l3fwd_em_route_array[port];
> -		entry.key.ip_dst[15] = (port + 1) % BYTE_VALUE_MAX;
> -		convert_ipv6_5tuple(&entry.key, &newkey);
> -		int32_t ret = rte_hash_add_key(h, (void *) &newkey);
> -
> -		if (ret < 0)
> -			rte_exit(EXIT_FAILURE, "Unable to add entry %u\n", i);
> -
> -		ipv6_l3fwd_out_if[ret] = (uint8_t) entry.if_out;
> -
> -	}
> -	printf("Hash: Adding 0x%x keys\n", nr_flow);
> +		(uint64_t)route_num_v6);
>  }
> 
>  /* Requirements:
> @@ -972,17 +1046,53 @@ em_event_main_loop_tx_q_burst_vector(__rte_unused void *dummy)
>  	return 0;
>  }
> 
> +static void
> +free_em_routes(void)
> +{
> +	free(em_route_base_v4);
> +	free(em_route_base_v6);
> +	em_route_base_v4 = NULL;
> +	em_route_base_v6 = NULL;
> +	route_num_v4 = 0;
> +	route_num_v6 = 0;
> +}
> +
>  /* Load rules from the input file */
>  void
>  read_config_files_em(void)
>  {
> -	/* Empty till config file support added to EM */
> +	/* ipv4 check */
> +	if (parm_config.rule_ipv4_name != NULL) {
> +		route_num_v4 = em_add_rules(parm_config.rule_ipv4_name,
> +					&em_route_base_v4, &em_parse_v4_rule);
> +		if (route_num_v4 < 0) {
> +			free_em_routes();
> +			rte_exit(EXIT_FAILURE, "Failed to add EM IPv4 rules\n");
> +		}
> +	} else {
> +		RTE_LOG(ERR, L3FWD, "EM IPv4 rule file not specified\n");
> +		rte_exit(EXIT_FAILURE, "Failed to get valid EM options\n");
> +	}
> +
> +	/* ipv6 check */
> +	if (parm_config.rule_ipv6_name != NULL) {
> +		route_num_v6 = em_add_rules(parm_config.rule_ipv6_name,
> +					&em_route_base_v6, &em_parse_v6_rule);
> +		if (route_num_v6 < 0) {
> +			free_em_routes();
> +			rte_exit(EXIT_FAILURE, "Failed to add EM IPv6 rules\n");
> +		}
> +	} else {
> +		RTE_LOG(ERR, L3FWD, "EM IPv6 rule file not specified\n");
> +		rte_exit(EXIT_FAILURE, "Failed to get valid EM options\n");
> +	}
>  }
> 
>  /* Initialize exact match (hash) parameters. 8< */
>  void
>  setup_hash(const int socketid)
>  {
> +	printf("IPPPROTO_UDP: %d\n", IPPROTO_UDP);

Looks unrelated, pls remove.

>  	struct rte_hash_parameters ipv4_l3fwd_hash_params = {
>  		.name = NULL,
>  		.entries = L3FWD_HASH_ENTRIES,
> @@ -1023,35 +1133,18 @@ setup_hash(const int socketid)
>  			"Unable to create the l3fwd hash on socket %d\n",
>  			socketid);
>