DPDK patches and discussions
 help / color / Atom feed
From: yang_y_yi@163.com
To: dev@dpdk.org
Cc: jiayu.hu@intel.com, thomas@monjalon.net, yangyi01@inspur.com,
	yang_y_yi@163.com
Subject: [dpdk-dev] [PATCH V1 2/3] mbuf: change free_cb interface to adapt to GSO case
Date: Thu, 30 Jul 2020 20:08:59 +0800
Message-ID: <20200730120900.108232-3-yang_y_yi@163.com> (raw)
In-Reply-To: <20200730120900.108232-1-yang_y_yi@163.com>

From: Yi Yang <yangyi01@inspur.com>

In GSO case, segmented mbufs are attached to original
mbuf which can't be freed when it is external. The issue
is free_cb doesn't know original mbuf and doesn't free
it when refcnt of shinfo is 0.

Original mbuf can be freed by rte_pktmbuf_free segmented
mbufs or by rte_pktmbuf_free original mbuf. Two kind of
cases should have different behaviors. free_cb won't
explicitly call rte_pktmbuf_free to free original mbuf
if it is freed by rte_pktmbuf_free original mbuf, but it
has to call rte_pktmbuf_free to free original mbuf if it
is freed by rte_pktmbuf_free segmented mbufs.

In order to fix this issue, free_cb interface has to been
changed, __rte_pktmbuf_free_extbuf must deliver called
mbuf pointer to free_cb, argument opaque can be defined
as a custom struct by user, it can includes original mbuf
pointer, user-defined free_cb can compare caller mbuf with
mbuf in opaque struct, free_cb should free original mbuf
if they are not same, this corresponds to rte_pktmbuf_free
segmented mbufs case, otherwise, free_cb won't free original
mbuf because the caller explicitly called rte_pktmbuf_free
to free it.

Here is pseduo code to show two kind of cases.

case 1. rte_pktmbuf_free segmented mbufs

nb_tx = rte_gso_segment(original_mbuf, /* original mbuf */
                      &gso_ctx,
                      /* segmented mbuf */
                      (struct rte_mbuf **)&gso_mbufs,
                      MAX_GSO_MBUFS);
rte_eth_tx_burst(dev->port_id, qid, gso_mbufs, nb_tx);

case 2. rte_pktmbuf_free original mbuf

rte_eth_tx_burst(dev->port_id, qid, original_mbuf, 1);

Here are user defined free_cb and opaque.

struct shinfo_arg {
	void *buf;
	struct rte_mbuf *mbuf;
};

custom_free_cb(struct rte_mbuf *caller_m, void *opaque)
{
	struct shinfo_arg *arg = (struct shinfo_arg *)opaque;

	rte_free(arg->buf);
	if (caller_m != arg->mbuf)
		rte_pktmbuf_free(arg->mbuf);
	rte_free(arg);
}

struct shinfo_arg *arg = (struct shinfo_arg *)
rte_malloc(NULL, sizeof(struct shinfo_arg),
	   RTE_CACHE_LINE_SIZE);

arg->buf = buf;
arg->mbuf = pkt;
shinfo->free_cb = custom_free_cb;
shinfo->fcb_opaque = arg;

Signed-off-by: Yi Yang <yangyi01@inspur.com>
---
 app/test-compress-perf/comp_perf_test_common.c | 2 +-
 app/test/test_compressdev.c                    | 2 +-
 app/test/test_mbuf.c                           | 2 +-
 drivers/net/mlx5/mlx5_rxtx.c                   | 2 +-
 drivers/net/mlx5/mlx5_rxtx.h                   | 2 +-
 drivers/net/netvsc/hn_rxtx.c                   | 3 ++-
 lib/librte_mbuf/rte_mbuf.c                     | 4 ++--
 lib/librte_mbuf/rte_mbuf.h                     | 2 +-
 lib/librte_mbuf/rte_mbuf_core.h                | 2 +-
 lib/librte_vhost/virtio_net.c                  | 2 +-
 10 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/app/test-compress-perf/comp_perf_test_common.c b/app/test-compress-perf/comp_perf_test_common.c
index b402a0d..b1eb733 100644
--- a/app/test-compress-perf/comp_perf_test_common.c
+++ b/app/test-compress-perf/comp_perf_test_common.c
@@ -115,7 +115,7 @@ struct cperf_buffer_info {
 }
 
 static void
-comp_perf_extbuf_free_cb(void *addr __rte_unused, void *opaque __rte_unused)
+comp_perf_extbuf_free_cb(struct rte_mbuf *m __rte_unused, void *opaque __rte_unused)
 {
 }
 
diff --git a/app/test/test_compressdev.c b/app/test/test_compressdev.c
index 0571c17..a4a5d7c 100644
--- a/app/test/test_compressdev.c
+++ b/app/test/test_compressdev.c
@@ -778,7 +778,7 @@ struct test_private_arrays {
 }
 
 static void
-extbuf_free_callback(void *addr __rte_unused, void *opaque __rte_unused)
+extbuf_free_callback(struct rte_mbuf *m __rte_unused, void *opaque __rte_unused)
 {
 }
 
diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
index 06e44f0..f9e5ca5 100644
--- a/app/test/test_mbuf.c
+++ b/app/test/test_mbuf.c
@@ -2300,7 +2300,7 @@ struct test_case {
 
 /* Define a free call back function to be used for external buffer */
 static void
-ext_buf_free_callback_fn(void *addr __rte_unused, void *opaque)
+ext_buf_free_callback_fn(struct rte_mbuf *caller_m __rte_unused, void *opaque)
 {
 	void *ext_buf_addr = opaque;
 
diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 3eb0243..f084488 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -1622,7 +1622,7 @@ enum mlx5_txcmp_code {
 }
 
 void
-mlx5_mprq_buf_free_cb(void *addr __rte_unused, void *opaque)
+mlx5_mprq_buf_free_cb(struct rte_mbuf *caller_m __rte_unused, void *opaque)
 {
 	struct mlx5_mprq_buf *buf = opaque;
 
diff --git a/drivers/net/mlx5/mlx5_rxtx.h b/drivers/net/mlx5/mlx5_rxtx.h
index c02a007..8c230c6 100644
--- a/drivers/net/mlx5/mlx5_rxtx.h
+++ b/drivers/net/mlx5/mlx5_rxtx.h
@@ -499,7 +499,7 @@ struct mlx5_txq_ctrl *mlx5_txq_hairpin_new
 uint16_t mlx5_rx_burst(void *dpdk_rxq, struct rte_mbuf **pkts, uint16_t pkts_n);
 void mlx5_rxq_initialize(struct mlx5_rxq_data *rxq);
 __rte_noinline int mlx5_rx_err_handle(struct mlx5_rxq_data *rxq, uint8_t vec);
-void mlx5_mprq_buf_free_cb(void *addr, void *opaque);
+void mlx5_mprq_buf_free_cb(struct rte_mbuf *caller_m, void *opaque);
 void mlx5_mprq_buf_free(struct mlx5_mprq_buf *buf);
 uint16_t mlx5_rx_burst_mprq(void *dpdk_rxq, struct rte_mbuf **pkts,
 			    uint16_t pkts_n);
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index 86a4c0d..30af404 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -519,7 +519,8 @@ int hn_dev_tx_descriptor_status(void *arg, uint16_t offset)
 	return 0;
 }
 
-static void hn_rx_buf_free_cb(void *buf __rte_unused, void *opaque)
+static void hn_rx_buf_free_cb(struct rte_mbuf *caller_m __rte_unused,
+			      void *opaque)
 {
 	struct hn_rx_bufinfo *rxb = opaque;
 	struct hn_data *hv = rxb->hv;
diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
index 8a456e5..52445b3 100644
--- a/lib/librte_mbuf/rte_mbuf.c
+++ b/lib/librte_mbuf/rte_mbuf.c
@@ -118,11 +118,11 @@
  * buffer.
  */
 static void
-rte_pktmbuf_free_pinned_extmem(void *addr, void *opaque)
+rte_pktmbuf_free_pinned_extmem(struct rte_mbuf *caller_m, void *opaque)
 {
 	struct rte_mbuf *m = opaque;
 
-	RTE_SET_USED(addr);
+	RTE_SET_USED(caller_m);
 	RTE_ASSERT(RTE_MBUF_HAS_EXTBUF(m));
 	RTE_ASSERT(RTE_MBUF_HAS_PINNED_EXTBUF(m));
 	RTE_ASSERT(m->shinfo->fcb_opaque == m);
diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index 7259575..5f8626f 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -1193,7 +1193,7 @@ static inline void rte_pktmbuf_attach(struct rte_mbuf *mi, struct rte_mbuf *m)
 	RTE_ASSERT(m->shinfo != NULL);
 
 	if (rte_mbuf_ext_refcnt_update(m->shinfo, -1) == 0)
-		m->shinfo->free_cb(m->buf_addr, m->shinfo->fcb_opaque);
+		m->shinfo->free_cb(m, m->shinfo->fcb_opaque);
 }
 
 /**
diff --git a/lib/librte_mbuf/rte_mbuf_core.h b/lib/librte_mbuf/rte_mbuf_core.h
index 8cd7137..d194429 100644
--- a/lib/librte_mbuf/rte_mbuf_core.h
+++ b/lib/librte_mbuf/rte_mbuf_core.h
@@ -671,7 +671,7 @@ struct rte_mbuf {
 /**
  * Function typedef of callback to free externally attached buffer.
  */
-typedef void (*rte_mbuf_extbuf_free_callback_t)(void *addr, void *opaque);
+typedef void (*rte_mbuf_extbuf_free_callback_t)(struct rte_mbuf *, void *);
 
 /**
  * Shared data at the end of an external buffer.
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 95a0bc1..e663fd4 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -2137,7 +2137,7 @@ uint16_t rte_vhost_poll_enqueue_completed(int vid, uint16_t queue_id,
 }
 
 static void
-virtio_dev_extbuf_free(void *addr __rte_unused, void *opaque)
+virtio_dev_extbuf_free(struct rte_mbuf * caller_m __rte_unused, void *opaque)
 {
 	rte_free(opaque);
 }
-- 
1.8.3.1


  parent reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-30 12:08 [dpdk-dev] [PATCH V1 0/3] Fix external mbuf free issue in " yang_y_yi
2020-07-30 12:08 ` [dpdk-dev] [PATCH V1 1/3] gso: fix refcnt update issue for external mbuf yang_y_yi
2020-07-30 12:08 ` yang_y_yi [this message]
2020-07-31 15:15   ` [dpdk-dev] [PATCH V1 2/3] mbuf: change free_cb interface to adapt to GSO case Olivier Matz
2020-08-01 23:12     ` yang_y_yi
2020-08-02 20:29       ` Olivier Matz
2020-08-02 20:45         ` Olivier Matz
2020-08-03  1:32           ` yang_y_yi
2020-08-03  1:26         ` yang_y_yi
2020-08-03  8:11           ` Olivier Matz
2020-08-03  9:42             ` yang_y_yi
2020-08-03 12:34               ` Olivier Matz
2020-08-04  1:31                 ` yang_y_yi
2020-07-30 12:09 ` [dpdk-dev] [PATCH V1 3/3] vhost: use new free_cb interface to fix mbuf free issue yang_y_yi

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200730120900.108232-3-yang_y_yi@163.com \
    --to=yang_y_yi@163.com \
    --cc=dev@dpdk.org \
    --cc=jiayu.hu@intel.com \
    --cc=thomas@monjalon.net \
    --cc=yangyi01@inspur.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox