From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 903F45F13 for ; Tue, 13 Mar 2018 10:51:05 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Mar 2018 02:51:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,464,1515484800"; d="scan'208";a="23856719" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga007.fm.intel.com with ESMTP; 13 Mar 2018 02:51:03 -0700 Received: from bgsmsx152.gar.corp.intel.com (10.224.48.50) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 13 Mar 2018 02:51:03 -0700 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.233]) by BGSMSX152.gar.corp.intel.com ([169.254.6.113]) with mapi id 14.03.0319.002; Tue, 13 Mar 2018 15:20:57 +0530 From: "Yang, Zhiyong" To: Thomas Monjalon CC: "Tan, Jianfeng" , Maxime Coquelin , "dev@dpdk.org" , "yliu@fridaylinux.org" , "Bie, Tiwei" , "Wang, Zhihong" , "Wang, Dong1" Thread-Topic: [PATCH 1/4] vhost: move fdset functions from fd_man.c to fd_man.h Thread-Index: AQHTpaObh7TjSFTKI0K8qBTZkBuQHaO4P8wAgADYGYCAACGWgIABZMIAgACJTgCABjVOYP//uu+AgAzrR7D//7TUAIAAXkug Date: Tue, 13 Mar 2018 09:50:57 +0000 Message-ID: References: <20180214145330.4679-1-zhiyong.yang@intel.com> <4529581.QgL3cdPo1v@xps> <9059097.ABoCBN0gVk@xps> In-Reply-To: <9059097.ABoCBN0gVk@xps> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.223.10.10] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: Tue, 13 Mar 2018 09:51:06 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Tuesday, March 13, 2018 5:43 PM > To: Yang, Zhiyong > Cc: Tan, Jianfeng ; Maxime Coquelin > ; dev@dpdk.org; yliu@fridaylinux.org; Bie, > Tiwei ; Wang, Zhihong ; > Wang, Dong1 > Subject: Re: [PATCH 1/4] vhost: move fdset functions from fd_man.c to > fd_man.h >=20 > 13/03/2018 09:46, Yang, Zhiyong: > > Hi Thomas, > > > > From: Thomas Monjalon [mailto:thomas@monjalon.net] > > > 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 expose= d. > > > > > > > > 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 mes= sy. > > > > > > > > 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? >=20 > Yes merge with interrupt thread in EAL. > I didn't look at the details, but it seems the right place for such thing= . >=20 Ok, we have to expose them as new APIs. Expect that somebody as DPDK user= s can use and like them as well. :) Thanks Zhiyong