From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 88EECA0A0C; Tue, 3 Aug 2021 14:49:50 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4FC32411A7; Tue, 3 Aug 2021 14:49:50 +0200 (CEST) Received: from mail-wm1-f47.google.com (mail-wm1-f47.google.com [209.85.128.47]) by mails.dpdk.org (Postfix) with ESMTP id BCF2840E3C; Tue, 3 Aug 2021 14:49:48 +0200 (CEST) Received: by mail-wm1-f47.google.com with SMTP id o7-20020a05600c5107b0290257f956e02dso1664800wms.1; Tue, 03 Aug 2021 05:49:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Iw89mnu32M5Dyv30On1CtI4v53B0wQJEn0yjnmTVd8E=; b=puqYYTCVZarImyonhog/hICMpTWF+esKGkNCMQs/6DFnrcu9PiRElZXaE2tnkgkbXk b5FMYxar/3zp6TMdHeqmCCaGJ0uh7MDhG7jvUh5pKMlhAn/zNPWbsdrnhOeR6bljAPBK qEpqph1LSgVnWNkMXZkupzI5xM5ZuYoHFp8be/QiKp8kYC+Krx9chKdO6IsJnu6Y9z5d 4THKtS2OLrqn+5ocO7gaS/XL0Mj0u8+FYhNJ/O3WMu/0dpiXJa9IgPGRzXLtXFHRt4X4 gqmVblOuy3D2yVR1MZP/0PTGvQ6xRh6qj1TaIL6Vme7oksbD6BiSkIoZEDL5v5gnrwI5 dIzw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Iw89mnu32M5Dyv30On1CtI4v53B0wQJEn0yjnmTVd8E=; b=mxIVvh8MkCTGgIRPkp7qQgGO+wJyW4LkvS6LRzJmBIlEpdxkpenlpSg72L0iNQ3T6u DV42FKzxXB0vTEmP6SpwgEoiLPmDNS90owIVXy4BGMt6paxTtdW13w4KQZJnwwrVGyOL e093Ewm49kEP2xjEqECk2NOODghOWBPpoGihgNOh7+3C5bWackBjSkItqqoRIQ7qiXaR m5dgZeUXGoXMeTuwrwKXYJ98hdfDg0Lx5/6pcmJnZCXh/xEzg6bWKbZYD5i/k2r9SCw1 TYMXt9qNkz0ZDL5lGNep5xpZY6yIJjGosiyDRI0dGgk20O2Sl7nojvctouo+c1kPUgAZ nong== X-Gm-Message-State: AOAM532zhZ+3UAhrcGiKrQZdgRsOSTg9MubZQ3oLDBFiLaya6AgGzfXO GBXYaHWktKdo53biWu+tHOAZGfhMMJaWVvArsOs= X-Google-Smtp-Source: ABdhPJz353VTkAQDuHfXh3VXCFZMT/gaq3qOB7oJZ6DqQ3S1duy2/DJhOrLtpue0fvWvKbfcPF/kIYOkoLZqsgm7Eto= X-Received: by 2002:a7b:cd87:: with SMTP id y7mr4220023wmj.59.1627994988504; Tue, 03 Aug 2021 05:49:48 -0700 (PDT) MIME-Version: 1.0 References: <20210630110404.21209-1-mohsin.kazmi14@gmail.com> <20210707094019.40945-1-mohsin.kazmi14@gmail.com> <0efac6a6-f5d1-48f1-3a95-a8b87469cc23@oktetlabs.ru> <6d7c41bf-6e9f-8fbd-bfc8-11ca07c777d8@oktetlabs.ru> In-Reply-To: <6d7c41bf-6e9f-8fbd-bfc8-11ca07c777d8@oktetlabs.ru> From: Mohsin Kazmi Date: Tue, 3 Aug 2021 13:49:37 +0100 Message-ID: To: Andrew Rybchenko Cc: Olivier Matz , dev@dpdk.org, stable@dpdk.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [dpdk-dev] [PATCH v3] net: fix Intel-specific Prepare the outer ipv4 hdr for checksum X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Sat, Jul 31, 2021 at 1:49 PM Andrew Rybchenko < andrew.rybchenko@oktetlabs.ru> wrote: > On 7/30/21 2:11 PM, Olivier Matz wrote: > > On Wed, Jul 28, 2021 at 06:46:53PM +0300, Andrew Rybchenko wrote: > >> On 7/7/21 12:40 PM, Mohsin Kazmi wrote: > >>> Preparation the headers for the hardware offload > >>> misses the outer ipv4 checksum offload. > >>> It results in bad checksum computed by hardware NIC. > >>> > >>> This patch fixes the issue by setting the outer ipv4 > >>> checksum field to 0. > >>> > >>> Fixes: 4fb7e803eb1a ("ethdev: add Tx preparation") > >>> Cc: stable@dpdk.org > >>> > >>> Signed-off-by: Mohsin Kazmi > >>> Acked-by: Qi Zhang > >>> --- > >>> v3: > >>> * Update the conditional test with PKT_TX_OUTER_IP_CKSUM. > >>> * Update the commit title with "Intel-specific". > >>> > >>> v2: > >>> * Update the commit message with Fixes. > >>> > >>> lib/net/rte_net.h | 15 +++++++++++++-- > >>> 1 file changed, 13 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/lib/net/rte_net.h b/lib/net/rte_net.h > >>> index 434435ffa2..3f4c8c58b9 100644 > >>> --- a/lib/net/rte_net.h > >>> +++ b/lib/net/rte_net.h > >>> @@ -125,11 +125,22 @@ rte_net_intel_cksum_flags_prepare(struct > rte_mbuf *m, uint64_t ol_flags) > >>> * Mainly it is required to avoid fragmented headers check if > >>> * no offloads are requested. > >>> */ > >>> - if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | > PKT_TX_TCP_SEG))) > >>> + if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK | > PKT_TX_TCP_SEG | > >>> + PKT_TX_OUTER_IP_CKSUM))) > >>> return 0; > >>> - if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) > >>> + if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6)) { > >>> inner_l3_offset += m->outer_l2_len + m->outer_l3_len; > >>> + /* > >>> + * prepare outer ipv4 header checksum by setting it to 0, > >>> + * in order to be computed by hardware NICs. > >>> + */ > >>> + if (ol_flags & PKT_TX_OUTER_IP_CKSUM) { > >>> + ipv4_hdr = rte_pktmbuf_mtod_offset(m, > >>> + struct rte_ipv4_hdr *, > m->outer_l2_len); > >>> + ipv4_hdr->hdr_checksum = 0; > >> > >> Here we assume that the field is located in the first segment. > >> Unlikely but it still could be false. We must handle it properly. > > > > This is specified in the API comment, so I think it has to be checked > > by the caller. > > If no, what's the point to spoil memory here if stricter check is > done few lines below. > We have two possibilities: 1) take the whole block of above code after the strict check: Then strict check will use m->outer_l2_len + m->outer_l3_len directly without any condition and we will be on the mercy of drivers to initialize these to 0 if outer headers are not use. Drivers usually don't set the fields which they are not interested in because of performance reasons as setting these values per packet will cost them additional cycles. 2) Taking just PKT_TX_OUTER_IP_CKSUM conditional check after the strict fragmented check: In that case, each packet will hit an extra conditional check without getting benefit from it, again with a performance penalty. I am more inclined towards solution 1. But I also welcome other suggestions/comments. > > >>> + } > >>> + } > >>> /* > >>> * Check if headers are fragmented. > >>> > >> > >