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 1A3F7A0C43; Fri, 27 Aug 2021 15:44:20 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id ADC0B4067C; Fri, 27 Aug 2021 15:44:19 +0200 (CEST) Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by mails.dpdk.org (Postfix) with ESMTP id 8407A40150; Fri, 27 Aug 2021 15:44:17 +0200 (CEST) Received: by mail-wm1-f50.google.com with SMTP id f9-20020a05600c1549b029025b0f5d8c6cso9264228wmg.4; Fri, 27 Aug 2021 06:44:17 -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=Ok9vcLtS68ytqVIvs9GBONWtC2NZVjKCoX0lWb0i2BQ=; b=QRpsr1fiPN29CuzuNhGohaw27vbpfbuBkwa2wH3Lp4laRtjpgLjoXOWKz8uS4BN6im 7NDZVtVAXpWyFePQeNQkizDI89ZA+wXr6VC+ALUxPymVVVOg8JyjIIAfl2hB8DFZfYy7 RB4pixpq3FJoGm1z5wiw0luXC35ijHKqKWZL4sPolhD6UfLgddT8mMJIuKqW/e/9MV3p Vl/FyOiCZ10uJIrxItFDdIBeenwiQFXOmJRZY4BkAzzfMwGwGtlZBfjHVnEpp8t91yr5 eNPBfpVHtxuGIZ7TCVoFaHK4o5Gqxl/8d93OFNwvHkZ2lp8IJVSWUb9MA1POcYU85pXI +pVw== 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=Ok9vcLtS68ytqVIvs9GBONWtC2NZVjKCoX0lWb0i2BQ=; b=JZVTvd1kuEUj0BjnJQfeNlUu92NNb0UUZwSncQwH1m6/GoHzdyq8M0WxR+rIHP4wNH 13xVXXkLFvJmwZWuCcFk64T4AiWhX0nMfYG8fbWmgZXhfhnBZBahi/rTxYK2XgNsyskd n345U6vTQXJqUJV8YSbtNe1QVM6yF6Y4+qNcpv5J0B+esL949RFqW/EGtgP6vkyCzjEh f//YtAZHexwkUNesYtWH7mysAYy+KWbYMl6qU8MXC0OS3V07xpr2srjbdya2CG82epoB bji3M5e0/JMjjnIf7eLFlx/ZjzgGd+29kgVZ44mWK8vrRlos0xdePce1D3pTS0yrOaJN 99kw== X-Gm-Message-State: AOAM530bc57ZOcHdl1MPMxhPO3qDMfGNW9qCksZuF2da0/Wlud0XmCLP ic7C73ZUTqHmttfRo5y3Tp0fG4Ws8pnG3AB8Vkk= X-Google-Smtp-Source: ABdhPJy/kJQgE8D4I1lyfblqgLBZqfXAHWVUxejJcZDdaPzaIpEDS9SC+eznfLQEjp9tvoaGzKAh5/GrUGh8jQZDaPM= X-Received: by 2002:a05:600c:3514:: with SMTP id h20mr19898695wmq.31.1630071857226; Fri, 27 Aug 2021 06:44:17 -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: From: Mohsin Kazmi Date: Fri, 27 Aug 2021 14:44:06 +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" Are we good with this patch with the current state? @Olivier: Any comments on the above suggestions? On Tue, Aug 3, 2021 at 1:49 PM Mohsin Kazmi wrote: > > > 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. >> >>> >> >> >> >>