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 <ma...@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. 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) > > > > > > > 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. Konstantin
