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 460D5A0526 for ; Tue, 10 Nov 2020 11:31:45 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 58F622AB; Tue, 10 Nov 2020 11:31:43 +0100 (CET) Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id 9B6692AB for ; Tue, 10 Nov 2020 11:31:41 +0100 (CET) Received: by mail-wr1-f66.google.com with SMTP id o15so4242975wru.6 for ; Tue, 10 Nov 2020 02:31:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:content-transfer-encoding:user-agent:mime-version; bh=iiqO/aAF4stVZJqDqXbx85I68fTPkXIqiYKWqcJ9Zbs=; b=R2MToeZvnE4od9Z186inZovY7EN7ZzA//z4AYPX+0sdFKQrB/+GpcSxhunt4ofP1kI bC9mstO85eR/MKNHgeJJFVad591QjPz6o2CTLuJl2Gk8mdqiL9SssQNOCWWEqwFjlCJS Q1oVUWZnIDRklC3ubMxIkrUwz4q16LqtIRhyp8PVAskzKn41ysKoY7YYRD9Dy87oqOr4 sfQeyEHFb2ilDAIlsWhhGR3J9fBSQa+Sgd06ZRrI9tFapsq0PJMeylY6fzpogf+76Qmm Z4dgpCQa6lxEgqSQakiPoDJwD8ATRhtwOVcmh6tFyCEfxE/KXJc4zACvBe6/4l2HkJ1x F+6g== X-Gm-Message-State: AOAM533mugpWTHxoUwShkJaHbzZ6VAzz3IcAn6ZGqTdYcgu5U046ca3a zNmdfgFD6MPzkTBCzgGMEOs= X-Google-Smtp-Source: ABdhPJwX5Bgp4qbAtGVfoIsDeKseEcsPG1wGfseuCp4p4TTUKw44Tc9Hz/e8dBIAA5WMSiOcN3PJMA== X-Received: by 2002:adf:edc5:: with SMTP id v5mr22640831wro.112.1605004300228; Tue, 10 Nov 2020 02:31:40 -0800 (PST) Received: from localhost ([88.98.246.218]) by smtp.gmail.com with ESMTPSA id r18sm17929629wrj.50.2020.11.10.02.31.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 10 Nov 2020 02:31:39 -0800 (PST) Message-ID: <62143ed3787d6b6f93159effe3ee010728558182.camel@debian.org> From: Luca Boccassi To: "Ananyev, Konstantin" , Yi Yang Cc: "Hu, Jiayu" , dpdk stable Date: Tue, 10 Nov 2020 10:31:38 +0000 In-Reply-To: References: <20201028104606.3504127-207-luca.boccassi@gmail.com> <20201109184111.3463090-1-luca.boccassi@gmail.com> <20201109184111.3463090-24-luca.boccassi@gmail.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.30.5-1.1 MIME-Version: 1.0 Subject: Re: [dpdk-stable] patch 'gso: fix mbuf freeing responsibility' has been queued to stable release 19.11.6 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 Mon, 2020-11-09 at 22:39 +0000, Ananyev, Konstantin wrote: > Hi Luca, >=20 >=20 > > Hi, > >=20 > > FYI, your patch has been queued to stable release 19.11.6 > >=20 > > Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet. > > It will be pushed if I get no objections before 11/11/20. So please > > shout if anyone has objections. >=20 > Not sure it has to be backported to 19.11 LTS. > Yes it fixes a problem, but it also introduces a functional change. > AFAIK, we have to avoid it with LTS patches. > Konstantin Ok, removed. > > Also note that after the patch there's a diff of the upstream commit vs= the > > patch applied to the branch. This will indicate if there was any rebasi= ng > > needed to apply to the stable branch. If there were code changes for re= basing > > (ie: not only metadata diffs), please double check that the rebase was > > correctly done. > >=20 > > Queued patches are on a temporary branch at: > > https://github.com/bluca/dpdk-stable > >=20 > > This queued commit can be viewed at: > > https://github.com/bluca/dpdk-stable/commit/6dce2ff12e8191ffd4d23e1ab93= 22f2d042f6e5b > >=20 > > Thanks. > >=20 > > Luca Boccassi > >=20 > > --- > > From 6dce2ff12e8191ffd4d23e1ab9322f2d042f6e5b Mon Sep 17 00:00:00 2001 > > From: Yi Yang > > Date: Mon, 26 Oct 2020 14:47:13 +0800 > > Subject: [PATCH] gso: fix mbuf freeing responsibility > >=20 > > [ upstream commit c0d002aed98d6d1d38d6bb318a5bd2ed5cdc01b1 ] > >=20 > > rte_gso_segment decreased refcnt of pkt by one, but > > it is wrong if pkt is external mbuf, pkt won't be > > freed because of incorrect refcnt, the result is > > application can't allocate mbuf from mempool because > > mbufs in mempool are run out of. > >=20 > > One correct way is application should call > > rte_pktmbuf_free after calling rte_gso_segment to free > > pkt explicitly. rte_gso_segment must not handle it, this > > should be responsibility of application. > >=20 > > This commit changed rte_gso_segment in functional behavior > > and return value, so the application must take appropriate > > actions according to return values, "ret < 0" means it > > should free and drop 'pkt', "ret =3D=3D 0" means 'pkt' isn't > > GSOed but 'pkt' can be transmitted as a normal packet, > > "ret > 0" means 'pkt' has been GSOed into two or multiple > > segments, it should use "pkts_out" to transmit these > > segments. The application must free 'pkt' after call > > rte_gso_segment when return value isn't equal to 0. > >=20 > > Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO") > >=20 > > Signed-off-by: Yi Yang > > Acked-by: Jiayu Hu > > Acked-by: Konstantin Ananyev > > --- > > app/test-pmd/csumonly.c | 12 ++++++++++-- > > .../generic_segmentation_offload_lib.rst | 7 +++++-- > > drivers/net/tap/rte_eth_tap.c | 12 ++++++++++-- > > lib/librte_gso/gso_tcp4.c | 6 ++---- > > lib/librte_gso/gso_tunnel_tcp4.c | 14 +++++--------- > > lib/librte_gso/gso_udp4.c | 6 ++---- > > lib/librte_gso/rte_gso.c | 15 +++------------ > > lib/librte_gso/rte_gso.h | 8 ++++++-- > > 8 files changed, 43 insertions(+), 37 deletions(-) > >=20 > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c > > index 7b92ab1195..d0eef8b51c 100644 > > --- a/app/test-pmd/csumonly.c > > +++ b/app/test-pmd/csumonly.c > > @@ -1050,9 +1050,17 @@ tunnel_update: > > ret =3D rte_gso_segment(pkts_burst[i], gso_ctx, > > &gso_segments[nb_segments], > > GSO_MAX_PKT_BURST - nb_segments); > > - if (ret >=3D 0) > > + if (ret >=3D 1) { > > + /* pkts_burst[i] can be freed safely here. */ > > + rte_pktmbuf_free(pkts_burst[i]); > > nb_segments +=3D ret; > > - else { > > + } else if (ret =3D=3D 0) { > > + /* 0 means it can be transmitted directly > > + * without gso. > > + */ > > + gso_segments[nb_segments] =3D pkts_burst[i]; > > + nb_segments +=3D 1; > > + } else { > > TESTPMD_LOG(DEBUG, "Unable to segment packet"); > > rte_pktmbuf_free(pkts_burst[i]); > > } > > diff --git a/doc/guides/prog_guide/generic_segmentation_offload_lib.rst= b/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > > index 205cb8a866..ad91c6e5fc 100644 > > --- a/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > > +++ b/doc/guides/prog_guide/generic_segmentation_offload_lib.rst > > @@ -25,8 +25,9 @@ Bearing that in mind, the GSO library enables DPDK ap= plications to segment > > packets in software. Note however, that GSO is implemented as a standa= lone > > library, and not via a 'fallback' mechanism (i.e. for when TSO is unsu= pported > > in the underlying hardware); that is, applications must explicitly inv= oke the > > -GSO library to segment packets. The size of GSO segments ``(segsz)`` i= s > > -configurable by the application. > > +GSO library to segment packets, they also must call ``rte_pktmbuf_free= ()`` > > +to free mbuf GSO segments attached after calling ``rte_gso_segment()``= . > > +The size of GSO segments (``segsz``) is configurable by the applicatio= n. > >=20 > > Limitations > > ----------- > > @@ -233,6 +234,8 @@ To segment an outgoing packet, an application must: > >=20 > > #. Invoke the GSO segmentation API, ``rte_gso_segment()``. > >=20 > > +#. Call ``rte_pktmbuf_free()`` to free mbuf ``rte_gso_segment()`` segm= ents. > > + > > #. If required, update the L3 and L4 checksums of the newly-created se= gments. > > For tunneled packets, the outer IPv4 headers' checksums should also= be > > updated. Alternatively, the application may offload checksum calcul= ation > > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_ta= p.c > > index cfbd579cd6..1e2f21d96f 100644 > > --- a/drivers/net/tap/rte_eth_tap.c > > +++ b/drivers/net/tap/rte_eth_tap.c > > @@ -713,8 +713,16 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, = uint16_t nb_pkts) > > if (num_tso_mbufs < 0) > > break; > >=20 > > - mbuf =3D gso_mbufs; > > - num_mbufs =3D num_tso_mbufs; > > + if (num_tso_mbufs >=3D 1) { > > + mbuf =3D gso_mbufs; > > + num_mbufs =3D num_tso_mbufs; > > + } else { > > + /* 0 means it can be transmitted directly > > + * without gso. > > + */ > > + mbuf =3D &mbuf_in; > > + num_mbufs =3D 1; > > + } > > } else { > > /* stats.errs will be incremented */ > > if (rte_pktmbuf_pkt_len(mbuf_in) > max_size) > > diff --git a/lib/librte_gso/gso_tcp4.c b/lib/librte_gso/gso_tcp4.c > > index ade172ac73..d31feaff95 100644 > > --- a/lib/librte_gso/gso_tcp4.c > > +++ b/lib/librte_gso/gso_tcp4.c > > @@ -50,15 +50,13 @@ gso_tcp4_segment(struct rte_mbuf *pkt, > > pkt->l2_len); > > frag_off =3D rte_be_to_cpu_16(ipv4_hdr->fragment_offset); > > if (unlikely(IS_FRAGMENTED(frag_off))) { > > - pkts_out[0] =3D pkt; > > - return 1; > > + return 0; > > } > >=20 > > /* Don't process the packet without data */ > > hdr_offset =3D pkt->l2_len + pkt->l3_len + pkt->l4_len; > > if (unlikely(hdr_offset >=3D pkt->pkt_len)) { > > - pkts_out[0] =3D pkt; > > - return 1; > > + return 0; > > } > >=20 > > pyld_unit_size =3D gso_size - hdr_offset; > > diff --git a/lib/librte_gso/gso_tunnel_tcp4.c b/lib/librte_gso/gso_tunn= el_tcp4.c > > index e0384c26d0..166aace73a 100644 > > --- a/lib/librte_gso/gso_tunnel_tcp4.c > > +++ b/lib/librte_gso/gso_tunnel_tcp4.c > > @@ -62,7 +62,7 @@ gso_tunnel_tcp4_segment(struct rte_mbuf *pkt, > > { > > struct rte_ipv4_hdr *inner_ipv4_hdr; > > uint16_t pyld_unit_size, hdr_offset, frag_off; > > - int ret =3D 1; > > + int ret; > >=20 > > hdr_offset =3D pkt->outer_l2_len + pkt->outer_l3_len + pkt->l2_len; > > inner_ipv4_hdr =3D (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char= *) + > > @@ -73,25 +73,21 @@ gso_tunnel_tcp4_segment(struct rte_mbuf *pkt, > > */ > > frag_off =3D rte_be_to_cpu_16(inner_ipv4_hdr->fragment_offset); > > if (unlikely(IS_FRAGMENTED(frag_off))) { > > - pkts_out[0] =3D pkt; > > - return 1; > > + return 0; > > } > >=20 > > hdr_offset +=3D pkt->l3_len + pkt->l4_len; > > /* Don't process the packet without data */ > > if (hdr_offset >=3D pkt->pkt_len) { > > - pkts_out[0] =3D pkt; > > - return 1; > > + return 0; > > } > > pyld_unit_size =3D gso_size - hdr_offset; > >=20 > > /* Segment the payload */ > > ret =3D gso_do_segment(pkt, hdr_offset, pyld_unit_size, direct_pool, > > indirect_pool, pkts_out, nb_pkts_out); > > - if (ret <=3D 1) > > - return ret; > > - > > - update_tunnel_ipv4_tcp_headers(pkt, ipid_delta, pkts_out, ret); > > + if (ret > 1) > > + update_tunnel_ipv4_tcp_headers(pkt, ipid_delta, pkts_out, ret); > >=20 > > return ret; > > } > > diff --git a/lib/librte_gso/gso_udp4.c b/lib/librte_gso/gso_udp4.c > > index 6fa68f243a..5d0186aa24 100644 > > --- a/lib/librte_gso/gso_udp4.c > > +++ b/lib/librte_gso/gso_udp4.c > > @@ -52,8 +52,7 @@ gso_udp4_segment(struct rte_mbuf *pkt, > > pkt->l2_len); > > frag_off =3D rte_be_to_cpu_16(ipv4_hdr->fragment_offset); > > if (unlikely(IS_FRAGMENTED(frag_off))) { > > - pkts_out[0] =3D pkt; > > - return 1; > > + return 0; > > } > >=20 > > /* > > @@ -65,8 +64,7 @@ gso_udp4_segment(struct rte_mbuf *pkt, > >=20 > > /* Don't process the packet without data. */ > > if (unlikely(hdr_offset + pkt->l4_len >=3D pkt->pkt_len)) { > > - pkts_out[0] =3D pkt; > > - return 1; > > + return 0; > > } > >=20 > > /* pyld_unit_size must be a multiple of 8 because frag_off > > diff --git a/lib/librte_gso/rte_gso.c b/lib/librte_gso/rte_gso.c > > index 751b5b625e..896350ebc8 100644 > > --- a/lib/librte_gso/rte_gso.c > > +++ b/lib/librte_gso/rte_gso.c > > @@ -30,7 +30,6 @@ rte_gso_segment(struct rte_mbuf *pkt, > > uint16_t nb_pkts_out) > > { > > struct rte_mempool *direct_pool, *indirect_pool; > > - struct rte_mbuf *pkt_seg; > > uint64_t ol_flags; > > uint16_t gso_size; > > uint8_t ipid_delta; > > @@ -44,8 +43,7 @@ rte_gso_segment(struct rte_mbuf *pkt, > >=20 > > if (gso_ctx->gso_size >=3D pkt->pkt_len) { > > pkt->ol_flags &=3D (~(PKT_TX_TCP_SEG | PKT_TX_UDP_SEG)); > > - pkts_out[0] =3D pkt; > > - return 1; > > + return 0; > > } > >=20 > > direct_pool =3D gso_ctx->direct_pool; > > @@ -75,18 +73,11 @@ rte_gso_segment(struct rte_mbuf *pkt, > > indirect_pool, pkts_out, nb_pkts_out); > > } else { > > /* unsupported packet, skip */ > > - pkts_out[0] =3D pkt; > > RTE_LOG(DEBUG, GSO, "Unsupported packet type\n"); > > - return 1; > > + ret =3D 0; > > } > >=20 > > - if (ret > 1) { > > - pkt_seg =3D pkt; > > - while (pkt_seg) { > > - rte_mbuf_refcnt_update(pkt_seg, -1); > > - pkt_seg =3D pkt_seg->next; > > - } > > - } else if (ret < 0) { > > + if (ret < 0) { > > /* Revert the ol_flags in the event of failure. */ > > pkt->ol_flags =3D ol_flags; > > } > > diff --git a/lib/librte_gso/rte_gso.h b/lib/librte_gso/rte_gso.h > > index 3aab297f44..d93ee8e5b1 100644 > > --- a/lib/librte_gso/rte_gso.h > > +++ b/lib/librte_gso/rte_gso.h > > @@ -89,8 +89,11 @@ struct rte_gso_ctx { > > * the GSO segments are sent to should support transmission of multi-s= egment > > * packets. > > * > > - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefo= re, > > - * when all GSO segments are freed, the input packet is freed automati= cally. > > + * If the input packet is GSO'd, all the indirect segments are attache= d to the > > + * input packet. > > + * > > + * rte_gso_segment() will not free the input packet no matter whether = it is > > + * GSO'd or not, the application should free it after calling rte_gso_= segment(). > > * > > * If the memory space in pkts_out or MBUF pools is insufficient, this > > * function fails, and it returns (-1) * errno. Otherwise, GSO succeed= s, > > @@ -109,6 +112,7 @@ struct rte_gso_ctx { > > * > > * @return > > * - The number of GSO segments filled in pkts_out on success. > > + * - Return 0 if it does not need to be GSO'd. > > * - Return -ENOMEM if run out of memory in MBUF pools. > > * - Return -EINVAL for invalid parameters. > > */ > > -- > > 2.27.0 > >=20 > > --- > > Diff of the applied patch vs upstream commit (please double-check if = non-empty: > > --- > > --- - 2020-11-09 18:40:12.168193728 +0000 > > +++ 0024-gso-fix-mbuf-freeing-responsibility.patch 2020-11-09 18:40:11.= 103310846 +0000 > > @@ -1 +1 @@ > > -From c0d002aed98d6d1d38d6bb318a5bd2ed5cdc01b1 Mon Sep 17 00:00:00 2001 > > +From 6dce2ff12e8191ffd4d23e1ab9322f2d042f6e5b Mon Sep 17 00:00:00 2001 > > @@ -5,0 +6,2 @@ > > +[ upstream commit c0d002aed98d6d1d38d6bb318a5bd2ed5cdc01b1 ] > > + > > @@ -28 +29,0 @@ > > -Cc: stable@dpdk.org > > @@ -36 +36,0 @@ > > - doc/guides/rel_notes/release_20_11.rst | 6 ++++++ > > @@ -43 +43 @@ > > - 9 files changed, 49 insertions(+), 37 deletions(-) > > + 8 files changed, 43 insertions(+), 37 deletions(-) > > @@ -46 +46 @@ > > -index 3d7d244d1e..d813d4fae0 100644 > > +index 7b92ab1195..d0eef8b51c 100644 > > @@ -49 +49 @@ > > -@@ -1080,9 +1080,17 @@ tunnel_update: > > +@@ -1050,9 +1050,17 @@ tunnel_update: > > @@ -94,17 +93,0 @@ > > -diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_n= otes/release_20_11.rst > > -index 7c8246d1b3..1524f61915 100644 > > ---- a/doc/guides/rel_notes/release_20_11.rst > > -+++ b/doc/guides/rel_notes/release_20_11.rst > > -@@ -569,6 +569,12 @@ API Changes > > - > > - * bpf: ``RTE_BPF_XTYPE_NUM`` has been dropped from ``rte_bpf_xtype``. > > - > > -+* gso: Changed ``rte_gso_segment`` behaviour and return value: > > -+ > > -+ * ``pkt`` is not saved to ``pkts_out[0]`` if not GSOed. > > -+ * Return 0 instead of 1 for the above case. > > -+ * ``pkt`` is not freed, no matter whether it is GSOed, leaving to t= he caller. > > -+ > > - * acl: ``RTE_ACL_CLASSIFY_NUM`` enum value has been removed. > > - This enum value was not used inside DPDK, while it prevented to add= new > > - classify algorithms without causing an ABI breakage. > > @@ -112 +95 @@ > > -index 81c688471d..2f8abb12c5 100644 > > +index cfbd579cd6..1e2f21d96f 100644 > > @@ -115 +98 @@ > > -@@ -751,8 +751,16 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs,= uint16_t nb_pkts) > > +@@ -713,8 +713,16 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs,= uint16_t nb_pkts)