DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Tiwei Bie <tiwei.bie@intel.com>
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>,
	Zhihong Wang <zhihong.wang@intel.com>, <dev@dpdk.org>,
	Dilshod Urazov <Dilshod.Urazov@oktetlabs.ru>, <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add Tx preparation
Date: Wed, 5 Jun 2019 10:28:14 +0300
Message-ID: <8fb39bf5-2bd2-7faa-9ab7-551e234d77c5@solarflare.com> (raw)
In-Reply-To: <20190605014117.GA20728@___>

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.

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-03 18:50 Andrew Rybchenko
2019-06-05  1:41 ` Tiwei Bie
2019-06-05  7:28   ` Andrew Rybchenko [this message]
2019-06-05  9:37     ` Tiwei Bie
2019-06-16 10:17 ` [dpdk-dev] [PATCH v3 1/2] " Andrew Rybchenko
2019-06-16 10:17   ` [dpdk-dev] [PATCH v3 2/2] net/virtio: move VLAN tag insertion to Tx prepare Andrew Rybchenko
2019-06-17  8:54     ` Tiwei Bie
2019-06-17 11:35       ` Andrew Rybchenko
2019-06-17  8:52   ` [dpdk-dev] [PATCH v3 1/2] net/virtio: add Tx preparation Tiwei Bie
2019-06-17 11:31 ` [dpdk-dev] [PATCH v4 " Andrew Rybchenko
2019-06-17 11:31   ` [dpdk-dev] [PATCH v4 2/2] net/virtio: move VLAN tag insertion to Tx prepare Andrew Rybchenko
2019-06-18  4:42     ` Tiwei Bie
2019-06-20  9:58     ` Maxime Coquelin
2019-06-20  9:58   ` [dpdk-dev] [PATCH v4 1/2] net/virtio: add Tx preparation Maxime Coquelin

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8fb39bf5-2bd2-7faa-9ab7-551e234d77c5@solarflare.com \
    --to=arybchenko@solarflare.com \
    --cc=Dilshod.Urazov@oktetlabs.ru \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=stable@dpdk.org \
    --cc=tiwei.bie@intel.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org dpdk-dev@archiver.kernel.org
	public-inbox-index dpdk-dev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox