From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Peng, Yuan" Subject: Re: [PATCH v6 07/11] app/testpmd: fix RSS flow action configuration Date: Mon, 7 May 2018 05:23:16 +0000 Message-ID: <67D543A150B29E4CAAE53918F64EDAEA37500C36@SHSMSX103.ccr.corp.intel.com> References: <20180416150058.2620-1-adrien.mazarguil@6wind.com> <20180419100204.5728-1-adrien.mazarguil@6wind.com> <20180419100204.5728-8-adrien.mazarguil@6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "stable@dpdk.org" , "Lu, Wenzhuo" , "Wu, Jingjing" , Xueming Li , "Zhao1, Wei" , "Xing, Beilei" To: Adrien Mazarguil , "dev@dpdk.org" Return-path: In-Reply-To: <20180419100204.5728-8-adrien.mazarguil@6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Adrien, There is a core dump when I set i40e fdir flexbytes rule, I bisect the comm= it version, d0ad8648b1c57c0e311ab7a3192bc3b507de5bf6 is the first bad commit commit d0ad8648b1c57c0e311ab7a3192bc3b507de5bf6 Author: Adrien Mazarguil Date: Thu Apr 19 12:07:37 2018 +0200 app/testpmd: fix RSS flow action configuration Except for a list of queues, RSS configuration (hash key and fields) ca= nnot be specified from the flow command line and testpmd does not provide sa= fe defaults either. In order to validate their implementation with testpmd, PMDs had to interpret its NULL RSS configuration parameters somehow, however this h= as never been valid to begin with. This patch makes testpmd always provide default values. The list of RSS types to use is exclusively taken from the global "rss_= hf" variable, itself configured through the "port config all rss" command o= r --rss-ip/--rss-udp command-line options. Fixes: 05d34c6e9d2c ("app/testpmd: add queue actions to flow command") Cc: stable@dpdk.org Signed-off-by: Adrien Mazarguil Acked-by: Nelio Laranjeiro Acked-by: Ferruh Yigit :040000 040000 1e01ca01e840f43334f576a2fe8cf755433e9c90 1ff48b7ff1f01a399cd= 3403c5fc6e537d2f33021 M app The test steps are as below: ./usertools/dpdk-devbind.py -b igb_uio 05:00.0 05:00.1 ./x86_64-native-linuxapp-gcc/app/testpmd -c 0x1fffe -n 4 -w 0000:05:00.0 -= -file-prefix=3Dpf - -i --pkt-filter-mode=3Dperfect --disable-rss --rxq=3D16= --txq=3D16 testpmd> flow create 0 ingress pattern eth type is 0x0807 / raw relative i= s 1 pattern is abcdefghijklmnop / end actions queue index 1 / end PMD: Global register is changed during enable FDIR flexible payload Segmentation fault (core dumped) Could you help to check it? Thank you very much. Yuan. -----Original Message----- From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil Sent: Thursday, April 19, 2018 6:08 PM To: dev@dpdk.org Cc: stable@dpdk.org; Lu, Wenzhuo ; Wu, Jingjing ; Xueming Li Subject: [dpdk-dev] [PATCH v6 07/11] app/testpmd: fix RSS flow action confi= guration 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 interpr= et its NULL RSS configuration parameters somehow, however this has never be= en valid to begin with. This patch makes testpmd always provide default values. The list of RSS types to use is exclusively taken from the global "rss_hf" variable, itself configured through the "port config all rss" command or --= rss-ip/--rss-udp command-line options. Fixes: 05d34c6e9d2c ("app/testpmd: add queue actions to flow command") Cc: stable@dpdk.org Signed-off-by: Adrien Mazarguil Acked-by: Nelio Laranjeiro Cc: Wenzhuo Lu Cc: Jingjing Wu Cc: Xueming Li --- v4 changes: Removed reliance on rte_eth_dev_rss_hash_conf_get(), which as reported by X= ueming, is not necessarily supported and triggers a misleading "Function no= t implemented" warning. Updated commit log to reflect this. --- app/test-pmd/cmdline.c | 2 + app/test-pmd/cmdline_flow.c | 101 ++++++++++++++++++++++++---- app/test-pmd/config.c | 140 +++++++++++++++++++++++++++------------ 3 files changed, 190 insertions(+), 53 deletions(-) diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index 512e3b55= e..9704d0454 100644 --- a/app/test-pmd/cmdline.c +++ b/app/test-pmd/cmdline.c @@ -2033,6 +2033,8 @@ cmd_config_rss_parsed(void *parsed_result, return; } rss_conf.rss_key =3D NULL; + /* Update global configuration for RSS types. */ + rss_hf =3D rss_conf.rss_hf; for (i =3D 0; i < rte_eth_dev_count(); i++) { diag =3D rte_eth_dev_rss_hash_update(i, &rss_conf); if (diag < 0) diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c inde= x a0e06db36..d37c5f39f 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) =20 -/** Number of queue[] entries in struct rte_flow_action_rss. */ -#define A= CTION_RSS_NUM 32 - -/** Storage size for struct rte_flow_action_rss including queues. */ -#def= ine 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. */=20 +#define ACTION_RSS_QUEUE_NUM 32 + +/** Storage for struct rte_flow_action_rss including external data. */=20 +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; +}; =20 /** Maximum number of subsequent tokens and arguments on the stack. */ #d= efine CTX_STACK_SIZE 16 @@ -316,6 +322,13 @@ struct token { .size =3D (sz), \ }) =20 +/** Static initializer for ARGS() with arbitrary offset and size. */=20 +#define ARGS_ENTRY_ARB(o, s) \ + (&(const struct arg){ \ + .offset =3D (o), \ + .size =3D (s), \ + }) + /** Same as ARGS_ENTRY() using network byte ordering. */ #define ARGS_ENT= RY_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_v= c_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[] =3D { [ACTION_RSS] =3D { .name =3D "rss", .help =3D "spread packets among several queues", - .priv =3D PRIV_ACTION(RSS, ACTION_RSS_SIZE), + .priv =3D PRIV_ACTION(RSS, sizeof(union action_rss_data)), .next =3D NEXT(action_rss), - .call =3D parse_vc, + .call =3D parse_vc_action_rss, }, [ACTION_RSS_QUEUES] =3D { .name =3D "queues", @@ -2004,6 +2020,61 @@ parse_vc_conf(struct context *ctx, const struct toke= n *token, return len; } =20 +/** 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 =3D buf; + struct rte_flow_action *action; + union action_rss_data *action_rss_data; + unsigned int i; + int ret; + + ret =3D 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 =3D &out->args.vc.actions[out->args.vc.actions_n - 1]; + /* Point to selected object. */ + ctx->object =3D out->args.vc.data; + ctx->objmask =3D NULL; + /* Set up default configuration. */ + action_rss_data =3D ctx->object; + *action_rss_data =3D (union action_rss_data){ + .conf =3D (struct rte_flow_action_rss){ + .rss_conf =3D &action_rss_data->s.rss_conf, + .num =3D RTE_MIN(nb_rxq, ACTION_RSS_QUEUE_NUM), + }, + }; + action_rss_data->s.rss_conf =3D (struct rte_eth_rss_conf){ + .rss_key =3D action_rss_data->s.rss_key, + .rss_key_len =3D sizeof(action_rss_data->s.rss_key), + .rss_hf =3D 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 =3D 0; i < action_rss_data->conf.num; ++i) + action_rss_data->conf.queue[i] =3D i; + if (!port_id_is_invalid(ctx->port, DISABLED_WARN) && + ctx->port !=3D (portid_t)RTE_PORT_ALL) { + struct rte_eth_dev_info info; + + rte_eth_dev_info_get(ctx->port, &info); + action_rss_data->s.rss_conf.rss_key_len =3D + RTE_MIN(sizeof(action_rss_data->s.rss_key), + info.hash_key_size); + } + action->conf =3D &action_rss_data->conf; + return ret; +} + /** * Parse queue field for RSS action. * @@ -2015,6 +2086,7 @@ parse_vc_action_rss_queue(struct context *ctx, const = struct token *token, void *buf, unsigned int size) { static const enum index next[] =3D NEXT_ENTRY(ACTION_RSS_QUEUE); + union action_rss_data *action_rss_data; int ret; int i; =20 @@ -2028,9 +2100,13 @@ parse_vc_action_rss_queue(struct context *ctx, const= struct token *token, ctx->objdata &=3D 0xffff; return len; } - if (i >=3D ACTION_RSS_NUM) + if (i >=3D 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 =3D parse_int(ctx, token, str, len, NULL, 0); if (ret < 0) { @@ -2045,7 +2121,8 @@ parse_vc_action_rss_queue(struct context *ctx, const = struct token *token, ctx->next[ctx->next_num++] =3D next; if (!ctx->object) return len; - ((struct rte_flow_action_rss *)ctx->object)->num =3D i; + action_rss_data =3D ctx->object; + action_rss_data->conf.num =3D i; return len; } =20 diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index dd051f5ca.= .97a959b2a 100644 --- a/app/test-pmd/config.c +++ b/app/test-pmd/config.c @@ -998,31 +998,51 @@ static const struct { MK_FLOW_ITEM(GENEVE, sizeof(struct rte_flow_item_geneve)), }; =20 -/** Compute storage space needed by item specification. */ -static void -f= low_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. */=20 +static size_t flow_item_spec_copy(void *buf, const struct rte_flow_item=20 +*item, + enum item_spec_type type) { - if (!item->spec) { - *size =3D 0; + size_t size =3D 0; + const void *item_spec =3D + type =3D=3D ITEM_SPEC ? item->spec : + type =3D=3D ITEM_LAST ? item->last : + type =3D=3D 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; =20 case RTE_FLOW_ITEM_TYPE_RAW: - spec.raw =3D item->spec; - *size =3D offsetof(struct rte_flow_item_raw, pattern) + - spec.raw->length * sizeof(*spec.raw->pattern); + src.raw =3D item_spec; + dst.raw =3D buf; + size =3D 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 =3D flow_item[item->type].size; + size =3D flow_item[item->type].size; + if (buf) + memcpy(buf, item_spec, size); break; } empty: - *pad =3D RTE_ALIGN_CEIL(*size, sizeof(double)) - *size; + return RTE_ALIGN_CEIL(size, sizeof(double)); } =20 /** Generate flow_action[] entry. */ @@ -1052,31 +1072,72 @@ static const struct { MK_FLOW_ACTION(METER, sizeof(struct rte_flow_action_meter)), }; =20 -/** 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.=20 +*/ static size_t flow_action_conf_copy(void *buf, const struct=20 +rte_flow_action *action) { - if (!action->conf) { - *size =3D 0; + size_t size =3D 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; =20 case RTE_FLOW_ACTION_TYPE_RSS: - conf.rss =3D action->conf; - *size =3D offsetof(struct rte_flow_action_rss, queue) + - conf.rss->num * sizeof(*conf.rss->queue); + src.rss =3D action->conf; + dst.rss =3D buf; + off =3D 0; + if (dst.rss) + *dst.rss =3D (struct rte_flow_action_rss){ + .num =3D src.rss->num, + }; + off +=3D offsetof(struct rte_flow_action_rss, queue); + if (src.rss->num) { + size =3D sizeof(*src.rss->queue) * src.rss->num; + if (dst.rss) + memcpy(dst.rss->queue, src.rss->queue, size); + off +=3D size; + } + off =3D RTE_ALIGN_CEIL(off, sizeof(double)); + if (dst.rss) { + dst.rss->rss_conf =3D (void *)((uintptr_t)dst.rss + off); + *(struct rte_eth_rss_conf *)(uintptr_t) + dst.rss->rss_conf =3D (struct rte_eth_rss_conf){ + .rss_key_len =3D src.rss->rss_conf->rss_key_len, + .rss_hf =3D src.rss->rss_conf->rss_hf, + }; + } + off +=3D sizeof(*src.rss->rss_conf); + if (src.rss->rss_conf->rss_key_len) { + off =3D RTE_ALIGN_CEIL(off, sizeof(double)); + size =3D 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 =3D + (void *)((uintptr_t)dst.rss + off); + memcpy(dst.rss->rss_conf->rss_key, + src.rss->rss_conf->rss_key, + size); + } + off +=3D size; + } + size =3D off; break; default: - *size =3D flow_action[action->type].size; + size =3D flow_action[action->type].size; + if (buf) + memcpy(buf, action->conf, size); break; } empty: - *pad =3D RTE_ALIGN_CEIL(*size, sizeof(double)) - *size; + return RTE_ALIGN_CEIL(size, sizeof(double)); } =20 /** Generate a port_flow entry from attributes/pattern/actions. */ @@ -108= 9,7 +1150,6 @@ port_flow_new(const struct rte_flow_attr *attr, const struct rte_flow_action *action; struct port_flow *pf =3D NULL; size_t tmp; - size_t pad; size_t off1 =3D 0; size_t off2 =3D 0; int err =3D ENOTSUP; @@ -1107,24 +1167,23 @@ port_flow_new(const struct rte_flow_attr *attr, if (pf) dst =3D memcpy(pf->data + off1, item, sizeof(*item)); off1 +=3D sizeof(*item); - flow_item_spec_size(item, &tmp, &pad); if (item->spec) { if (pf) - dst->spec =3D memcpy(pf->data + off2, - item->spec, tmp); - off2 +=3D tmp + pad; + dst->spec =3D pf->data + off2; + off2 +=3D flow_item_spec_copy + (pf ? pf->data + off2 : NULL, item, ITEM_SPEC); } if (item->last) { if (pf) - dst->last =3D memcpy(pf->data + off2, - item->last, tmp); - off2 +=3D tmp + pad; + dst->last =3D pf->data + off2; + off2 +=3D flow_item_spec_copy + (pf ? pf->data + off2 : NULL, item, ITEM_LAST); } if (item->mask) { if (pf) - dst->mask =3D memcpy(pf->data + off2, - item->mask, tmp); - off2 +=3D tmp + pad; + dst->mask =3D pf->data + off2; + off2 +=3D flow_item_spec_copy + (pf ? pf->data + off2 : NULL, item, ITEM_MASK); } off2 =3D RTE_ALIGN_CEIL(off2, sizeof(double)); } while ((item++)->type !=3D RTE_FLOW_ITEM_TYPE_END); @@ -1141,12 +1200,1= 1 @@ port_flow_new(const struct rte_flow_attr *attr, if (pf) dst =3D memcpy(pf->data + off1, action, sizeof(*action)); off1 +=3D sizeof(*action); - flow_action_conf_size(action, &tmp, &pad); if (action->conf) { if (pf) - dst->conf =3D memcpy(pf->data + off2, - action->conf, tmp); - off2 +=3D tmp + pad; + dst->conf =3D pf->data + off2; + off2 +=3D flow_action_conf_copy + (pf ? pf->data + off2 : NULL, action); } off2 =3D RTE_ALIGN_CEIL(off2, sizeof(double)); } while ((action++)->type !=3D RTE_FLOW_ACTION_TYPE_END); -- 2.11.0