Re: [dpdk-dev] [PATCH 1/4] vhost: move fdset functions from fd_man.c to fd_man.h

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, February 28, 2018 4:45 PM
> To: Yang, Zhiyong; dev@dpdk.org; yliu@fridaylinux.org; Tan, Jianfeng; Bie,
> Tiwei; Wang, Zhihong
> Cc: Wang, Dong1
> Subject: Re: [PATCH 1/4] vhost: move fdset functions from fd_man.c to
> fd_man.h
> 
> 
> 
> On 02/28/2018 02:36 AM, Yang, Zhiyong wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> >> Sent: Wednesday, February 28, 2018 1:52 AM
> >> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org;
> >> yliu@fridaylinux.org; Tan, Jianfeng <jianfeng.tan@intel.com>; Bie, Tiwei
> >> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>
> >> Cc: Wang, Dong1 <dong1.wang@intel.com>
> >> Subject: Re: [PATCH 1/4] vhost: move fdset functions from fd_man.c to
> >> fd_man.h
> >>
> >> Hi Zhiyong,
> >>
> >> On 02/14/2018 03:53 PM, Zhiyong Yang wrote:
> >>> The patch moves fdset related funcitons from fd_man.c to fd_man.h in
> >>> order to reuse these funcitons from the perspective of PMDs.
> >>>
> >>> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
> >>> ---
> >>>    lib/librte_vhost/Makefile |   3 +-
> >>>    lib/librte_vhost/fd_man.c | 274 ----------------------------------------------
> >>>    lib/librte_vhost/fd_man.h | 258
> >> +++++++++++++++++++++++++++++++++++++++++--
> >>>    3 files changed, 253 insertions(+), 282 deletions(-)
> >>>    delete mode 100644 lib/librte_vhost/fd_man.c
> >>
> >> I disagree with the patch.
> >> It is a good thing to reuse the code, but to do it, you need to extend the
> >> vhost lib API.
> >>
> >> New API need to be prefixed with rte_vhost_, and be declared in
> >> rte_vhost.h.
> >>
> >> And no need to move the functions from the .c to the .h file, as it
> moreover
> >> makes you inline them, which is not necessary here.
> >
> > Thanks for your reviewing the series firstly, Maxime. :)
> >
> > I considered to do it as you said. However I still preferred this one at last.
> > Here are my reasons.
> > 1) As far as I know, this set of functions are used privately in librte_vhost
> before this feature.
> > No strong request from the perspective of DPDK application. If I
> understand well,  It is enough to expose the functions to all PMDs
> > And it is better to keep internal use in DPDK.
> 
> But what the patch is doing is adding fd_man.h to the API, without doing
> it properly. fd_man.h will be installed with other header files, and any
> external application can use it.
> 
> >
> > 2) These functions help to implement vhost user, but they are not strongly
> related to other APIs of vhost user which have already exposed.
> > if we want to expose them as APIs at lib layer, many functions and related
> data structure has to be exposed in rte_vhost.h. it looks messy.
> > Your opinion?
> 
> Yes, it is not really vhost-related, it could be part of a more generic
> library. It is maybe better to duplicate these lines, or to move this
> code in a existing or new library.

I vote to move it to generic library, maybe eal. Poll() has better compatibility even though poll() is not as performant as epoll().

Thomas, how do you think?

Thanks,
Jianfeng 

> 
> Cheers,
> Maxime
> 
> > thanks
> > Zhiyong
> >