Re: [dpdk-dev] [PATCH 2/2] doc: announce new mbuf field for LRO

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> Hi
> From: Ananyev, Konstantin
> > 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.
> > >
> > > Yes, but I think it is better to supply the segs number which is an accurate
> > number instead of average size of segment.
> > > Then, user can decide any calculation he prefers.
> > >
> > > >
> > > > >
> > > > >
> > > > > > 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.
> > >
> > > The user should know that LRO is working. LRO flag should be set in any
> > case.
> >
> > Well, then I think you trying to introduce a new requirement for PMD.
> 
> Yes, this is the patch purpose.
> 
> > Right now, as I can see it is optional, and supposed to be set only when PMD
> > RX path updates tso_segsz.
> >
> > /**
> >  * When packets are coalesced by a hardware or virtual driver, this flag
> >  * can be set in the RX mbuf, meaning that the m->tso_segsz field is
> >  * valid and is set to the segment size of original packets.
> >  */
> > #define PKT_RX_LRO           (1ULL << 16)
> 
> Yes, need to be changed to:
> /**
>  * When packets are coalesced by a hardware or virtual driver, this flag
>   * is set in the RX mbuf.
>   */
> 
> 
> > >
> > > > >
> > > > > 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.
> > >
> > > I think it should be changed to set the flag.
> > >
> > > >
> > > > >
> > > > > > 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?
> > >
> > > Or wait to for the first LRO packet, or we can add a new ethdev capability
> > for it.
> > >
> > > What do you think?
> >
> > I still in doubt is it really worth to support that feature at all...
> > Though if we'll decide to add it, then I think it needs to be a new capability
> > DEV_RX_OFFLOAD_LRO_STAT (or so) and probably new mbuf.ol_flag value
> > for it.
> 
> I don't think that new mbuf ol_flag should be introduced. Only capability.
> 
> We discussed on 2 topics:
> 
> 1. Changing the LRO field meaning in mbuf. segs size => segs num.
> 2. Whether to set the PKT_RX_LRO when the PMD cannot supply the field but can  coalesce packets or not.
> 
> If we do only 1, the flag setting method can be stay as is.
> 
> Can you agree with 1? this is the main reason for this patch.

Ok, NP with #1.

> 
> I think it is problematic that ixgbe PMD doesn't allow to the user to know whether the packet is coalesced or not.
> 
> IMO setting the PKT_RX_LRO flag and 0 in the LRO field is better.
>