From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 86799A04B1; Thu, 5 Nov 2020 10:09:28 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AFEA972EE; Thu, 5 Nov 2020 10:09:26 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by dpdk.org (Postfix) with ESMTP id C54C26CAC; Thu, 5 Nov 2020 10:09:24 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 746E97F609; Thu, 5 Nov 2020 12:09:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 746E97F609 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1604567363; bh=hQF+dAd5x7ehaJwhIUNoc/K2W2IgvaIAUnaZRickqMM=; h=From:Subject:To:References:Cc:Date:In-Reply-To; b=GAn8BRLoWWa6Y5+DuExQFEjJtCu4W9i7DToHVqiQwLzyXEtkMh1VehyDwn4regnDK 1bUHZSWN16uNYiMUwidqWqGTUeW0W2XmxxpP+8y8ohjRWSklVejdAhrs71KN1q4YfJ Fj7NRdwHrvKTYDOGnKWJn1WmHqfjjNu1jywmxCNI= From: Andrew Rybchenko To: dev@dpdk.org References: <20201104170007.8026-1-olivier.matz@6wind.com> <20201105074626.GL1898@platinum> Cc: =?UTF-8?Q?Morten_Br=c3=b8rup?= , "Ananyev, Konstantin" , dpdk stable , Olivier Matz Organization: OKTET Labs Message-ID: <1f1b16c6-09d3-f020-ee7b-10ac6536554e@oktetlabs.ru> Date: Thu, 5 Nov 2020 12:09:23 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: <20201105074626.GL1898@platinum> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" Just resend with lost Cc restored. On 11/5/20 10:46 AM, Olivier Matz wrote: > On Thu, Nov 05, 2020 at 12:15:49AM +0000, Ananyev, Konstantin wrote: >> >> Hi Olivier, >> >>> m->nb_seg must be reset on mbuf free whatever the value of m->next, >>> because it can happen that m->nb_seg is != 1. For instance in this >>> case: >>> >>> m1 = rte_pktmbuf_alloc(mp); >>> rte_pktmbuf_append(m1, 500); >>> m2 = rte_pktmbuf_alloc(mp); >>> rte_pktmbuf_append(m2, 500); >>> rte_pktmbuf_chain(m1, m2); >>> m0 = rte_pktmbuf_alloc(mp); >>> rte_pktmbuf_append(m0, 500); >>> rte_pktmbuf_chain(m0, m1); >>> >>> As rte_pktmbuf_chain() does not reset nb_seg in the initial m1 >>> segment (this is not required), after this code the mbuf chain >>> have 3 segments: >>> - m0: next=m1, nb_seg=3 >>> - m1: next=m2, nb_seg=2 >>> - m2: next=NULL, nb_seg=1 >>> >>> Freeing this mbuf chain will not restore nb_seg=1 in the second >>> segment. >> >> Hmm, not sure why is that? >> You are talking about freeing m1, right? >> rte_pktmbuf_prefree_seg(struct rte_mbuf *m) >> { >> ... >> if (m->next != NULL) { >> m->next = NULL; >> m->nb_segs = 1; >> } >> >> m1->next != NULL, so it will enter the if() block, >> and will reset both next and nb_segs. >> What I am missing here? >> Thinking in more generic way, that change: >> - if (m->next != NULL) { >> - m->next = NULL; >> - m->nb_segs = 1; >> - } >> + m->next = NULL; >> + m->nb_segs = 1; > > Ah, sorry. I oversimplified the example and now it does not > show the issue... > > The full example also adds a split() to break the mbuf chain > between m1 and m2. The kind of thing that would be done for > software TCP segmentation. > If so, may be the right solution is to care about nb_segs when next is set to NULL on split? Any place when next is set to NULL. Just to keep the optimization in a more generic place. > After this operation, we have 2 mbuf chain: > - m0 with 2 segments, the last one has next=NULL but nb_seg=2 > - new_m with 1 segment > > Freeing m0 will not restore nb_seg=1 in the second segment. > >> Assumes that it is ok to have an mbuf with >> nb_seg > 1 and next == NULL. >> Which seems wrong to me. > > I don't think it is wrong: nb_seg is just ignored when not in the first > segment, and there is nothing saying it should be set to 1. Typically, > rte_pktmbuf_chain() does not change it, and I guess it's the same for > many similar functions in applications. > > Olivier > >> >> >>> This is expected that mbufs stored in pool have their >>> nb_seg field set to 1. >>> >>> Fixes: 8f094a9ac5d7 ("mbuf: set mbuf fields while in pool") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Olivier Matz >>> --- >>> lib/librte_mbuf/rte_mbuf.c | 6 ++---- >>> lib/librte_mbuf/rte_mbuf.h | 12 ++++-------- >>> 2 files changed, 6 insertions(+), 12 deletions(-) >>> >>> diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c >>> index 8a456e5e64..e632071c23 100644 >>> --- a/lib/librte_mbuf/rte_mbuf.c >>> +++ b/lib/librte_mbuf/rte_mbuf.c >>> @@ -129,10 +129,8 @@ rte_pktmbuf_free_pinned_extmem(void *addr, void *opaque) >>> >>> rte_mbuf_ext_refcnt_set(m->shinfo, 1); >>> m->ol_flags = EXT_ATTACHED_MBUF; >>> - if (m->next != NULL) { >>> - m->next = NULL; >>> - m->nb_segs = 1; >>> - } >>> + m->next = NULL; >>> + m->nb_segs = 1; >>> rte_mbuf_raw_free(m); >>> } >>> >>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h >>> index a1414ed7cd..ef5800c8ef 100644 >>> --- a/lib/librte_mbuf/rte_mbuf.h >>> +++ b/lib/librte_mbuf/rte_mbuf.h >>> @@ -1329,10 +1329,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m) >>> return NULL; >>> } >>> >>> - if (m->next != NULL) { >>> - m->next = NULL; >>> - m->nb_segs = 1; >>> - } >>> + m->next = NULL; >>> + m->nb_segs = 1; >>> >>> return m; >>> >>> @@ -1346,10 +1344,8 @@ rte_pktmbuf_prefree_seg(struct rte_mbuf *m) >>> return NULL; >>> } >>> >>> - if (m->next != NULL) { >>> - m->next = NULL; >>> - m->nb_segs = 1; >>> - } >>> + m->next = NULL; >>> + m->nb_segs = 1; >>> rte_mbuf_refcnt_set(m, 1); >>> >>> return m; >>> -- >>> 2.25.1 >>