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
