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 D5566A04B1; Thu, 5 Nov 2020 08:46:33 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1DD065B3A; Thu, 5 Nov 2020 08:46:32 +0100 (CET) Received: from mail-wm1-f67.google.com (mail-wm1-f67.google.com [209.85.128.67]) by dpdk.org (Postfix) with ESMTP id 67AF25A51 for ; Thu, 5 Nov 2020 08:46:29 +0100 (CET) Received: by mail-wm1-f67.google.com with SMTP id p22so563217wmg.3 for ; Wed, 04 Nov 2020 23:46:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=t/GLLTSUmtCZbqSJjutpZDQBT0AhsXwcKb+g0NrLbzs=; b=b6YebkjQtecmVtYa0BZ6S5PhY0cNM6QG69VF3q/sB9egVrh7JQbb+gYohVGLiEN2Oa KtBfdcxKvYf4Fpr0WKMWFWcHEjAG5Q97PeQvnoG7EEuCiluYQx3vLkohXdD/NNI2ukpM RZPvKs1MWSIIVh64izasBRgnXUczgLiiKqT8iTfJbpjtrAa/485vCa1Ohvm4ImFlgIin Y203m345LlKRfY6Nxd7bF7Gholk64iEURn/AVqlU1h8osrFx3jXIX6papxvuEiOj7w4/ Z+EtGBhR46OJos9j1Z1nfEYPrQQOglh8oy5m2lffb3Cpx1uWay7nKk0dBrikOHeKgk9c ac6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=t/GLLTSUmtCZbqSJjutpZDQBT0AhsXwcKb+g0NrLbzs=; b=XaVeeF1j3mFP0LwM3lcuJMc2vkjIwWshGf2bBV2qXQkshtilRRvqh9oxn9MYPtxQs5 cSycYsFKIBausZcVgXWurky/KLcInc2t++KIBXEIeIvp6U4ryrsJWZ3uWrbOwDxP5SNc vFDTfY7G2RetjCIkk6SxYq+73X9NuMKLqjaLeTlHI6TS/6SFb2OR2hnWtZ3LIjNLVrqR 3awG1p7gJQ3CV+7jdlqzetgkpU4xyFV1VwCiMo5o5QLqj0GaqTfKZtqFVxRxmIg4+0Xz RCfvUHBbta94Uyra9Rb6MAzBz1rtlAC3DUxmtF8I2oUGGMJWAbFuX5MgV5k3tLiimwq3 ivUQ== X-Gm-Message-State: AOAM532Wa936KoWxYGlNykX1ciGGp51o3AbmY4bJWlARK/6etovU1Fng PxF8ufTYACiooL/rLy2zRjXW7g== X-Google-Smtp-Source: ABdhPJwpb9346Nr8YBXhgGR1ZH07BQ4ekpBTkn76kwvMtPgdy2I4y86Mxmg8sg2N7DzMo7Cq6tZfwQ== X-Received: by 2002:a1c:ed15:: with SMTP id l21mr1309042wmh.26.1604562388128; Wed, 04 Nov 2020 23:46:28 -0800 (PST) Received: from 6wind.com (2a01cb0c0005a600345636f7e65ed1a0.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:3456:36f7:e65e:d1a0]) by smtp.gmail.com with ESMTPSA id k22sm1280466wmi.34.2020.11.04.23.46.27 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Nov 2020 23:46:27 -0800 (PST) Date: Thu, 5 Nov 2020 08:46:26 +0100 From: Olivier Matz To: "Ananyev, Konstantin" Cc: "dev@dpdk.org" , "stable@dpdk.org" Message-ID: <20201105074626.GL1898@platinum> References: <20201104170007.8026-1-olivier.matz@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) 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" 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. 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 >