From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Matan Azrad <matan@mellanox.com>, "dev@dpdk.org" <dev@dpdk.org>
Cc: Thomas Monjalon <thomas@monjalon.net>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>,
Andrew Rybchenko <arybchenko@solarflare.com>,
Olivier Matz <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH 2/2] doc: announce new mbuf field for LRO
Date: Wed, 7 Aug 2019 14:18:41 +0000 [thread overview]
Message-ID: <2601191342CEEE43887BDE71AB9772580168A6325D@irsmsx105.ger.corp.intel.com> (raw)
In-Reply-To: <AM0PR0502MB401934C9CA0D4E1BD7A1F238D2D40@AM0PR0502MB4019.eurprd05.prod.outlook.com>
>
> Hi Konstantin
>
> From: Ananyev, Konstantin
> > Sent: Wednesday, August 7, 2019 1:18 PM
> > To: Matan Azrad <matan@mellanox.com>; dev@dpdk.org
> > Cc: Thomas Monjalon <thomas@monjalon.net>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Andrew Rybchenko
> > <arybchenko@solarflare.com>; Olivier Matz <olivier.matz@6wind.com>
> > Subject: RE: [PATCH 2/2] doc: announce new mbuf field for LRO
> >
> > Hi Matan,
> >
> > > > >
> > > > > The API breakage is because the ``tso_segsz`` field was documented
> > > > > for LRO.
> > > > >
> > > > > The ``tso_segsz`` field in mbuf indicates the size of each segment
> > > > > in the LRO packet in Rx path and should be provided by the LRO
> > > > > packet port.
> > > > >
> > > > > While the generic LRO packet may aggregate different segments
> > > > > sizes in one packet, it is impossible to expose this information
> > > > > for each segment by one field and it doesn't make sense to expose
> > > > > all the segments sizes in the mbuf.
> > > > >
> > > > > A new field may be added as union with the above field to expose
> > > > > the number of segments aggregated in the LRO packet.
> > > > >
> > > > > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > > > > ---
> > > > > doc/guides/rel_notes/deprecation.rst | 4 ++++
> > > > > 1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > > > b/doc/guides/rel_notes/deprecation.rst
> > > > > index c0cd9bc..e826b69 100644
> > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > @@ -45,6 +45,10 @@ Deprecation Notices
> > > > > - ``eal_parse_pci_DomBDF`` replaced by ``rte_pci_addr_parse``
> > > > > - ``rte_eal_compare_pci_addr`` replaced by ``rte_pci_addr_cmp``
> > > > >
> > > > > +* mbuf: Remove ``tso_segsz`` mbuf field providing for LRO support.
> > > > > +Use union
> > > > > + block for the field memory to be shared with a new field
> > > > > +``lro_segs_n``
> > > > > + indicates the number of segments aggregated in the LRO packet.
> > > > > +
> > > >
> > > > Wonder how the upper layer will use that information (except for stats)?
> > > > Could you guys provide any examples?
> > >
> > > 1. Stats, allow to calc accurate PPS.
> > > 2. Supply accurate information unlike the seg size which cannot be
> > accurate.
> > > 2. Let the user all the information (segs num allow an average seg
> > > size calculation)
> >
> > So just for stats, right?
>
> Stats it is one option.
>
> The user configured LRO, means he wants X > 1 packets to be aggregated by the port.
>
> Don't you think X is interesting for the user?
>
> For example, maybe there is Y for the next calculation:
>
> If average(X) < Y:
> Stop LRO - not very good for performance to aggregate small number of packets - stop LRO.
>
Might be, but I think user can use other metrics (let say average aggregated packet size)
for that purpose.
>
>
> > If so, wouldn't it be more plausible to extend PMD itself to provide some
> > extra statistics?
> > Just a thought.
>
> Yes, may be interesting but it can be redundant work when the user don't need it.
>
> >
> > >
> > > > Also what PMD should do if HW does supports LRO, but doesn't to
> > > > information?
> > >
> > > If the PMD knows all the segments size he can calculate it, no?
> > > 0 means PMD doesn't support it.
> >
> > I mean HW/PMD might support LRO, but doesn't provide information about
> > number of coalesced segments.
> > What PMD should do in that case?
>
> As I said, to set this field with 0 and set the PKT_RX_LRO flag in ol_flags.
> 0 in this case means support LRO but cannot supply the segments num.
Ok..., but then what for then to set PKT_RX_LRO at all?
From PMD perspective it would be easier not to set that flag at all and not to touch tso_segsz.
>
> Do you familiar with PMDs that supports LRO but cannot provide the segments num?
> If so, what do these PMDs can provide instead?
Yes, ixgbe PMD.
It does support TCP_LRO offload, and when enabled, does coalesce the packets,
but doesn't set PKT_RX_LRO and doesn't touch tso_segsz.
>
> > Still set DEV_RX_OFFLOAD_TCP_LRO as enabled RX offload, but don't set
> > PKT_RX_LRO flag in the RX-ed mbuf, even if it does contain coalesced
> > packets?
>
> No, read above.
>
> > As I understand that what happens now.
> > It is probably ok by me (as means no changes in ixgbe PMD)...
> > But wouldn't that mean no defined way for the user to determine will
> > HW/PMD provide that information or not?
>
> Will compare to 0, see above.
I mean how the user will determine in advance would given PMD/HW provide that info in tso_segsz or not?
Wait for the first LRO packet? Something else?
>
>
> > Konstantin
>
>
> > > > > * dpaa2: removal of ``rte_dpaa2_memsegs`` structure which has
> > > > > been
> > > > replaced
> > > > > by a pa-va search library. This structure was earlier being used for
> > holding
> > > > > memory segments used by dpaa2 driver for faster pa->va translation.
> > > > > This
> > > > > --
> > > > > 1.8.3.1
next prev parent reply other threads:[~2019-08-07 14:20 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-06 14:56 [dpdk-dev] [PATCH 1/2] doc: announce ethdev ABI change for LRO fields Matan Azrad
2019-08-06 14:56 ` [dpdk-dev] [PATCH 2/2] doc: announce new mbuf field for LRO Matan Azrad
2019-08-06 15:58 ` Ananyev, Konstantin
2019-08-06 18:50 ` Matan Azrad
2019-08-07 10:17 ` Ananyev, Konstantin
2019-08-07 12:35 ` Matan Azrad
2019-08-07 14:18 ` Ananyev, Konstantin [this message]
2019-08-08 10:16 ` Matan Azrad
2019-08-08 10:48 ` Ananyev, Konstantin
2019-08-08 11:16 ` Matan Azrad
2019-08-08 16:26 ` Ananyev, Konstantin
2019-08-06 18:17 ` Andrew Rybchenko
2019-08-10 21:31 ` Thomas Monjalon
2020-05-24 23:41 ` Thomas Monjalon
2020-06-02 6:49 ` Matan Azrad
2020-07-27 8:00 ` Olivier Matz
2020-07-27 8:41 ` Matan Azrad
2020-07-27 9:07 ` Olivier Matz
2023-06-12 16:38 ` Stephen Hemminger
2019-08-06 15:27 ` [dpdk-dev] [PATCH 1/2] doc: announce ethdev ABI change for LRO fields Andrew Rybchenko
2019-08-10 21:40 ` Thomas Monjalon
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=2601191342CEEE43887BDE71AB9772580168A6325D@irsmsx105.ger.corp.intel.com \
--to=konstantin.ananyev@intel.com \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=matan@mellanox.com \
--cc=olivier.matz@6wind.com \
--cc=thomas@monjalon.net \
/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).