From: Ajit Khaparde <ajit.khaparde@broadcom.com>
To: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
Cc: dpdk-dev <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] net/bnxt: fixes for vxlan decap full offload
Date: Mon, 2 Nov 2020 16:23:54 -0800 [thread overview]
Message-ID: <CACZ4nhsXPDqEXnbJWa1ypRPSMYvFg7Q==JMeLE3jyQLTyEX-5Q@mail.gmail.com> (raw)
In-Reply-To: <1604249501-28929-1-git-send-email-venkatkumar.duvvuru@broadcom.com>
On Sun, Nov 1, 2020 at 8:52 AM Venkat Duvvuru <
venkatkumar.duvvuru@broadcom.com> wrote:
> 1. When a PMD application queries for flow counters, it could ask PMD
> to reset the counters when the application is doing the counters
> accumulation. In this case, PMD should not accumulate rather reset
> the counter.
>
> 2. Some of the PMD applications may set the protocol field in the IPv4
> spec but don't set the mask. So, consider the mask in the proto
> value calculation.
>
> 4. The cached tunnel inner flow is not getting installed in the
> context of tunnel outer flow create because of the wrong
> error code check when tunnel outer flow is installed in the
> hardware.
>
> 5. When a dpdk application offloads the same tunnel inner flow on
> all the uplink ports, other than the first one the driver rejects
> the rest of them. However, the first tunnel inner flow request
> might not be of the correct physical port. This is fixed by
> caching the tunnel inner flow entry for all the ports on which
> the flow offload request has arrived on. The tunnel inner flows
> which were cached on the irrelevant ports will eventually get
> aged out as there won't be any traffic on these ports.
>
> Fixes: 3c0da5ee4064 ("net/bnxt: add VXLAN decap offload support")
>
> Signed-off-by: Venkat Duvvuru <venkatkumar.duvvuru@broadcom.com>
> Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
>
Updated the fixes tag and commit message and applied to dpdk-next-net-brcm.
---
> drivers/net/bnxt/tf_ulp/bnxt_ulp_flow.c | 1 +
> drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c | 9 ++---
> drivers/net/bnxt/tf_ulp/ulp_flow_db.c | 18 +++++++--
> drivers/net/bnxt/tf_ulp/ulp_flow_db.h | 3 +-
> drivers/net/bnxt/tf_ulp/ulp_rte_parser.c | 14 +++++++
> drivers/net/bnxt/tf_ulp/ulp_template_struct.h | 1 +
> drivers/net/bnxt/tf_ulp/ulp_tun.c | 55
> +++++++++++++++++++++------
> drivers/net/bnxt/tf_ulp/ulp_tun.h | 13 +++++--
> 8 files changed, 89 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/bnxt/tf_ulp/bnxt_ulp_flow.c
> b/drivers/net/bnxt/tf_ulp/bnxt_ulp_flow.c
> index 75a7dbe..a53b1cf 100644
> --- a/drivers/net/bnxt/tf_ulp/bnxt_ulp_flow.c
> +++ b/drivers/net/bnxt/tf_ulp/bnxt_ulp_flow.c
> @@ -175,6 +175,7 @@ bnxt_ulp_flow_create(struct rte_eth_dev *dev,
> params.fid = fid;
> params.func_id = func_id;
> params.priority = attr->priority;
> + params.port_id = bnxt_get_phy_port_id(dev->data->port_id);
> /* Perform the rte flow post process */
> ret = bnxt_ulp_rte_parser_post_process(¶ms);
> if (ret == BNXT_TF_RC_ERROR)
> diff --git a/drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c
> b/drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c
> index 734b419..4502551 100644
> --- a/drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c
> +++ b/drivers/net/bnxt/tf_ulp/ulp_fc_mgr.c
> @@ -624,11 +624,10 @@ int ulp_fc_mgr_query_count_get(struct
> bnxt_ulp_context *ctxt,
> pthread_mutex_unlock(&ulp_fc_info->fc_lock);
> } else if (params.resource_sub_type ==
>
> BNXT_ULP_RESOURCE_SUB_TYPE_INDEX_TYPE_INT_COUNT_ACC) {
> - /* Get the stats from the parent child table */
> - ulp_flow_db_parent_flow_count_get(ctxt,
> - flow_id,
> - &count->hits,
> - &count->bytes);
> + /* Get stats from the parent child table */
> + ulp_flow_db_parent_flow_count_get(ctxt, flow_id,
> + &count->hits,
> &count->bytes,
> + count->reset);
> count->hits_set = 1;
> count->bytes_set = 1;
> } else {
> diff --git a/drivers/net/bnxt/tf_ulp/ulp_flow_db.c
> b/drivers/net/bnxt/tf_ulp/ulp_flow_db.c
> index 5e7c8ab..a331410 100644
> --- a/drivers/net/bnxt/tf_ulp/ulp_flow_db.c
> +++ b/drivers/net/bnxt/tf_ulp/ulp_flow_db.c
> @@ -868,8 +868,9 @@ ulp_flow_db_fid_free(struct bnxt_ulp_context *ulp_ctxt,
> enum bnxt_ulp_fdb_type flow_type,
> uint32_t fid)
> {
> - struct bnxt_ulp_flow_db *flow_db;
> + struct bnxt_tun_cache_entry *tun_tbl;
> struct bnxt_ulp_flow_tbl *flow_tbl;
> + struct bnxt_ulp_flow_db *flow_db;
>
> flow_db = bnxt_ulp_cntxt_ptr2_flow_db_get(ulp_ctxt);
> if (!flow_db) {
> @@ -908,6 +909,12 @@ ulp_flow_db_fid_free(struct bnxt_ulp_context
> *ulp_ctxt,
> if (flow_type == BNXT_ULP_FDB_TYPE_REGULAR)
> ulp_flow_db_func_id_set(flow_db, fid, 0);
>
> + tun_tbl = bnxt_ulp_cntxt_ptr2_tun_tbl_get(ulp_ctxt);
> + if (!tun_tbl)
> + return -EINVAL;
> +
> + ulp_clear_tun_inner_entry(tun_tbl, fid);
> +
> /* all good, return success */
> return 0;
> }
> @@ -1812,9 +1819,8 @@ ulp_flow_db_parent_flow_count_update(struct
> bnxt_ulp_context *ulp_ctxt,
> */
> int32_t
> ulp_flow_db_parent_flow_count_get(struct bnxt_ulp_context *ulp_ctxt,
> - uint32_t parent_fid,
> - uint64_t *packet_count,
> - uint64_t *byte_count)
> + uint32_t parent_fid, uint64_t
> *packet_count,
> + uint64_t *byte_count, uint8_t
> count_reset)
> {
> struct bnxt_ulp_flow_db *flow_db;
> struct ulp_fdb_parent_child_db *p_pdb;
> @@ -1835,6 +1841,10 @@ ulp_flow_db_parent_flow_count_get(struct
> bnxt_ulp_context *ulp_ctxt,
>
> p_pdb->parent_flow_tbl[idx].pkt_count;
> *byte_count =
>
> p_pdb->parent_flow_tbl[idx].byte_count;
> + if (count_reset) {
> +
> p_pdb->parent_flow_tbl[idx].pkt_count = 0;
> +
> p_pdb->parent_flow_tbl[idx].byte_count = 0;
> + }
> }
> return 0;
> }
> diff --git a/drivers/net/bnxt/tf_ulp/ulp_flow_db.h
> b/drivers/net/bnxt/tf_ulp/ulp_flow_db.h
> index f7dfd67..a2a04ce 100644
> --- a/drivers/net/bnxt/tf_ulp/ulp_flow_db.h
> +++ b/drivers/net/bnxt/tf_ulp/ulp_flow_db.h
> @@ -390,7 +390,8 @@ int32_t
> ulp_flow_db_parent_flow_count_get(struct bnxt_ulp_context *ulp_ctxt,
> uint32_t parent_fid,
> uint64_t *packet_count,
> - uint64_t *byte_count);
> + uint64_t *byte_count,
> + uint8_t count_reset);
>
> /*
> * reset the parent accumulation counters
> diff --git a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c
> b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c
> index df38b83..a54c55c 100644
> --- a/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c
> +++ b/drivers/net/bnxt/tf_ulp/ulp_rte_parser.c
> @@ -1012,6 +1012,13 @@ ulp_rte_ipv4_hdr_handler(const struct rte_flow_item
> *item,
> ULP_COMP_FLD_IDX_WR(params, BNXT_ULP_CF_IDX_O_L3, 1);
> }
>
> + /* Some of the PMD applications may set the protocol field
> + * in the IPv4 spec but don't set the mask. So, consider
> + * the mask in the proto value calculation.
> + */
> + if (ipv4_mask)
> + proto &= ipv4_mask->hdr.next_proto_id;
> +
> /* Update the field protocol hdr bitmap */
> ulp_rte_l3_proto_type_update(params, proto, inner_flag);
> ULP_COMP_FLD_IDX_WR(params, BNXT_ULP_CF_IDX_L3_HDR_CNT, ++cnt);
> @@ -1150,6 +1157,13 @@ ulp_rte_ipv6_hdr_handler(const struct rte_flow_item
> *item,
> ULP_COMP_FLD_IDX_WR(params, BNXT_ULP_CF_IDX_O_L3, 1);
> }
>
> + /* Some of the PMD applications may set the protocol field
> + * in the IPv6 spec but don't set the mask. So, consider
> + * the mask in proto value calculation.
> + */
> + if (ipv6_mask)
> + proto &= ipv6_mask->hdr.proto;
> +
> /* Update the field protocol hdr bitmap */
> ulp_rte_l3_proto_type_update(params, proto, inner_flag);
> ULP_COMP_FLD_IDX_WR(params, BNXT_ULP_CF_IDX_L3_HDR_CNT, ++cnt);
> diff --git a/drivers/net/bnxt/tf_ulp/ulp_template_struct.h
> b/drivers/net/bnxt/tf_ulp/ulp_template_struct.h
> index 9d690a9..6e5e8d6 100644
> --- a/drivers/net/bnxt/tf_ulp/ulp_template_struct.h
> +++ b/drivers/net/bnxt/tf_ulp/ulp_template_struct.h
> @@ -77,6 +77,7 @@ struct ulp_rte_parser_params {
> uint32_t parent_flow;
> uint32_t parent_fid;
> uint16_t func_id;
> + uint16_t port_id;
> uint32_t class_id;
> uint32_t act_tmpl;
> struct bnxt_ulp_context *ulp_ctx;
> diff --git a/drivers/net/bnxt/tf_ulp/ulp_tun.c
> b/drivers/net/bnxt/tf_ulp/ulp_tun.c
> index e8d2861..44b9b4b 100644
> --- a/drivers/net/bnxt/tf_ulp/ulp_tun.c
> +++ b/drivers/net/bnxt/tf_ulp/ulp_tun.c
> @@ -55,7 +55,8 @@ ulp_install_outer_tun_flow(struct ulp_rte_parser_params
> *params,
> RTE_ETHER_ADDR_LEN);
>
> tun_entry->valid = true;
> - tun_entry->state = BNXT_ULP_FLOW_STATE_TUN_O_OFFLD;
> + tun_entry->tun_flow_info[params->port_id].state =
> + BNXT_ULP_FLOW_STATE_TUN_O_OFFLD;
> tun_entry->outer_tun_flow_id = params->fid;
>
> /* F1 and it's related F2s are correlated based on
> @@ -83,20 +84,23 @@ ulp_install_outer_tun_flow(struct
> ulp_rte_parser_params *params,
>
> /* This function programs the inner tunnel flow in the hardware. */
> static void
> -ulp_install_inner_tun_flow(struct bnxt_tun_cache_entry *tun_entry)
> +ulp_install_inner_tun_flow(struct bnxt_tun_cache_entry *tun_entry,
> + struct ulp_rte_parser_params *tun_o_params)
> {
> struct bnxt_ulp_mapper_create_parms mparms = { 0 };
> + struct ulp_per_port_flow_info *flow_info;
> struct ulp_rte_parser_params *params;
> int ret;
>
> /* F2 doesn't have tunnel dmac, use the tunnel dmac that was
> * stored during F1 programming.
> */
> - params = &tun_entry->first_inner_tun_params;
> + flow_info = &tun_entry->tun_flow_info[tun_o_params->port_id];
> + params = &flow_info->first_inner_tun_params;
> memcpy(¶ms->hdr_field[ULP_TUN_O_DMAC_HDR_FIELD_INDEX],
> tun_entry->t_dmac, RTE_ETHER_ADDR_LEN);
> params->parent_fid = tun_entry->outer_tun_flow_id;
> - params->fid = tun_entry->first_inner_tun_flow_id;
> + params->fid = flow_info->first_tun_i_fid;
>
> bnxt_ulp_init_mapper_params(&mparms, params,
> BNXT_ULP_FDB_TYPE_REGULAR);
> @@ -117,18 +121,20 @@ ulp_post_process_outer_tun_flow(struct
> ulp_rte_parser_params *params,
> enum bnxt_ulp_tun_flow_state flow_state;
> int ret;
>
> - flow_state = tun_entry->state;
> + flow_state = tun_entry->tun_flow_info[params->port_id].state;
> ret = ulp_install_outer_tun_flow(params, tun_entry, tun_idx);
> - if (ret)
> + if (ret == BNXT_TF_RC_ERROR) {
> + PMD_DRV_LOG(ERR, "Failed to create outer tunnel flow.");
> return ret;
> + }
>
> /* If flow_state == BNXT_ULP_FLOW_STATE_NORMAL before installing
> * F1, that means F2 is not deferred. Hence, no need to install F2.
> */
> if (flow_state != BNXT_ULP_FLOW_STATE_NORMAL)
> - ulp_install_inner_tun_flow(tun_entry);
> + ulp_install_inner_tun_flow(tun_entry, params);
>
> - return 0;
> + return BNXT_TF_RC_FID;
> }
>
> /* This function will be called if inner tunnel flow request comes before
> @@ -138,6 +144,7 @@ static int32_t
> ulp_post_process_first_inner_tun_flow(struct ulp_rte_parser_params
> *params,
> struct bnxt_tun_cache_entry
> *tun_entry)
> {
> + struct ulp_per_port_flow_info *flow_info;
> int ret;
>
> ret = ulp_matcher_pattern_match(params, ¶ms->class_id);
> @@ -153,11 +160,12 @@ ulp_post_process_first_inner_tun_flow(struct
> ulp_rte_parser_params *params,
> * So, just cache the F2 information and program it in the context
> * of F1 flow installation.
> */
> - memcpy(&tun_entry->first_inner_tun_params, params,
> + flow_info = &tun_entry->tun_flow_info[params->port_id];
> + memcpy(&flow_info->first_inner_tun_params, params,
> sizeof(struct ulp_rte_parser_params));
>
> - tun_entry->first_inner_tun_flow_id = params->fid;
> - tun_entry->state = BNXT_ULP_FLOW_STATE_TUN_I_CACHED;
> + flow_info->first_tun_i_fid = params->fid;
> + flow_info->state = BNXT_ULP_FLOW_STATE_TUN_I_CACHED;
>
> /* F1 and it's related F2s are correlated based on
> * Tunnel Destination IP Address. It could be already set, if
> @@ -259,7 +267,7 @@ ulp_post_process_tun_flow(struct ulp_rte_parser_params
> *params)
> if (rc == BNXT_TF_RC_ERROR)
> return rc;
>
> - flow_state = tun_entry->state;
> + flow_state = tun_entry->tun_flow_info[params->port_id].state;
> /* Outer tunnel flow validation */
> outer_tun_sig = BNXT_OUTER_TUN_SIGNATURE(l3_tun, params);
> outer_tun_flow = BNXT_OUTER_TUN_FLOW(outer_tun_sig);
> @@ -308,3 +316,26 @@ ulp_clear_tun_entry(struct bnxt_tun_cache_entry
> *tun_tbl, uint8_t tun_idx)
> memset(&tun_tbl[tun_idx], 0,
> sizeof(struct bnxt_tun_cache_entry));
> }
> +
> +/* When a dpdk application offloads the same tunnel inner flow
> + * on all the uplink ports, a tunnel inner flow entry is cached
> + * even if it is not for the right uplink port. Such tunnel
> + * inner flows will eventually get aged out as there won't be
> + * any traffic on these ports. When such a flow destroy is
> + * called, cleanup the tunnel inner flow entry.
> + */
> +void
> +ulp_clear_tun_inner_entry(struct bnxt_tun_cache_entry *tun_tbl, uint32_t
> fid)
> +{
> + struct ulp_per_port_flow_info *flow_info;
> + int i, j;
> +
> + for (i = 0; i < BNXT_ULP_MAX_TUN_CACHE_ENTRIES ; i++) {
> + for (j = 0; j < RTE_MAX_ETHPORTS; j++) {
> + flow_info = &tun_tbl[i].tun_flow_info[j];
> + if (flow_info->first_tun_i_fid == fid &&
> + flow_info->state ==
> BNXT_ULP_FLOW_STATE_TUN_I_CACHED)
> + memset(flow_info, 0, sizeof(*flow_info));
> + }
> + }
> +}
> diff --git a/drivers/net/bnxt/tf_ulp/ulp_tun.h
> b/drivers/net/bnxt/tf_ulp/ulp_tun.h
> index ad70ae6..a4ccdb5 100644
> --- a/drivers/net/bnxt/tf_ulp/ulp_tun.h
> +++ b/drivers/net/bnxt/tf_ulp/ulp_tun.h
> @@ -70,8 +70,13 @@ enum bnxt_ulp_tun_flow_state {
> BNXT_ULP_FLOW_STATE_TUN_I_CACHED
> };
>
> -struct bnxt_tun_cache_entry {
> +struct ulp_per_port_flow_info {
> enum bnxt_ulp_tun_flow_state state;
> + uint32_t first_tun_i_fid;
> + struct ulp_rte_parser_params first_inner_tun_params;
> +};
> +
> +struct bnxt_tun_cache_entry {
> bool valid;
> bool t_dst_ip_valid;
> uint8_t t_dmac[RTE_ETHER_ADDR_LEN];
> @@ -80,13 +85,15 @@ struct bnxt_tun_cache_entry {
> uint8_t t_dst_ip6[16];
> };
> uint32_t outer_tun_flow_id;
> - uint32_t first_inner_tun_flow_id;
> uint16_t outer_tun_rej_cnt;
> uint16_t inner_tun_rej_cnt;
> - struct ulp_rte_parser_params first_inner_tun_params;
> + struct ulp_per_port_flow_info tun_flow_info[RTE_MAX_ETHPORTS];
> };
>
> void
> ulp_clear_tun_entry(struct bnxt_tun_cache_entry *tun_tbl, uint8_t
> tun_idx);
>
> +void
> +ulp_clear_tun_inner_entry(struct bnxt_tun_cache_entry *tun_tbl, uint32_t
> fid);
> +
> #endif
> --
> 2.7.4
>
>
prev parent reply other threads:[~2020-11-03 0:24 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-01 16:51 Venkat Duvvuru
2020-11-03 0:23 ` Ajit Khaparde [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACZ4nhsXPDqEXnbJWa1ypRPSMYvFg7Q==JMeLE3jyQLTyEX-5Q@mail.gmail.com' \
--to=ajit.khaparde@broadcom.com \
--cc=dev@dpdk.org \
--cc=venkatkumar.duvvuru@broadcom.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).