Hi Yuanhan, > -----Original Message----- > From: Yuanhan Liu [mailto:y...@fridaylinux.org] > Sent: Tuesday, July 4, 2017 5:03 PM > To: Hu, Jiayu <jiayu...@intel.com> > Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.anan...@intel.com>; > step...@networkplumber.org; Tan, Jianfeng <jianfeng....@intel.com>; Wu, > Jingjing <jingjing...@intel.com>; Yao, Lei A <lei.a....@intel.com>; Bie, > Tiwei <tiwei....@intel.com> > 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: > > + 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.
Thanks, I will add {}. > > > + } > > + > > + /* 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. Thanks, I will fix these issues. BRs, Jiayu > > --yliu