From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) by dpdk.org (Postfix) with ESMTP id 305795F22 for ; 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: 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 To: "Yang, Zhiyong" Cc: "Tan, Jianfeng" , Maxime Coquelin , "dev@dpdk.org" , "yliu@fridaylinux.org" , "Bie, Tiwei" , "Wang, Zhihong" , "Wang, Dong1" Date: Mon, 05 Mar 2018 09:54:55 +0100 Message-ID: <4529581.QgL3cdPo1v@xps> In-Reply-To: References: <20180214145330.4679-1-zhiyong.yang@intel.com> <3334571.8h0X0LI7U9@xps> 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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?