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

[Yuanhan Liu <yliu@fridaylinux.org>]

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.

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

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

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

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.

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

	--yliu