Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Friday, July 23, 2021 3:25 PM
> To: maxime.coque...@redhat.com; Xia, Chenbo <chenbo....@intel.com>
> Cc: Jiang, Cheng1 <cheng1.ji...@intel.com>; dev@dpdk.org; Hu, Jiayu
> <jiayu...@intel.com>; Yang, YvonneX <yvonnex.y...@intel.com>;
> david.march...@redhat.com; Yigit, Ferruh <ferruh.yi...@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v7 0/5] vhost: handle memory hotplug for async
> vhost
>
> 23/07/2021 07:06, Xia, Chenbo:
> > From: Thomas Monjalon <tho...@monjalon.net>
> > > 22/07/2021 07:07, Xia, Chenbo:
> > > > From: Jiang, Cheng1 <cheng1.ji...@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 doc must be updated in each patch.
> Sometimes, the same line is updated to add a word related to the patch.
Thanks for the guidance!
>
> > > - 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?
>
> Read the comment at the beginning of the section, it explains
> how things must be sorted:
>
> Suggested order in release notes items:
> * Core libs (EAL, mempool, ring, mbuf, buses)
> * Device abstraction libs and PMDs (ordered alphabetically by vendor
> name)
> - ethdev (lib, PMDs)
> - cryptodev (lib, PMDs)
> - eventdev (lib, PMDs)
> - etc
> * Other libs
> * Apps, Examples, Tools (if significant)
>
> vhost is usually at the end of ethdev PMDs.
Oops.. I should notice it..
>
> > > 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.
>
> So you can improve doxygen and remove this part of the guide.
> The guide should be an overview, a tutorial and an internal design reference.
Make sense to me. For this patch, I suggest to keep the api doc in vhost guide.
Then I will send a patch to move them all if we all agree on this.
Thanks,
Chenbo
>
> > @Maxime What do you think?
>
>
