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

Reply via email to