Hi Yuanhan,

-----Original Message-----
From: Yuanhan Liu [mailto:yliu@fridaylinux.org]
Sent: Tuesday, July 4, 2017 5:03 PM
To: Hu, Jiayu <redacted>
Cc: dev@dpdk.org; Ananyev, Konstantin <redacted>;
stephen@networkplumber.org; Tan, Jianfeng [off-list ref]; Wu,
Jingjing [off-list ref]; Yao, Lei A [off-list ref]; Bie,
Tiwei [off-list ref]
Subject: Re: [PATCH v10 2/3] lib/gro: add TCP/IPv4 GRO support

Again, just some quick comments after a glimpse.

On Sat, Jul 01, 2017 at 07:08:42PM +0800, Jiayu Hu wrote:

quoted

+	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)) {

quoted

+			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

quoted

+		} 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.

Thanks, I will add {}.

quoted

+	}
+
+	/* 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.

quoted

+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.

quoted

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

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

Thanks, I will fix these issues.

BRs,
Jiayu

	--yliu