gso: fix mbuf freeing responsibility

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")
Cc: stable@dpdk.org
Signed-off-by: Yi Yang <yangyi01@inspur.com>
Acked-by: Jiayu Hu <jiayu.hu@intel.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 3d7d244d1eecc2c12e1e1590ee76bf5fead0dc22..d813d4fae09658d034c494134a73a4363eb6cd8b 100644 (file)
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -1080,9 +1080,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 205cb8a866dc548a26db64c0c85d273894d16b3a..ad91c6e5fce343ed397c495c114bc4ee8b6038db 100644 (file)
--- 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/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index 7c8246d1b3222d716afce7f65ab5014a97992722..1524f619157ed49d69e94d31fbc146502b06e5b5 100644 (file)
--- 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.

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 81c688471d3f5d0777dbb2b404490db8fd6e6a61..2f8abb12c5c68421d7cca45c1faebca78e774827 100644 (file)
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -751,8 +751,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 ade172ac73ced021dafea7feafffe0144814a4c7..d31feaff95cdd7a25c51f051ea98a4f47f5209a1 100644 (file)
--- 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 e0384c26d0659358e83d0fe6f1ac75fc65f84e8d..166aace73a92e30a6995f0b77f8939b08f302091 100644 (file)
--- 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 6fa68f243a3196e8f056ba9e8ca2e3f27c4188ea..5d0186aa240b777d3d122c0549de3d5f2bbace94 100644 (file)
--- 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 751b5b625e34bf31066abc7ba75e420aa2b1fc23..896350ebc88bdf8281a4cf07b35701220d1e9ec9 100644 (file)
--- 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 3aab297f44aaca4fd7b61dff25d9bbdd28059a52..d93ee8e5b17130cfc18d91173b3fcea79f0f3fa0 100644 (file)
--- 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.
  */