From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 8B4522C18 for ; Mon, 5 Mar 2018 08:43:22 +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 fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Mar 2018 23:43:19 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,426,1515484800"; d="scan'208";a="21859764" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga007.fm.intel.com with ESMTP; 04 Mar 2018 23:43:19 -0800 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 4 Mar 2018 23:43:19 -0800 Received: from BGSMSX107.gar.corp.intel.com (10.223.4.191) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 4 Mar 2018 23:43:18 -0800 Received: from bgsmsx101.gar.corp.intel.com ([169.254.1.25]) by BGSMSX107.gar.corp.intel.com ([169.254.9.47]) with mapi id 14.03.0319.002; Mon, 5 Mar 2018 13:13:16 +0530 From: "Yang, Zhiyong" To: Thomas Monjalon , "Tan, Jianfeng" CC: 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: AQHTpaObh7TjSFTKI0K8qBTZkBuQHaO4P8wAgADYGYCAACGWgIABZMIAgACJTgCABjVOYA== Date: Mon, 5 Mar 2018 07:43:15 +0000 Message-ID: References: <20180214145330.4679-1-zhiyong.yang@intel.com> <3334571.8h0X0LI7U9@xps> In-Reply-To: <3334571.8h0X0LI7U9@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: Mon, 05 Mar 2018 07:43:24 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas@monjalon.net] > Sent: Thursday, March 1, 2018 10:14 PM > To: Tan, Jianfeng > Cc: Maxime Coquelin ; Yang, Zhiyong > ; 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 > 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 o= ne 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? >=20 > 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? >=20 If needed to expose them in eal.h,=20 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