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

Yang, Zhiyong zhiyong.yang at intel.com
Tue Mar 13 10:50:57 CET 2018



> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas at monjalon.net]
> Sent: Tuesday, March 13, 2018 5:43 PM
> To: Yang, Zhiyong <zhiyong.yang at intel.com>
> Cc: Tan, Jianfeng <jianfeng.tan at intel.com>; Maxime Coquelin
> <maxime.coquelin at redhat.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
> 
> 13/03/2018 09:46, Yang, Zhiyong:
> > Hi Thomas,
> >
> > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > 05/03/2018 08:43, Yang, Zhiyong:
> > > > From: Thomas Monjalon [mailto:thomas at monjalon.net]
> > > > > 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.
> > >
> > > It looks to be very close of the interrupt thread.
> > > Can we have all merged in an unique event dispatcher thread?
> > >
> >
> > If I understand right, do you mean that we can merge them in lib eal ?  right?
> 
> Yes merge with interrupt thread in EAL.
> I didn't look at the details, but it seems the right place for such thing.
> 
Ok,  we have to expose them as new APIs.  Expect that somebody as DPDK users can use and like them as well. :)

Thanks
Zhiyong


More information about the dev mailing list