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 2A744A0527 for ; Mon, 9 Nov 2020 19:42:39 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1D56069C8; Mon, 9 Nov 2020 19:42:38 +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 9897C72E2 for ; Mon, 9 Nov 2020 19:42:35 +0100 (CET) Received: by mail-wr1-f66.google.com with SMTP id p1so9856061wrf.12 for ; Mon, 09 Nov 2020 10:42:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=as8FItqkNCedGjbRNYkV2dM39H2IXzNaIeu+jaJ4v2Q=; b=qEI2GrQjc+CnMu4Uigz/PfkBkmlzDnr67HhKSs1PNAPAgZcRd3gOKjvLp5GDAigO7l HCcfrd+SwcopfKCV4B02j/02nAkh2iY3mLoTR5B7oJ0nBsWWLxyb17nF6YvR/hasgaff 3KETsnrWZ4ntQ/N4BLg/sZZYZbns5zB4N7K42Ay8DTn0u465BdnNupGCASCuK+sfGuzP ruXvNEGRLMgCA388WH9OiFhmbvKjJpZ/YScOXIk8cyXPuQt8D/ICMoHumHABLJzuTCdM hCV/79GhsDE1mSHjnL7KusCD8kTUHrjuE2Ie0sYYvk3BdRsJRPn33LFSeIDnKJKjXDHq 2jKg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=as8FItqkNCedGjbRNYkV2dM39H2IXzNaIeu+jaJ4v2Q=; b=XtmY6vUIY5ZOqruez6IUd3hF+KuWx0rrfSSMBzmeWk2XC7nOCEuayQOyY9NtZqjO5q yBS56ww7SGdtRAORd5TbrG637l/jkmfznxwUw3ndNA4ojnqfkcXQBPQLfTNtwAmBAZRI 35A1u/XZVMTs3QnFPPVDwB7ZT9IsoBkx4orb0fEWwOTC0Ngt9MRet4VpKznBfZLuxKSb 67LjggSEKJ7G4IxHErLVHl10ohhJ/NMKhSkGLXQs5h5imM49/txROkl86nHH9+qDmgud dpG6j7agRtVEWp35Q8z83hOw4Ot7CbVrUwqW8OshN5IZvQgOfK4GWtSN3A4u8IaqPLed TEcg== X-Gm-Message-State: AOAM532ovlcvK+hNgyZjcxkutH4zJfsGzbYUULNVSSi5nIF1/c9tQ9QF B6QTUmcTC8Jf8SnHlR8w4lbOzVrBirQ4tQ== X-Google-Smtp-Source: ABdhPJxmg5MEi/+dw7Aa6WvKMzc6EmRHgepUFUepirEzEGakzMGVbM+22EnVaDC9oVFBLAoemw3GJQ== X-Received: by 2002:a05:6000:1188:: with SMTP id g8mr18946726wrx.422.1604947355293; Mon, 09 Nov 2020 10:42:35 -0800 (PST) Received: from localhost ([88.98.246.218]) by smtp.gmail.com with ESMTPSA id i5sm7571855wrw.45.2020.11.09.10.42.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Nov 2020 10:42:34 -0800 (PST) From: luca.boccassi@gmail.com To: Yi Yang Cc: Jiayu Hu , Konstantin Ananyev , dpdk stable Date: Mon, 9 Nov 2020 18:40:12 +0000 Message-Id: <20201109184111.3463090-24-luca.boccassi@gmail.com> X-Mailer: git-send-email 2.27.0 In-Reply-To: <20201109184111.3463090-1-luca.boccassi@gmail.com> References: <20201028104606.3504127-207-luca.boccassi@gmail.com> <20201109184111.3463090-1-luca.boccassi@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [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" Hi, FYI, your patch has been queued to stable release 19.11.6 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. 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 rebasing needed to apply to the stable branch. If there were code changes for rebasing (ie: not only metadata diffs), please double check that the rebase was correctly done. Queued patches are on a temporary branch at: https://github.com/bluca/dpdk-stable This queued commit can be viewed at: https://github.com/bluca/dpdk-stable/commit/6dce2ff12e8191ffd4d23e1ab9322f2d042f6e5b Thanks. Luca Boccassi --- >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 [ upstream commit c0d002aed98d6d1d38d6bb318a5bd2ed5cdc01b1 ] 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. 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. 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 == 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. Fixes: 119583797b6a ("gso: support TCP/IPv4 GSO") 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(-) 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 = rte_gso_segment(pkts_burst[i], gso_ctx, &gso_segments[nb_segments], GSO_MAX_PKT_BURST - nb_segments); - if (ret >= 0) + if (ret >= 1) { + /* pkts_burst[i] can be freed safely here. */ + rte_pktmbuf_free(pkts_burst[i]); nb_segments += ret; - else { + } else if (ret == 0) { + /* 0 means it can be transmitted directly + * without gso. + */ + gso_segments[nb_segments] = pkts_burst[i]; + nb_segments += 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 applications to segment packets in software. Note however, that GSO is implemented as a standalone library, and not via a 'fallback' mechanism (i.e. for when TSO is unsupported in the underlying hardware); that is, applications must explicitly invoke the -GSO library to segment packets. The size of GSO segments ``(segsz)`` is -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 application. Limitations ----------- @@ -233,6 +234,8 @@ To segment an outgoing packet, an application must: #. Invoke the GSO segmentation API, ``rte_gso_segment()``. +#. Call ``rte_pktmbuf_free()`` to free mbuf ``rte_gso_segment()`` segments. + #. If required, update the L3 and L4 checksums of the newly-created segments. For tunneled packets, the outer IPv4 headers' checksums should also be updated. Alternatively, the application may offload checksum calculation diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.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; - mbuf = gso_mbufs; - num_mbufs = num_tso_mbufs; + if (num_tso_mbufs >= 1) { + mbuf = gso_mbufs; + num_mbufs = num_tso_mbufs; + } else { + /* 0 means it can be transmitted directly + * without gso. + */ + mbuf = &mbuf_in; + num_mbufs = 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 = rte_be_to_cpu_16(ipv4_hdr->fragment_offset); if (unlikely(IS_FRAGMENTED(frag_off))) { - pkts_out[0] = pkt; - return 1; + return 0; } /* Don't process the packet without data */ hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len; if (unlikely(hdr_offset >= pkt->pkt_len)) { - pkts_out[0] = pkt; - return 1; + return 0; } pyld_unit_size = gso_size - hdr_offset; diff --git a/lib/librte_gso/gso_tunnel_tcp4.c b/lib/librte_gso/gso_tunnel_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 = 1; + int ret; hdr_offset = pkt->outer_l2_len + pkt->outer_l3_len + pkt->l2_len; inner_ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) + @@ -73,25 +73,21 @@ gso_tunnel_tcp4_segment(struct rte_mbuf *pkt, */ frag_off = rte_be_to_cpu_16(inner_ipv4_hdr->fragment_offset); if (unlikely(IS_FRAGMENTED(frag_off))) { - pkts_out[0] = pkt; - return 1; + return 0; } hdr_offset += pkt->l3_len + pkt->l4_len; /* Don't process the packet without data */ if (hdr_offset >= pkt->pkt_len) { - pkts_out[0] = pkt; - return 1; + return 0; } pyld_unit_size = gso_size - hdr_offset; /* Segment the payload */ ret = gso_do_segment(pkt, hdr_offset, pyld_unit_size, direct_pool, indirect_pool, pkts_out, nb_pkts_out); - if (ret <= 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); 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 = rte_be_to_cpu_16(ipv4_hdr->fragment_offset); if (unlikely(IS_FRAGMENTED(frag_off))) { - pkts_out[0] = pkt; - return 1; + return 0; } /* @@ -65,8 +64,7 @@ gso_udp4_segment(struct rte_mbuf *pkt, /* Don't process the packet without data. */ if (unlikely(hdr_offset + pkt->l4_len >= pkt->pkt_len)) { - pkts_out[0] = pkt; - return 1; + return 0; } /* 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, if (gso_ctx->gso_size >= pkt->pkt_len) { pkt->ol_flags &= (~(PKT_TX_TCP_SEG | PKT_TX_UDP_SEG)); - pkts_out[0] = pkt; - return 1; + return 0; } direct_pool = 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] = pkt; RTE_LOG(DEBUG, GSO, "Unsupported packet type\n"); - return 1; + ret = 0; } - if (ret > 1) { - pkt_seg = pkt; - while (pkt_seg) { - rte_mbuf_refcnt_update(pkt_seg, -1); - pkt_seg = pkt_seg->next; - } - } else if (ret < 0) { + if (ret < 0) { /* Revert the ol_flags in the event of failure. */ pkt->ol_flags = 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-segment * packets. * - * If the input packet is GSO'd, its mbuf refcnt reduces by 1. Therefore, - * when all GSO segments are freed, the input packet is freed automatically. + * If the input packet is GSO'd, all the indirect segments are attached 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 succeeds, @@ -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 --- 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_notes/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 the 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)