Re: [PATCH 4/4] examples/dma: add minimal copy size parameter

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, Apr 11, 2022 at 10:56:34AM +0800, Chengwen Feng wrote:
> This patch adds minimal copy size parameter(-m/--min-copy-size), so
> when do copy by CPU or DMA, the real copy size will be the maximum of
> mbuf's data_len and this parameter.
> 
> This parameter was designed to compare the performance between CPU copy
> and DMA copy. User could send small packets with a high rate to drive
> the performance test.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>

Hi,

few comments inline below.

/Bruce

> ---
>  examples/dma/dmafwd.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/dma/dmafwd.c b/examples/dma/dmafwd.c
> index 6b1b777cb8..83094ba378 100644
> --- a/examples/dma/dmafwd.c
> +++ b/examples/dma/dmafwd.c
> @@ -25,6 +25,7 @@
>  #define CMD_LINE_OPT_RING_SIZE "ring-size"
>  #define CMD_LINE_OPT_BATCH_SIZE "dma-batch-size"
>  #define CMD_LINE_OPT_FRAME_SIZE "max-frame-size"
> +#define CMD_LINE_OPT_COPY_SIZE	"min-copy-size"

While I'm not sure this strictly belongs in an example app to show use of
dmadev, I can see the value of it. However, I suggest we need to make it
clearer that it's not directly relevant to the normal use of the app. I
suggest making the parameter "force-min-copy-size" to make it clearer that
it's an explicit override.

>  #define CMD_LINE_OPT_STATS_INTERVAL "stats-interval"
>  
>  /* configurable number of RX/TX ring descriptors */
> @@ -119,6 +120,7 @@ static volatile bool force_quit;
>  
>  static uint32_t dma_batch_sz = MAX_PKT_BURST;
>  static uint32_t max_frame_size;
> +static uint32_t min_copy_size;
>  
>  /* ethernet addresses of ports */
>  static struct rte_ether_addr dma_ports_eth_addr[RTE_MAX_ETHPORTS];
> @@ -208,7 +210,12 @@ print_stats(char *prgname)
>  		"Rx Queues = %d, ", nb_queues);
>  	status_strlen += snprintf(status_string + status_strlen,
>  		sizeof(status_string) - status_strlen,
> -		"Ring Size = %d", ring_size);
> +		"Ring Size = %d\n", ring_size);
> +	status_strlen += snprintf(status_string + status_strlen,
> +		sizeof(status_string) - status_strlen,
> +		"Min Copy Size = %u Packet Data Room Size = %u",
> +		min_copy_size, rte_pktmbuf_data_room_size(dma_pktmbuf_pool) -
> +		RTE_PKTMBUF_HEADROOM);
>  
>  	memset(&ts, 0, sizeof(struct total_statistics));
>  
> @@ -307,7 +314,8 @@ static inline void
>  pktmbuf_sw_copy(struct rte_mbuf *src, struct rte_mbuf *dst)
>  {
>  	rte_memcpy(rte_pktmbuf_mtod(dst, char *),
> -		rte_pktmbuf_mtod(src, char *), src->data_len);
> +		rte_pktmbuf_mtod(src, char *),
> +		RTE_MAX(src->data_len, min_copy_size));
>  }
>  /* >8 End of perform packet copy there is a user-defined function. */
>  
> @@ -324,7 +332,8 @@ dma_enqueue_packets(struct rte_mbuf *pkts[], struct rte_mbuf *pkts_copy[],
>  		ret = rte_dma_copy(dev_id, 0,
>  			rte_pktmbuf_iova(pkts[i]),
>  			rte_pktmbuf_iova(pkts_copy[i]),
> -			rte_pktmbuf_data_len(pkts[i]), 0);
> +			RTE_MAX(rte_pktmbuf_data_len(pkts[i]), min_copy_size),
> +			0);
>  
>  		if (ret < 0)
>  			break;
> @@ -576,6 +585,7 @@ dma_usage(const char *prgname)
>  	printf("%s [EAL options] -- -p PORTMASK [-q NQ]\n"
>  		"  -b --dma-batch-size: number of requests per DMA batch\n"
>  		"  -f --max-frame-size: max frame size\n"
> +		"  -m --min-copy-size: minimum copy length\n"

The help text needs to be expanded, again to make clear that this is for
perf comparison and the like. Something like "Force a minimum copy length,
	even for smaller packets"