From: Olivier MATZ <olivier.matz@6wind.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
Jerin Jacob <jerin.jacob@caviumnetworks.com>
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2] doc: announce ABI change for rte_eth_dev structure
Date: Thu, 28 Jul 2016 15:58:54 +0200 [thread overview]
Message-ID: <579A0F9E.6090200@6wind.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB97725836B8AB57@irsmsx105.ger.corp.intel.com>
Hi,
Jumping into this thread, it looks it's the last pending patch remaining
for the release.
For reference, the idea of tx_prep() was mentionned some time ago in
http://dpdk.org/ml/archives/dev/2014-May/002504.html
Few comments below.
On 07/28/2016 03:01 PM, Ananyev, Konstantin wrote:
> Right now to make HW TX offloads to work user is required to do particular actions:
> 1. set mbuf.ol_flags properly.
> 2. setup mbuf.tx_offload fields properly.
> 3. update L3/L4 header fields in a particular way.
>
> We move #3 into tx_prep(), to hide that complexity from the user simplify things for him.
> Though if he still prefers to do #3 by himself - that's ok too.
I think moving #3 out of the application is a good idea. Today, for TSO,
the offload dpdk API requires to set a specific pseudo header checksum
(which does not include the ip len, as expected by Intel drivers), and
set the IP checksum to 0.
In our app, the network stack sets the TCP checksum to the standard
pseudo header checksum, and before sending the mbuf:
- packets are split in sw if the driver does not support tso
- the tcp csum is patched to match dpdk api if the driver supports tso
In the patchs I've recently sent adding tso support for virtio-pmd, it
conforms to the dpdk API (phdr csum without ip len), so the tx function
need to patch the mbuf data inside the driver, which is something what
we want to avoid, for some good reasons explained by Konstantin.
So, I think having a tx_prep would also be the occasion to standardize a
bit the dpdk offload api, and let the driver-specific stuff in tx_prep().
Konstantin, any opinion about this?
>>> Another main purpose of tx_prep(): for multi-segment packets is to
>>> check that number of segments doesn't exceed HW limit.
>>> Again right now users have to do that on their own.
If calling tx_prep() is optional, does it mean that this check may be
done twice? (once in tx_prep() and once in tx_burst())
What would be the advantage of doing this check in tx_prep() instead of
keeping it in tx_burst(), as it does not touch the mbuf data?
>>> 3. Having it a s separate function would allow user control when/where
>>> to call it, let say only for some packets, or probably call tx_prep()
>>> on one core, and do actual tx_burst() for these packets on the other.
Yes, from what I remember, the pipeline model was the main reason why we
do not just modify the packet in tx_burst(). Right?
>>>>> If you refer as lost cycles here something like:
>>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_prep, -ENOTSUP); then
>>>>> yes.
>>>>> Though comparing to actual work need to be done for most HW TX
>>>>> offloads, I think it is neglectable.
>>>>
>>>> Not sure.
I think doing this test on a per-bulk basis should not impact performance.
> To be honest, I don't understand what is your concern here.
> That proposed change doesn't break any existing functionality,
> doesn't introduce any new requirements to the existing API,
> and wouldn't introduce any performance regression for existing apps.
> It is a an extension, and user is free not to use it, if it doesn't fit his needs.
> From other side there are users who are interested in that functionality,
> and they do have use-cases for it.
In my opinion, using tx_prep() will implicitly become mandatory as soon
as the application want to do offload. An application that won't use it
will have to prepare the mbuf, and this preparation will depend on the
device, which is not acceptable inside an application.
So, to conclude, the api change notification looks good to me, even if
there is still some room to discuss the implementation details.
Acked-by: Olivier Matz <olivier.matz@6wind.com>
next prev parent reply other threads:[~2016-07-28 13:58 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-20 14:24 [dpdk-dev] [PATCH] " Tomasz Kulasek
2016-07-20 15:01 ` Thomas Monjalon
2016-07-20 15:13 ` Ananyev, Konstantin
2016-07-20 15:22 ` Thomas Monjalon
2016-07-20 15:42 ` Kulasek, TomaszX
2016-07-21 15:24 ` [dpdk-dev] [PATCH v2] " Tomasz Kulasek
2016-07-21 22:48 ` Ananyev, Konstantin
2016-07-27 8:59 ` Thomas Monjalon
2016-07-27 17:10 ` Jerin Jacob
2016-07-27 17:33 ` Ananyev, Konstantin
2016-07-27 17:41 ` Jerin Jacob
2016-07-27 20:51 ` Ananyev, Konstantin
2016-07-28 2:13 ` Jerin Jacob
2016-07-28 10:36 ` Ananyev, Konstantin
2016-07-28 11:38 ` Jerin Jacob
2016-07-28 12:07 ` Avi Kivity
2016-07-28 13:01 ` Ananyev, Konstantin
2016-07-28 13:58 ` Olivier MATZ [this message]
2016-07-28 14:21 ` Ananyev, Konstantin
2016-07-28 13:59 ` Jerin Jacob
2016-07-28 14:52 ` Thomas Monjalon
2016-07-28 16:25 ` Jerin Jacob
2016-07-28 17:07 ` Thomas Monjalon
2016-07-31 7:50 ` Vlad Zolotarov
2016-07-28 12:04 ` Avi Kivity
2016-07-31 7:46 ` [dpdk-dev] [PATCH] " Vlad Zolotarov
2016-07-31 8:10 ` Vlad Zolotarov
Reply instructions:
You may reply publicly 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=579A0F9E.6090200@6wind.com \
--to=olivier.matz@6wind.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=konstantin.ananyev@intel.com \
--cc=thomas.monjalon@6wind.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).