> -----Original Message----- > From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] > Sent: Wednesday, February 28, 2018 4:45 PM > To: Yang, Zhiyong; dev@dpdk.org; y...@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.coque...@redhat.com] > >> Sent: Wednesday, February 28, 2018 1:52 AM > >> To: Yang, Zhiyong <zhiyong.y...@intel.com>; dev@dpdk.org; > >> y...@fridaylinux.org; Tan, Jianfeng <jianfeng....@intel.com>; Bie, Tiwei > >> <tiwei....@intel.com>; Wang, Zhihong <zhihong.w...@intel.com> > >> Cc: Wang, Dong1 <dong1.w...@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.y...@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 > >
