[dpdk-dev] [PATCH v10 1/3] lib: add Generic Receive Offload API framework

[Hu, Jiayu]

Hi Yuanhan,

> -----Original Message-----
> From: Yuanhan Liu [mailto:yliu at fridaylinux.org]
> Sent: Tuesday, July 4, 2017 4:37 PM
> To: Hu, Jiayu <jiayu.hu at intel.com>
> Cc: dev at dpdk.org; Ananyev, Konstantin <konstantin.ananyev at intel.com>;
> stephen at networkplumber.org; Tan, Jianfeng <jianfeng.tan at intel.com>; Wu,
> Jingjing <jingjing.wu at intel.com>; Yao, Lei A <lei.a.yao at intel.com>; Bie,
> Tiwei <tiwei.bie at intel.com>
> Subject: Re: [PATCH v10 1/3] lib: add Generic Receive Offload API framework
> 
> 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.

Thanks, I will fix the style issues.

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

Thanks, I will change it.

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

Thanks, I will remove them.

> 
> ...
> > +/**
> > + * 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#ade7de72f6c0f8102d01a0b3
> 438856900).
> 
> 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.

Thanks, I will modify the comments style.

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

Thanks, I will change the order.

BRs,
Jiayu
> 
> 	--yliu