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

[Yang, Zhiyong]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net]
> Sent: Thursday, March 1, 2018 10:14 PM
> To: Tan, Jianfeng <jianfeng.tan at intel.com>
> Cc: Maxime Coquelin <maxime.coquelin at redhat.com>; Yang, Zhiyong
> <zhiyong.yang at intel.com>; dev at dpdk.org; yliu at fridaylinux.org; Bie, Tiwei
> <tiwei.bie at intel.com>; Wang, Zhihong <zhihong.wang at intel.com>; Wang,
> Dong1 <dong1.wang at intel.com>
> Subject: Re: [PATCH 1/4] vhost: move fdset functions from fd_man.c to
> fd_man.h
> 
> 01/03/2018 07:02, Tan, Jianfeng:
> > From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> > > On 02/28/2018 02:36 AM, Yang, Zhiyong wrote:
> > > > From: Maxime Coquelin [mailto:maxime.coquelin at redhat.com]
> > > >> On 02/14/2018 03:53 PM, Zhiyong Yang wrote:
> > > >>>    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?
> 
> I don't see why it should be exported outside of DPDK, except for PMDs.
> I would tend to keep it internal but I understand that it would mean
> duplicating some code, which is not ideal.
> Please could you show what would be the content of the .h in EAL?
> 

If needed to expose them in eal.h, 
I think that they should be the whole fdset mechanism as followings.

typedef void (*fd_cb)(int fd, void *dat, int *remove);

struct fdentry {
	int fd;		/* -1 indicates this entry is empty */
	fd_cb rcb;	/* callback when this fd is readable. */
	fd_cb wcb;	/* callback when this fd is writeable.*/
	void *dat;	/* fd context */
	int busy;	/* whether this entry is being used in cb. */
};

struct fdset {
	struct pollfd rwfds[MAX_FDS];
	struct fdentry fd[MAX_FDS];
	pthread_mutex_t fd_mutex;
	int num;	/* current fd number of this fdset */
};

void fdset_init(struct fdset *pfdset);    (not used in the patchset)

int fdset_add(struct fdset *pfdset, int fd,
	fd_cb rcb, fd_cb wcb, void *dat);     (used in this patchset)

void *fdset_del(struct fdset *pfdset, int fd); (not used in the patchset)

void *fdset_event_dispatch(void *arg);   (used in this patchset)

seems that we have 4 options.
1) expose them in librte_vhost
2) expose them in other existing or new libs. for example,  eal.
3) duplicate the code lines at PMD layer.
4) do it as the patch does that.

thanks
Zhiyong