From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id B04B7374C for ; Tue, 4 Jul 2017 18:01:25 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP; 04 Jul 2017 09:01:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,309,1496127600"; d="scan'208";a="874693789" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by FMSMGA003.fm.intel.com with ESMTP; 04 Jul 2017 09:01:23 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 4 Jul 2017 09:01:23 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.146]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.197]) with mapi id 14.03.0319.002; Wed, 5 Jul 2017 00:01:20 +0800 From: "Hu, Jiayu" To: Yuanhan Liu CC: "dev@dpdk.org" , "Ananyev, Konstantin" , "stephen@networkplumber.org" , "Tan, Jianfeng" , "Wu, Jingjing" , "Yao, Lei A" , "Bie, Tiwei" Thread-Topic: [PATCH v10 1/3] lib: add Generic Receive Offload API framework Thread-Index: AQHS8lo++XjK4V/ikEaKrenKsbqne6JC1qgAgAEBdYA= Date: Tue, 4 Jul 2017 16:01:20 +0000 Message-ID: References: <1498805618-63649-1-git-send-email-jiayu.hu@intel.com> <1498907323-17563-1-git-send-email-jiayu.hu@intel.com> <1498907323-17563-2-git-send-email-jiayu.hu@intel.com> <20170704083714.GK11626@yliu-home> In-Reply-To: <20170704083714.GK11626@yliu-home> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v10 1/3] lib: add Generic Receive Offload API framework 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: Tue, 04 Jul 2017 16:01:26 -0000 Hi Yuanhan, > -----Original Message----- > From: Yuanhan Liu [mailto:yliu@fridaylinux.org] > Sent: Tuesday, July 4, 2017 4:37 PM > To: Hu, Jiayu > Cc: dev@dpdk.org; Ananyev, Konstantin ; > stephen@networkplumber.org; Tan, Jianfeng ; Wu, > Jingjing ; Yao, Lei A ; Bie, > Tiwei > Subject: Re: [PATCH v10 1/3] lib: add Generic Receive Offload API framewo= rk >=20 > Haven't looked at the details yet, and below are some quick comments > after a glimpse. >=20 > On Sat, Jul 01, 2017 at 07:08:41PM +0800, Jiayu Hu wrote: > ... > > +void *rte_gro_tbl_create(const > > + const struct rte_gro_param *param) >=20 > The DPDK style is: >=20 > void * > rte_gro_tbl_destroy(...) >=20 > Also you should revisit all other functions, as I have seen quite many > coding style issues like this. Thanks, I will fix the style issues. >=20 > > +{ > > + gro_tbl_create_fn create_tbl_fn; > > + gro_tbl_destroy_fn destroy_tbl_fn; > > + struct gro_tbl *gro_tbl; > > + uint64_t gro_type_flag =3D 0; > > + uint8_t i, j; > > + > > + gro_tbl =3D rte_zmalloc_socket(__func__, > > + sizeof(struct gro_tbl), > > + RTE_CACHE_LINE_SIZE, > > + param->socket_id); > > + if (gro_tbl =3D=3D NULL) > > + return NULL; > > + gro_tbl->max_packet_size =3D param->max_packet_size; > > + gro_tbl->max_timeout_cycles =3D param->max_timeout_cycles; > > + gro_tbl->desired_gro_types =3D param->desired_gro_types; > > + > > + for (i =3D 0; i < RTE_GRO_TYPE_MAX_NUM; i++) { > > + gro_type_flag =3D 1 << i; > > + > > + if ((param->desired_gro_types & gro_type_flag) =3D=3D 0) > > + continue; > > + create_tbl_fn =3D tbl_create_functions[i]; > > + if (create_tbl_fn =3D=3D NULL) > > + continue; > > + > > + gro_tbl->tbls[i] =3D create_tbl_fn( > > + param->socket_id, > > + param->max_flow_num, > > + param->max_item_per_flow); > > + if (gro_tbl->tbls[i] =3D=3D NULL) { > > + /* destroy all allocated tables */ > > + for (j =3D 0; j < i; j++) { > > + gro_type_flag =3D 1 << j; > > + if ((param->desired_gro_types & > gro_type_flag) =3D=3D 0) > > + continue; > > + destroy_tbl_fn =3D tbl_destroy_functions[j]; > > + if (destroy_tbl_fn) > > + destroy_tbl_fn(gro_tbl->tbls[j]); > > + } > > + rte_free(gro_tbl); > > + return NULL; >=20 > The typical way to handle this is to re-use rte_gro_tbl_destroy() as > much as possible. This saves duplicate code. Thanks, I will change it. >=20 > > + } > > + } > > + return gro_tbl; > > +} > > + > > +void rte_gro_tbl_destroy(void *tbl) > > +{ > > + gro_tbl_destroy_fn destroy_tbl_fn; > > + struct gro_tbl *gro_tbl =3D (struct gro_tbl *)tbl; >=20 > The cast (from void *) is unnecessary and can be dropped. Thanks, I will remove them. >=20 > ... > > +/** > > + * the max packets number that rte_gro_reassemble_burst can > > + * process in each invocation. > > + */ > > +#define RTE_GRO_MAX_BURST_ITEM_NUM 128UL > > + > > +/* max number of supported GRO types */ > > +#define RTE_GRO_TYPE_MAX_NUM 64 > > +#define RTE_GRO_TYPE_SUPPORT_NUM 0 /**< current supported > GRO num */ >=20 > The reason we need use comment style of "/**< ... */" is because this > is what the doc generator (doxygen) recognizes. If not doing this, your > comment won't be displayed at the generated doc page (for example, > http://dpdk.org/doc/api/rte__ethdev_8h.html#ade7de72f6c0f8102d01a0b3 > 438856900). >=20 > The format, as far as I known, could be: >=20 > /**< here is a comment */ > #define A_MACRO x >=20 > Or the one you did for RTE_GRO_TYPE_SUPPORT_NUM: put it at the end > of the line. >=20 > That being said, the comments for RTE_GRO_MAX_BURST_ITEM_NUM and > RTE_GRO_TYPE_MAX_NUM should be changed. Again, you should revisit > other places. Thanks, I will modify the comments style. >=20 > > + > > + > > +struct rte_gro_param { > > + uint64_t desired_gro_types; /**< desired GRO types */ > > + uint32_t max_packet_size; /**< max length of merged packets > */ > > + uint16_t max_flow_num; /**< max flow number */ > > + uint16_t max_item_per_flow; /**< max packet number per flow > */ > > + > > + /* socket index where the Ethernet port connects to */ >=20 > Ditto. >=20 > ... > > +++ b/lib/librte_gro/rte_gro_version.map > > @@ -0,0 +1,12 @@ > > +DPDK_17.08 { > > + global: > > + > > + rte_gro_tbl_create; > > + rte_gro_tbl_destroy; > > + rte_gro_reassemble_burst; > > + rte_gro_reassemble; > > + rte_gro_timeout_flush; > > + rte_gro_tbl_item_num; >=20 > The undocumented habit is to list them in alpha order. Thanks, I will change the order. BRs, Jiayu >=20 > --yliu