From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR02-VE1-obe.outbound.protection.outlook.com (mail-eopbgr20084.outbound.protection.outlook.com [40.107.2.84]) by dpdk.org (Postfix) with ESMTP id 246AB5F28 for ; Thu, 18 Oct 2018 08:34:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Xzexrd0NwUD2k8lfDV6FparOckuG2aZdNBsxvnesvfY=; b=RgQer1rWYZRdnNCegkoZKg/I711FLhk+5Z+HEj47lkwEWW5L01cWDyUx09BFjRsKKbbp5JPMhEujcPcj5+20U0V0LQgSS+4c0K9xuDxGBzu2V+SD78wOIaX1VQ7Jtq8ZH6hQdMkXEM5ZfmL7zXH0EVu6il9VHJWPVX03Ldf+qpE= Received: from DB7PR05MB4426.eurprd05.prod.outlook.com (52.134.109.15) by DB7PR05MB5260.eurprd05.prod.outlook.com (20.178.42.20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1228.26; Thu, 18 Oct 2018 06:34:02 +0000 Received: from DB7PR05MB4426.eurprd05.prod.outlook.com ([fe80::80e:e6b:baf2:d973]) by DB7PR05MB4426.eurprd05.prod.outlook.com ([fe80::80e:e6b:baf2:d973%3]) with mapi id 15.20.1228.020; Thu, 18 Oct 2018 06:34:02 +0000 From: Shahaf Shuler To: Slava Ovsiienko , Yongseok Koh CC: "dev@dpdk.org" Thread-Topic: [PATCH v2] net/mlx5: flow counters support on the Linux-rdma v19 base Thread-Index: AQHUZiDnQxlUeLiYvESJrQcMOuANyaUkfTRw Date: Thu, 18 Oct 2018 06:34:01 +0000 Message-ID: References: <1538580540-2224-1-git-send-email-viacheslavo@mellanox.com> <1539784438-29242-1-git-send-email-viacheslavo@mellanox.com> In-Reply-To: <1539784438-29242-1-git-send-email-viacheslavo@mellanox.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=shahafs@mellanox.com; x-originating-ip: [31.154.10.105] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB7PR05MB5260; 6:Pa2FbQqmr/h0Dijmxx5/MmuUx8ZJb4Qitl2Ya+w4cGCh7OLXe6+A4WAEohINisVLHlXympTix7DwI9Zd/nUfQiD3FqjQ1/H2+rAMukWPcG7sneea2HE3IEnjLlBAAVcFSB9WpqBNSXHYU4NwZHRlqIrO7jXTb9xrND0bdzlXuQTgtpopqpuyl9oQKMDOY68kI8u1v7VwX6MJpw8jUUu7GF/1oOWjTLYUn5wiZa23AmMRTc2UtlBfB1GQKbUOmxqEcPS5QPcIS1yFvwZCLSNjUmTjEP5cgMCZGZK7Zhx972yQn8bZp2SJqj8NhO2AUlTSqHyPZJQeJXluIcsCYKnCnDOB1OJYy/7ggORqRaie6m3KWSChO2qvsEG13ASFhPdnYxaU4/EU9GSOQIRjHe9TAJmkJG6zbiHj8ukd2WPbsc42sD55Ps9FaOKIVvd22e6qSySbD8/zERbZVBRlWPiM5g==; 5:dRf68tZnmVG8EyWwiEiDYtgTme3oEow2YM+81ZywdmlLpICQJpCCshIE/x8atMfpcfJ3B5xgoPGYdZcpnUBI++ii6TU9O1XSzxLXLqbfsddTJRf+d6rDEo7yA4XyMaSpxlQIQRQuRduj5IribOB4b1QjnVUJoPT0FiRU6cgzqyA=; 7:ulKSs34LNr+I0sCi3huSXrw6GNkCBxSP92atmCZbN2BHXVf9YGWY9VQhnoq+uull4HpGvsfwxX6qPLlI4V/Yde9gtknEF9MoFDmS32bPvWZ4GBvj2zwUiH7x3A4ZKAfShMCLiOFyOsOJStGPSVLBI5qvWYmD2f57kW9GJRaYYuu2CsTtSwqzhfDed5vGH4GPelcw84TeEI/eE+lSWQ4dBDWBZcmA7UvcOuuMMX/S5rkHkekYwmTpAiUwwNqNQ1c8 x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 1179710b-0149-4973-15d2-08d634c3b17a x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(5600074)(711020)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020); SRVR:DB7PR05MB5260; x-ms-traffictypediagnostic: DB7PR05MB5260: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(17755550239193); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(8121501046)(5005006)(3231355)(944501410)(4982022)(52105095)(3002001)(10201501046)(93006095)(93001095)(6055026)(149066)(150057)(6041310)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(20161123564045)(20161123562045)(20161123558120)(201708071742011)(7699051)(76991095); SRVR:DB7PR05MB5260; BCL:0; PCL:0; RULEID:; SRVR:DB7PR05MB5260; x-forefront-prvs: 08296C9B35 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(979002)(346002)(136003)(376002)(396003)(39860400002)(366004)(199004)(189003)(4326008)(6436002)(476003)(55016002)(229853002)(966005)(3846002)(33656002)(86362001)(575784001)(5660300001)(8676002)(446003)(11346002)(6116002)(5250100002)(106356001)(486006)(105586002)(71200400001)(19627235002)(186003)(478600001)(14454004)(53936002)(9686003)(68736007)(6306002)(4744004)(71190400001)(7696005)(256004)(14444005)(5024004)(6246003)(305945005)(26005)(2906002)(316002)(110136005)(81166006)(102836004)(53946003)(7736002)(81156014)(76176011)(66066001)(74316002)(2900100001)(99286004)(8936002)(25786009)(97736004)(6506007)(6636002)(579004)(309714004)(969003)(989001)(999001)(1009001)(1019001); DIR:OUT; SFP:1101; SCL:1; SRVR:DB7PR05MB5260; H:DB7PR05MB4426.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: 4dzjVYRpvOAoY3/CfBjrG5+cYUfH9sCSgewj85OsBeQbWZyYZLjjEyf/tNpSs5rqNQ8LZlXdRH6NV35XPE5LKwpyyKlM3R+23NXhsgn0q+X3mIOm513M7+mmQchB8U82ATo9nGANG3ssU+Usyl3hh31CYTYTVAQgnSvdbK/StR2aayGeRZ+XY57RtT6Z/Ibvw21TuRUurfoeXac2S8SrSI4y5Fd0Z638H9Nw9TLIiXZ4yqXIF/IhlG2NPDNwJLayedr3zEjPQY3bI69yhU+dltGiyGWRP92GpRobvgWi3pSoKfqEI28TAYswwtcar5SCkWS+73ASpIp2A0PQtvWsjGMXyqQed6xhtoWX2SLn8sQ= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1179710b-0149-4973-15d2-08d634c3b17a X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Oct 2018 06:34:01.9401 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB7PR05MB5260 Subject: Re: [dpdk-dev] [PATCH v2] net/mlx5: flow counters support on the Linux-rdma v19 base 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: , X-List-Received-Date: Thu, 18 Oct 2018 06:34:05 -0000 Hi Slava, Please see some comments below. Also - please rebase on top of series https://patches.dpdk.org/project/dpdk= /list/?series=3D1961 It moves the verbs flow query to the mlx5_flow_verbs.c Wednesday, October 17, 2018 4:54 PM, Slava Ovsiienko: > Subject: [PATCH v2] net/mlx5: flow counters support on the Linux-rdma v19 > base >=20 > Mellanox mlx5 PMD supports Flow Counters via Verbs library. > The current implementation is based on the Mellanox proprietary Verbs > library included in MLNX OFED packages. The Flow Counter support is > recently added into linux-rdma release (v19), so the mlx5 PMD update is > needed to provide Counter feature on the base of linux-rdma. Counter -> counter >=20 > mlx5 PMD can be compiled with MLNX OFED or linux-rdma v19+ and provide > flow counters for both. >=20 > Signed-off-by: Viacheslav Ovsiienko >=20 > --- > v2: > - rebased on top of master-net-mlx branch > - new compilation flags are introduced: > - HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V42, kernel/verbs > library provides the flow counter support in style of > MLNX_OFED_4.2 to MLNX_OFED_4.4 > - HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT_V45, kernel/verbs > library provides the flow counter support in style of > MLNX_OFED_4.5 or higher >=20 > v1: > - http://patches.dpdk.org/patch/45972/ > --- > drivers/net/mlx5/Makefile | 7 ++- > drivers/net/mlx5/meson.build | 4 +- > drivers/net/mlx5/mlx5.c | 13 +++++- > drivers/net/mlx5/mlx5_flow.c | 27 +++++++++--- > drivers/net/mlx5/mlx5_flow.h | 5 +++ > drivers/net/mlx5/mlx5_flow_verbs.c | 81 > +++++++++++++++++++++++++++-------- > drivers/net/mlx5/mlx5_glue.c | 88 > +++++++++++++++++++++++++++++++------- > drivers/net/mlx5/mlx5_glue.h | 22 +++++++++- > 8 files changed, 202 insertions(+), 45 deletions(-) >=20 > diff --git a/drivers/net/mlx5/Makefile b/drivers/net/mlx5/Makefile index > 1e9c0b4..fecb57c 100644 > --- a/drivers/net/mlx5/Makefile > +++ b/drivers/net/mlx5/Makefile > @@ -157,11 +157,16 @@ mlx5_autoconf.h.new: > $(RTE_SDK)/buildtools/auto-config-h.sh > enum ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT \ > $(AUTOCONF_OUTPUT) > $Q sh -- '$<' '$@' \ > - HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT \ > + HAVE_IBV_DEVICE_COUNTERS_SET_V42 \ Better to have this change of MACRO name as a separate commit.=20 > infiniband/verbs.h \ > type 'struct ibv_counter_set_init_attr' \ > $(AUTOCONF_OUTPUT) > $Q sh -- '$<' '$@' \ > + HAVE_IBV_DEVICE_COUNTERS_SET_V45 \ > + infiniband/verbs.h \ > + type 'struct ibv_counters_init_attr' \ > + $(AUTOCONF_OUTPUT) > + $Q sh -- '$<' '$@' \ > HAVE_RDMA_NL_NLDEV \ > rdma/rdma_netlink.h \ > enum RDMA_NL_NLDEV \ > diff --git a/drivers/net/mlx5/meson.build b/drivers/net/mlx5/meson.build > index c192d44..e8cbe3e 100644 > --- a/drivers/net/mlx5/meson.build > +++ b/drivers/net/mlx5/meson.build > @@ -79,8 +79,10 @@ if build > has_member_args =3D [ > [ 'HAVE_IBV_MLX5_MOD_SWP', 'infiniband/mlx5dv.h', > 'struct mlx5dv_sw_parsing_caps', 'sw_parsing_offloads' ], > - [ 'HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT', > 'infiniband/verbs.h', > + [ 'HAVE_IBV_DEVICE_COUNTERS_SET_V42', > 'infiniband/verbs.h', > 'struct ibv_counter_set_init_attr', 'counter_set_id' ], > + [ 'HAVE_IBV_DEVICE_COUNTERS_SET_V45', > 'infiniband/verbs.h', > + 'struct ibv_counters_init_attr', 'comp_mask' ], > ] > # input array for meson symbol search: > # [ "MACRO to define if found", "header for the search", diff --git > a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c index > 795a219..7dbe4bc 100644 > --- a/drivers/net/mlx5/mlx5.c > +++ b/drivers/net/mlx5/mlx5.c > @@ -739,7 +739,7 @@ > unsigned int mprq_max_stride_size_n =3D 0; > unsigned int mprq_min_stride_num_n =3D 0; > unsigned int mprq_max_stride_num_n =3D 0; -#ifdef > HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42 > struct ibv_counter_set_description cs_desc =3D { .counter_type =3D 0 }; > #endif > struct ether_addr mac; > @@ -1009,13 +1009,22 @@ > config.hw_csum =3D !!(attr.device_cap_flags_ex & > IBV_DEVICE_RAW_IP_CSUM); > DRV_LOG(DEBUG, "checksum offloading is %ssupported", > (config.hw_csum ? "" : "not ")); > -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > +#if !defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) && \ > + !defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) Like we discussed over the phone. Since this condition (has any of the flow= counter) repeats multiple times on this commit, it is better to define it = as a macro. Something like #define MLX5_VERBS_HAS_FLOW_COUNTER (defined(HAVE_IBV_DEVICE_COUNTERS_SET_V= 42) || defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45)) > + DRV_LOG(DEBUG, "no flow counters support found, OFED 4.2+ " > + "is required for counters support"); #endif #ifdef > +HAVE_IBV_DEVICE_COUNTERS_SET_V42 > config.flow_counter_en =3D !!attr.max_counter_sets; > mlx5_glue->describe_counter_set(ctx, 0, &cs_desc); > DRV_LOG(DEBUG, "counter type =3D %d, num of cs =3D %ld, attributes =3D > %d", > cs_desc.counter_type, cs_desc.num_of_cs, > cs_desc.attributes); I never liked this log, it is too developer specific. Also the flwo_counter= _en seems redundant, assuming we have the APIs we can just try to create a = counter and if it fails then there is not counter support on the kernel lev= el (this is actually the logic on the new counter approach). =20 How about the following logic (following previous commit): 1. remove the flow_counter_en flag (separate commit) 2. the logic here will be: #ifdef MLX5_VERBS_HAS_FLOW_COUNTER DRV_LOG(DEBUG, "Flow counters are supported") //the user doesn't really c= are if those are the new/old #else DRV_LOG(DEBUG, "Flow counters are not supported") #endif ? > #endif > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 > + DRV_LOG(DEBUG, "Flow counters support OFED 4.5+/upstream > assumed"); > + config.flow_counter_en =3D 1; > +#endif > config.ind_table_max_size =3D > attr.rss_caps.max_rwq_indirection_table_size; > /* > diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c > index bd70fce..4df114d 100644 > --- a/drivers/net/mlx5/mlx5_flow.c > +++ b/drivers/net/mlx5/mlx5_flow.c > @@ -2352,15 +2352,17 @@ struct rte_flow * > * @return > * 0 on success, a negative errno value otherwise and rte_errno is set= . > */ > +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \ > + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) Better to insert the ifdef inside the function, so that you will not need t= o declare it two times.=20 Then it will be like: mlx5_flow_query_count(...) { #ifdef MLX5_VERBS_HAS_FLOW_COUNTER Function logic #else Return not supported } > static int > -mlx5_flow_query_count(struct rte_flow *flow __rte_unused, > - void *data __rte_unused, > +mlx5_flow_query_count(struct rte_flow *flow, > + void *data, > struct rte_flow_error *error) > { > -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > if (flow->actions & MLX5_FLOW_ACTION_COUNT) { > struct rte_flow_query_count *qc =3D data; > uint64_t counters[2] =3D {0, 0}; Better to have the logic from here.. > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42 > struct ibv_query_counter_set_attr query_cs_attr =3D { > .cs =3D flow->counter->cs, > .query_flags =3D IBV_COUNTER_SET_FORCE_UPDATE, > @@ -2371,7 +2373,14 @@ struct rte_flow * > }; > int err =3D mlx5_glue->query_counter_set(&query_cs_attr, > &query_out); > - > +#endif > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 > + int err =3D mlx5_glue->query_counter_set( > + flow->counter->cs, > + counters, > + RTE_DIM(counters), > + > IBV_READ_COUNTERS_ATTR_PREFER_CACHED); > +#endif Till here in a separate function.=20 Also use: #Ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42 Logic #else ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 Logic #endif It is safer and the code is more clear.=20 > if (err) > return rte_flow_error_set > (error, err, > @@ -2392,12 +2401,20 @@ struct rte_flow * > RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > NULL, > "flow does not have counter"); > -#endif > +} > +#else > +static int > +mlx5_flow_query_count(struct rte_flow *flow __rte_unused, > + void *data __rte_unused, > + struct rte_flow_error *error) > +{ > return rte_flow_error_set(error, ENOTSUP, > RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > NULL, > "counters are not available"); > } > +#endif /* HAVE_IBV_DEVICE_COUNTERS_SET_Vxx */ > + >=20 > /** > * Query a flows. > diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flow.h > index 094f666..d2856c3 100644 > --- a/drivers/net/mlx5/mlx5_flow.h > +++ b/drivers/net/mlx5/mlx5_flow.h > @@ -224,7 +224,12 @@ struct mlx5_flow_counter { > uint32_t shared:1; /**< Share counter ID with other flow rules. */ > uint32_t ref_cnt:31; /**< Reference counter. */ > uint32_t id; /**< Counter ID. */ > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42 > struct ibv_counter_set *cs; /**< Holds the counters for the rule. */ > +#endif > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 > + struct ibv_counters *cs; /**< Holds the counters for the rule. */ > +#endif Ditto.=20 If -> else if -> endif > uint64_t hits; /**< Number of packets matched by the rule. */ > uint64_t bytes; /**< Number of bytes matched by the rule. */ }; diff > --git a/drivers/net/mlx5/mlx5_flow_verbs.c > b/drivers/net/mlx5/mlx5_flow_verbs.c > index 65c849c..6d5f900 100644 > --- a/drivers/net/mlx5/mlx5_flow_verbs.c > +++ b/drivers/net/mlx5/mlx5_flow_verbs.c > @@ -46,30 +46,37 @@ > * @return > * A pointer to the counter, NULL otherwise and rte_errno is set. > */ > +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \ > + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) Like before, move the macro to the function body.=20 And need to return NULL and errno =3D ENOTSUP in case there is no support.= =20 > static struct mlx5_flow_counter * > flow_verbs_counter_new(struct rte_eth_dev *dev, uint32_t shared, > uint32_t id) { > struct priv *priv =3D dev->data->dev_private; > struct mlx5_flow_counter *cnt; >=20 > - LIST_FOREACH(cnt, &priv->flow_counters, next) { > - if (!cnt->shared || cnt->shared !=3D shared) > - continue; > - if (cnt->id !=3D id) > - continue; > - cnt->ref_cnt++; > - return cnt; > + if (shared) { > + LIST_FOREACH(cnt, &priv->flow_counters, next) > + if (cnt->shared && cnt->id =3D=3D id) { > + cnt->ref_cnt++; > + return cnt; > + } This logic change should be on a separate commit as it is not part of the n= ew counter support.=20 > } > -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT >=20 > struct mlx5_flow_counter tmpl =3D { > .shared =3D shared, > .id =3D id, Better to have all the logic of the counter creation along with the macros = for the diff implementation inside a separate function.=20 > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42 > .cs =3D mlx5_glue->create_counter_set > (priv->ctx, > &(struct ibv_counter_set_init_attr){ > .counter_set_id =3D id, > }), > +#endif > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 > + .cs =3D mlx5_glue->create_counter_set > + (priv->ctx, > + &(struct ibv_counters_init_attr){0}), #endif > .hits =3D 0, > .bytes =3D 0, > }; > @@ -78,17 +85,36 @@ > rte_errno =3D errno; > return NULL; > } > + > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 > + struct ibv_counter_attach_attr attach_attr =3D {0}; > + int ret; > + > + attach_attr.counter_desc =3D IBV_COUNTER_PACKETS; > + attach_attr.index =3D 0; > + ret =3D ibv_attach_counters_point_flow(tmpl.cs, &attach_attr, NULL); > + if (!ret) { > + attach_attr.counter_desc =3D IBV_COUNTER_BYTES; > + attach_attr.index =3D 1; > + ret =3D ibv_attach_counters_point_flow(tmpl.cs, > + &attach_attr, > + NULL); > + } > + if (ret) { > + claim_zero(mlx5_glue->destroy_counter_set(tmpl.cs)); > + rte_errno =3D ret; > + return NULL; > + } > +#endif > cnt =3D rte_calloc(__func__, 1, sizeof(*cnt), 0); > if (!cnt) { > + claim_zero(mlx5_glue->destroy_counter_set(tmpl.cs)); Bug fix?=20 If yes then separate commit. For bug fixes it is super important so that we= can have it backported to the stable releases.=20 > rte_errno =3D ENOMEM; > return NULL; > } > *cnt =3D tmpl; > LIST_INSERT_HEAD(&priv->flow_counters, cnt, next); > return cnt; > -#endif > - rte_errno =3D ENOTSUP; > - return NULL; > } >=20 > /** > @@ -106,6 +132,7 @@ > rte_free(counter); > } > } > +#endif /* HAVE_IBV_DEVICE_COUNTERS_SET_Vxx */ >=20 > /** > * Add a verbs item specification into @p flow. > @@ -927,6 +954,8 @@ > * @return > * 0 On success else a negative errno value is returned and rte_errno = is set. > */ > +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \ > + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) Ditto.=20 Macro inside and on the else part return notsup.=20 > static int > flow_verbs_translate_action_count(struct rte_eth_dev *dev, > const struct rte_flow_action *action, @@ - > 936,13 +965,11 @@ { > const struct rte_flow_action_count *count =3D action->conf; > struct rte_flow *flow =3D dev_flow->flow; -#ifdef > HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > unsigned int size =3D sizeof(struct ibv_flow_spec_counter_action); > struct ibv_flow_spec_counter_action counter =3D { > .type =3D IBV_FLOW_SPEC_ACTION_COUNT, > .size =3D size, > }; > -#endif >=20 > if (!flow->counter) { > flow->counter =3D flow_verbs_counter_new(dev, count- > >shared, @@ -955,12 +982,16 @@ > " context."); > } > *action_flags |=3D MLX5_FLOW_ACTION_COUNT; -#ifdef > HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42 > counter.counter_set_handle =3D flow->counter->cs->handle; > - flow_verbs_spec_add(dev_flow, &counter, size); > #endif > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 > + counter.counters =3D flow->counter->cs; > +#endif > + flow_verbs_spec_add(dev_flow, &counter, size); > return 0; > } > +#endif /* HAVE_IBV_DEVICE_COUNTERS_SET_Vxx */ >=20 > /** > * Internal validation function. For validating both actions and items. > @@ -1157,12 +1188,15 @@ > return ret; > action_flags |=3D MLX5_FLOW_ACTION_RSS; > break; > +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \ > + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) This is not needed. It is OK to validate the count action, the code will fa= il to create the counters after with errno of not supported. On the current logic you just ignore the counter action request instead of = rejecting it as not supported.=20 > case RTE_FLOW_ACTION_TYPE_COUNT: > ret =3D mlx5_flow_validate_action_count(dev, attr, > error); > if (ret < 0) > return ret; > action_flags |=3D MLX5_FLOW_ACTION_COUNT; > break; > +#endif > default: > return rte_flow_error_set(error, ENOTSUP, >=20 > RTE_FLOW_ERROR_TYPE_ACTION, > @@ -1219,11 +1253,12 @@ > case RTE_FLOW_ACTION_TYPE_RSS: > detected_actions |=3D MLX5_FLOW_ACTION_RSS; > break; > +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \ > + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) > case RTE_FLOW_ACTION_TYPE_COUNT: > -#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > size +=3D sizeof(struct ibv_flow_spec_counter_action); > -#endif > detected_actions |=3D MLX5_FLOW_ACTION_COUNT; > +#endif Keep the ifdef as before (only on the size+=3D). The counter creation will = fail on the translate.=20 > break; > default: > break; > @@ -1406,7 +1441,6 @@ > if (priority =3D=3D MLX5_FLOW_PRIO_RSVD) > priority =3D priv->config.flow_prio - 1; > for (; actions->type !=3D RTE_FLOW_ACTION_TYPE_END; actions++) { > - int ret; > switch (actions->type) { > case RTE_FLOW_ACTION_TYPE_VOID: > break; > @@ -1434,7 +1468,11 @@ > &action_flags, > dev_flow); > break; > - case RTE_FLOW_ACTION_TYPE_COUNT: > +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \ > + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) Same. No need for this macro, you have one inside the translate function.=20 > + case RTE_FLOW_ACTION_TYPE_COUNT: { > + int ret; > + > ret =3D flow_verbs_translate_action_count(dev, > actions, >=20 > &action_flags, > @@ -1443,6 +1481,8 @@ > if (ret < 0) > return ret; > break; > + } > +#endif > default: > return rte_flow_error_set(error, ENOTSUP, >=20 > RTE_FLOW_ERROR_TYPE_ACTION, > @@ -1538,10 +1578,13 @@ > verbs->hrxq =3D NULL; > } > } > +#if defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) || \ > + defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) Why the macro is needed? If no counter support flow->counter will always be= NULL.=20 > if (flow->counter) { > flow_verbs_counter_release(flow->counter); > flow->counter =3D NULL; > } > +#endif > } >=20 > /** > diff --git a/drivers/net/mlx5/mlx5_glue.c b/drivers/net/mlx5/mlx5_glue.c > index 48590df..d77adb8 100644 > --- a/drivers/net/mlx5/mlx5_glue.c > +++ b/drivers/net/mlx5/mlx5_glue.c For the whole glue part, you didn't address my comments from the V1. Attach= ing those again: " Pay attention to how the old counter supported was added to the mlx5_glue= lib. There are no ifdefs of the function pointer declaration nor on the function= declaration. This was because it was causing link issues between this lib = to verbs.=20 Same should be for the new counters. Need to declare all needed structs if = not declared on the mlx5_glue.h file, and always the function pointer and t= he declaration.=20 The ifdef is only inside the function implementation." > @@ -211,28 +211,84 @@ > return ibv_dereg_mr(mr); > } >=20 > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 static struct ibv_counters * > +mlx5_glue_create_counters(struct ibv_context *context, > + struct ibv_counters_init_attr *init_attr) { > + return ibv_create_counters(context, init_attr); } > + > +static int > +mlx5_glue_destroy_counters(struct ibv_counters *counters) { > + return ibv_destroy_counters(counters); } > + > +static int > +mlx5_glue_attach_counters(struct ibv_counters *counters, > + struct ibv_counter_attach_attr *attr, > + struct ibv_flow *flow) > +{ > + return ibv_attach_counters_point_flow(counters, attr, flow); } > + > +static int > +mlx5_glue_query_counters(struct ibv_counters *counters, > + uint64_t *counters_value, > + uint32_t ncounters, > + uint32_t flags) > +{ > + return ibv_read_counters(counters, counters_value, ncounters, > flags); > +} #endif /* HAVE_IBV_DEVICE_COUNTERS_SET_V45 */ > + > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V42 static struct > ibv_counter_set * > +mlx5_glue_create_counter_set(struct ibv_context *context, > + struct ibv_counter_set_init_attr *init_attr) { > + return ibv_create_counter_set(context, init_attr); } > + > +static int > +mlx5_glue_destroy_counter_set(struct ibv_counter_set *cs) { > + return ibv_destroy_counter_set(cs); > +} > + > +static int > +mlx5_glue_describe_counter_set(struct ibv_context *context, > + uint16_t counter_set_id, > + struct ibv_counter_set_description *cs_desc) { > + return ibv_describe_counter_set(context, counter_set_id, cs_desc); > } > + > +static int > +mlx5_glue_query_counter_set(struct ibv_query_counter_set_attr > *query_attr, > + struct ibv_counter_set_data *cs_data) { > + return ibv_query_counter_set(query_attr, cs_data); } #endif /* > +HAVE_IBV_DEVICE_COUNTERS_SET_V42 */ > + > +#if !defined(HAVE_IBV_DEVICE_COUNTERS_SET_V42) && \ > + !defined(HAVE_IBV_DEVICE_COUNTERS_SET_V45) > static struct ibv_counter_set * > mlx5_glue_create_counter_set(struct ibv_context *context, > struct ibv_counter_set_init_attr *init_attr) { - > #ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > (void)context; > (void)init_attr; > return NULL; > -#else > - return ibv_create_counter_set(context, init_attr); > -#endif > } >=20 > static int > mlx5_glue_destroy_counter_set(struct ibv_counter_set *cs) { -#ifndef > HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > (void)cs; > return ENOTSUP; > -#else > - return ibv_destroy_counter_set(cs); > -#endif > } >=20 > static int > @@ -240,28 +296,21 @@ > uint16_t counter_set_id, > struct ibv_counter_set_description *cs_desc) { - > #ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > (void)context; > (void)counter_set_id; > (void)cs_desc; > return ENOTSUP; > -#else > - return ibv_describe_counter_set(context, counter_set_id, cs_desc); > -#endif > } >=20 > static int > mlx5_glue_query_counter_set(struct ibv_query_counter_set_attr > *query_attr, > struct ibv_counter_set_data *cs_data) { -#ifndef > HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > (void)query_attr; > (void)cs_data; > return ENOTSUP; > -#else > - return ibv_query_counter_set(query_attr, cs_data); > -#endif > } > +#endif /* HAVE_IBV_DEVICE_COUNTERS_SET_Vxx */ >=20 > static void > mlx5_glue_ack_async_event(struct ibv_async_event *event) @@ -420,10 > +469,17 @@ > .modify_qp =3D mlx5_glue_modify_qp, > .reg_mr =3D mlx5_glue_reg_mr, > .dereg_mr =3D mlx5_glue_dereg_mr, > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 > + .create_counter_set =3D mlx5_glue_create_counters, > + .destroy_counter_set =3D mlx5_glue_destroy_counters, > + .attach_counter_set =3D mlx5_glue_attach_counters, > + .query_counter_set =3D mlx5_glue_query_counters, #else > .create_counter_set =3D mlx5_glue_create_counter_set, > .destroy_counter_set =3D mlx5_glue_destroy_counter_set, > .describe_counter_set =3D mlx5_glue_describe_counter_set, > .query_counter_set =3D mlx5_glue_query_counter_set, > +#endif > .ack_async_event =3D mlx5_glue_ack_async_event, > .get_async_event =3D mlx5_glue_get_async_event, > .port_state_str =3D mlx5_glue_port_state_str, diff --git > a/drivers/net/mlx5/mlx5_glue.h b/drivers/net/mlx5/mlx5_glue.h index > f6e4e38..b9033de 100644 > --- a/drivers/net/mlx5/mlx5_glue.h > +++ b/drivers/net/mlx5/mlx5_glue.h > @@ -23,7 +23,11 @@ > #define MLX5_GLUE_VERSION "" > #endif >=20 > -#ifndef HAVE_IBV_DEVICE_COUNTERS_SET_SUPPORT > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 struct ibv_counters; struct > +ibv_counters_attach_data; struct ibv_counters_init_attr; #else > struct ibv_counter_set; > struct ibv_counter_set_data; > struct ibv_counter_set_description; > @@ -96,6 +100,21 @@ struct mlx5_glue { > struct ibv_mr *(*reg_mr)(struct ibv_pd *pd, void *addr, > size_t length, int access); > int (*dereg_mr)(struct ibv_mr *mr); > +#ifdef HAVE_IBV_DEVICE_COUNTERS_SET_V45 > + struct ibv_counters *(*create_counter_set) > + (struct ibv_context *context, > + struct ibv_counters_init_attr *init_attr); > + int (*destroy_counter_set)(struct ibv_counters *cs); > + int (*attach_counter_set) > + (struct ibv_counters *cs, > + struct ibv_counter_attach_attr *attr, > + struct ibv_flow *flow); > + int (*query_counter_set) > + (struct ibv_counters *cs, > + uint64_t *counters_value, > + uint32_t ncounters, > + uint32_t flags); > +#else > struct ibv_counter_set *(*create_counter_set) > (struct ibv_context *context, > struct ibv_counter_set_init_attr *init_attr); @@ -106,6 > +125,7 @@ struct mlx5_glue { > struct ibv_counter_set_description *cs_desc); > int (*query_counter_set)(struct ibv_query_counter_set_attr > *query_attr, > struct ibv_counter_set_data *cs_data); > +#endif > void (*ack_async_event)(struct ibv_async_event *event); > int (*get_async_event)(struct ibv_context *context, > struct ibv_async_event *event); > -- > 1.8.3.1