Re: [dpdk-dev] [PATCH v7 0/5] vhost: handle memory hotplug for async vhost

["Xia, Chenbo" <chenbo.xia@intel.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, July 23, 2021 12:13 AM
> To: Jiang, Cheng1 <cheng1.jiang@intel.com>; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: maxime.coquelin@redhat.com; dev@dpdk.org; Hu, Jiayu <jiayu.hu@intel.com>;
> Yang, YvonneX <yvonnex.yang@intel.com>; david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v7 0/5] vhost: handle memory hotplug for async
> vhost
> 
> 22/07/2021 07:07, Xia, Chenbo:
> > From: Jiang, Cheng1 <cheng1.jiang@intel.com>
> > > When the guest memory is hotplugged, the vhost application which
> > > enables DMA acceleration must stop DMA transfers before the vhost
> > > re-maps the guest memory.
> > >
> > > This patch set is to provide an unsafe API to drain inflight pkts
> > > which are submitted to DMA engine in vhost async data path, and
> > > notify the vhost application of stopping DMA transfers. And enable it
> > > in vhost example.
> >
> > Series applied to next-virtio/main. Thanks
> 
> I cannot pull this series in main branch.
> 
> There is a compilation error seen on Arm cross-compilation:
> 
> examples/vhost/main.c:1493:51: error: assignment to 'int32_t (*)(int,
> uint16_t,  struct rte_vhost_async_desc *, struct rte_vhost_async_status *,
> uint16_t)' {aka 'int (*)(int,  short unsigned int,  struct
> rte_vhost_async_desc *, struct rte_vhost_async_status *, short unsigned int)'}
> from incompatible pointer type 'uint32_t (*)(int,  uint16_t,  struct
> rte_vhost_async_desc *, struct rte_vhost_async_status *, uint16_t)' {aka
> 'unsigned int (*)(int,  short unsigned int,  struct rte_vhost_async_desc *,
> struct rte_vhost_async_status *, short unsigned int)'} [-Werror=incompatible-
> pointer-types]
>  1493 |                         channel_ops.transfer_data =
> ioat_transfer_data_cb;
>       |                                                   ^

I see. @Cheng, please fix it in new version.

> 
> Other comments about the last patch:
> - it is updating doc out of the original patch doing the code changes
> - there is not even a reference to the code patch (Fixes: line)

I think the doc patch could be combined with the code patch in the same series.
But personally, sometimes I am not very clear when doc patch should be split.
For example, in this case we can combine as the update in release note is related
only to the code patch. What if it's related to multiple patch? Should we split or
add doc changes to every related patches? Just a bit confused. Maybe you can give
me some general guidance so that we will be on the same page.

> - the addition in the release notes is not sorted

Not very clear on this. The change is put in the bottom. Is there any sorting
rules?

> 
> Last question while at it, why having the API documentation
> in the vhost guide (rst file)?
> Doxygen is not enough to describe the functions?

Good point. To be honest, I have not thought about it :P

I think it could be moved to the doxygen later (maybe in another patch). The only
concern of mine is some API description in the vhost guide is a bit long.

@Maxime What do you think?

Thanks,
Chenbo

>