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 E4594A04B1 for ; Thu, 5 Nov 2020 10:03:13 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C83DC6CAC; Thu, 5 Nov 2020 10:03:12 +0100 (CET) Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by dpdk.org (Postfix) with ESMTP id 88F7E6CAC for ; Thu, 5 Nov 2020 10:03:11 +0100 (CET) Received: by mail-wr1-f67.google.com with SMTP id g12so788227wrp.10 for ; Thu, 05 Nov 2020 01:03:11 -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:content-transfer-encoding:in-reply-to :user-agent; bh=rrLKszWT2svxHSwHAr02sH60wePDHu44QaoE8EYSsdQ=; b=Jk6JLGDis4+ghwkjxibcGgfZ8bxH1k+3+3KKDCBRwLV2mKFIwntIeya2T3eP7dj/wa 9+2XjPrRLZlBgt0wD/IAWJvQB6ztmIvaBSmcFKDZiGFLmPn5XAWpPEJktvqTUzHaITu8 eL66cb2YNU0nvmUxYi1FqGCqLnMd88ckpXBzs9qHY6ePZyy9eKANu5+hxQNUQqiEIyhp sa+kEZ+QY+6qPdoURzhgqS0qzRTUkx3MLRvV1XFZWLXenTP6H9MocHzql2E2bs0LrJ1B h/1d6su5GtJtOnOB0mBVMisGiltHG73ftOTwNTq3IhakPJ5HP8vXYf4WcdEEjdU8Vj1V F9sg== 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:content-transfer-encoding :in-reply-to:user-agent; bh=rrLKszWT2svxHSwHAr02sH60wePDHu44QaoE8EYSsdQ=; b=NM+sakZ355o4NWCG9DhLCgCqfa11WgBtZbWaQoAH2vtTu8q8BMYMeQTV/Jhjkjo0Q6 k1MYqWiFCoazmnI+6IAsLVMh0GyT15iWytbU3EBLOLKzVYTYJbhCoJpRQy766yZq7Mpm oRyLaXBONf+RBL+34oFS9IsnnZJoShN3CceuJiLfaM8lu3YtBVOSH2a62+tZvVdD418/ JuMhYoIXoFqzuprmsZYkJG2M/2LRqLjjPvtHnNxk3FLzUmIhak03qRPqdv2qdfdnY5aK nO4Fo+wm3IJSUiDDAzJqLuXhpuHQE2RcChi+eYU/bulIEy/K5Nn8l3cxVDuG8kQhzYYq CZ7g== X-Gm-Message-State: AOAM532hu1uTFBF9eCjHNPy6PlYX+zbUcczXQg7UkA/jNCyR5XFq11Sh nqXBIwQbMwz0gzoS512Jy5SyFw== X-Google-Smtp-Source: ABdhPJy+xMY/uz70P3Sl1PWKuHuy0MuMfv4ZcDJveWh/slnOsPIbMH6vXut+8CGDkW4oBgznYTvZAQ== X-Received: by 2002:adf:bb07:: with SMTP id r7mr1753680wrg.150.1604566990242; Thu, 05 Nov 2020 01:03:10 -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 z11sm1540819wrr.66.2020.11.05.01.03.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 05 Nov 2020 01:03:09 -0800 (PST) Date: Thu, 5 Nov 2020 10:03:08 +0100 From: Olivier Matz To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: "Ananyev, Konstantin" , dev@dpdk.org, stable@dpdk.org Message-ID: <20201105090308.GN1898@platinum> References: <20201104170007.8026-1-olivier.matz@6wind.com> <20201105074626.GL1898@platinum> <98CBD80474FA8B44BF855DF32C47DC35C613EB@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35C613EB@smartserver.smartshare.dk> User-Agent: Mutt/1.10.1 (2018-07-13) Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] mbuf: fix reset on mbuf free X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" On Thu, Nov 05, 2020 at 09:33:58AM +0100, Morten Brørup wrote: > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz > > Sent: Thursday, November 5, 2020 8:46 AM > > > > 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 > > Acked-by: Morten Brørup > > And while you are at it, please consider extending the description of the two mbuf fields with their invariants: > 1. nb_segs is only valid for the first segment of a multi-segment packet. > 2. next is NULL for non-segmented packets. Good point, will add it in v2. > > > > > > > > > > > > >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 > > > >