Re: [dpdk-dev] [PATCH v2] net/virtio: add Tx preparation

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi,

On 6/5/19 4:41 AM, Tiwei Bie wrote:
> Hi,
>
> Thanks for the patch!

More will follow. At least Tx checksum offload is broken when used together
with VLAN insertion since the later prepend to mbuf, but do nothing with
l2_len/outer_l2_len. We'll send a patch to move rte_vlan_insert()
to Tx prepare and suggest separate patch to update l2_len or outer_l2_len
when software VLAN insertion is done.

> On Mon, Jun 03, 2019 at 07:50:05PM +0100, Andrew Rybchenko wrote:
> [...]
>>   uint16_t
>> +virtio_xmit_pkts_prepare(void *tx_queue __rte_unused, struct rte_mbuf **tx_pkts,
>> +			uint16_t nb_pkts)
>> +{
>> +	uint16_t nb_tx;
>> +	int error;
>> +
>> +	for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
>> +		struct rte_mbuf *m = tx_pkts[nb_tx];
>> +
>> +#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>> +		error = rte_validate_tx_offload(m);
>> +		if (unlikely(error)) {
>> +			rte_errno = -error;
>> +			break;
>> +		}
>> +#endif
>> +
>> +		error = rte_net_intel_cksum_prepare(m);
>> +		if (unlikely(error)) {
>> +			rte_errno = -error;
> It's a bit confusing here.
> Based on the API doc of rte_eth_tx_prepare():
>
> https://github.com/DPDK/dpdk/blob/7f9f46d6cef5b03681a3935b9a18378e08ca6f62/lib/librte_ethdev/rte_ethdev.h#L4360-L4362
>
> It should set negative value to rte_errno when error happens,

I'm pretty sure that it is a bug in documentation. rte_errno must be 
positive.
I'll send a patch to fix it.
Even the code just below sets positive rte_errno. Simple cases are very 
easy to find:
$ git grep 'rte_errno = E' | wc -l
557
$ git grep 'rte_errno = -E' | wc -l
50

A bit more complex cases which require careful review:
$ git grep -e 'rte_errno = -[a-z]' | wc -l
37
$ git grep -e 'rte_errno = [a-z]' | wc -l
150

Cases which look right from the first sight overweight wrong 3 times.
But it is still too many cases which are potentially wrong.

Andrew.