From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <thomas@monjalon.net>
Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com
 [66.111.4.29]) by dpdk.org (Postfix) with ESMTP id 305795F22
 for <dev@dpdk.org>; Mon,  5 Mar 2018 09:55:19 +0100 (CET)
Received: from compute1.internal (compute1.nyi.internal [10.202.2.41])
 by mailout.nyi.internal (Postfix) with ESMTP id 982F020C7A;
 Mon,  5 Mar 2018 03:55:18 -0500 (EST)
Received: from frontend2 ([10.202.2.161])
 by compute1.internal (MEProxy); Mon, 05 Mar 2018 03:55:18 -0500
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h=
 cc:content-transfer-encoding:content-type:date:from:in-reply-to
 :message-id:mime-version:references:subject:to:x-me-sender
 :x-me-sender:x-sasl-enc; s=mesmtp; bh=iVUAJjgO2lQPWwS9fKCBHU5PN2
 +GC9loFAtkdF+qW5s=; b=DyMCY7tybUfI03tuP50NQtGfFGCcNR8/Ko8HIxklQG
 1WuCx7RjNdY19aXUgIgtEpxmS4MuVGlnfX1Bvfv8z5Ro2JuSoeMJFIXn6l7kH5x0
 j8beY8vrP+X6agnagEsYIrTiGbNCZ0u9/3WAZYmrjJwbzIQtaP7aNRRMTIGHxx+f
 E=
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:content-transfer-encoding:content-type
 :date:from:in-reply-to:message-id:mime-version:references
 :subject:to:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; bh=iVUAJj
 gO2lQPWwS9fKCBHU5PN2+GC9loFAtkdF+qW5s=; b=ho7ugCICd0sgDapF4op97a
 YfdmbUmf+Jp0UfcbZkotu2tJ+I87q+qcTBGBQFAj8l6mcnw2AAinSHmFRa0FE5Rj
 dSHnFcX5ZjqjVLYuxTTB6trOxHpNKYqS2cFg6Y4DJu5EijO7tSkeIX4vl5yEy8O2
 zwlHb2ZdUhiQoAalco1DhOXGiuHwmAv0S4IfeSjRlacoF4ZMAIpmIuhBax7/sGBG
 uf/gMO/fbAtKXwhM76pHnCePeBGzin/plkv/H6xKBsTWakEgvUN3uMvW7PgJ//RI
 EZfiWIAYEQ0sBDQT3TpWGU8eEkRWNlhppfPbbjB0bDMJc9ak5hoPseb3EsquOndw
 ==
X-ME-Sender: <xms:9gWdWqaf7cEIFWAq21p8dO14RQzb5hVubvu8V1p1x3QEE8ZTIxfogw>
Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184])
 by mail.messagingengine.com (Postfix) with ESMTPA id 8A4DE24550;
 Mon,  5 Mar 2018 03:55:17 -0500 (EST)
From: Thomas Monjalon <thomas@monjalon.net>
To: "Yang, Zhiyong" <zhiyong.yang@intel.com>
Cc: "Tan, Jianfeng" <jianfeng.tan@intel.com>,
 Maxime Coquelin <maxime.coquelin@redhat.com>, "dev@dpdk.org" <dev@dpdk.org>,
 "yliu@fridaylinux.org" <yliu@fridaylinux.org>, "Bie,
 Tiwei" <tiwei.bie@intel.com>, "Wang, Zhihong" <zhihong.wang@intel.com>, "Wang,
 Dong1" <dong1.wang@intel.com>
Date: Mon, 05 Mar 2018 09:54:55 +0100
Message-ID: <4529581.QgL3cdPo1v@xps>
In-Reply-To: <E182254E98A5DA4EB1E657AC7CB9BD2A8B058E6D@BGSMSX101.gar.corp.intel.com>
References: <20180214145330.4679-1-zhiyong.yang@intel.com>
 <3334571.8h0X0LI7U9@xps>
 <E182254E98A5DA4EB1E657AC7CB9BD2A8B058E6D@BGSMSX101.gar.corp.intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Subject: Re: [dpdk-dev] [PATCH 1/4] vhost: move fdset functions from
	fd_man.c to fd_man.h
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Mon, 05 Mar 2018 08:55:19 -0000

05/03/2018 08:43, Yang, Zhiyong:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 01/03/2018 07:02, Tan, Jianfeng:
> > > From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> > > > On 02/28/2018 02:36 AM, Yang, Zhiyong wrote:
> > > > > From: Maxime Coquelin [mailto:maxime.coquelin@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?