From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 4FB49A0540; Mon, 13 Jul 2020 10:33:21 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3B14E1D52B; Mon, 13 Jul 2020 10:33:20 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id F11401D508 for ; Mon, 13 Jul 2020 10:33:17 +0200 (CEST) IronPort-SDR: sS5gaqQZHx7VA9sEb5zz4VaDOVtPHTZouMIVneixxNguJSqXvpTtWBqxgjX1oYojywa6TMVbIp M6RcLwHETSNg== X-IronPort-AV: E=McAfee;i="6000,8403,9680"; a="128631509" X-IronPort-AV: E=Sophos;i="5.75,346,1589266800"; d="scan'208";a="128631509" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Jul 2020 01:33:15 -0700 IronPort-SDR: SKAzymiPpgHreoIOvE3MxbZIfO4D3waa9esX2sr1EdTmdg7IWlN7X6piYxLDWhi/K37K0vwGSO acV2a5RUvZKg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.75,346,1589266800"; d="scan'208";a="485395827" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga005.fm.intel.com with ESMTP; 13 Jul 2020 01:33:15 -0700 Received: from FMSEDG001.ED.cps.intel.com (10.1.192.133) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 13 Jul 2020 01:33:15 -0700 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.172) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 13 Jul 2020 01:33:14 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Hp8j3OrU3FJ3d/EdIoaFRUheQ65mxd5QjrC4B/SN7IUNE79Y+4i1sqmQzRNPdfbLPYmZIYJzKQgKoY+jPVb8uDsTCckZs4FfhWo6UXSyCyuDAlOiSFxP1hv7zlkwhYp4sMyKDYqxPRA1u03m5FmzicA9cfm/f8jeMudz9lMuUS8ffsJFPUYx83iMpkJFyLIyan07Q2WSjdoyjIm7XBfHIYx8cCVL+4hlGP0gBfO3dFKAJdl+OjsejDKMZ2TjmcDzQn9gj4PkgctmmSG3al4psjvuadbmEbKUzUB27EBb1JTDjcri46ubdjdqIjopkT/k5MyzmmNm7EvM9jJyAUqsgA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=RDaGIOKzoYZUgIuO8EsNmyOMP2M0r9aOc/tpTOvHaQI=; b=HH5/9cNnFhtuEAwWqtLTkXclaaHX+WMwB6Wel5a+m97QeluNNu5NpdlB+BUZEMGVDiiOiHv4q9nrYeQUu3cxYaVNqsjrFDDVJqogbk5VpfTH2ioI2/P0fj8ydtl8zdCMhIWv551cpfQCvgJOuJBzAiurzaVsURC3pD2FOGKZciW6wC3KNjKj+YG1E5sY3OQyi16bEew+/RoOIrNyTBf3xUEUlkSM4eqnbQpjJ735BWRfv9UpWUPAYHmcWrKKyPGNawGWYK7qM990h+47ZMU7rGxfDmF4N40R/H+DbSQzupSQpSkIFWMNr0vL8DqjYkNceA78rcT9lx+MOV6VsjCFlw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=RDaGIOKzoYZUgIuO8EsNmyOMP2M0r9aOc/tpTOvHaQI=; b=l9EELzSwqRD9Qbg/Hg7c2qDcB3BR+XVgZCxV2ub6sm9R42E+CLtUXirzTwAMJTAJ/6quRCeg+dbDO5bI9RmgnEfB2h2fp5CVgb0iToeYr87H6ryScBmcbkbOP2UC4gdft9LKpwkHf8cHGG/RTIvYxsp12ta7gYY0juxY6Swqf+k= Received: from SN6PR11MB2829.namprd11.prod.outlook.com (2603:10b6:805:62::28) by SN6PR11MB3053.namprd11.prod.outlook.com (2603:10b6:805:d9::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3174.24; Mon, 13 Jul 2020 08:33:04 +0000 Received: from SN6PR11MB2829.namprd11.prod.outlook.com ([fe80::9db:f59b:3671:ab]) by SN6PR11MB2829.namprd11.prod.outlook.com ([fe80::9db:f59b:3671:ab%7]) with mapi id 15.20.3174.025; Mon, 13 Jul 2020 08:33:04 +0000 From: "Sun, Chenmin" To: "Zhang, Qi Z" , "Xing, Beilei" , "Wu, Jingjing" , "Wang, Haiyue" CC: "dev@dpdk.org" Thread-Topic: [PATCH V2] net/i40e: i40e FDIR update rate optimization Thread-Index: AQHWVbPUOhNXWQKnYUaXz5F6yOROQakAHlcAgAUQZnA= Date: Mon, 13 Jul 2020 08:33:04 +0000 Message-ID: References: <20200612180015.14760-1-chenmin.sun@intel.com> <20200709143932.35806-1-chenmin.sun@intel.com> <039ED4275CED7440929022BC67E706115485A08F@SHSMSX103.ccr.corp.intel.com> In-Reply-To: <039ED4275CED7440929022BC67E706115485A08F@SHSMSX103.ccr.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=intel.com; x-originating-ip: [192.102.204.45] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 727bf959-df33-40c9-9cf8-08d827075cd6 x-ms-traffictypediagnostic: SN6PR11MB3053: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:9508; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: kcmHZEMVibuN+X9iLxWHoQa64XhlpCBb2A2Mtkr3cmngCP0qwF3M279cm5pnR7fxHpEeJ83Cppa5rJ2F6q1TYK5xuRjZvQJU9tSuJiIhYl+ieih02Vase/hvZKjJ/c8zbkEyXShsz8rnNRoMVaJvySfj64yPcroHS9EKFJp1VFf6J/6s7KoHjFthUwoO4UpR9iTO0uKEIQGb6shbtVn5TsW1KaZZmXND4CXh+Y5XlQMMqgYZu3U/N67ZN6gWx7kT12rB36lTjqG7Z594kpVMw7Wuu5hHr0F4jr3ClJGYIvt5pBRwhN8/X/MCdv0NnmI4uIUJn7CoDKTrLmVzrjzfaA== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:SN6PR11MB2829.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(4636009)(136003)(366004)(39860400002)(346002)(396003)(376002)(4326008)(8936002)(83380400001)(5660300002)(478600001)(26005)(186003)(110136005)(33656002)(15650500001)(9686003)(30864003)(52536014)(55016002)(7696005)(71200400001)(8676002)(2906002)(86362001)(6636002)(66946007)(64756008)(66556008)(66476007)(316002)(76116006)(6506007)(53546011)(66446008)(579004); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: TqtmGIL4aV20CCQQBymzxFzYlIkRNjg1n/3jleSmupE0S4/8N1fg3wzVKLVo3NnE33JD+LWQ/R3BJ8xS7bB13KPLbK9bxzj3ZxovjMSzCPOeMayRvKv6DfSXt5a7WeXIIunsXeE1ckA/IMiudDuOGoFjPzyLbN4JiO2mMxWWIQUgf30N7H8heviy0OjLdTjgy1OI7iXwzM1kR1GvknDCtDhfNAe9sPmewpA/9EgM5RFxH9NAtg1L42kf+oRiO8NGiSZFRBj/np5J1bGvmJ2OiyEmVfML1ploB9AQedwVPJjrQrkdGeyb2LU8tLbsVqDShylPa6nhEgtq5oe8gwGmafIl/VUb1ak32RMfT+ZmxaGBvRzBpXoe4uIp+MTn5YW7Ht+PjnCUt3JTLL4tOES0ru/Oz20nSHVKhhNViH14ojN19cYJBhrA08OYc8xIoFQ/sUSc1msqFWGhJIVEkxwp0uQUwcn5Qp2rjFWGinPrHmtCxJBVs+xxCHY6lgXeVImU Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: SN6PR11MB2829.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 727bf959-df33-40c9-9cf8-08d827075cd6 X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Jul 2020 08:33:04.7688 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: +w4WS8zduRp1XdbCu9lgFgymKtb9Qu8ttVMjUhr4NdmK4rws8vst4Pffb8nqR5Untz4wjnkTvHBbD3ynAf5kMA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR11MB3053 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH V2] net/i40e: i40e FDIR update rate optimization X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Best Regards, Sun, Chenmin > -----Original Message----- > From: Zhang, Qi Z > Sent: Friday, July 10, 2020 10:51 AM > To: Sun, Chenmin ; Xing, Beilei > ; Wu, Jingjing ; Wang, > Haiyue > Cc: dev@dpdk.org > Subject: RE: [PATCH V2] net/i40e: i40e FDIR update rate optimization >=20 >=20 >=20 > > -----Original Message----- > > From: Sun, Chenmin > > Sent: Thursday, July 9, 2020 10:40 PM > > To: Zhang, Qi Z ; Xing, Beilei > > ; Wu, Jingjing ; Wang, > > Haiyue > > Cc: dev@dpdk.org; Sun, Chenmin > > Subject: [PATCH V2] net/i40e: i40e FDIR update rate optimization > > > > From: Chenmin Sun > > > > This patch optimized the fdir update rate for i40e PF, by tracking > > whether the fdir rule being inserted into the guaranteed space or share= d > space. > > For the flows that are inserted to the guaranteed space, we assume > > that the insertion will always succeed as the hardware only reports > > the "no enough space left" error. In this case, the software can > > directly return success and no need to retrieve the result from the > > hardware. See the fdir programming status descriptor format in the > datasheet for more details. > > > > This patch changes the global register GLQF_CTL. Therefore, when > > devarg ``support-multi-driver`` is set, the patch will not take effect > > to avoid affecting the normal behavior of other i40e drivers, e.g., Lin= ux > kernel driver. >=20 > Overall I think the patch is too big, is that possible to separate into 2= or more > patches? >=20 OK > For example: > 1.) you introduce some new data structure to tack the flow > 2) the optimization for flow programming. >=20 > More comments inline. > > > > Signed-off-by: Chenmin Sun > > --- > > > > v2: > > * Refine commit message and code comments. > > * Refine code style. > > * Fixed several memory free bugs. > > * Replace the bin_serch() with rte_bsf64() > > --- > > drivers/net/i40e/i40e_ethdev.c | 136 ++++++++++++++++++++++- > > drivers/net/i40e/i40e_ethdev.h | 63 ++++++++--- > > drivers/net/i40e/i40e_fdir.c | 190 +++++++++++++++++++++----------- > > drivers/net/i40e/i40e_flow.c | 167 ++++++++++++++++++++++------ > > drivers/net/i40e/i40e_rxtx.c | 15 ++- > > drivers/net/i40e/rte_pmd_i40e.c | 2 +- > > 6 files changed, 455 insertions(+), 118 deletions(-) > > > > diff --git a/drivers/net/i40e/i40e_ethdev.c > > b/drivers/net/i40e/i40e_ethdev.c index 3bc312c11..099f4c5e3 100644 > > --- a/drivers/net/i40e/i40e_ethdev.c > > +++ b/drivers/net/i40e/i40e_ethdev.c > > @@ -26,6 +26,7 @@ > > #include > > #include > > #include > > +#include > > > > #include "i40e_logs.h" > > #include "base/i40e_prototype.h" > > @@ -1045,8 +1046,17 @@ static int > > i40e_init_fdir_filter_list(struct rte_eth_dev *dev) { > > struct i40e_pf *pf =3D > > I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > > + struct i40e_hw *hw =3D I40E_PF_TO_HW(pf); > > struct i40e_fdir_info *fdir_info =3D &pf->fdir; > > char fdir_hash_name[RTE_HASH_NAMESIZE]; > > + void *mem =3D NULL; > > + uint32_t i =3D 0; > > + uint32_t bmp_size; > > + uint32_t alloc =3D 0; > > + uint32_t best =3D 0; > > + uint32_t pfqf_fdalloc =3D 0; > > + uint32_t glqf_ctl_reg =3D 0; > > + struct rte_bitmap *bmp =3D NULL; > > int ret; >=20 > Its better to follow RCT, and please apply on other functions. >=20 OK > > > > struct rte_hash_parameters fdir_hash_params =3D { @@ -1067,6 > +1077,7 > > @@ i40e_init_fdir_filter_list(struct rte_eth_dev *dev) > > PMD_INIT_LOG(ERR, "Failed to create fdir hash table!"); > > return -EINVAL; > > } > > + > > fdir_info->hash_map =3D rte_zmalloc("i40e_fdir_hash_map", > > sizeof(struct i40e_fdir_filter *) * > > I40E_MAX_FDIR_FILTER_NUM, > > @@ -1077,8 +1088,100 @@ i40e_init_fdir_filter_list(struct rte_eth_dev > > *dev) > > ret =3D -ENOMEM; > > goto err_fdir_hash_map_alloc; > > } > > + > > + fdir_info->fdir_filter_array =3D rte_zmalloc("fdir_filter", > > + sizeof(struct i40e_fdir_filter) * > > + I40E_MAX_FDIR_FILTER_NUM, > > + 0); > > + > > + if (!fdir_info->fdir_filter_array) { > > + PMD_INIT_LOG(ERR, > > + "Failed to allocate memory for fdir filter array!"); > > + ret =3D -ENOMEM; > > + goto err_fdir_filter_array_alloc; > > + } > > + > > + pfqf_fdalloc =3D i40e_read_rx_ctl(hw, I40E_PFQF_FDALLOC); > > + alloc =3D ((pfqf_fdalloc & I40E_PFQF_FDALLOC_FDALLOC_MASK) >> > > + I40E_PFQF_FDALLOC_FDALLOC_SHIFT); > > + best =3D ((pfqf_fdalloc & I40E_PFQF_FDALLOC_FDBEST_MASK) >> > > + I40E_PFQF_FDALLOC_FDBEST_SHIFT); > > + > > + glqf_ctl_reg =3D i40e_read_rx_ctl(hw, I40E_GLQF_CTL); > > + if (!pf->support_multi_driver) { > > + fdir_info->fdir_invalprio =3D 1; > > + glqf_ctl_reg |=3D I40E_GLQF_CTL_INVALPRIO_MASK; > > + PMD_DRV_LOG(INFO, "FDIR INVALPRIO set to guaranteed > first"); > > + } else { > > + if (glqf_ctl_reg | I40E_GLQF_CTL_INVALPRIO_MASK) { > > + fdir_info->fdir_invalprio =3D 1; > > + PMD_DRV_LOG(INFO, "FDIR INVALPRIO is: > guaranteed first"); > > + } else { > > + fdir_info->fdir_invalprio =3D 0; > > + PMD_DRV_LOG(INFO, "FDIR INVALPRIO is: shared > first"); > > + } > > + } > > + > > + i40e_write_rx_ctl(hw, I40E_GLQF_CTL, glqf_ctl_reg); > > + PMD_DRV_LOG(INFO, "FDIR guarantee space: %u, best_effort > > space %u.", > > + alloc * 32, best * 32); >=20 > I think *32 can be applied when you assign alloc and best. > Also its better to replace by macro with meaningful name and use bit shif= t << > but not *. I will get the alloc/best number from hw->func_caps instead > > + > > + fdir_info->fdir_space_size =3D (alloc + best) * 32; > > + fdir_info->fdir_actual_cnt =3D 0; > > + fdir_info->fdir_guarantee_available_space =3D alloc * 32; > > + fdir_info->fdir_guarantee_free_space =3D > > + fdir_info->fdir_guarantee_available_space; > > + > > + fdir_info->fdir_flow_bitmap.fdir_flow =3D > > + rte_zmalloc("i40e_fdir_flows", > > + sizeof(struct i40e_fdir_flows) * > > + fdir_info->fdir_space_size, > > + 0); > > + > > + if (!fdir_info->fdir_flow_bitmap.fdir_flow) { > > + PMD_INIT_LOG(ERR, > > + "Failed to allocate memory for bitmap flow!"); > > + ret =3D -ENOMEM; > > + goto err_fdir_bitmap_flow_alloc; > > + } > > + > > + for (i =3D 0; i < fdir_info->fdir_space_size; i++) > > + fdir_info->fdir_flow_bitmap.fdir_flow[i].idx =3D i; > > + > > + bmp_size =3D > > + rte_bitmap_get_memory_footprint(fdir_info- > >fdir_space_size); > > + > > + mem =3D rte_zmalloc("fdir_bmap", bmp_size, RTE_CACHE_LINE_SIZE); > > + if (mem =3D=3D NULL) { > > + PMD_INIT_LOG(ERR, > > + "Failed to allocate memory for fdir bitmap!"); > > + ret =3D -ENOMEM; > > + goto err_fdir_mem_alloc; > > + } > > + > > + bmp =3D rte_bitmap_init(fdir_info->fdir_space_size, mem, bmp_size); > > + if (bmp =3D=3D NULL) { > > + PMD_INIT_LOG(ERR, > > + "Failed to initialization fdir bitmap!"); > > + ret =3D -ENOMEM; > > + goto err_fdir_bmp_alloc; > > + } > > + > > + for (i =3D 0; i < fdir_info->fdir_space_size; i++) > > + rte_bitmap_set(bmp, i); > > + > > + fdir_info->fdir_flow_bitmap.b =3D bmp; > > + > > return 0; > > > > +err_fdir_bmp_alloc: > > + rte_free(mem); > > +err_fdir_mem_alloc: > > + rte_free(fdir_info->fdir_flow_bitmap.fdir_flow); > > +err_fdir_bitmap_flow_alloc: > > + rte_free(fdir_info->fdir_filter_array); > > +err_fdir_filter_array_alloc: > > + rte_free(fdir_info->hash_map); > > err_fdir_hash_map_alloc: > > rte_hash_free(fdir_info->hash_table); > > > > @@ -1749,18 +1852,34 @@ i40e_rm_fdir_filter_list(struct i40e_pf *pf) > > struct i40e_fdir_info *fdir_info; > > > > fdir_info =3D &pf->fdir; > > - /* Remove all flow director rules and hash */ > > + > > + /* Remove all flow director rules */ > > + while ((p_fdir =3D TAILQ_FIRST(&fdir_info->fdir_list))) > > + TAILQ_REMOVE(&fdir_info->fdir_list, p_fdir, rules); } > > + > > +static void > > +i40e_fdir_memory_cleanup(struct i40e_pf *pf) { > > + struct i40e_fdir_info *fdir_info; > > + > > + fdir_info =3D &pf->fdir; > > + > > + /* flow director memory cleanup */ > > if (fdir_info->hash_map) > > rte_free(fdir_info->hash_map); > > if (fdir_info->hash_table) > > rte_hash_free(fdir_info->hash_table); > > + if (fdir_info->fdir_flow_bitmap.b) > > + rte_bitmap_free(fdir_info->fdir_flow_bitmap.b); > > + if (fdir_info->fdir_flow_bitmap.fdir_flow) > > + rte_free(fdir_info->fdir_flow_bitmap.fdir_flow); > > + if (fdir_info->fdir_filter_array) > > + rte_free(fdir_info->fdir_filter_array); > > > > - while ((p_fdir =3D TAILQ_FIRST(&fdir_info->fdir_list))) { > > - TAILQ_REMOVE(&fdir_info->fdir_list, p_fdir, rules); > > - rte_free(p_fdir); > > - } > > } > > > > + > > void i40e_flex_payload_reg_set_default(struct i40e_hw *hw) { > > /* > > @@ -2618,9 +2737,14 @@ i40e_dev_close(struct rte_eth_dev *dev) > > /* Remove all flows */ > > while ((p_flow =3D TAILQ_FIRST(&pf->flow_list))) { > > TAILQ_REMOVE(&pf->flow_list, p_flow, node); > > - rte_free(p_flow); > > + /* Do not free FDIR flows since they are static allocated */ > > + if (p_flow->filter_type !=3D RTE_ETH_FILTER_FDIR) > > + rte_free(p_flow); > > } > > > > + /* release the fdir static allocated memory */ > > + i40e_fdir_memory_cleanup(pf); > > + > > /* Remove all Traffic Manager configuration */ > > i40e_tm_conf_uninit(dev); > > > > diff --git a/drivers/net/i40e/i40e_ethdev.h > > b/drivers/net/i40e/i40e_ethdev.h index 31ca05de9..5861c358b 100644 > > --- a/drivers/net/i40e/i40e_ethdev.h > > +++ b/drivers/net/i40e/i40e_ethdev.h > > @@ -264,6 +264,15 @@ enum i40e_flxpld_layer_idx { > > #define I40E_DEFAULT_DCB_APP_NUM 1 > > #define I40E_DEFAULT_DCB_APP_PRIO 3 > > > > +/* > > + * Struct to store flow created. > > + */ > > +struct rte_flow { > > + TAILQ_ENTRY(rte_flow) node; > > + enum rte_filter_type filter_type; > > + void *rule; > > +}; > > + > > /** > > * The overhead from MTU to max frame size. > > * Considering QinQ packet, the VLAN tag needs to be counted twice. > > @@ -674,17 +683,33 @@ struct i40e_fdir_filter { > > struct i40e_fdir_filter_conf fdir; > > }; > > > > +struct i40e_fdir_flows { >=20 > Why it be named as "flows" > Could you add more comment here, what's purpose of the data structure > and each field? >=20 Ok, I've added more comment for the new data structures, please check them = in v3 > > + uint32_t idx; > > + struct rte_flow flow; >=20 > Not sure if its better to move flow to the first field, so we cover betwe= en a > rte_flow point and a i40e_fdir_flow point directly. I'm not sure either... Could you explain a bit more? I'll move it to the first > > +}; > > + > > +struct i40e_fdir_flow_bitmap { > > + struct rte_bitmap *b; > > + struct i40e_fdir_flows *fdir_flow; > > +}; >=20 > Can we just add rte_bitmap into i40e_fdir_flow? >=20 I think the i40e_fdir_flow is a typo here, shoul be i40e_fdir_flows. Please check my feedback in the next comment > > + > > +#define FLOW_TO_FLOW_BITMAP(f) \ > > + container_of((f), struct i40e_fdir_flows, flow) > > + > > TAILQ_HEAD(i40e_fdir_filter_list, i40e_fdir_filter); > > /* > > * A structure used to define fields of a FDIR related info. > > */ > > struct i40e_fdir_info { > > +#define PRG_PKT_CNT 128 > > + > > struct i40e_vsi *fdir_vsi; /* pointer to fdir VSI structure */ > > uint16_t match_counter_index; /* Statistic counter index used for > > fdir*/ > > struct i40e_tx_queue *txq; > > struct i40e_rx_queue *rxq; > > - void *prg_pkt; /* memory for fdir program packet */ > > - uint64_t dma_addr; /* physic address of packet > > memory*/ > > + void *prg_pkt[PRG_PKT_CNT]; /* memory for fdir program packet > > */ > > + uint64_t dma_addr[PRG_PKT_CNT]; /* physic address of packet > > memory*/ > > + > > /* input set bits for each pctype */ > > uint64_t input_set[I40E_FILTER_PCTYPE_MAX]; > > /* > > @@ -698,6 +723,27 @@ struct i40e_fdir_info { > > struct i40e_fdir_filter **hash_map; > > struct rte_hash *hash_table; > > > > + struct i40e_fdir_filter *fdir_filter_array; > > + > > + /* > > + * Priority ordering at filter invalidation(destroying a flow) betwee= n > > + * "best effort" space and "guaranteed" space. > > + * > > + * 0 =3D At filter invalidation, the hardware first tries to incremen= t the > > + * "best effort" space. The "guaranteed" space is incremented only > > when > > + * the global "best effort" space is at it max value or the "best eff= ort" > > + * space of the PF is at its max value. > > + * 1 =3D At filter invalidation, the hardware first tries to incremen= t its > > + * "guaranteed" space. The "best effort" space is incremented only > > when > > + * it is already at its max value. > > + */ > > + uint32_t fdir_invalprio; > > + uint32_t fdir_space_size; > > + uint32_t fdir_actual_cnt; > > + uint32_t fdir_guarantee_available_space; > > + uint32_t fdir_guarantee_free_space; > > + struct i40e_fdir_flow_bitmap fdir_flow_bitmap; >=20 > What is the flow_bitmap usage? its free bitmap or alloc bitmap? > Better add more description here, or rename it with more clean purpose. >=20 This is neither a free bitmap nor an alloc bitmap. This bitmap manages all = pre-allocated rte_flow memory pools. so for the above one comment, I can't move the bitmap into i40e_fdir_flows I have add more description in v3 > > + > > /* Mark if flex pit and mask is set */ > > bool flex_pit_flag[I40E_MAX_FLXPLD_LAYER]; > > bool flex_mask_flag[I40E_FILTER_PCTYPE_MAX]; > > @@ -894,15 +940,6 @@ struct i40e_mirror_rule { > > > > TAILQ_HEAD(i40e_mirror_rule_list, i40e_mirror_rule); > > > > -/* > > - * Struct to store flow created. > > - */ > > -struct rte_flow { > > - TAILQ_ENTRY(rte_flow) node; > > - enum rte_filter_type filter_type; > > - void *rule; > > -}; > > - > > TAILQ_HEAD(i40e_flow_list, rte_flow); > > > > /* Struct to store Traffic Manager shaper profile. */ @@ -1335,8 > > +1372,8 @@ int i40e_add_del_fdir_filter(struct rte_eth_dev *dev, > > const struct rte_eth_fdir_filter *filter, > > bool add); > > int i40e_flow_add_del_fdir_filter(struct rte_eth_dev *dev, > > - const struct i40e_fdir_filter_conf *filter, > > - bool add); > > + const struct i40e_fdir_filter_conf *filter, > > + bool add, bool wait_status); > > int i40e_dev_tunnel_filter_set(struct i40e_pf *pf, > > struct rte_eth_tunnel_filter_conf *tunnel_filter, > > uint8_t add); > > diff --git a/drivers/net/i40e/i40e_fdir.c > > b/drivers/net/i40e/i40e_fdir.c index > > 4a778db4c..d7ba841d6 100644 > > --- a/drivers/net/i40e/i40e_fdir.c > > +++ b/drivers/net/i40e/i40e_fdir.c > > @@ -99,7 +99,7 @@ static int > > i40e_flow_fdir_filter_programming(struct i40e_pf *pf, > > enum i40e_filter_pctype pctype, > > const struct i40e_fdir_filter_conf *filter, > > - bool add); > > + bool add, bool wait_status); > > > > static int > > i40e_fdir_rx_queue_init(struct i40e_rx_queue *rxq) @@ -163,6 +163,7 > > @@ i40e_fdir_setup(struct i40e_pf *pf) > > char z_name[RTE_MEMZONE_NAMESIZE]; > > const struct rte_memzone *mz =3D NULL; > > struct rte_eth_dev *eth_dev =3D pf->adapter->eth_dev; > > + uint16_t i; > > > > if ((pf->flags & I40E_FLAG_FDIR) =3D=3D 0) { > > PMD_INIT_LOG(ERR, "HW doesn't support FDIR"); @@ - > 179,6 > > +180,7 @@ i40e_fdir_setup(struct i40e_pf *pf) > > PMD_DRV_LOG(INFO, "FDIR initialization has been done."); > > return I40E_SUCCESS; > > } > > + > > /* make new FDIR VSI */ > > vsi =3D i40e_vsi_setup(pf, I40E_VSI_FDIR, pf->main_vsi, 0); > > if (!vsi) { > > @@ -233,17 +235,27 @@ i40e_fdir_setup(struct i40e_pf *pf) > > eth_dev->device->driver->name, > > I40E_FDIR_MZ_NAME, > > eth_dev->data->port_id); > > - mz =3D i40e_memzone_reserve(z_name, I40E_FDIR_PKT_LEN, > > SOCKET_ID_ANY); > > + mz =3D i40e_memzone_reserve(z_name, I40E_FDIR_PKT_LEN * > > PRG_PKT_CNT, > > + SOCKET_ID_ANY); > > if (!mz) { > > PMD_DRV_LOG(ERR, "Cannot init memzone for " > > "flow director program packet."); > > err =3D I40E_ERR_NO_MEMORY; > > goto fail_mem; > > } > > - pf->fdir.prg_pkt =3D mz->addr; > > - pf->fdir.dma_addr =3D mz->iova; > > + > > + for (i =3D 0; i < PRG_PKT_CNT; i++) { > > + pf->fdir.prg_pkt[i] =3D (uint8_t *)mz->addr + > I40E_FDIR_PKT_LEN * > > + (i % PRG_PKT_CNT); >=20 > The loop is from 0 to PRG_PKT_CNT, why "i % PKG_PTK_CNT"? >=20 op "%" is useless, will remove it > > + pf->fdir.dma_addr[i] =3D mz->iova + I40E_FDIR_PKT_LEN * > > + (i % PRG_PKT_CNT); > > + } > > > > pf->fdir.match_counter_index =3D > > I40E_COUNTER_INDEX_FDIR(hw->pf_id); > > + pf->fdir.fdir_actual_cnt =3D 0; > > + pf->fdir.fdir_guarantee_free_space =3D > > + pf->fdir.fdir_guarantee_available_space; > > + > > PMD_DRV_LOG(INFO, "FDIR setup successfully, with programming > queue > > %u.", > > vsi->base_queue); > > return I40E_SUCCESS; > > @@ -327,6 +339,7 @@ i40e_init_flx_pld(struct i40e_pf *pf) > > pf->fdir.flex_set[index].src_offset =3D 0; > > pf->fdir.flex_set[index].size =3D > I40E_FDIR_MAX_FLEXWORD_NUM; > > pf->fdir.flex_set[index].dst_offset =3D 0; > > + > Dummy empty line. >=20 ok > > I40E_WRITE_REG(hw, I40E_PRTQF_FLX_PIT(index), > 0x0000C900); > > I40E_WRITE_REG(hw, > > I40E_PRTQF_FLX_PIT(index + 1), 0x0000FC29);/*non- > used*/ @@ > > -1557,11 +1570,11 @@ i40e_sw_fdir_filter_lookup(struct i40e_fdir_info > > *fdir_info, > > return fdir_info->hash_map[ret]; > > } > > > > -/* Add a flow director filter into the SW list */ static int > > i40e_sw_fdir_filter_insert(struct i40e_pf *pf, struct i40e_fdir_filter > > *filter) { > > struct i40e_fdir_info *fdir_info =3D &pf->fdir; > > + struct i40e_fdir_filter *hash_filter; > > int ret; > > > > if (filter->fdir.input.flow_ext.pkt_template) > > @@ -1577,9 +1590,14 @@ i40e_sw_fdir_filter_insert(struct i40e_pf *pf, > > struct i40e_fdir_filter *filter) > > ret); > > return ret; > > } > > - fdir_info->hash_map[ret] =3D filter; > > > > - TAILQ_INSERT_TAIL(&fdir_info->fdir_list, filter, rules); > > + if (fdir_info->hash_map[ret]) > > + return -1; > > + > > + hash_filter =3D &fdir_info->fdir_filter_array[ret]; > > + rte_memcpy(hash_filter, filter, sizeof(*filter)); > > + fdir_info->hash_map[ret] =3D hash_filter; > > + TAILQ_INSERT_TAIL(&fdir_info->fdir_list, hash_filter, rules); > > > > return 0; > > } > > @@ -1608,7 +1626,6 @@ i40e_sw_fdir_filter_del(struct i40e_pf *pf, > > struct i40e_fdir_input *input) > > fdir_info->hash_map[ret] =3D NULL; > > > > TAILQ_REMOVE(&fdir_info->fdir_list, filter, rules); > > - rte_free(filter); > > > > return 0; > > } > > @@ -1675,23 +1692,50 @@ i40e_add_del_fdir_filter(struct rte_eth_dev > > *dev, > > return ret; > > } > > > > +static inline unsigned char * > > +i40e_find_available_buffer(struct rte_eth_dev *dev) { > > + struct i40e_pf *pf =3D > > I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > > + struct i40e_fdir_info *fdir_info =3D &pf->fdir; > > + struct i40e_tx_queue *txq =3D pf->fdir.txq; > > + volatile struct i40e_tx_desc *txdp =3D &txq->tx_ring[txq->tx_tail + 1= ]; > > + uint32_t i; > > + > > + /* wait until the tx descriptor is ready */ > > + for (i =3D 0; i < I40E_FDIR_MAX_WAIT_US; i++) { > > + if ((txdp->cmd_type_offset_bsz & > > + rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) > =3D=3D > > + > rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE)) > > + break; > > + rte_delay_us(1); > > + } > > + if (i >=3D I40E_FDIR_MAX_WAIT_US) { > > + PMD_DRV_LOG(ERR, > > + "Failed to program FDIR filter: time out to get DD on tx > > queue."); > > + return NULL; > > + } > > + > > + return (unsigned char *)fdir_info->prg_pkt[txq->tx_tail / 2]; } >=20 > why tx_tail / 2, better some add some description here, and use " >> 1" i= f its > word / byte conversion. >=20 Because each fdir programming requires two tx descriptors, the number of tx= descriptors is twice of fdir buffer will use >> 1 > > + > > /** > > * i40e_flow_add_del_fdir_filter - add or remove a flow director filte= r. > > * @pf: board private structure > > * @filter: fdir filter entry > > * @add: 0 - delete, 1 - add > > */ > > + > > int > > i40e_flow_add_del_fdir_filter(struct rte_eth_dev *dev, > > const struct i40e_fdir_filter_conf *filter, > > - bool add) > > + bool add, bool wait_status) > > { > > struct i40e_hw *hw =3D > > I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > struct i40e_pf *pf =3D > > I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); > > - unsigned char *pkt =3D (unsigned char *)pf->fdir.prg_pkt; > > + unsigned char *pkt =3D NULL; > > enum i40e_filter_pctype pctype; > > struct i40e_fdir_info *fdir_info =3D &pf->fdir; > > - struct i40e_fdir_filter *fdir_filter, *node; > > + struct i40e_fdir_filter *node; > > struct i40e_fdir_filter check_filter; /* Check if the filter exists *= / > > int ret =3D 0; > > return 0; > > @@ -2324,7 +2390,7 @@ i40e_fdir_filter_restore(struct i40e_pf *pf) > > uint32_t best_cnt; /**< Number of filters in best effort spaces. = */ > > > > TAILQ_FOREACH(f, fdir_list, rules) > > - i40e_flow_add_del_fdir_filter(dev, &f->fdir, TRUE); > > + i40e_flow_add_del_fdir_filter(dev, &f->fdir, TRUE, TRUE); > > > > fdstat =3D I40E_READ_REG(hw, I40E_PFQF_FDSTAT); > > guarant_cnt =3D > > diff --git a/drivers/net/i40e/i40e_flow.c > > b/drivers/net/i40e/i40e_flow.c index 7cd537340..260e58dc1 100644 > > --- a/drivers/net/i40e/i40e_flow.c > > +++ b/drivers/net/i40e/i40e_flow.c > > @@ -17,6 +17,7 @@ > > #include > > #include > > #include > > +#include > > > > enum i40e_status_code > > i40e_fdir_setup_tx_resources(struct i40e_pf *pf) { > > struct i40e_tx_queue *txq; > > const struct rte_memzone *tz =3D NULL; > > - uint32_t ring_size; > > + uint32_t ring_size, i; > > struct rte_eth_dev *dev; > > + volatile struct i40e_tx_desc *txdp; > > > > if (!pf) { > > PMD_DRV_LOG(ERR, "PF is not available"); @@ -2987,6 > +2988,14 @@ > > i40e_fdir_setup_tx_resources(struct i40e_pf *pf) > > > > txq->tx_ring_phys_addr =3D tz->iova; > > txq->tx_ring =3D (struct i40e_tx_desc *)tz->addr; > > + > > + /* Set all the DD flags to 1 */ > > + for (i =3D 0; i < I40E_FDIR_NUM_TX_DESC; i +=3D 2) { > > + txdp =3D &txq->tx_ring[i + 1]; > > + txdp->cmd_type_offset_bsz |=3D > > I40E_TX_DESC_DTYPE_DESC_DONE; > > + txdp->buffer_addr =3D rte_cpu_to_le_64(pf->fdir.dma_addr[i > / 2]); > > + } >=20 > DD bits are assume to be reported by hardware, why we initialize them to = 1 ? >=20 Yes, it may easy to confuse people here. I do this because the i40e_find_av= ailable_buffer() checks the tx descriptor.DD field to determine whether the= descriptor can be used. After the NIC has taken the data of the tx descrip= tor, it infoms the software by setting the DD bit. The software can then sa= fely reuse the descriptor. Therefore, I set DD to 1 during initialization so that the software knows t= hat all descriptors are available. > > > > int > > -- > > 2.17.1 >=20