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 41DCC2A58 for ; Mon, 13 Jul 2015 04:00:26 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 12 Jul 2015 19:00:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,459,1432623600"; d="scan'208";a="763153744" Received: from pgsmsx105.gar.corp.intel.com ([10.221.44.96]) by orsmga002.jf.intel.com with ESMTP; 12 Jul 2015 19:00:23 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by PGSMSX105.gar.corp.intel.com (10.221.44.96) with Microsoft SMTP Server (TLS) id 14.3.224.2; Mon, 13 Jul 2015 09:59:53 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.246]) by shsmsx102.ccr.corp.intel.com ([169.254.2.165]) with mapi id 14.03.0224.002; Mon, 13 Jul 2015 09:59:52 +0800 From: "Xie, Huawei" To: Pavel Boldin Thread-Topic: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close Thread-Index: AdC9D5rLV8dlCf+ynE6WaHWpLcGybQ== Date: Mon, 13 Jul 2015 01:59:51 +0000 Message-ID: References: <1427994080-10163-1-git-send-email-pboldin@mirantis.com> <1429184910-30186-5-git-send-email-pboldin@mirantis.com> <1665929.SfCu3BinAE@xps13> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH v5 4/5] vhost: eventfd_link: replace copy-pasted sys_close X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Jul 2015 02:00:26 -0000 On 7/11/2015 11:08 PM, Pavel Boldin wrote:=0A= > Xie, All,=0A= >=0A= > Please find my comments intermixed below.=0A= >=0A= > On Fri, Jul 10, 2015 at 6:42 PM, Xie, Huawei > wrote:=0A= >=0A= > Don't know why previous mail get messed.=0A= >=0A= > On 7/10/2015 10:50 PM, Pavel Boldin wrote:=0A= > Xie,=0A= >=0A= > Regarding the patches:=0A= > 1. The replaced code in fourth patch is checked to be a copy-paste=0A= > of the `sys_close` syscall.=0A= >=0A= > sys_close does extra cleanup than the replaced coe. My concern is,=0A= > for example, sys_close will mark the fd as next-to-be-allocated=0A= > fd. Will there be issue when we allocate a new fd, because it will=0A= > be allocated starting from the value of next-to-be-allocted-fd? I=0A= > think kernel willn't blindly use that value, but not sure.=0A= >=0A= >=0A= > That is what applications do when call `close' libc function -- the=0A= > freed FD is ready to be allocated again and it is OK for applications=0A= > to reuse FDs.=0A= > =0A= Pavel:=0A= Maybe i don't make it clear. It is ok we call sys_close to close a fd,=0A= and then alloc_fd to reuse the previous fd, but the tricky thing for us=0A= is after sys_close, we call fd_install to install a file on the previous=0A= fd value.=0A= =0A= Consider the following code flow:=0A= 1. sys_close->__close_fd: close the fd and mark the value of fd as the=0A= next_fd=0A= 2. fd_install(fd, file): here we install the file onto just freed fd in=0A= our event_fd module.=0A= 3. In the later fd allocation call, =0A= new_fd =3D alloc_fd(...), fd =3D files->next_fd=0A= Is it possible we get the previous fd value, while it is still being=0A= used? If it is, then there is issue here.=0A= =0A= so both the following two are straightforward.=0A= 1) sys_close->allocate_fd=0A= 2) As in our previous code, we did some internal cleanup on our old fd,=0A= and call fd_install to install the fd with a new file.=0A= =0A= =0A= =0A= >=0A= > 2. It is not uncommon for the applications to close FD making it=0A= > allocated for a different file. In our particular case the file is=0A= > closed in the *source* process and *added* to a target process, so=0A= > matching fds should not be the problem.=0A= >=0A= > Yes, that is exactly what the old code does.=0A= > 3. There is an implementation of the exact same thing in the=0A= > SCM_RIGHTS [1] that can be used as the reference code.=0A= >=0A= > I did a rough check. Maybe i miss something. I see it calls=0A= > fd_install on a newly allocated fd. That is exactly what i want to=0A= > replace the current code with.=0A= > Currently we allcoate eventfd in user space and install a new file=0A= > onto it through fd_install. Actually we don't need to allocate the=0A= > eventfd in user space at all, what we should do is allocate a new=0A= > fd in kernel, and install the file onto it.=0A= >=0A= > new_fd =3D get_unused_fd_flags(...)=0A= > fd_install(new_fd, get_file(fp[i])=0A= >=0A= >=0A= > Well, this requires changes from the user-space side so I prefer not=0A= > to do it by myself at the moment, because I'm no expert in DPDK. I can=0A= > provide with the updated patches though but I will require a lab to=0A= > check that it works indeed.=0A= >=0A= Thanks.=0A= So if we aren't sure about this patch, is it ok we apply the previous=0A= three patches first?=0A= =0A= > No comments below this line.=0A= >=0A= > Pavel=0A= > =0A= >=0A= >=0A= > /huawei=0A= >=0A= > [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248= =0A= >=0A= > Pavel=0A= >=0A= > On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei >> wrote:=0A= > On 6/17/2015 11:24 PM, Thomas Monjalon wrote:=0A= > > 2015-05-07 06:54, Xie, Huawei:=0A= > >> On 4/16/2015 7:48 PM, Pavel Boldin wrote:=0A= > >>> + /* Closing the source_fd */=0A= > >>> + ret =3D sys_close(eventfd_copy.source_fd);=0A= > >> Pavel:=0A= > >> Here we close the fd and re-install a new file on this fd later.= =0A= > >> sys_close does all cleanup.=0A= > >> But, for instance, if we allocate new fd later, normally it=0A= > will reuse=0A= > >> the just freed fds by sys_close, is there issue here?=0A= > > Pavel, Huawei,=0A= > > Could we come to a conclusion on this patch series please?=0A= > For the previous 3 patches, i am OK except that i don't think=0A= > inline is=0A= > needed explicitly for non-performance critical function.=0A= > For this patch, didn't check the fs code.=0A= >=0A= > >=0A= > >=0A= >=0A= >=0A= >=0A= >=0A= =0A=