From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yuanhan Liu <yliu@fridaylinux.org>
Subject: Re: [PATCH v10 2/3] lib/gro: add TCP/IPv4 GRO support
Date: Tue, 4 Jul 2017 17:03:04 +0800
Message-ID: <20170704090304.GL11626@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-3-git-send-email-jiayu.hu@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
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
To: Jiayu Hu <jiayu.hu@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-pg0-f68.google.com (mail-pg0-f68.google.com [74.125.83.68])
 by dpdk.org (Postfix) with ESMTP id 2FBFA2C8
 for <dev@dpdk.org>; Tue,  4 Jul 2017 11:03:13 +0200 (CEST)
Received: by mail-pg0-f68.google.com with SMTP id u36so25928374pgn.3
 for <dev@dpdk.org>; Tue, 04 Jul 2017 02:03:13 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <1498907323-17563-3-git-send-email-jiayu.hu@intel.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Again, just some quick comments after a glimpse.

On Sat, Jul 01, 2017 at 07:08:42PM +0800, Jiayu Hu wrote:
> +	for (i = 0; i < nb_pkts; i++) {
> +		if (RTE_ETH_IS_IPV4_HDR(pkts[i]->packet_type) &&
> +				(pkts[i]->packet_type & RTE_PTYPE_L4_TCP)) {
> +			ret = gro_tcp4_reassemble(pkts[i],
> +					&tcp_tbl,
> +					param->max_packet_size,
> +					current_time);
> +			if (ret > 0)
> +				/* merge successfully */
> +				nb_after_gro--;
> +			else if (ret < 0)
> +				unprocess_pkts[unprocess_num++] =
> +					pkts[i];

Even it's just one statement, if the statement is spawned more than
one line, including the comment, the {} should be used.

Section 1.6.2. Control Statements and Loops of:
http://dpdk.org/doc/guides/contributing/coding_style.html

> +		} else
> +			unprocess_pkts[unprocess_num++] =
> +				pkts[i];

Besides, why breaking it to two lines, judging that it can be fit into
one line smaller than 80 chars.

> +	}
> +
> +	/* re-arrange GROed packets */
> +	if (nb_after_gro < nb_pkts) {
> +		i = gro_tcp4_tbl_timeout_flush(&tcp_tbl, 0,
> +				pkts, nb_pkts);
> +		if (unprocess_num > 0)
> +			memcpy(&pkts[i], unprocess_pkts,
> +					sizeof(struct rte_mbuf *) *
> +					unprocess_num);

Ditto.

> +void *gro_tcp4_tbl_create(uint16_t socket_id,
> +		uint16_t max_flow_num,
> +		uint16_t max_item_per_flow)
> +{
> +	size_t size;
> +	uint32_t entries_num;
> +	struct gro_tcp4_tbl *tbl;
> +
> +	entries_num = max_flow_num * max_item_per_flow;
> +	entries_num = entries_num > GRO_TCP4_TBL_MAX_ITEM_NUM ?
> +		GRO_TCP4_TBL_MAX_ITEM_NUM : entries_num;
> +
> +	if (entries_num == 0)
> +		return NULL;
> +
> +	tbl = (struct gro_tcp4_tbl *)rte_zmalloc_socket(
> +			__func__,
> +			sizeof(struct gro_tcp4_tbl),
> +			RTE_CACHE_LINE_SIZE,
> +			socket_id);

Again, the cast (from void *) is unnessary and should be dropped.

> +	memcpy(&(tbl->keys[key_idx].key),
> +			&key, sizeof(struct tcp4_key));

Again, I believe they two can be fit into one single line.

	--yliu