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 4586146072; Mon, 13 Jan 2025 17:31:03 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3732B40A80; Mon, 13 Jan 2025 17:31:03 +0100 (CET) Received: from mail-pj1-f54.google.com (mail-pj1-f54.google.com [209.85.216.54]) by mails.dpdk.org (Postfix) with ESMTP id 1D59F402A7 for ; Mon, 13 Jan 2025 17:31:02 +0100 (CET) Received: by mail-pj1-f54.google.com with SMTP id 98e67ed59e1d1-2ee76befe58so7570023a91.2 for ; Mon, 13 Jan 2025 08:31:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1736785861; x=1737390661; 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=629EQavPalLW600GaIIiBPOxhc2dAuJiReShsLmeaNI=; b=lG6VfmhwRdxnr8yptL9nSz1VVmAtkyvxKhCUat+1mgx/GDMw9NflAXKBvhgbPELkvQ iKa+8AbShdXxNTwsbvqRPVDNbi8BX6qvYvWCfgB0A7ErsALJt1YqTYHyC/80zLmp94Mo ynibdEWtcNGkPZd4fsueqdSaggpDggx0X8oVIcKoKDCxAZcsypoguoACaVdeNim0+QTN 3fJo8QKqHKlGl4NQpp5f6lsbjKkfWkeFdDyhlfqzQMp8xeYTjr0lM5Cw9DIIJjrnsS4O H/3312VYIdv4W/bEU7L38PxbC9oOEYDLAHeTLVFr0E7y8HzeC4t69LUsAhDy0cz9PB7x +IcQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736785861; x=1737390661; 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=629EQavPalLW600GaIIiBPOxhc2dAuJiReShsLmeaNI=; b=ViOV2ax8ET98IJp8Ej8LCvnZQetX0p5Do6vigDIedKKz4FfZUW7DxBBo2cBfC9HPS5 tS0t3QlPjvhL3ZtLa+0TIK9YEq0p7x1CC5eIb5RKKcFhtVWXg2a4uZDKsM91+2Cj42RA TnTMQfquU2jfM2ibnwCIDpOiSjwsoGvfxnZ3N5SKn5HAf5/uqTdbRSl6reH+whfAuBiO LVPhY7FwwYiAYB65LMoDkZ6l4CVynOwMGQMTI9gC/2KgHhqZJU/OWyPKImQIFV/KALNH +9+h3qlgAcGXLMzW6feON+e5uTlsXssnps2d77g+6E5SGmz2vm1ENmo7yYylVTuTUi/Z EP3g== X-Gm-Message-State: AOJu0YzZRLFcdjmONcWZ9E1/axd6bmI46qW2ezb34E+m/dgwZdGz8AWP DmYoZN+ggB/CNUNfSaXEJa94+xrTEMbv+D2EgU2MG0xBUsX/zaF0mXx8qNzm6NY= X-Gm-Gg: ASbGncvPfvVNn/RC/WZA8dvm5Incynzn713XeqC+UHHdQSvplvXPeFnnBeT7I7CDmbb g2BV6JV2XmmCImVJ/Xbk++2/P9+QJkSzqDkhJBlI1Cp9gRGiOVRtUdkPpATuNGBsTSZek56ANnC ncWa5V1GGQEFNyRUy4ukCimPzwF8E7Mejx3B7Zid8SXDMPqNRh2YBp7sZJAZLIzuLDLK8pcM9WC SR9ODRLPWX1gZlqV0wHi0pJPVFSp9eWdnujhXZDS6J/GgHpUPqNF4fWuEUg74liyzIvezV/53qA GDu6xN9XmOC44ojYFNjwGpGt2aPya1autQ== X-Google-Smtp-Source: AGHT+IGeU3okJW64dfgEoi3cv4uTm/MYR3ClTTwZ76jCkw0benq3OzF3ZO0XfRFyW+M8mt3DED6Lcw== X-Received: by 2002:a17:90b:2e41:b0:2ee:a4f2:b307 with SMTP id 98e67ed59e1d1-2f548e9ca14mr30501839a91.4.1736785859631; Mon, 13 Jan 2025 08:30:59 -0800 (PST) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2f54a26ae4csm11309175a91.11.2025.01.13.08.30.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Jan 2025 08:30:59 -0800 (PST) Date: Mon, 13 Jan 2025 08:30:57 -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: <20250113083057.6b19e4c8@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> <20250111090714.70ef40a1@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, 13 Jan 2025 10:04:50 +0000 Bruce Richardson wrote: > On Sat, Jan 11, 2025 at 09:07:14AM -0800, Stephen Hemminger wrote: > > 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). > > > > If CRC is not to be stripped, then crc_len should be zero here also, in > which case nothing happens for freeing data. [If crc_len is not zero in > that case, that's a bug in the config path, not datapath here] > > /Bruce Ok, that makes sense.