From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Olivier MATZ <olivier.matz@6wind.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 14:21:01 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB97725836B8ABEB@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <579A0F9E.6090200@6wind.com>
Hi Olivier,
>
> 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.
Yep, that would be another good use-case for tx_prep() I suppose.
>
> 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?
Yes, that sounds like a good thing to me.
>
> >>> 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())
I meant 'optional' in a way, that if user doesn't want to use tx_prep() and
do step #3 from the above on his own (what happens now) that is still ok.
But I think step #3 (modify packet's data) still needs to be done before tx_burst() is called for the packets.
>
> 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?
Yes.
>
> >>>>> 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.
Yes, I also hope that most apps that do use TX offloads will start to use it,
as I think it will be much more convenient way, then what we have right now.
I just liked to emphasis that user wouldn't be forced to.
Konstantin
>
>
> 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 14:21 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
2016-07-28 14:21 ` Ananyev, Konstantin [this message]
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=2601191342CEEE43887BDE71AB97725836B8ABEB@irsmsx105.ger.corp.intel.com \
--to=konstantin.ananyev@intel.com \
--cc=dev@dpdk.org \
--cc=jerin.jacob@caviumnetworks.com \
--cc=olivier.matz@6wind.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).