diff mbox series

vhost: fix unnecessary dirty page logging

Message ID 20220707065513.66458-1-xuan.ding@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: fix unnecessary dirty page logging |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Ding, Xuan July 7, 2022, 6:55 a.m. UTC 
  From: Xuan Ding <xuan.ding@intel.com>

The dirty page logging is only required in vhost enqueue direction for
live migration. This patch removes the unnecessary dirty page logging
in vhost dequeue direction. Otherwise, it will result in a performance
drop. Some if-else judgements are also optimized to improve performance.

Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf refactoring")
Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")

Signed-off-by: Xuan Ding <xuan.ding@intel.com>
---
 lib/vhost/virtio_net.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

Comments

Hu, Jiayu July 7, 2022, 8:23 a.m. UTC | #1 
> -----Original Message-----
> From: Ding, Xuan <xuan.ding@intel.com>
> Sent: Thursday, July 7, 2022 2:55 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> Jiang, Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
> Subject: [PATCH] vhost: fix unnecessary dirty page logging
> 
> From: Xuan Ding <xuan.ding@intel.com>
> 
> The dirty page logging is only required in vhost enqueue direction for live
> migration. This patch removes the unnecessary dirty page logging in vhost
> dequeue direction. Otherwise, it will result in a performance drop. Some if-
> else judgements are also optimized to improve performance.
> 
> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
> refactoring")
> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> 
> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> ---
>  lib/vhost/virtio_net.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c index
> e842c35fef..12b7fbe7f9 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -1113,27 +1113,27 @@ sync_fill_seg(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>  			rte_memcpy((void *)((uintptr_t)(buf_addr)),
>  				rte_pktmbuf_mtod_offset(m, void *,
> mbuf_offset),
>  				cpy_len);
> +			vhost_log_cache_write_iova(dev, vq, buf_iova,
> cpy_len);
> +			PRINT_PACKET(dev, (uintptr_t)(buf_addr), cpy_len,
> 0);
>  		} else {
>  			rte_memcpy(rte_pktmbuf_mtod_offset(m, void *,
> mbuf_offset),
>  				(void *)((uintptr_t)(buf_addr)),
>  				cpy_len);
>  		}
> -		vhost_log_cache_write_iova(dev, vq, buf_iova, cpy_len);
> -		PRINT_PACKET(dev, (uintptr_t)(buf_addr), cpy_len, 0);
>  	} else {
>  		if (to_desc) {
>  			batch_copy[vq->batch_copy_nb_elems].dst =
>  				(void *)((uintptr_t)(buf_addr));
>  			batch_copy[vq->batch_copy_nb_elems].src =
>  				rte_pktmbuf_mtod_offset(m, void *,
> mbuf_offset);
> +			batch_copy[vq->batch_copy_nb_elems].log_addr =
> buf_iova;
> +			batch_copy[vq->batch_copy_nb_elems].len =
> cpy_len;
>  		} else {
>  			batch_copy[vq->batch_copy_nb_elems].dst =
>  				rte_pktmbuf_mtod_offset(m, void *,
> mbuf_offset);
>  			batch_copy[vq->batch_copy_nb_elems].src =
>  				(void *)((uintptr_t)(buf_addr));
>  		}
> -		batch_copy[vq->batch_copy_nb_elems].log_addr =
> buf_iova;
> -		batch_copy[vq->batch_copy_nb_elems].len = cpy_len;
>  		vq->batch_copy_nb_elems++;
>  	}
>  }
> @@ -2739,18 +2739,14 @@ desc_to_mbuf(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>  			if (async_fill_seg(dev, vq, cur, mbuf_offset,
>  					   buf_iova + buf_offset, cpy_len,
> false) < 0)
>  				goto error;
> +		} else if (likely(hdr && cur == m)) {
> +			rte_memcpy(rte_pktmbuf_mtod_offset(cur, void *,
> mbuf_offset),
> +				(void *)((uintptr_t)(buf_addr + buf_offset)),
> +				cpy_len);
>  		} else {
> -			if (hdr && cur == m) {
> -				rte_memcpy(rte_pktmbuf_mtod_offset(cur,
> void *, mbuf_offset),
> -					(void *)((uintptr_t)(buf_addr +
> buf_offset)),
> -					cpy_len);
> -				vhost_log_cache_write_iova(dev, vq,
> buf_iova + buf_offset, cpy_len);
> -				PRINT_PACKET(dev, (uintptr_t)(buf_addr +
> buf_offset), cpy_len, 0);
> -			} else {
> -				sync_fill_seg(dev, vq, cur, mbuf_offset,
> -					buf_addr + buf_offset,
> -					buf_iova + buf_offset, cpy_len, false);
> -			}
> +			sync_fill_seg(dev, vq, cur, mbuf_offset,
> +				      buf_addr + buf_offset,
> +				      buf_iova + buf_offset, cpy_len, false);
>  		}
> 
>  		mbuf_avail  -= cpy_len;
> @@ -2804,9 +2800,8 @@ desc_to_mbuf(struct virtio_net *dev, struct
> vhost_virtqueue *vq,
>  		async_iter_finalize(async);
>  		if (hdr)
>  			pkts_info[slot_idx].nethdr = *hdr;
> -	} else {
> -		if (hdr)
> -			vhost_dequeue_offload(dev, hdr, m,
> legacy_ol_flags);
> +	} else if (hdr) {
> +		vhost_dequeue_offload(dev, hdr, m, legacy_ol_flags);
>  	}
> 
>  	return 0;
> --
> 2.17.1

Reviewed-by: Jiayu Hu <jiayu.hu@intel.com>

Thanks,
Jiayu
Chenbo Xia July 7, 2022, 9:51 a.m. UTC | #2 
> -----Original Message-----
> From: Ding, Xuan <xuan.ding@intel.com>
> Sent: Thursday, July 7, 2022 2:55 PM
> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>; Jiang,
> Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
> Subject: [PATCH] vhost: fix unnecessary dirty page logging
> 
> From: Xuan Ding <xuan.ding@intel.com>
> 
> The dirty page logging is only required in vhost enqueue direction for
> live migration. This patch removes the unnecessary dirty page logging
> in vhost dequeue direction. Otherwise, it will result in a performance
> drop. Some if-else judgements are also optimized to improve performance.
> 
> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
> refactoring")
> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> 
> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> ---

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

Although it's late in release, we can consider to merge this as I see it impacts
the performance by 5%:
http://inbox.dpdk.org/dev/BYAPR11MB2711F13CDA2B0A4535A6591EFE839@BYAPR11MB2711.namprd11.prod.outlook.com/T/#t

But also, it will be good to know the performance issue is solved by sharing the
test results.

Thanks,
Chenbo
Maxime Coquelin July 7, 2022, 11:30 a.m. UTC | #3 
On 7/7/22 11:51, Xia, Chenbo wrote:
>> -----Original Message-----
>> From: Ding, Xuan <xuan.ding@intel.com>
>> Sent: Thursday, July 7, 2022 2:55 PM
>> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
>> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>; Jiang,
>> Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
>> Subject: [PATCH] vhost: fix unnecessary dirty page logging
>>
>> From: Xuan Ding <xuan.ding@intel.com>
>>
>> The dirty page logging is only required in vhost enqueue direction for
>> live migration. This patch removes the unnecessary dirty page logging
>> in vhost dequeue direction. Otherwise, it will result in a performance
>> drop. Some if-else judgements are also optimized to improve performance.
>>
>> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
>> refactoring")
>> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
>>
>> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
>> ---
> 
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> 
> Although it's late in release, we can consider to merge this as I see it impacts
> the performance by 5%:
> http://inbox.dpdk.org/dev/BYAPR11MB2711F13CDA2B0A4535A6591EFE839@BYAPR11MB2711.namprd11.prod.outlook.com/T/#t

Yes, I raised we need it in -rc4 at today's Release status meeting.
I'll review it today.

> But also, it will be good to know the performance issue is solved by sharing the
> test results.

Yes, Intel performance results would be appreciated.

> Thanks,
> Chenbo
> 

Thanks,
Maxime
Maxime Coquelin July 7, 2022, 12:04 p.m. UTC | #4 
On 7/7/22 08:55, xuan.ding@intel.com wrote:
> From: Xuan Ding <xuan.ding@intel.com>
> 
> The dirty page logging is only required in vhost enqueue direction for
> live migration. This patch removes the unnecessary dirty page logging
> in vhost dequeue direction. Otherwise, it will result in a performance
> drop. Some if-else judgements are also optimized to improve performance.
> 
> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf refactoring")
> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> 
> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> ---
>   lib/vhost/virtio_net.c | 31 +++++++++++++------------------
>   1 file changed, 13 insertions(+), 18 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
Ding, Xuan July 8, 2022, 7:04 a.m. UTC | #5 
Hi,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2022年7月7日 19:31
> To: Xia, Chenbo <chenbo.xia@intel.com>; Ding, Xuan <xuan.ding@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>; Jiang,
> Cheng1 <cheng1.jiang@intel.com>
> Subject: Re: [PATCH] vhost: fix unnecessary dirty page logging
> 
> 
> 
> On 7/7/22 11:51, Xia, Chenbo wrote:
> >> -----Original Message-----
> >> From: Ding, Xuan <xuan.ding@intel.com>
> >> Sent: Thursday, July 7, 2022 2:55 PM
> >> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> >> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> >> Jiang,
> >> Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
> >> Subject: [PATCH] vhost: fix unnecessary dirty page logging
> >>
> >> From: Xuan Ding <xuan.ding@intel.com>
> >>
> >> The dirty page logging is only required in vhost enqueue direction
> >> for live migration. This patch removes the unnecessary dirty page
> >> logging in vhost dequeue direction. Otherwise, it will result in a
> >> performance drop. Some if-else judgements are also optimized to improve
> performance.
> >>
> >> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
> >> refactoring")
> >> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> >>
> >> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> >> ---
> >
> > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> >
> > Although it's late in release, we can consider to merge this as I see
> > it impacts the performance by 5%:
> >
> http://inbox.dpdk.org/dev/BYAPR11MB2711F13CDA2B0A4535A6591EFE839@B
> YAPR
> > 11MB2711.namprd11.prod.outlook.com/T/#t
> 
> Yes, I raised we need it in -rc4 at today's Release status meeting.
> I'll review it today.
> 
> > But also, it will be good to know the performance issue is solved by
> > sharing the test results.
> 
> Yes, Intel performance results would be appreciated. 

This fix patch is for the issue reported at 22.07-rc3. The refactoring patch brings a 3%~5% perf drop in vhost sync path.
With fix patch, the perf drop introduced by refactoring is solved.

However, the testing result shows there still exists ~5% packed ring perf drop compared with 22.03.
We find the improving checksum offload patch series in 22.07 may contribute to the packed ring perf drop.
Because we always do checksum checks in PMD.

Could you help to double check this patch series, is it as expected?
Your assistance is really appreciated.

Regards,
Xuan

> 
> > Thanks,
> > Chenbo
> >
> 
> Thanks,
> Maxime
He, Xingguang July 8, 2022, 7:53 a.m. UTC | #6 
> -----Original Message-----
> From: Ding, Xuan <xuan.ding@intel.com>
> Sent: Friday, July 8, 2022 3:04 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> Jiang, Cheng1 <cheng1.jiang@intel.com>
> Subject: RE: [PATCH] vhost: fix unnecessary dirty page logging
> 
> Hi,
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: 2022年7月7日 19:31
> > To: Xia, Chenbo <chenbo.xia@intel.com>; Ding, Xuan
> > <xuan.ding@intel.com>
> > Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> > Jiang,
> > Cheng1 <cheng1.jiang@intel.com>
> > Subject: Re: [PATCH] vhost: fix unnecessary dirty page logging
> >
> >
> >
> > On 7/7/22 11:51, Xia, Chenbo wrote:
> > >> -----Original Message-----
> > >> From: Ding, Xuan <xuan.ding@intel.com>
> > >> Sent: Thursday, July 7, 2022 2:55 PM
> > >> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> > >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > >> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> > >> Jiang,
> > >> Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
> > >> Subject: [PATCH] vhost: fix unnecessary dirty page logging
> > >>
> > >> From: Xuan Ding <xuan.ding@intel.com>
> > >>
> > >> The dirty page logging is only required in vhost enqueue direction
> > >> for live migration. This patch removes the unnecessary dirty page
> > >> logging in vhost dequeue direction. Otherwise, it will result in a
> > >> performance drop. Some if-else judgements are also optimized to
> > >> improve
> > performance.
> > >>
> > >> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
> > >> refactoring")
> > >> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> > >>
> > >> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> > >> ---
> > >
> > > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> > >
> > > Although it's late in release, we can consider to merge this as I
> > > see it impacts the performance by 5%:
> > >
> >
> http://inbox.dpdk.org/dev/BYAPR11MB2711F13CDA2B0A4535A6591EFE839
> @B
> > YAPR
> > > 11MB2711.namprd11.prod.outlook.com/T/#t
> >
> > Yes, I raised we need it in -rc4 at today's Release status meeting.
> > I'll review it today.
> >
> > > But also, it will be good to know the performance issue is solved by
> > > sharing the test results.
> >
> > Yes, Intel performance results would be appreciated.
> 
> This fix patch is for the issue reported at 22.07-rc3. The refactoring patch
> brings a 3%~5% perf drop in vhost sync path.
> With fix patch, the perf drop introduced by refactoring is solved.

The fix is for a critical issue, I tested dpdk22.07-rc3 with the patch, 
the performance drop introduced by refactoring have been fixed 
and no other regressions were seen as a result of the patch, 
but the packed ring still exist drop around 5%.  

Tested-by: Xingguang He<xingguang.he@intel.com>

> 
> However, the testing result shows there still exists ~5% packed ring perf drop
> compared with 22.03.
> We find the improving checksum offload patch series in 22.07 may contribute
> to the packed ring perf drop.
> Because we always do checksum checks in PMD.

I tested the performance of Vhost before and after checksum offload patch series, 
there exist  ~5% performance drop.

> 
> Could you help to double check this patch series, is it as expected?
> Your assistance is really appreciated.
> 
> Regards,
> Xuan
> 
> >
> > > Thanks,
> > > Chenbo
> > >
> >
> > Thanks,
> > Maxime
Maxime Coquelin July 8, 2022, 7:57 a.m. UTC | #7 
Hi,

On 7/8/22 09:04, Ding, Xuan wrote:
> Hi,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: 2022年7月7日 19:31
>> To: Xia, Chenbo <chenbo.xia@intel.com>; Ding, Xuan <xuan.ding@intel.com>
>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
>> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>; Jiang,
>> Cheng1 <cheng1.jiang@intel.com>
>> Subject: Re: [PATCH] vhost: fix unnecessary dirty page logging
>>
>>
>>
>> On 7/7/22 11:51, Xia, Chenbo wrote:
>>>> -----Original Message-----
>>>> From: Ding, Xuan <xuan.ding@intel.com>
>>>> Sent: Thursday, July 7, 2022 2:55 PM
>>>> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>>>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
>>>> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
>>>> Jiang,
>>>> Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
>>>> Subject: [PATCH] vhost: fix unnecessary dirty page logging
>>>>
>>>> From: Xuan Ding <xuan.ding@intel.com>
>>>>
>>>> The dirty page logging is only required in vhost enqueue direction
>>>> for live migration. This patch removes the unnecessary dirty page
>>>> logging in vhost dequeue direction. Otherwise, it will result in a
>>>> performance drop. Some if-else judgements are also optimized to improve
>> performance.
>>>>
>>>> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
>>>> refactoring")
>>>> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
>>>>
>>>> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
>>>> ---
>>>
>>> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
>>>
>>> Although it's late in release, we can consider to merge this as I see
>>> it impacts the performance by 5%:
>>>
>> http://inbox.dpdk.org/dev/BYAPR11MB2711F13CDA2B0A4535A6591EFE839@B
>> YAPR
>>> 11MB2711.namprd11.prod.outlook.com/T/#t
>>
>> Yes, I raised we need it in -rc4 at today's Release status meeting.
>> I'll review it today.
>>
>>> But also, it will be good to know the performance issue is solved by
>>> sharing the test results.
>>
>> Yes, Intel performance results would be appreciated.
> 
> This fix patch is for the issue reported at 22.07-rc3. The refactoring patch brings a 3%~5% perf drop in vhost sync path.
> With fix patch, the perf drop introduced by refactoring is solved.

Good.

> However, the testing result shows there still exists ~5% packed ring perf drop compared with 22.03.
> We find the improving checksum offload patch series in 22.07 may contribute to the packed ring perf drop.
> Because we always do checksum checks in PMD.
> 
> Could you help to double check this patch series, is it as expected?
> Your assistance is really appreciated.

I will look again, but if your analysis is right, there's not much we
can do. The series is filling gaps in the checksum offload support.

I will not have time to work on it for v22.07, we're too late in the
cycle. What surprises me is that only packed ring is impacted.

Regards,
Maxime

> 
> Regards,
> Xuan
> 
>>
>>> Thanks,
>>> Chenbo
>>>
>>
>> Thanks,
>> Maxime
>
Ding, Xuan July 8, 2022, 8:09 a.m. UTC | #8 
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2022年7月8日 15:58
> To: Ding, Xuan <xuan.ding@intel.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>; Jiang,
> Cheng1 <cheng1.jiang@intel.com>
> Subject: Re: [PATCH] vhost: fix unnecessary dirty page logging
> 
> Hi,
> 
> On 7/8/22 09:04, Ding, Xuan wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: 2022年7月7日 19:31
> >> To: Xia, Chenbo <chenbo.xia@intel.com>; Ding, Xuan
> >> <xuan.ding@intel.com>
> >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> >> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> >> Jiang,
> >> Cheng1 <cheng1.jiang@intel.com>
> >> Subject: Re: [PATCH] vhost: fix unnecessary dirty page logging
> >>
> >>
> >>
> >> On 7/7/22 11:51, Xia, Chenbo wrote:
> >>>> -----Original Message-----
> >>>> From: Ding, Xuan <xuan.ding@intel.com>
> >>>> Sent: Thursday, July 7, 2022 2:55 PM
> >>>> To: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> >>>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> >>>> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> >>>> Jiang,
> >>>> Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
> >>>> Subject: [PATCH] vhost: fix unnecessary dirty page logging
> >>>>
> >>>> From: Xuan Ding <xuan.ding@intel.com>
> >>>>
> >>>> The dirty page logging is only required in vhost enqueue direction
> >>>> for live migration. This patch removes the unnecessary dirty page
> >>>> logging in vhost dequeue direction. Otherwise, it will result in a
> >>>> performance drop. Some if-else judgements are also optimized to
> >>>> improve
> >> performance.
> >>>>
> >>>> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
> >>>> refactoring")
> >>>> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> >>>>
> >>>> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> >>>> ---
> >>>
> >>> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> >>>
> >>> Although it's late in release, we can consider to merge this as I
> >>> see it impacts the performance by 5%:
> >>>
> >>
> http://inbox.dpdk.org/dev/BYAPR11MB2711F13CDA2B0A4535A6591EFE839@B
> >> YAPR
> >>> 11MB2711.namprd11.prod.outlook.com/T/#t
> >>
> >> Yes, I raised we need it in -rc4 at today's Release status meeting.
> >> I'll review it today.
> >>
> >>> But also, it will be good to know the performance issue is solved by
> >>> sharing the test results.
> >>
> >> Yes, Intel performance results would be appreciated.
> >
> > This fix patch is for the issue reported at 22.07-rc3. The refactoring patch
> brings a 3%~5% perf drop in vhost sync path.
> > With fix patch, the perf drop introduced by refactoring is solved.
> 
> Good.
> 
> > However, the testing result shows there still exists ~5% packed ring perf drop
> compared with 22.03.
> > We find the improving checksum offload patch series in 22.07 may contribute
> to the packed ring perf drop.
> > Because we always do checksum checks in PMD.
> >
> > Could you help to double check this patch series, is it as expected?
> > Your assistance is really appreciated.
> 
> I will look again, but if your analysis is right, there's not much we can do. The
> series is filling gaps in the checksum offload support.
 
Yes, I think so.

> 
> I will not have time to work on it for v22.07, we're too late in the cycle. What
> surprises me is that only packed ring is impacted.
 
From the testing results, the packed ring is more sensitive to the patch series,
while split ring shows a much lower drop in performance.

Thanks for your support!

Regards,
Xuan

> 
> Regards,
> Maxime
> 
> >
> > Regards,
> > Xuan
> >
> >>
> >>> Thanks,
> >>> Chenbo
> >>>
> >>
> >> Thanks,
> >> Maxime
> >
He, Xingguang July 8, 2022, 8:13 a.m. UTC | #9 
> -----Original Message-----
> From: He, Xingguang <xingguang.he@intel.com>
> Sent: Friday, July 8, 2022 3:53 PM
> To: Ding, Xuan <xuan.ding@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>; Jiang, Cheng1 <cheng1.jiang@intel.com>
> Subject: RE: [PATCH] vhost: fix unnecessary dirty page logging
> 
> > -----Original Message-----
> > From: Ding, Xuan <xuan.ding@intel.com>
> > Sent: Friday, July 8, 2022 3:04 PM
> > To: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> > <chenbo.xia@intel.com>
> > Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> > Jiang, Cheng1 <cheng1.jiang@intel.com>
> > Subject: RE: [PATCH] vhost: fix unnecessary dirty page logging
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > Sent: 2022年7月7日 19:31
> > > To: Xia, Chenbo <chenbo.xia@intel.com>; Ding, Xuan
> > > <xuan.ding@intel.com>
> > > Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > > <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> > > Jiang,
> > > Cheng1 <cheng1.jiang@intel.com>
> > > Subject: Re: [PATCH] vhost: fix unnecessary dirty page logging
> > >
> > >
> > >
> > > On 7/7/22 11:51, Xia, Chenbo wrote:
> > > >> -----Original Message-----
> > > >> From: Ding, Xuan <xuan.ding@intel.com>
> > > >> Sent: Thursday, July 7, 2022 2:55 PM
> > > >> To: maxime.coquelin@redhat.com; Xia, Chenbo
> > > >> <chenbo.xia@intel.com>
> > > >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > > >> <xingguang.he@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>;
> > > >> Jiang,
> > > >> Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
> > > >> Subject: [PATCH] vhost: fix unnecessary dirty page logging
> > > >>
> > > >> From: Xuan Ding <xuan.ding@intel.com>
> > > >>
> > > >> The dirty page logging is only required in vhost enqueue
> > > >> direction for live migration. This patch removes the unnecessary
> > > >> dirty page logging in vhost dequeue direction. Otherwise, it will
> > > >> result in a performance drop. Some if-else judgements are also
> > > >> optimized to improve
> > > performance.
> > > >>
> > > >> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
> > > >> refactoring")
> > > >> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> > > >>
> > > >> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> > > >> ---
> > > >
> > > > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> > > >
> > > > Although it's late in release, we can consider to merge this as I
> > > > see it impacts the performance by 5%:
> > > >
> > >
> >
> http://inbox.dpdk.org/dev/BYAPR11MB2711F13CDA2B0A4535A6591EFE839
> > @B
> > > YAPR
> > > > 11MB2711.namprd11.prod.outlook.com/T/#t
> > >
> > > Yes, I raised we need it in -rc4 at today's Release status meeting.
> > > I'll review it today.
> > >
> > > > But also, it will be good to know the performance issue is solved
> > > > by sharing the test results.
> > >
> > > Yes, Intel performance results would be appreciated.
> >
> > This fix patch is for the issue reported at 22.07-rc3. The refactoring
> > patch brings a 3%~5% perf drop in vhost sync path.
> > With fix patch, the perf drop introduced by refactoring is solved.
> 
> The fix is for a critical issue, I tested dpdk22.07-rc3 with the patch, the
> performance drop introduced by refactoring have been fixed and no other
> regressions were seen as a result of the patch, but the packed ring still exist
> drop around 5%.
> 
> Tested-by: Xingguang He<xingguang.he@intel.com>
> 
> >
> > However, the testing result shows there still exists ~5% packed ring
> > perf drop compared with 22.03.
> > We find the improving checksum offload patch series in 22.07 may
> > contribute to the packed ring perf drop.
> > Because we always do checksum checks in PMD.
> 
> I tested the performance of Vhost before and after checksum offload patch
> series, there exist  ~5% performance drop.

Sorry, the drop of checksum offload patch series is ~3%. 
The drop between DPDK22.03 and DPDK22.07-rc3 is ~5%.  

> 
> >
> > Could you help to double check this patch series, is it as expected?
> > Your assistance is really appreciated.
> >
> > Regards,
> > Xuan
> >
> > >
> > > > Thanks,
> > > > Chenbo
> > > >
> > >
> > > Thanks,
> > > Maxime
Maxime Coquelin July 8, 2022, 9:11 a.m. UTC | #10 
On 7/7/22 08:55, xuan.ding@intel.com wrote:
> From: Xuan Ding <xuan.ding@intel.com>
> 
> The dirty page logging is only required in vhost enqueue direction for
> live migration. This patch removes the unnecessary dirty page logging
> in vhost dequeue direction. Otherwise, it will result in a performance
> drop. Some if-else judgements are also optimized to improve performance.
> 
> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf refactoring")
> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> 
> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> ---
>   lib/vhost/virtio_net.c | 31 +++++++++++++------------------
>   1 file changed, 13 insertions(+), 18 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime
He, Xingguang July 11, 2022, 8:09 a.m. UTC | #11 
Hi,

> -----Original Message-----
> From: Ding, Xuan <xuan.ding@intel.com>
> Sent: Friday, July 8, 2022 4:09 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> Jiang, Cheng1 <cheng1.jiang@intel.com>
> Subject: RE: [PATCH] vhost: fix unnecessary dirty page logging
> 
> 
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: 2022年7月8日 15:58
> > To: Ding, Xuan <xuan.ding@intel.com>; Xia, Chenbo
> > <chenbo.xia@intel.com>
> > Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> > Jiang,
> > Cheng1 <cheng1.jiang@intel.com>
> > Subject: Re: [PATCH] vhost: fix unnecessary dirty page logging
> >
> > Hi,
> >
> > On 7/8/22 09:04, Ding, Xuan wrote:
> > > Hi,
> > >
> > >> -----Original Message-----
> > >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > >> Sent: 2022年7月7日 19:31
> > >> To: Xia, Chenbo <chenbo.xia@intel.com>; Ding, Xuan
> > >> <xuan.ding@intel.com>
> > >> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > >> <xingguang.he@intel.com>; Yang, YvonneX <yvonnex.yang@intel.com>;
> > >> Jiang,
> > >> Cheng1 <cheng1.jiang@intel.com>
> > >> Subject: Re: [PATCH] vhost: fix unnecessary dirty page logging
> > >>
> > >>
> > >>
> > >> On 7/7/22 11:51, Xia, Chenbo wrote:
> > >>>> -----Original Message-----
> > >>>> From: Ding, Xuan <xuan.ding@intel.com>
> > >>>> Sent: Thursday, July 7, 2022 2:55 PM
> > >>>> To: maxime.coquelin@redhat.com; Xia, Chenbo
> > >>>> <chenbo.xia@intel.com>
> > >>>> Cc: dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>; He, Xingguang
> > >>>> <xingguang.he@intel.com>; Yang, YvonneX
> <yvonnex.yang@intel.com>;
> > >>>> Jiang,
> > >>>> Cheng1 <cheng1.jiang@intel.com>; Ding, Xuan <xuan.ding@intel.com>
> > >>>> Subject: [PATCH] vhost: fix unnecessary dirty page logging
> > >>>>
> > >>>> From: Xuan Ding <xuan.ding@intel.com>
> > >>>>
> > >>>> The dirty page logging is only required in vhost enqueue
> > >>>> direction for live migration. This patch removes the unnecessary
> > >>>> dirty page logging in vhost dequeue direction. Otherwise, it will
> > >>>> result in a performance drop. Some if-else judgements are also
> > >>>> optimized to improve
> > >> performance.
> > >>>>
> > >>>> Fixes: 6d823bb302c7 ("vhost: prepare sync for descriptor to mbuf
> > >>>> refactoring")
> > >>>> Fixes: b6eee3e83402 ("vhost: fix sync dequeue offload")
> > >>>>
> > >>>> Signed-off-by: Xuan Ding <xuan.ding@intel.com>
> > >>>> ---
> > >>>
> > >>> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> > >>>
> > >>> Although it's late in release, we can consider to merge this as I
> > >>> see it impacts the performance by 5%:
> > >>>
> > >>
> >
> http://inbox.dpdk.org/dev/BYAPR11MB2711F13CDA2B0A4535A6591EFE839
> @B
> > >> YAPR
> > >>> 11MB2711.namprd11.prod.outlook.com/T/#t
> > >>
> > >> Yes, I raised we need it in -rc4 at today's Release status meeting.
> > >> I'll review it today.
> > >>
> > >>> But also, it will be good to know the performance issue is solved
> > >>> by sharing the test results.
> > >>
> > >> Yes, Intel performance results would be appreciated.
> > >
> > > This fix patch is for the issue reported at 22.07-rc3. The
> > > refactoring patch
> > brings a 3%~5% perf drop in vhost sync path.
> > > With fix patch, the perf drop introduced by refactoring is solved.
> >
> > Good.
> >
> > > However, the testing result shows there still exists ~5% packed ring
> > > perf drop
> > compared with 22.03.
> > > We find the improving checksum offload patch series in 22.07 may
> > > contribute
> > to the packed ring perf drop.
> > > Because we always do checksum checks in PMD.
> > >
> > > Could you help to double check this patch series, is it as expected?
> > > Your assistance is really appreciated.
> >
> > I will look again, but if your analysis is right, there's not much we
> > can do. The series is filling gaps in the checksum offload support.

What is the result of your investigation? As dpdk-22.07 will be released soon, 
I'd like to know your advice about this issue. Thanks.

> 
> Yes, I think so.
> 
> >
> > I will not have time to work on it for v22.07, we're too late in the
> > cycle. What surprises me is that only packed ring is impacted.
> 
> From the testing results, the packed ring is more sensitive to the patch series,
> while split ring shows a much lower drop in performance.
> 
> Thanks for your support!
> 

Regards,
Xingguang

> Regards,
> Xuan
> 
> >
> > Regards,
> > Maxime
> >
> > >
> > > Regards,
> > > Xuan
> > >
> > >>
> > >>> Thanks,
> > >>> Chenbo
> > >>>
> > >>
> > >> Thanks,
> > >> Maxime
> > >
diff mbox series

Patch

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index e842c35fef..12b7fbe7f9 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1113,27 +1113,27 @@  sync_fill_seg(struct virtio_net *dev, struct vhost_virtqueue *vq,
 			rte_memcpy((void *)((uintptr_t)(buf_addr)),
 				rte_pktmbuf_mtod_offset(m, void *, mbuf_offset),
 				cpy_len);
+			vhost_log_cache_write_iova(dev, vq, buf_iova, cpy_len);
+			PRINT_PACKET(dev, (uintptr_t)(buf_addr), cpy_len, 0);
 		} else {
 			rte_memcpy(rte_pktmbuf_mtod_offset(m, void *, mbuf_offset),
 				(void *)((uintptr_t)(buf_addr)),
 				cpy_len);
 		}
-		vhost_log_cache_write_iova(dev, vq, buf_iova, cpy_len);
-		PRINT_PACKET(dev, (uintptr_t)(buf_addr), cpy_len, 0);
 	} else {
 		if (to_desc) {
 			batch_copy[vq->batch_copy_nb_elems].dst =
 				(void *)((uintptr_t)(buf_addr));
 			batch_copy[vq->batch_copy_nb_elems].src =
 				rte_pktmbuf_mtod_offset(m, void *, mbuf_offset);
+			batch_copy[vq->batch_copy_nb_elems].log_addr = buf_iova;
+			batch_copy[vq->batch_copy_nb_elems].len = cpy_len;
 		} else {
 			batch_copy[vq->batch_copy_nb_elems].dst =
 				rte_pktmbuf_mtod_offset(m, void *, mbuf_offset);
 			batch_copy[vq->batch_copy_nb_elems].src =
 				(void *)((uintptr_t)(buf_addr));
 		}
-		batch_copy[vq->batch_copy_nb_elems].log_addr = buf_iova;
-		batch_copy[vq->batch_copy_nb_elems].len = cpy_len;
 		vq->batch_copy_nb_elems++;
 	}
 }
@@ -2739,18 +2739,14 @@  desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 			if (async_fill_seg(dev, vq, cur, mbuf_offset,
 					   buf_iova + buf_offset, cpy_len, false) < 0)
 				goto error;
+		} else if (likely(hdr && cur == m)) {
+			rte_memcpy(rte_pktmbuf_mtod_offset(cur, void *, mbuf_offset),
+				(void *)((uintptr_t)(buf_addr + buf_offset)),
+				cpy_len);
 		} else {
-			if (hdr && cur == m) {
-				rte_memcpy(rte_pktmbuf_mtod_offset(cur, void *, mbuf_offset),
-					(void *)((uintptr_t)(buf_addr + buf_offset)),
-					cpy_len);
-				vhost_log_cache_write_iova(dev, vq, buf_iova + buf_offset, cpy_len);
-				PRINT_PACKET(dev, (uintptr_t)(buf_addr + buf_offset), cpy_len, 0);
-			} else {
-				sync_fill_seg(dev, vq, cur, mbuf_offset,
-					buf_addr + buf_offset,
-					buf_iova + buf_offset, cpy_len, false);
-			}
+			sync_fill_seg(dev, vq, cur, mbuf_offset,
+				      buf_addr + buf_offset,
+				      buf_iova + buf_offset, cpy_len, false);
 		}
 
 		mbuf_avail  -= cpy_len;
@@ -2804,9 +2800,8 @@  desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		async_iter_finalize(async);
 		if (hdr)
 			pkts_info[slot_idx].nethdr = *hdr;
-	} else {
-		if (hdr)
-			vhost_dequeue_offload(dev, hdr, m, legacy_ol_flags);
+	} else if (hdr) {
+		vhost_dequeue_offload(dev, hdr, m, legacy_ol_flags);
 	}
 
 	return 0;