Re: [dpdk-dev] [PATCH v1 5/9] app/testpmd: fix RSS flow action configuration

["Zhao1, Wei" <wei.zhao1@intel.com>]

Hi,  Adrien Mazarguil

	Once we apply this patch ,we will meet an abnormal issue, rte_flow command will cause core dump.
If we do not apply it, this will disappear. I have check this issue, it seems there is something wrong in function
of port_flow_new() in Config.c file, core dump happen here every time.
This is find out by Peng yuan first,  she will tell you how reappear this issue.



> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Friday, March 23, 2018 8:58 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Subject: [dpdk-dev] [PATCH v1 5/9] app/testpmd: fix RSS flow action
> configuration
> 
> Except for a list of queues, RSS configuration (hash key and fields) cannot be
> specified from the flow command line and testpmd does not provide safe
> defaults either.
> 
> In order to validate their implementation with testpmd, PMDs had to
> interpret its NULL RSS configuration parameters somehow, however this has
> never been valid to begin with.
> 
> This patch makes testpmd always provide default values.
> 
> Fixes: 05d34c6e9d2c ("app/testpmd: add queue actions to flow command")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>
> Cc: Jingjing Wu <jingjing.wu@intel.com>
> ---
>  app/test-pmd/cmdline_flow.c | 104 +++++++++++++++++++++++++----
>  app/test-pmd/config.c       | 141 +++++++++++++++++++++++++++-----------
> -
>  2 files changed, 192 insertions(+), 53 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index c2cf415ef..890c36d8e 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -184,13 +184,19 @@ enum index {
>  #define ITEM_RAW_SIZE \
>  	(offsetof(struct rte_flow_item_raw, pattern) +
> ITEM_RAW_PATTERN_SIZE)
> 
> -/** Number of queue[] entries in struct rte_flow_action_rss. */ -#define
> ACTION_RSS_NUM 32
> -
> -/** Storage size for struct rte_flow_action_rss including queues. */ -
> #define ACTION_RSS_SIZE \
> -	(offsetof(struct rte_flow_action_rss, queue) + \
> -	 sizeof(*((struct rte_flow_action_rss *)0)->queue) *
> ACTION_RSS_NUM)
> +/** Maximum number of queue indices in struct rte_flow_action_rss. */
> +#define ACTION_RSS_QUEUE_NUM 32
> +
> +/** Storage for struct rte_flow_action_rss including external data. */
> +union action_rss_data {
> +	struct rte_flow_action_rss conf;
> +	struct {
> +		uint8_t conf_data[offsetof(struct rte_flow_action_rss,
> queue)];
> +		uint16_t queue[ACTION_RSS_QUEUE_NUM];
> +		struct rte_eth_rss_conf rss_conf;
> +		uint8_t rss_key[RSS_HASH_KEY_LENGTH];
> +	} s;
> +};
> 
>  /** Maximum number of subsequent tokens and arguments on the stack.
> */  #define CTX_STACK_SIZE 16 @@ -316,6 +322,13 @@ struct token {
>  		.size = (sz), \
>  	})
> 
> +/** Static initializer for ARGS() with arbitrary offset and size. */
> +#define ARGS_ENTRY_ARB(o, s) \
> +	(&(const struct arg){ \
> +		.offset = (o), \
> +		.size = (s), \
> +	})
> +
>  /** Same as ARGS_ENTRY() using network byte ordering. */  #define
> ARGS_ENTRY_HTON(s, f) \
>  	(&(const struct arg){ \
> @@ -650,6 +663,9 @@ static int parse_vc_spec(struct context *, const struct
> token *,
>  			 const char *, unsigned int, void *, unsigned int);
> static int parse_vc_conf(struct context *, const struct token *,
>  			 const char *, unsigned int, void *, unsigned int);
> +static int parse_vc_action_rss(struct context *, const struct token *,
> +			       const char *, unsigned int, void *,
> +			       unsigned int);
>  static int parse_vc_action_rss_queue(struct context *, const struct token *,
>  				     const char *, unsigned int, void *,
>  				     unsigned int);
> @@ -1573,9 +1589,9 @@ static const struct token token_list[] = {
>  	[ACTION_RSS] = {
>  		.name = "rss",
>  		.help = "spread packets among several queues",
> -		.priv = PRIV_ACTION(RSS, ACTION_RSS_SIZE),
> +		.priv = PRIV_ACTION(RSS, sizeof(union action_rss_data)),
>  		.next = NEXT(action_rss),
> -		.call = parse_vc,
> +		.call = parse_vc_action_rss,
>  	},
>  	[ACTION_RSS_QUEUES] = {
>  		.name = "queues",
> @@ -2004,6 +2020,64 @@ parse_vc_conf(struct context *ctx, const struct
> token *token,
>  	return len;
>  }
> 
> +/** Parse RSS action. */
> +static int
> +parse_vc_action_rss(struct context *ctx, const struct token *token,
> +		    const char *str, unsigned int len,
> +		    void *buf, unsigned int size)
> +{
> +	struct buffer *out = buf;
> +	struct rte_flow_action *action;
> +	union action_rss_data *action_rss_data;
> +	unsigned int i;
> +	int ret;
> +
> +	ret = parse_vc(ctx, token, str, len, buf, size);
> +	if (ret < 0)
> +		return ret;
> +	/* Nothing else to do if there is no buffer. */
> +	if (!out)
> +		return ret;
> +	if (!out->args.vc.actions_n)
> +		return -1;
> +	action = &out->args.vc.actions[out->args.vc.actions_n - 1];
> +	/* Point to selected object. */
> +	ctx->object = out->args.vc.data;
> +	ctx->objmask = NULL;
> +	/* Set up default configuration. */
> +	action_rss_data = ctx->object;
> +	*action_rss_data = (union action_rss_data){
> +		.conf = (struct rte_flow_action_rss){
> +			.rss_conf = &action_rss_data->s.rss_conf,
> +			.num = RTE_MIN(nb_rxq,
> ACTION_RSS_QUEUE_NUM),
> +		},
> +	};
> +	action_rss_data->s.rss_conf = (struct rte_eth_rss_conf){
> +		.rss_key = action_rss_data->s.rss_key,
> +		.rss_key_len = sizeof(action_rss_data->s.rss_key),
> +		.rss_hf = rss_hf,
> +	};
> +	strncpy((void *)action_rss_data->s.rss_key,
> +		"testpmd's default RSS hash key",
> +		sizeof(action_rss_data->s.rss_key));
> +	for (i = 0; i < action_rss_data->conf.num; ++i)
> +		action_rss_data->conf.queue[i] = i;
> +	if (!port_id_is_invalid(ctx->port, DISABLED_WARN) &&
> +	    ctx->port != (portid_t)RTE_PORT_ALL) {
> +		if (rte_eth_dev_rss_hash_conf_get
> +		    (ctx->port, &action_rss_data->s.rss_conf) < 0) {
> +			struct rte_eth_dev_info info;
> +
> +			rte_eth_dev_info_get(ctx->port, &info);
> +			action_rss_data->s.rss_conf.rss_key_len =
> +				RTE_MIN(sizeof(action_rss_data->s.rss_key),
> +					info.hash_key_size);
> +		}
> +	}
> +	action->conf = &action_rss_data->conf;
> +	return ret;
> +}
> +
>  /**
>   * Parse queue field for RSS action.
>   *
> @@ -2015,6 +2089,7 @@ parse_vc_action_rss_queue(struct context *ctx,
> const struct token *token,
>  			  void *buf, unsigned int size)
>  {
>  	static const enum index next[] = NEXT_ENTRY(ACTION_RSS_QUEUE);
> +	union action_rss_data *action_rss_data;
>  	int ret;
>  	int i;
> 
> @@ -2028,9 +2103,13 @@ parse_vc_action_rss_queue(struct context *ctx,
> const struct token *token,
>  		ctx->objdata &= 0xffff;
>  		return len;
>  	}
> -	if (i >= ACTION_RSS_NUM)
> +	if (i >= ACTION_RSS_QUEUE_NUM)
>  		return -1;
> -	if (push_args(ctx, ARGS_ENTRY(struct rte_flow_action_rss,
> queue[i])))
> +	if (push_args(ctx,
> +		      ARGS_ENTRY_ARB(offsetof(struct rte_flow_action_rss,
> +					      queue) +
> +				     i * sizeof(action_rss_data->s.queue[i]),
> +				     sizeof(action_rss_data->s.queue[i]))))
>  		return -1;
>  	ret = parse_int(ctx, token, str, len, NULL, 0);
>  	if (ret < 0) {
> @@ -2045,7 +2124,8 @@ parse_vc_action_rss_queue(struct context *ctx,
> const struct token *token,
>  	ctx->next[ctx->next_num++] = next;
>  	if (!ctx->object)
>  		return len;
> -	((struct rte_flow_action_rss *)ctx->object)->num = i;
> +	action_rss_data = ctx->object;
> +	action_rss_data->conf.num = i;
>  	return len;
>  }
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 4bb255c62..a4b54f86a 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -982,31 +982,52 @@ static const struct {
>  	MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)),  };
> 
> -/** Compute storage space needed by item specification. */ -static void -
> flow_item_spec_size(const struct rte_flow_item *item,
> -		    size_t *size, size_t *pad)
> +/** Pattern item specification types. */ enum item_spec_type {
> +	ITEM_SPEC,
> +	ITEM_LAST,
> +	ITEM_MASK,
> +};
> +
> +/** Compute storage space needed by item specification and copy it. */
> +static size_t flow_item_spec_copy(void *buf, const struct rte_flow_item
> +*item,
> +		    enum item_spec_type type)
>  {
> -	if (!item->spec) {
> -		*size = 0;
> +	size_t size = 0;
> +
> +	const void *item_spec =
> +		type == ITEM_SPEC ? item->spec :
> +		type == ITEM_LAST ? item->last :
> +		type == ITEM_MASK ? item->mask :
> +		NULL;
> +
> +	if (!item_spec)
>  		goto empty;
> -	}
>  	switch (item->type) {
>  		union {
>  			const struct rte_flow_item_raw *raw;
> -		} spec;
> +		} src;
> +		union {
> +			struct rte_flow_item_raw *raw;
> +		} dst;
> 
>  	case RTE_FLOW_ITEM_TYPE_RAW:
> -		spec.raw = item->spec;
> -		*size = offsetof(struct rte_flow_item_raw, pattern) +
> -			spec.raw->length * sizeof(*spec.raw->pattern);
> +		src.raw = item_spec;
> +		dst.raw = buf;
> +		size = offsetof(struct rte_flow_item_raw, pattern) +
> +			src.raw->length * sizeof(*src.raw->pattern);
> +		if (dst.raw)
> +			memcpy(dst.raw, src.raw, size);
>  		break;
>  	default:
> -		*size = flow_item[item->type].size;
> +		size = flow_item[item->type].size;
> +		if (buf)
> +			memcpy(buf, item_spec, size);
>  		break;
>  	}
>  empty:
> -	*pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size;
> +	return RTE_ALIGN_CEIL(size, sizeof(double));
>  }
> 
>  /** Generate flow_action[] entry. */
> @@ -1036,31 +1057,72 @@ static const struct {
>  	MK_FLOW_ACTION(METER, sizeof(struct
> rte_flow_action_meter)),  };
> 
> -/** Compute storage space needed by action configuration. */ -static void -
> flow_action_conf_size(const struct rte_flow_action *action,
> -		      size_t *size, size_t *pad)
> +/** Compute storage space needed by action configuration and copy it.
> +*/ static size_t flow_action_conf_copy(void *buf, const struct
> +rte_flow_action *action)
>  {
> -	if (!action->conf) {
> -		*size = 0;
> +	size_t size = 0;
> +
> +	if (!action->conf)
>  		goto empty;
> -	}
>  	switch (action->type) {
>  		union {
>  			const struct rte_flow_action_rss *rss;
> -		} conf;
> +		} src;
> +		union {
> +			struct rte_flow_action_rss *rss;
> +		} dst;
> +		size_t off;
> 
>  	case RTE_FLOW_ACTION_TYPE_RSS:
> -		conf.rss = action->conf;
> -		*size = offsetof(struct rte_flow_action_rss, queue) +
> -			conf.rss->num * sizeof(*conf.rss->queue);
> +		src.rss = action->conf;
> +		dst.rss = buf;
> +		off = 0;
> +		if (dst.rss)
> +			*dst.rss = (struct rte_flow_action_rss){
> +				.num = src.rss->num,
> +			};
> +		off += offsetof(struct rte_flow_action_rss, queue);
> +		if (src.rss->num) {
> +			size = sizeof(*src.rss->queue) * src.rss->num;
> +			if (dst.rss)
> +				memcpy(dst.rss->queue, src.rss->queue,
> size);
> +			off += size;
> +		}
> +		off = RTE_ALIGN_CEIL(off, sizeof(double));
> +		if (dst.rss) {
> +			dst.rss->rss_conf = (void *)((uintptr_t)dst.rss + off);
> +			*(struct rte_eth_rss_conf *)(uintptr_t)
> +				dst.rss->rss_conf = (struct
> rte_eth_rss_conf){
> +				.rss_key_len = src.rss->rss_conf-
> >rss_key_len,
> +				.rss_hf = src.rss->rss_conf->rss_hf,
> +			};
> +		}
> +		off += sizeof(*src.rss->rss_conf);
> +		if (src.rss->rss_conf->rss_key_len) {
> +			off = RTE_ALIGN_CEIL(off, sizeof(double));
> +			size = sizeof(*src.rss->rss_conf->rss_key) *
> +				src.rss->rss_conf->rss_key_len;
> +			if (dst.rss) {
> +				((struct rte_eth_rss_conf *)(uintptr_t)
> +				 dst.rss->rss_conf)->rss_key =
> +					(void *)((uintptr_t)dst.rss + off);
> +				memcpy(dst.rss->rss_conf->rss_key,
> +				       src.rss->rss_conf->rss_key,
> +				       size);
> +			}
> +			off += size;
> +		}
> +		size = off;
>  		break;
>  	default:
> -		*size = flow_action[action->type].size;
> +		size = flow_action[action->type].size;
> +		if (buf)
> +			memcpy(buf, action->conf, size);
>  		break;
>  	}
>  empty:
> -	*pad = RTE_ALIGN_CEIL(*size, sizeof(double)) - *size;
> +	return RTE_ALIGN_CEIL(size, sizeof(double));
>  }
> 
>  /** Generate a port_flow entry from attributes/pattern/actions. */ @@ -
> 1073,7 +1135,6 @@ port_flow_new(const struct rte_flow_attr *attr,
>  	const struct rte_flow_action *action;
>  	struct port_flow *pf = NULL;
>  	size_t tmp;
> -	size_t pad;
>  	size_t off1 = 0;
>  	size_t off2 = 0;
>  	int err = ENOTSUP;
> @@ -1091,24 +1152,23 @@ port_flow_new(const struct rte_flow_attr *attr,
>  		if (pf)
>  			dst = memcpy(pf->data + off1, item, sizeof(*item));
>  		off1 += sizeof(*item);
> -		flow_item_spec_size(item, &tmp, &pad);
>  		if (item->spec) {
>  			if (pf)
> -				dst->spec = memcpy(pf->data + off2,
> -						   item->spec, tmp);
> -			off2 += tmp + pad;
> +				dst->spec = pf->data + off2;
> +			off2 += flow_item_spec_copy
> +				(pf ? pf->data + off2 : NULL, item,
> ITEM_SPEC);
>  		}
>  		if (item->last) {
>  			if (pf)
> -				dst->last = memcpy(pf->data + off2,
> -						   item->last, tmp);
> -			off2 += tmp + pad;
> +				dst->last = pf->data + off2;
> +			off2 += flow_item_spec_copy
> +				(pf ? pf->data + off2 : NULL, item,
> ITEM_LAST);
>  		}
>  		if (item->mask) {
>  			if (pf)
> -				dst->mask = memcpy(pf->data + off2,
> -						   item->mask, tmp);
> -			off2 += tmp + pad;
> +				dst->mask = pf->data + off2;
> +			off2 += flow_item_spec_copy
> +				(pf ? pf->data + off2 : NULL, item,
> ITEM_MASK);
>  		}
>  		off2 = RTE_ALIGN_CEIL(off2, sizeof(double));
>  	} while ((item++)->type != RTE_FLOW_ITEM_TYPE_END); @@ -
> 1125,12 +1185,11 @@ port_flow_new(const struct rte_flow_attr *attr,
>  		if (pf)
>  			dst = memcpy(pf->data + off1, action,
> sizeof(*action));
>  		off1 += sizeof(*action);
> -		flow_action_conf_size(action, &tmp, &pad);
>  		if (action->conf) {
>  			if (pf)
> -				dst->conf = memcpy(pf->data + off2,
> -						   action->conf, tmp);
> -			off2 += tmp + pad;
> +				dst->conf = pf->data + off2;
> +			off2 += flow_action_conf_copy
> +				(pf ? pf->data + off2 : NULL, action);
>  		}
>  		off2 = RTE_ALIGN_CEIL(off2, sizeof(double));
>  	} while ((action++)->type != RTE_FLOW_ACTION_TYPE_END);
> --
> 2.11.0