[v2] net/virtio: add Tx preparation

  From: Dilshod Urazov <Dilshod.Urazov@oktetlabs.ru>

Virtio requires pseudo-header checksum in TCP/UDP checksum to do
offload, but it was lost when Tx prepare is introduced. Also
rte_validate_tx_offload() should be used to validate Tx offloads.

Also it is incorrect to do virtio_tso_fix_cksum() after prepend
to mbuf without taking prepended size into account, since layer 2/3/4
lengths provide incorrect offsets after prepend.

Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation")
Cc: stable@dpdk.org

Signed-off-by: Dilshod Urazov <Dilshod.Urazov@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
v2:
 - fix typo in E-mail address

 drivers/net/virtio/virtio_ethdev.c |  1 +
 drivers/net/virtio/virtio_ethdev.h |  3 +++
 drivers/net/virtio/virtio_rxtx.c   | 32 +++++++++++++++++++++++++++++++-
 3 files changed, 35 insertions(+), 1 deletion(-)
  

Hi,

Thanks for the patch!

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,
and that's also what some other PMDs do, e.g.:

https://github.com/DPDK/dpdk/blob/7f9f46d6cef5b03681a3935b9a18378e08ca6f62/drivers/net/iavf/iavf_rxtx.c#L1701
https://github.com/DPDK/dpdk/blob/7f9f46d6cef5b03681a3935b9a18378e08ca6f62/drivers/net/iavf/iavf_rxtx.c#L1725
https://github.com/DPDK/dpdk/blob/7f9f46d6cef5b03681a3935b9a18378e08ca6f62/drivers/net/vmxnet3/vmxnet3_rxtx.c#L364

But some PMDs and rte_eth_tx_prepare() itself don't do this:

https://github.com/DPDK/dpdk/blob/7f9f46d6cef5b03681a3935b9a18378e08ca6f62/lib/librte_ethdev/rte_ethdev.h#L4377
https://github.com/DPDK/dpdk/blob/7f9f46d6cef5b03681a3935b9a18378e08ca6f62/lib/librte_ethdev/rte_ethdev.h#L4387


> +			break;
> +		}
> +
> +		if (m->ol_flags & PKT_TX_TCP_SEG)
> +			virtio_tso_fix_cksum(m);
> +	}
> +
> +	return nb_tx;
> +}
> +
> +uint16_t
>  virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
>  			uint16_t nb_pkts)
>  {
> -- 
> 1.8.3.1
>
  

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.
  

On Wed, Jun 05, 2019 at 10:28:14AM +0300, Andrew Rybchenko wrote:
> 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.
> 

Thanks :)

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

Yeah. Negative rte_errno looks confusing.

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

Thanks,
Tiwei