Re: [dpdk-dev] [PATCH v3 1/4] ethdev: add the API for getting burst mode information

["Wang, Haiyue" <haiyue.wang@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, October 15, 2019 00:39
> To: Wang, Haiyue <haiyue.wang@intel.com>; dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>
> Cc: Kinsella, Ray <ray.kinsella@intel.com>; Iremonger, Bernard <bernard.iremonger@intel.com>; Sun,
> Chenmin <chenmin.sun@intel.com>
> Subject: Re: [PATCH v3 1/4] ethdev: add the API for getting burst mode information
> 
> On 10/14/2019 4:35 PM, Haiyue Wang wrote:
> > Some PMDs have more than one RX/TX burst paths, add the ethdev API
> > that allows an application to retrieve the mode information about
> > Rx/Tx packet burst such as Scalar or Vector, and Vector technology
> > like AVX2.
> >
> > Signed-off-by: Haiyue Wang <haiyue.wang@intel.com>
> > Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
> 
> As far as I can see Bernard has ack only on testpmd patch, 4/4, not for reset of
> the patchset, can you please confirm this offline?
> 
> > Reviewed-by: Xiaolong Ye <xiaolong.ye@intel.com>
> 
> <...>
> 
> > +const char *
> > +rte_eth_burst_mode_option_name(uint64_t option)
> > +{
> > +	switch (option) {
> > +	case RTE_ETH_BURST_SCALAR: return "Scalar";
> > +	case RTE_ETH_BURST_VECTOR: return "Vector";
> > +
> > +	case RTE_ETH_BURST_ALTIVEC: return "AltiVec";
> > +	case RTE_ETH_BURST_NEON: return "Neon";
> > +	case RTE_ETH_BURST_SSE: return "SSE";
> > +	case RTE_ETH_BURST_AVX2: return "AVX2";
> > +	case RTE_ETH_BURST_AVX512: return "AVX512";
> > +
> > +	case RTE_ETH_BURST_SCATTERED: return "Scattered";
> > +	case RTE_ETH_BURST_BULK_ALLOC: return "Bulk Alloc";
> > +	case RTE_ETH_BURST_SIMPLE: return "Simple";
> > +
> > +	case RTE_ETH_BURST_PER_QUEUE: return "Per Queue";
> > +	}
> > +
> > +	return "";
> > +}
> 
> Hi Haiyue,
> 
> The string representation of a vector mode is a data, and I think better to keep
> it separately as an array instead of keeping this information in the function
> and make the function use that data.
> So that when new type are added it won't require to update the function itself.
> 

Hi Ferruh,

Even the vector mode is a data, it still is a bit field option, if we treated them
differently, that will make 'rte_eth_burst_mode_option_name' ugly like:

	switch (option) {
	case RTE_ETH_BURST_SCALAR: return "Scalar";
	case RTE_ETH_BURST_VECTOR: return "Vector";

	case RTE_ETH_BURST_SCATTERED: return "Scattered";
	case RTE_ETH_BURST_BULK_ALLOC: return "Bulk Alloc";
	case RTE_ETH_BURST_SIMPLE: return "Simple";

	case RTE_ETH_BURST_PER_QUEUE: return "Per Queue";
	}


static const struct {
	uint64_t vector;
	const char *name;
} rte_burst_vector_names[] = {
	{ RTE_ETH_BURST_ALTIVEC, "AltiVec" },
	{ RTE_ETH_BURST_NEON, "Neon" },
	{ RTE_ETH_BURST_SSE, "SSE" },
	{ RTE_ETH_BURST_AVX2, "AVX2" },
	{ RTE_ETH_BURST_AVX512, "AVX512" },
};

	for (i = 0; i < RTE_DIM(rte_burst_vector_names); ++i) {
		if (option == rte_burst_ vector _names[i].option)
			return rte_burst_option_names[i].name; 
	}

Why just put them together ?

static const struct {
	uint64_t option;
	const char *name;
} rte_burst_option_names[] = {
	{ RTE_ETH_BURST_SCALAR, "Scalar" },
	{ RTE_ETH_BURST_VECTOR, "Vector" },

	{ RTE_ETH_BURST_ALTIVEC, "AltiVec" },
	{ RTE_ETH_BURST_NEON, "Neon" },
	{ RTE_ETH_BURST_SSE, "SSE" },
	{ RTE_ETH_BURST_AVX2, "AVX2" },
	{ RTE_ETH_BURST_AVX512, "AVX512" },

	{ RTE_ETH_BURST_SCATTERED, "Scattered" },
	{ RTE_ETH_BURST_BULK_ALLOC, "Bulk Alloc" },
	{ RTE_ETH_BURST_SIMPLE, "Simple" },
	{ RTE_ETH_BURST_PER_QUEUE, "Per Queue" },
};

const char *
rte_eth_burst_mode_option_name(uint64_t option)
{
	const char *name = "";
	unsigned int i;

	for (i = 0; i < RTE_DIM(rte_burst_option_names); ++i) {
		if (option == rte_burst_option_names[i].option) {
			name = rte_burst_option_names[i].name;
			break;
		}
	}

	return name;
}

> 'rte_rx_offload_names' and 'rte_eth_dev_rx_offload_name()' is the good sample of
> what I mentioned above.
> 
> Thanks,
> ferruh