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-boun...@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Friday, March 23, 2018 8:58 PM
> To: dev@dpdk.org
> Cc: sta...@dpdk.org; Lu, Wenzhuo <wenzhuo...@intel.com>; Wu, Jingjing
> <jingjing...@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: sta...@dpdk.org
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarg...@6wind.com>
> Cc: Wenzhuo Lu <wenzhuo...@intel.com>
> Cc: Jingjing Wu <jingjing...@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
