From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f178.google.com (mail-pf0-f178.google.com [209.85.192.178]) by dpdk.org (Postfix) with ESMTP id 96EDB2C8 for ; Tue, 4 Jul 2017 10:37:23 +0200 (CEST) Received: by mail-pf0-f178.google.com with SMTP id c73so112137984pfk.2 for ; Tue, 04 Jul 2017 01:37:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fridaylinux-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=y23ojkc6eIhlxQG46wfxWoAssSproHJLkzdA4dtvgZ4=; b=pP4Li/z3GLb9eWyK47AHOSeXnwnbjGbbteAP2VAylIDf2zWyNRUVA8Kwu2hvHYwPYt RZscLz+MN9nqOqyl2rJ380tDKzr4XlqdwHHK+KP9TBtKaeg5Yd7aA/egkkUfrtH5FNHO 07zhr5U7WfUW1i1lhzy7mDhJ1wxVyho/BQL27GxWSHHakz+ynMcJdSK/KqupiSyjCb5K M9c5lDk6R4OKLHSp1aBdgAzXuERfGXiuemBBZJj1figIzoY9xNyzBl6p5pjN1riFX0nx kuQipfJgvTeWl4JD3hc1tf9pwjmZ+iUjWEf422AXcvr4XC6tAukY6u5PSmjijoXAeYIf g1ug== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=y23ojkc6eIhlxQG46wfxWoAssSproHJLkzdA4dtvgZ4=; b=ulQj0GFtscduSEY6LailbOL/0iR5qb3FcQHcEqvRINPUcxV3AHuBnk9nsnY3q2TX6t Db9Ki+yn9cFqvSNrn3p1L8hh/bVUqFXEtIw2+CFPVUzr2be7ucwcV95B4562oBu1d+op wQziOlAuhfnOojBOwxxmFDWdQ6mvH9pIROBr3RUTTSs4rgmn4gymjNMTqH4/amv2x17p 9cQxFvti/tBocb1z/o6fdyHUFDTiX9pn6NhFky6hGst4+Rkyvfam0lD5XuGxCTQFR6AB kkT1qYxLyRJljuUCXYggTQDbGgsgfxvocSuHj+YF/66h1eyye4RJryRhC0Uktz3wfwCi wUMA== X-Gm-Message-State: AIVw111Uj90C2CrvTzhcOwm2HgD9tahvWDeK82mpJWFvp/XEEQIpnPJb BRBLyhR2JjcamJa/ X-Received: by 10.98.53.134 with SMTP id c128mr14089220pfa.36.1499157442648; Tue, 04 Jul 2017 01:37:22 -0700 (PDT) Received: from yliu-home ([45.63.61.64]) by smtp.gmail.com with ESMTPSA id f24sm39560029pff.74.2017.07.04.01.37.19 (version=TLS1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 04 Jul 2017 01:37:21 -0700 (PDT) Date: Tue, 4 Jul 2017 16:37:14 +0800 From: Yuanhan Liu To: Jiayu Hu Cc: dev@dpdk.org, konstantin.ananyev@intel.com, stephen@networkplumber.org, jianfeng.tan@intel.com, jingjing.wu@intel.com, lei.a.yao@intel.com, tiwei.bie@intel.com Message-ID: <20170704083714.GK11626@yliu-home> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1498907323-17563-2-git-send-email-jiayu.hu@intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) 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 08:37:24 -0000 Haven't looked at the details yet, and below are some quick comments after a glimpse. 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) The DPDK style is: void * rte_gro_tbl_destroy(...) Also you should revisit all other functions, as I have seen quite many coding style issues like this. > +{ > + gro_tbl_create_fn create_tbl_fn; > + gro_tbl_destroy_fn destroy_tbl_fn; > + struct gro_tbl *gro_tbl; > + uint64_t gro_type_flag = 0; > + uint8_t i, j; > + > + gro_tbl = rte_zmalloc_socket(__func__, > + sizeof(struct gro_tbl), > + RTE_CACHE_LINE_SIZE, > + param->socket_id); > + if (gro_tbl == NULL) > + return NULL; > + gro_tbl->max_packet_size = param->max_packet_size; > + gro_tbl->max_timeout_cycles = param->max_timeout_cycles; > + gro_tbl->desired_gro_types = param->desired_gro_types; > + > + for (i = 0; i < RTE_GRO_TYPE_MAX_NUM; i++) { > + gro_type_flag = 1 << i; > + > + if ((param->desired_gro_types & gro_type_flag) == 0) > + continue; > + create_tbl_fn = tbl_create_functions[i]; > + if (create_tbl_fn == NULL) > + continue; > + > + gro_tbl->tbls[i] = create_tbl_fn( > + param->socket_id, > + param->max_flow_num, > + param->max_item_per_flow); > + if (gro_tbl->tbls[i] == NULL) { > + /* destroy all allocated tables */ > + for (j = 0; j < i; j++) { > + gro_type_flag = 1 << j; > + if ((param->desired_gro_types & gro_type_flag) == 0) > + continue; > + destroy_tbl_fn = tbl_destroy_functions[j]; > + if (destroy_tbl_fn) > + destroy_tbl_fn(gro_tbl->tbls[j]); > + } > + rte_free(gro_tbl); > + return NULL; The typical way to handle this is to re-use rte_gro_tbl_destroy() as much as possible. This saves duplicate code. > + } > + } > + return gro_tbl; > +} > + > +void rte_gro_tbl_destroy(void *tbl) > +{ > + gro_tbl_destroy_fn destroy_tbl_fn; > + struct gro_tbl *gro_tbl = (struct gro_tbl *)tbl; The cast (from void *) is unnecessary and can be dropped. ... > +/** > + * 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 */ 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#ade7de72f6c0f8102d01a0b3438856900). The format, as far as I known, could be: /**< here is a comment */ #define A_MACRO x Or the one you did for RTE_GRO_TYPE_SUPPORT_NUM: put it at the end of the line. 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. > + > + > +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 */ Ditto. ... > +++ 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; The undocumented habit is to list them in alpha order. --yliu