mbox series

[RFC,00/14] vhost: clean-up and simplify async implementation

Message ID 20211007220013.355530-1-maxime.coquelin@redhat.com (mailing list archive)
Headers
Series vhost: clean-up and simplify async implementation |

Message

Maxime Coquelin Oct. 7, 2021, 9:59 p.m. UTC
  This series aims at cleaning and simplifying the async
enqueue path. I think it makes the code easier to
understand, and is necessary before integrating new
changes.

I may have more reworks to propose in next revisions,
but I wanted to share my current status early so that
you have time to review/test it.

It is only compile-tested, as I don't have HW with IOAT
support to test it.

Maxime Coquelin (14):
  vhost: move async data in a dedicated structure
  vhost: hide inflight async structure
  vhost: simplify async IO vectors
  vhost: simplify async IO vectors iterators
  vhost: remove async batch threshold
  vhost: introduce specific iovec structure
  vhost: remove useless fields in async iterator struct
  vhost: improve IO vector logic
  vhost: remove notion of async descriptor
  vhost: simplify async enqueue completion
  vhost: simplify getting the first in-flight index
  vhost: prepare async for mbuf to desc refactoring
  vhost: prepare sync for mbuf to desc refactoring
  vhost: merge sync and async mbuf to desc filling

 examples/vhost/ioat.c       |  30 +-
 examples/vhost/ioat.h       |   2 +-
 lib/vhost/rte_vhost_async.h |  42 +--
 lib/vhost/vhost.c           | 129 +++----
 lib/vhost/vhost.h           |  67 ++--
 lib/vhost/vhost_user.c      |   4 +-
 lib/vhost/virtio_net.c      | 697 ++++++++++++++----------------------
 7 files changed, 379 insertions(+), 592 deletions(-)
  

Comments

David Marchand Oct. 8, 2021, 12:36 p.m. UTC | #1
On Fri, Oct 8, 2021 at 12:13 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This series aims at cleaning and simplifying the async
> enqueue path. I think it makes the code easier to
> understand, and is necessary before integrating new
> changes.
>
> I may have more reworks to propose in next revisions,

There is some consolidation that could be done in split/packed
sync/async rx helpers too.
Is this what you have in mind?


> but I wanted to share my current status early so that
> you have time to review/test it.
>
> It is only compile-tested, as I don't have HW with IOAT
> support to test it.
>
> Maxime Coquelin (14):
>   vhost: move async data in a dedicated structure
>   vhost: hide inflight async structure
>   vhost: simplify async IO vectors
>   vhost: simplify async IO vectors iterators
>   vhost: remove async batch threshold
>   vhost: introduce specific iovec structure
>   vhost: remove useless fields in async iterator struct
>   vhost: improve IO vector logic
>   vhost: remove notion of async descriptor
>   vhost: simplify async enqueue completion
>   vhost: simplify getting the first in-flight index
>   vhost: prepare async for mbuf to desc refactoring
>   vhost: prepare sync for mbuf to desc refactoring
>   vhost: merge sync and async mbuf to desc filling
>
>  examples/vhost/ioat.c       |  30 +-
>  examples/vhost/ioat.h       |   2 +-
>  lib/vhost/rte_vhost_async.h |  42 +--
>  lib/vhost/vhost.c           | 129 +++----
>  lib/vhost/vhost.h           |  67 ++--
>  lib/vhost/vhost_user.c      |   4 +-
>  lib/vhost/virtio_net.c      | 697 ++++++++++++++----------------------
>  7 files changed, 379 insertions(+), 592 deletions(-)

Just had a quick look.
Nice to shrink vq memory footprint again.
And less code is always better.

+1 from me.
  
Hu, Jiayu Oct. 12, 2021, 6:24 a.m. UTC | #2
Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, October 8, 2021 6:00 AM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Hu, Jiayu
> <jiayu.hu@intel.com>; Wang, YuanX <yuanx.wang@intel.com>; Ma,
> WenwuX <wenwux.ma@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Mcnamara, John
> <john.mcnamara@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [RFC 00/14] vhost: clean-up and simplify async implementation
> 
> This series aims at cleaning and simplifying the async enqueue path. I think it
> makes the code easier to understand, and is necessary before integrating
> new changes.

I really appreciate those changes, and the code looks better, especially for
improving io vector logic, as the existed implementation has a potential risk
of OOB reads/writes.

About the fifth patch, removing async batch threshold, this change is OK for me,
as current vhost is packet level offloading and buffer level DMA batching copy
will be done inside the DMA callback, so no need to keep the batching threshold.
But when integrate DMA logic inside vhost, the async data path still needs to handle
buffer level batching, and some of deleted code may need to be recovered.

Thanks,
Jiayu
> 
> I may have more reworks to propose in next revisions, but I wanted to share
> my current status early so that you have time to review/test it.
> 
> It is only compile-tested, as I don't have HW with IOAT support to test it.
> 
> Maxime Coquelin (14):
>   vhost: move async data in a dedicated structure
>   vhost: hide inflight async structure
>   vhost: simplify async IO vectors
>   vhost: simplify async IO vectors iterators
>   vhost: remove async batch threshold
>   vhost: introduce specific iovec structure
>   vhost: remove useless fields in async iterator struct
>   vhost: improve IO vector logic
>   vhost: remove notion of async descriptor
>   vhost: simplify async enqueue completion
>   vhost: simplify getting the first in-flight index
>   vhost: prepare async for mbuf to desc refactoring
>   vhost: prepare sync for mbuf to desc refactoring
>   vhost: merge sync and async mbuf to desc filling
> 
>  examples/vhost/ioat.c       |  30 +-
>  examples/vhost/ioat.h       |   2 +-
>  lib/vhost/rte_vhost_async.h |  42 +--
>  lib/vhost/vhost.c           | 129 +++----
>  lib/vhost/vhost.h           |  67 ++--
>  lib/vhost/vhost_user.c      |   4 +-
>  lib/vhost/virtio_net.c      | 697 ++++++++++++++----------------------
>  7 files changed, 379 insertions(+), 592 deletions(-)
> 
> --
> 2.31.1