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 D2B5246041; Sat, 11 Jan 2025 18:07:19 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 59DEE40265; Sat, 11 Jan 2025 18:07:19 +0100 (CET) Received: from mail-pl1-f175.google.com (mail-pl1-f175.google.com [209.85.214.175]) by mails.dpdk.org (Postfix) with ESMTP id 104144025D for ; Sat, 11 Jan 2025 18:07:17 +0100 (CET) Received: by mail-pl1-f175.google.com with SMTP id d9443c01a7336-2166360285dso51877475ad.1 for ; Sat, 11 Jan 2025 09:07:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1736615237; x=1737220037; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=EGd22yNVVF2lW1IkA1pcaiMllbAtNh0FAewlYWHwgNs=; b=oRycOwX9e3dFn8NApJJwEBE4UCImY7lyGxkhEaiisNcvdaKPNer6k/POAXmIQ82DDj yRtvTlK4O9EPujF5tPD70+0nKjJ+6+H2mj65hbYEN+VPcM93B2+h3+dIFRH9rceLjJ9F Em/wZB56/BshHyBVtkyYKxgmYV77/tmqyRS4msS2qV5ZBplZAwDdzuwlEAI1j+LBW/DA WuZ9cAt1zv2NsjWavbu9GCKjURm510DaIyKMKe+NJ5MxyEJ/Ilr4+r342lRkVCczUf8P GuQANihUlskKz4HGt2rehjr8RRCjSOnFPedIC1ILq0C7uylWXfBo/ZBZKTaTVkL9LHtS UaKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736615237; x=1737220037; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=EGd22yNVVF2lW1IkA1pcaiMllbAtNh0FAewlYWHwgNs=; b=iuErSYhNldEa1qOWLt3qECoboa0QTwp2lvX1r9LxelJAlyp30ONF816tBX2V9d8nPn 6Y4Cg8uBnz3kbxLp1f+Sx8JY1esokES5a9jvdwWzi8HF2/RJ8tCHPcXN+mW7KKlrNZ1W DJpAUW6YeR5tbqEHplaGP/R+KVvMjp9z1liUtfecDYyEgGhwEMNjXAeYHZoBP77Ia1ay rNtyxRGJ4hP2giF7TXxkk5I1pgFffd69o/nuMoTc5JocyGufZNP9IZ7X5aeuNU7UmKdq Uq/XY2elBeKI+S3WVWDIZNgaYTKQnJzFfjENAtAy/Bptd1KMA3wHewaqyzg2XafL6tuB Q+ng== X-Gm-Message-State: AOJu0YyCYSBxaVlfgV7B0ohhi0uj2sHe7mmQPa3dLEkUJ2IdPrEBG+gM LgviPnHZAh+S0gs4ufo8wY47h/VkGLFqWIuPG5aC+DuSriXoyx1FY1neYwxxQEk= X-Gm-Gg: ASbGncs9oHrnWZDbwNm3nWpgdcTl51tGgbPAVDjuD0v+hhoY1+x98oujeu8BhLNcKlN voeF9L6q/FPEAqv43bw0xbPDyYwTLYYiPlSn3j+wOFW6ij44kgTcCw6Lqoxwea8L1X8NiRbLmxY Kj8J6Q86Nwzmt+6POg8vY0mHhBXmTieS4NSqEzg4C62itnMKwn6upltovntQoeBR8tbL2bkXDJ0 JiYzz1S+aG93EGa4+lFHM1C1tvx7YREC3VNmwfs79fLKKZ71JaQu29sXbKT8GQ6qPhr1xutPFpR 9Smxbtv0V46QhZ/u0t28mY+r3RP2UrXgVg== X-Google-Smtp-Source: AGHT+IH1PfKDePBF9vrunCGpzIfz3gandWO+UZD8brDumdbi34SzjwBm3wGo29vmlA6Qye3dc4RkKw== X-Received: by 2002:a05:6a20:4394:b0:1e1:b8bf:8e80 with SMTP id adf61e73a8af0-1e88d2fc8c4mr25123558637.41.1736615236962; Sat, 11 Jan 2025 09:07:16 -0800 (PST) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 41be03b00d2f7-a3196da0bf3sm4525671a12.42.2025.01.11.09.07.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 11 Jan 2025 09:07:16 -0800 (PST) Date: Sat, 11 Jan 2025 09:07:14 -0800 From: Stephen Hemminger To: Bruce Richardson Cc: , David Christensen , Ian Stokes , Konstantin Ananyev , Wathsala Vithanage , Vladimir Medvedkin , Anatoly Burakov Subject: Re: [PATCH v4 01/24] net/_common_intel: add pkt reassembly fn for intel drivers Message-ID: <20250111090714.70ef40a1@hermes.local> In-Reply-To: References: <20241122125418.2857301-1-bruce.richardson@intel.com> <20241220143925.609044-1-bruce.richardson@intel.com> <20241220143925.609044-2-bruce.richardson@intel.com> <20241220081540.1a6a3da5@hermes.local> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 On Mon, 6 Jan 2025 14:25:05 +0000 Bruce Richardson wrote: > On Fri, Dec 20, 2024 at 08:15:40AM -0800, Stephen Hemminger wrote: > > On Fri, 20 Dec 2024 14:38:58 +0000 > > Bruce Richardson wrote: > > > > > + > > > + if (!split_flags[buf_idx]) { > > > + /* it's the last packet of the set */ > > > + start->hash = end->hash; > > > + start->vlan_tci = end->vlan_tci; > > > + start->ol_flags = end->ol_flags; > > > + /* we need to strip crc for the whole packet */ > > > + start->pkt_len -= crc_len; > > > + if (end->data_len > crc_len) { > > > + end->data_len -= crc_len; > > > + } else { > > > + /* free up last mbuf */ > > > + struct rte_mbuf *secondlast = start; > > > + > > > + start->nb_segs--; > > > + while (secondlast->next != end) > > > + secondlast = secondlast->next; > > > + secondlast->data_len -= (crc_len - end->data_len); > > > + secondlast->next = NULL; > > > + rte_pktmbuf_free_seg(end); > > > + } > > > > The problem with freeing the last buffer is that the CRC will be garbage. > > What if the CRC is sitting past the last mbuf? > > > > +-----------------------+ +-----+ > > | Data +--->+ CRC | > > +-----------------------+ +-----+ > > > > This part (from original code) will free the second mbuf which contains > > the CRC. The whole "don't strip CRC and leave it past the mbuf data" model > > of mbuf's is a danger trap. > > Can you explain more clearly what you see as the issue with the current > code above? > > The "crc_len" variable contains the length of the CRC included in the > packet, which should be removed from that before returning the mbuf from RX. > It contains "0" if the CRC is HW stripped, and "4" if the CRC needs to be > removed by software - something that is done just by subtracting the CRC > length from the packet and buffer lengths. The freeing of the last segment > occurs only in the case that the last segment contains the CRC only - or > part of the CRC only, as otherwise we would have an extra empty buffer at > the end of the chain. But if the application is using the driver in "keep CRC" mode, then it probably intends to look at the CRC. In that case crc_len is 4, and that trailing buffer may need to be retained. It all goes back to the design mistake of the semantics of "keep CRC" mode. In that mode, the mbuf chain has hidden data (past pkt_len and data_len).