RE: [PATCH v1 1/2] net/memif: add a Rx fast path

[Joyce Kong <Joyce.Kong@arm.com>]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
> Sent: Thursday, May 19, 2022 12:53 AM
> To: Joyce Kong <Joyce.Kong@arm.com>; Jakub Grajciar <jgrajcia@cisco.com>
> Cc: Ruifeng Wang <Ruifeng.Wang@arm.com>; dev@dpdk.org; nd
> <nd@arm.com>
> Subject: Re: [PATCH v1 1/2] net/memif: add a Rx fast path
> 
> On 5/17/2022 11:51 AM, Joyce Kong wrote:
> > For memif non-zero-copy mode, there is a branch to compare the mbuf
> > and memif buffer size during memory copying. Add a fast memory copy
> > path by removing this branch with mbuf and memif buffer size defined
> > at compile time. The removal of the branch leads to considerable
> > performance uplift.
> >
> > When memif <= buffer size, Rx chooses the fast memcpy path, otherwise
> > it would choose the original path.
> >
> > Test with 1p1q on Ampere Altra AArch64 server,
> > --------------------------------------------
> >    buf size  | memif <= mbuf | memif > mbuf |
> > --------------------------------------------
> > non-zc gain |     4.30%     |    -0.52%    |
> > --------------------------------------------
> >     zc gain  |     2.46%     |     0.70%    |
> > --------------------------------------------
> >
> > Test with 1p1q on Cascade Lake Xeon X86server,
> > -------------------------------------------
> >    buf size  | memif <= mbuf | memif > mbuf |
> > -------------------------------------------
> > non-zc gain |     2.13%     |    -1.40%    |
> > -------------------------------------------
> >     zc gain  |     0.18%     |     0.48%    |
> > -------------------------------------------
> >
> 
> 
> Hi Joyce,
> 
> I have multiple questions,
> 
> 1) The patch updates only non-zero-copy mode Rx path ('eth_memif_rx'), why
> zero-copy path performance also impacted?
> 
For memif zero-copy mode, only client runs 'eth_memif_rx_zc', and server still runs
'eth_memif_rx', so the patch would impacts zero-copy mode.

> 2) As far as I can see there is a behavior change, more details below
> 
> 3) patch talking about memif buffer size being defined in compile time, is the
> big "memif <= mbuf" if block optimized out?
> Since 'pkt_buffer_size' is a devarg, so it can change from run to run and it is not
> known in compile time, I doubt that it is optimized out.
> Is having  'pkt_buffer_size' as devarg breaks your logic?
> 
From memif run to run, run.pkt_buffer_size would change, and cfg.pkt_buffer_size
which is the reserved max buffer size would not change. For patch details, I use
cfg.pkt_buffer_size to implement the logic.

> 4) One option gains performance and other loose performance, do you think
> gain performance case is more common use case? Is there any data around it?
> 
Yes, I think the gain performance case is more common case, as the default memif
buffer size equals to mbuf size. In theory, when memif buf size >= mbuf size, the Rx
runs the original path, it would not lead to obvious impact.

> 
> Jakub,
> 
> Do you want to test this patch first before progressing with it?
> 
> > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > ---
> >   drivers/net/memif/rte_eth_memif.c | 124 ++++++++++++++++++++----------
> >   1 file changed, 84 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/net/memif/rte_eth_memif.c
> > b/drivers/net/memif/rte_eth_memif.c
> > index 587ad45576..f55776ca46 100644
> > --- a/drivers/net/memif/rte_eth_memif.c
> > +++ b/drivers/net/memif/rte_eth_memif.c
> > @@ -342,66 +342,111 @@ eth_memif_rx(void *queue, struct rte_mbuf
> **bufs, uint16_t nb_pkts)
> >   		goto refill;
> >   	n_slots = last_slot - cur_slot;
> >
> > -	while (n_slots && n_rx_pkts < nb_pkts) {
> > -		mbuf_head = rte_pktmbuf_alloc(mq->mempool);
> > -		if (unlikely(mbuf_head == NULL))
> > -			goto no_free_bufs;
> > -		mbuf = mbuf_head;
> > -		mbuf->port = mq->in_port;
> > +	if (likely(mbuf_size >= pmd->cfg.pkt_buffer_size)) {
> > +		while (n_slots && n_rx_pkts < nb_pkts) {
> > +			mbuf_head = rte_pktmbuf_alloc(mq->mempool);
> > +			if (unlikely(mbuf_head == NULL))
> > +				goto no_free_bufs;
> > +			mbuf = mbuf_head;
> > +			mbuf->port = mq->in_port;
> > +
> > +next_slot1:
> > +			s0 = cur_slot & mask;
> > +			d0 = &ring->desc[s0];
> >
> > -next_slot:
> > -		s0 = cur_slot & mask;
> > -		d0 = &ring->desc[s0];
> > +			cp_len = d0->length;
> >
> > -		src_len = d0->length;
> > -		dst_off = 0;
> > -		src_off = 0;
> > +			rte_pktmbuf_data_len(mbuf) = cp_len;
> > +			rte_pktmbuf_pkt_len(mbuf) = cp_len;
> > +			if (mbuf != mbuf_head)
> > +				rte_pktmbuf_pkt_len(mbuf_head) += cp_len;
> >
> > -		do {
> > -			dst_len = mbuf_size - dst_off;
> > -			if (dst_len == 0) {
> > -				dst_off = 0;
> > -				dst_len = mbuf_size;
> > +			rte_memcpy(rte_pktmbuf_mtod(mbuf, void *),
> > +				(uint8_t *)memif_get_buffer(proc_private, d0),
> cp_len);
> > +
> > +			cur_slot++;
> > +			n_slots--;
> >
> > -				/* store pointer to tail */
> > +			if (d0->flags & MEMIF_DESC_FLAG_NEXT) {
> >   				mbuf_tail = mbuf;
> >   				mbuf = rte_pktmbuf_alloc(mq->mempool);
> >   				if (unlikely(mbuf == NULL))
> >   					goto no_free_bufs;
> > -				mbuf->port = mq->in_port;
> >   				ret = memif_pktmbuf_chain(mbuf_head,
> mbuf_tail, mbuf);
> >   				if (unlikely(ret < 0)) {
> >   					MIF_LOG(ERR, "number-of-segments-
> overflow");
> >   					rte_pktmbuf_free(mbuf);
> >   					goto no_free_bufs;
> >   				}
> > +				goto next_slot1;
> >   			}
> 
> It is very hard to comment on the correct part of the patch, since it is mixed a
> lot, but
> - previously when memif buffer is segmented, and its size is less than mbuf;
> mbuf is filled with as much memif data as possible and later switched to next
> mbuf, like:
> 
>    memif buffer
> +-+  +-+  +-+  +-+
> |a|->|b|->|c|->|d|
> +-+  +-+  +-+  +-+
> 
> +---+  +---+
> |abc|->|d  |
> +---+  +---+
>    mbuf
> 
> 
> - Now each memif segment is a mbuf,
> 
>    memif buffer
> +-+  +-+  +-+  +-+
> |a|->|b|->|c|->|d|
> +-+  +-+  +-+  +-+
> 
> +---+  +---+  +---+  +---+
> |a  |->|b  |->|c  |->|d  |
> +---+  +---+  +---+  +---+
>    mbuf
> 
> Can you please confirm this behavior change? If so can you please highlight is
> more in the commit log?
> And is this tradeoff something preferred?