[dpdk-dev] [PATCH] pmd: Add generic support for TCP TSO (Transmit Segmentation Offload)
Walukiewicz, Miroslaw
Miroslaw.Walukiewicz at intel.com
Mon Oct 20 14:45:57 CEST 2014
Hi Thomas,
Thank for your comments. My responses are inline.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Monday, October 20, 2014 1:30 PM
> To: Walukiewicz, Miroslaw
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] pmd: Add generic support for TCP TSO
> (Transmit Segmentation Offload)
>
> Hi Miroslaw,
>
> I'll try to comment your patch, but I don't know if you'll receive it.
> Indeed, you didn't reply to the previous comments.
> Please configure your email client to receive these emails.
> This is not a write-only list.
>
> 2014-10-20 05:42, miroslaw.walukiewicz at intel.com:
> > Add new PKT_TX_TCP_SEG flag
> > Add new fields in the tx offload fields indicating MSS and L4 len
>
> You should explain why these additions are needed.
I will resend a patch with better description of new fields.
>
> > /* fields to support TX offloads */
> > - union {
> > - uint16_t l2_l3_len; /**< combined l2/l3 lengths as single var
> */
> > - struct {
> > - uint16_t l3_len:9; /**< L3 (IP) Header Length. */
> > - uint16_t l2_len:7; /**< L2 (MAC) Header Length.
> */
> > + /* two bytes - l2/l3 len for compatibility (endian issues)
> > + * two bytes - reseved for alignment
> > + * two bytes - l4 len (TCP/UDP) header len
> > + * two bytes - TCP tso segment size
> > + */
> > + struct {
> > + union {
> > + uint16_t l2_l3_len; /**< combined l2/l3 len */
> > + struct {
> > + uint16_t l3_len:9; /**< L3 (IP) Header */
> > + uint16_t l2_len:7; /**< L2 (MAC) Header */
> > + };
> > };
>
> Why nesting these fields in an anonymous structure?
I want to keep a source compatibility with non-TSO applications using that field for example IP checksum computing by NIC.
Keeping this structure anonymous I do not require changes in old applications that do not need TSO support.
The second argument is that in original patch extending the rte_mbuf to 128 bytes made by Bruce the author made this structure anonymous and I follow this assumption too.
>
> > + uint16_t reserved_tx_offload;
> > + uint16_t l4_len; /**< TCP/UDP header len */
> > + uint16_t tso_segsz; /**< TCP TSO segment size */
> > };
>
> What means reserved_tx_offload?
It is really for alignment. I want to keep all this structure 8 byte long.
Really I found an issue in my patch. I think that all tx offload fields should be available in single 64-bit dword to make correct operation on in pkt_mbuf_reset and pkt_mbuf_attach.
Today these macros use only first 32-bits from structure and keeps l4_len and tso_segsz untouched.
I will modify my patch also in this direction.
>
> Is there an impact on performance of actual drivers ?
>
I did not observed on my machine any significant differences when aligned and non-aligned structure is used.
I agree that alignment is important for small packets. The TSO is used for using very long TCP segments usually.
> How this patch is related with previous work in progress about TSO?
>
As the original Bruce's patch defining a new rte_mbuf structure did not follow exactly the concept proposed by Olivier Matz I made the closest approximation.
I defined PKT_TX_TCP_SEG, l4_len, mss = tso_segsz
Using mss could be misinterpreted. I think tso_segsz much better describes this field meaning.
I completely agree that the pseudo header checksum could be computed outside driver and I also followed this assumption.
Mirek
> --
> Thomas
More information about the dev
mailing list