From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Sat, 11 Jan 2025 18:07:17 +0100 (CET)
Received: by mail-pl1-f175.google.com with SMTP id
 d9443c01a7336-2166360285dso51877475ad.1
 for <dev@dpdk.org>; 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 <stephen@networkplumber.org>
To: Bruce Richardson <bruce.richardson@intel.com>
Cc: <dev@dpdk.org>, David Christensen <drc@linux.ibm.com>, Ian Stokes
 <ian.stokes@intel.com>, Konstantin Ananyev
 <konstantin.v.ananyev@yandex.ru>, Wathsala Vithanage
 <wathsala.vithanage@arm.com>, Vladimir Medvedkin
 <vladimir.medvedkin@intel.com>, Anatoly Burakov <anatoly.burakov@intel.com>
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: <Z3vnwWCslb5_e5V8@bricha3-mobl1.ger.corp.intel.com>
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>
 <Z3vnwWCslb5_e5V8@bricha3-mobl1.ger.corp.intel.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org

On Mon, 6 Jan 2025 14:25:05 +0000
Bruce Richardson <bruce.richardson@intel.com> 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 <bruce.richardson@intel.com> 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).