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

[Thomas Monjalon <thomas@monjalon.net>]

23/07/2021 07:06, Xia, Chenbo:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 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 doc must be updated in each patch.
Sometimes, the same line is updated to add a word related to the patch.

> > - 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.

> > 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.

> @Maxime What do you think?