Re: [dpdk-dev] [PATCH V1 2/3] mbuf: change free_cb interface to adapt to GSO case

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On Thu, Jul 30, 2020 at 08:08:59PM +0800, yang_y_yi@163.com wrote:
> 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);

I'm sorry but it is not very clear to me what operations are done by
rte_gso_segment().

In the current code, I only see calls to rte_pktmbuf_attach(),
which do not deal with external buffers. Am I missing something?

Are you able to show the issue only with mbuf functions? It would
be helpful to understand what does not work.


Thanks,
Olivier



> 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
>