From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Ananyev, Konstantin" Subject: Re: [PATCH v1] gso: fix marking TCP checksum flag in TCP segments Date: Tue, 24 Apr 2018 10:56:09 +0000 Message-ID: <2601191342CEEE43887BDE71AB977258AEA5081C@IRSMSX102.ger.corp.intel.com> References: <1524406859-29585-1-git-send-email-ophirmu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Thomas Monjalon , Olga Shern , Pascal Mazon , "stable@dpdk.org" To: Ophir Munk , "Hu, Jiayu" , "dev@dpdk.org" Return-path: In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Ophir Munk [mailto:ophirmu@mellanox.com] > Sent: Tuesday, April 24, 2018 10:44 AM > To: Hu, Jiayu ; dev@dpdk.org; Ananyev, Konstantin > Cc: Thomas Monjalon ; Olga Shern ; Pascal Mazon ; > stable@dpdk.org > Subject: RE: [PATCH v1] gso: fix marking TCP checksum flag in TCP segment= s >=20 > Hi Jiayu, > Please find comments inline >=20 > > -----Original Message----- > > From: Hu, Jiayu [mailto:jiayu.hu@intel.com] > > Sent: Monday, April 23, 2018 7:14 AM > > To: Ophir Munk ; dev@dpdk.org; Ananyev, > > Konstantin > > Cc: Thomas Monjalon ; Olga Shern > > ; Pascal Mazon ; > > stable@dpdk.org > > Subject: RE: [PATCH v1] gso: fix marking TCP checksum flag in TCP segme= nts > > > > Hi Ophir, > > > > In the GSO design, the GSO library doesn't care about checksums, which > > means it doesn't check if input packets have correct checksums, and it > > doesn't do any checksum related work for the output GSO segments. It > > depends on the callers to use HW or SW checksum calculation for output > > packets. This is why the GSO library doesn't set PKT_TX_TCP_CKSUM. So I > > don't think it's a bug. > > >=20 > Can you please reconsider this design? I think the GSO library should imi= tate the HW behavior where TCP segments checksum is > automatically calculated without explicitly requesting it. I am not sayin= g that GSO library itself should calculate the checksums - but at least > it should mark each segment as requiring this calculation. But gso has no idea how this packet will be processed after it. Caller can choose to calculate L3/L4 cksum in SW or might be going to use H= W offloads. In later case nothing stops the caller to update mbuf->ol_flags in a way he= likes (TCP_CKSUM, IP_CKSUM, etc.). Konstantin >=20 > > In my opinion, it's not a good idea to enable HW TCP checksum calculati= on > > silently, and without the aware of the caller. In fact, the caller alwa= ys know it > > does SW TSO (i.e. GSO), instead of real HW TSO. >=20 > This is not correct. Consider net_failsafe with 2 sub-devices: one is a H= W PCI device, the other one is a SW TAP device. Failsafe must work > transparently with these two sub-devices and the caller cannot tell if TS= O is done in SW or HW. >=20 > > If the caller wants HW > > checksum calculation, it can add PKT_TX_TCP_CKSUM to ol_flags before or > > after calling the GSO library. > > >=20 > FYI - TAP TSO patches were submitted to dpdk.org mailing list. These patc= hes use the GSO library. > https://dpdk.org/dev/patchwork/patch/38666/ > https://dpdk.org/dev/patchwork/patch/38667/ >=20 > Running testpmd with TAP TSO is currently broken without the suggested li= brte_gso patch. > Please note testpmd implementation (app/test-pmd/csumonly.c b/app/test-pm= d/csumonly.c) in case *both* TSO and TCP CKSUM are > configured: >=20 > if (tso_segsz) > ol_flags |=3D PKT_TX_TCP_SEG; // *** if TSO is applicable - the = packet flags are only marked with PKT_TX_TCP_SEG and no > PKT_TX_TCP_CKSUM *** > else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) > ol_flags |=3D PKT_TX_TCP_CKSUM; // *** PKT_TX_TCP_CKSUM is mar= ked only if TSO is not applicable *** > else { > tcp_hdr->cksum =3D > get_udptcp_checksum(l3_hdr, tcp_hdr, >=20 > In other words - testpmd does not set TCP_CKSUM along with TCP_SEG theref= ore using testpmd with TAP/TSO will result in TCP segments > with 0 (incorrect) TCP checksums. >=20 > In addition - please note the comments in lib/librte_mbuf/rte_mbuf.h whic= h specify that PKT_TX_TCP_SEG flag implies the > PKT_TX_TCP_CKSUM (hence it is not required to be explicitly set by the ca= ller) >=20 > /** > * TCP segmentation offload. To enable this offload feature for a > * packet to be transmitted on hardware supporting TSO: > * - set the PKT_TX_TCP_SEG flag in mbuf->ol_flags (this flag implies > * PKT_TX_TCP_CKSUM) > ... >=20 > > Add Konstantin for more suggestions. > > > > Thanks, > > Jiayu > > > > > -----Original Message----- > > > From: Ophir Munk [mailto:ophirmu@mellanox.com] > > > Sent: Sunday, April 22, 2018 10:21 PM > > > To: dev@dpdk.org; Hu, Jiayu > > > Cc: Thomas Monjalon ; Olga Shern > > > ; Pascal Mazon ; > > Ophir > > > Munk ; stable@dpdk.org > > > Subject: [PATCH v1] gso: fix marking TCP checksum flag in TCP segment= s > > > > > > Large TCP packets which are marked with PKT_TX_TCP_SEG flag are > > > segmented and the flag is cleared in the resulting segments, however, > > > the segments checksum is not updated. It is therefore required to set > > > the PKT_TX_TCP_CKSUM flag in each TCP segment in order to mark for th= e > > > sending driver the need to update the TCP checksum before transmittin= g > > > the segment. > > > > > > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO") > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Ophir Munk > > > --- > > > lib/librte_gso/rte_gso.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c inde= x > > > a44e3d4..e9ce9ce 100644 > > > --- a/lib/librte_gso/rte_gso.c > > > +++ b/lib/librte_gso/rte_gso.c > > > @@ -50,12 +50,14 @@ rte_gso_segment(struct rte_mbuf *pkt, > > > ((IS_IPV4_GRE_TCP4(pkt->ol_flags) && > > > (gso_ctx->gso_types & > > > DEV_TX_OFFLOAD_GRE_TNL_TSO)))) { > > > pkt->ol_flags &=3D (~PKT_TX_TCP_SEG); > > > + pkt->ol_flags |=3D PKT_TX_TCP_CKSUM; > > > ret =3D gso_tunnel_tcp4_segment(pkt, gso_size, ipid_delta, > > > direct_pool, indirect_pool, > > > pkts_out, nb_pkts_out); > > > } else if (IS_IPV4_TCP(pkt->ol_flags) && > > > (gso_ctx->gso_types & > > > DEV_TX_OFFLOAD_TCP_TSO)) { > > > pkt->ol_flags &=3D (~PKT_TX_TCP_SEG); > > > + pkt->ol_flags |=3D PKT_TX_TCP_CKSUM; > > > ret =3D gso_tcp4_segment(pkt, gso_size, ipid_delta, > > > direct_pool, indirect_pool, > > > pkts_out, nb_pkts_out); > > > -- > > > 2.7.4