Message ID | 1429184910-30186-5-git-send-email-pboldin@mirantis.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 6CE9FC38E; Thu, 16 Apr 2015 13:48:47 +0200 (CEST) Received: from mail-lb0-f169.google.com (mail-lb0-f169.google.com [209.85.217.169]) by dpdk.org (Postfix) with ESMTP id D1074C346 for <dev@dpdk.org>; Thu, 16 Apr 2015 13:48:38 +0200 (CEST) Received: by lbbzk7 with SMTP id zk7so56517776lbb.0 for <dev@dpdk.org>; Thu, 16 Apr 2015 04:48:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mirantis.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=dvdfr3KyANWFYRZQ0EfCmUrG01urPtVXnO2v/pN9GHA=; b=I9DapjIH4u3nqZ8esZZY4zEvmvU+b1ggIeE6YWPxGBgGFc5IIDJZB7zQzIRlIKBjPp SAfpWZIPq5NJc22ZSzC+L2yejaQ2AhTITPFdrET6F0irrEOnlsfguchQDM5W+wS/6qNG vmbTT58f5b3nYlMnrPqWfwL1ixX9Vq35F+DZQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=dvdfr3KyANWFYRZQ0EfCmUrG01urPtVXnO2v/pN9GHA=; b=CdIL5IsLWVp2CzJ5L7RROK8c8L+MiU6YKOpx6ZoNxwLjOQ9c8w9RWxQNX38iRGWzKT gxuhkTyjLo9FOVl4jCDyLcRvZbH7BhaU5du9gcgLTjEALL/uFxz1MO9yK3fCNmkJ6DWi f/wBdoJN2wDBv7EtRZEVofzi+/4PVFctZNKDrxSkV/YfSusUq9/oYpP/vgtVhroR3/5A SFpIqpDAsRahEAJtXSdOpcXugdraK0XJ06J72maVpz+5KifbUk3dlUqAS1mlfG0iYLDR kCK2jYx/ZVFe3c33TxuqjfH9q0PA0QSHzErwDoELXbAdwzkTDWssjiflUjCeE/PzKzqr XyPw== X-Gm-Message-State: ALoCoQlC8skwG+tMA98pWxzrmODXEPfpzMthEgGYS/ljHGobI4CqfMtIloc1KnrjNoUX/ZAxw28y X-Received: by 10.152.178.197 with SMTP id da5mr28889856lac.56.1429184918629; Thu, 16 Apr 2015 04:48:38 -0700 (PDT) Received: from pboldin-pc.kha.mirantis.net ([194.213.110.67]) by mx.google.com with ESMTPSA id oy3sm1636510lbb.1.2015.04.16.04.48.37 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 16 Apr 2015 04:48:38 -0700 (PDT) From: Pavel Boldin <pboldin@mirantis.com> To: dev@dpdk.org Date: Thu, 16 Apr 2015 14:48:29 +0300 Message-Id: <1429184910-30186-5-git-send-email-pboldin@mirantis.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1429184910-30186-1-git-send-email-pboldin@mirantis.com> References: <1427994080-10163-1-git-send-email-pboldin@mirantis.com> <1429184910-30186-1-git-send-email-pboldin@mirantis.com> Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://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: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Pavel Boldin
April 16, 2015, 11:48 a.m. UTC
Replace copy-pasted `fget_from_files' -> `filp_close' with
a `sys_close' call.
Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
lib/librte_vhost/eventfd_link/eventfd_link.c | 49 +++++++---------------------
1 file changed, 12 insertions(+), 37 deletions(-)
Comments
On 4/16/2015 7:48 PM, Pavel Boldin wrote: > Replace copy-pasted `fget_from_files' -> `filp_close' with > a `sys_close' call. > > Signed-off-by: Pavel Boldin <pboldin@mirantis.com> > --- > lib/librte_vhost/eventfd_link/eventfd_link.c | 49 +++++++--------------------- > 1 file changed, 12 insertions(+), 37 deletions(-) > > diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c > index 0a06594..9bc52a3 100644 > --- a/lib/librte_vhost/eventfd_link/eventfd_link.c > +++ b/lib/librte_vhost/eventfd_link/eventfd_link.c > @@ -88,9 +88,8 @@ eventfd_link_ioctl_copy(unsigned long arg) > { > > + /* Closing the source_fd */ > + ret = sys_close(eventfd_copy.source_fd); Pavel: Here we close the fd and re-install a new file on this fd later. sys_close does all cleanup. But, for instance, if we allocate new fd later, normally it will reuse the just freed fds by sys_close, is there issue here? > + if (ret) > goto out_task; > - } > - > - /* > - * Release the existing eventfd in the source process > - */ > - spin_lock(&files->file_lock); > - fput(file); > - filp_close(file, files); > - fdt = files_fdtable(files); > - fdt->fd[eventfd_copy.source_fd] = NULL; > - spin_unlock(&files->file_lock); > - > - put_files_struct(files); > + ret = -ESTALE; > > /* > * Find the file struct associated with the target fd. > */ > > - ret = -ESTALE; > - files = get_files_struct(task_target); > - if (files == NULL) { > + target_files = get_files_struct(task_target); > + if (target_files == NULL) { > pr_info("Failed to get target files struct\n"); > goto out_task; > } > > ret = -EBADF; > - file = fget_from_files(files, eventfd_copy.target_fd); > - put_files_struct(files); > + target_file = fget_from_files(target_files, eventfd_copy.target_fd); > + put_files_struct(target_files); > > - if (file == NULL) { > + if (target_file == NULL) { > pr_info("Failed to get fd %d from target\n", > eventfd_copy.target_fd); > goto out_task; > @@ -164,7 +139,7 @@ eventfd_link_ioctl_copy(unsigned long arg) > * file desciptor of the source process, > */ > > - fd_install(eventfd_copy.source_fd, file); > + fd_install(eventfd_copy.source_fd, target_file); > ret = 0; > > out_task:
2015-05-07 06:54, Xie, Huawei: > On 4/16/2015 7:48 PM, Pavel Boldin wrote: > > + /* Closing the source_fd */ > > + ret = sys_close(eventfd_copy.source_fd); > Pavel: > Here we close the fd and re-install a new file on this fd later. > sys_close does all cleanup. > But, for instance, if we allocate new fd later, normally it will reuse > the just freed fds by sys_close, is there issue here? Pavel, Huawei, Could we come to a conclusion on this patch series please?
Ping 2015-06-17 17:24, Thomas Monjalon: > 2015-05-07 06:54, Xie, Huawei: > > On 4/16/2015 7:48 PM, Pavel Boldin wrote: > > > + /* Closing the source_fd */ > > > + ret = sys_close(eventfd_copy.source_fd); > > Pavel: > > Here we close the fd and re-install a new file on this fd later. > > sys_close does all cleanup. > > But, for instance, if we allocate new fd later, normally it will reuse > > the just freed fds by sys_close, is there issue here? > > Pavel, Huawei, > Could we come to a conclusion on this patch series please?
On 6/17/2015 11:24 PM, Thomas Monjalon wrote: > 2015-05-07 06:54, Xie, Huawei: >> On 4/16/2015 7:48 PM, Pavel Boldin wrote: >>> + /* Closing the source_fd */ >>> + ret = sys_close(eventfd_copy.source_fd); >> Pavel: >> Here we close the fd and re-install a new file on this fd later. >> sys_close does all cleanup. >> But, for instance, if we allocate new fd later, normally it will reuse >> the just freed fds by sys_close, is there issue here? > Pavel, Huawei, > Could we come to a conclusion on this patch series please? For the previous 3 patches, i am OK except that i don't think inline is needed explicitly for non-performance critical function. For this patch, didn't check the fs code. > >
Xie, Regarding the patches: 1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall. 2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem. 3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code. [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248 Pavel On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com> wrote: > On 6/17/2015 11:24 PM, Thomas Monjalon wrote: > > 2015-05-07 06:54, Xie, Huawei: > >> On 4/16/2015 7:48 PM, Pavel Boldin wrote: > >>> + /* Closing the source_fd */ > >>> + ret = sys_close(eventfd_copy.source_fd); > >> Pavel: > >> Here we close the fd and re-install a new file on this fd later. > >> sys_close does all cleanup. > >> But, for instance, if we allocate new fd later, normally it will reuse > >> the just freed fds by sys_close, is there issue here? > > Pavel, Huawei, > > Could we come to a conclusion on this patch series please? > For the previous 3 patches, i am OK except that i don't think inline is > needed explicitly for non-performance critical function. > For this patch, didn't check the fs code. > > > > > > >
On 7/10/2015 10:50 PM, Pavel Boldin wrote: Xie, Regarding the patches: 1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall. sys_close does extra cleanup than the replaced code. My concern is, for example, sys_close will mark the fd as next-to-be-allocated fd. Will there be issue when we allocate a new fd? Because it will be allocated starting from the next-to-be-allocated-fd. I think Kernel will do some check on that value, but not sure. 2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem. Sure, that is what the old code does. 3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code. I did a rough check. Maybe i miss something. I see it calls fd_install on a newly allocated fd. That is exactly what i think is the clean fix. Currently we allocate eventfd in user space, and install a new file onto it. Actually we don't need to allocate eventfd in user space at all, what we should do is allocate a new fd, and install the file on the newly allocated fd. new_fd = get_unused_fd_flags(...) fd_install(new_fd, get_file(fp[i])); /huawei [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248 Pavel On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote: On 6/17/2015 11:24 PM, Thomas Monjalon wrote: > 2015-05-07 06:54, Xie, Huawei: >> On 4/16/2015 7:48 PM, Pavel Boldin wrote: >>> + /* Closing the source_fd */ >>> + ret = sys_close(eventfd_copy.source_fd); >> Pavel: >> Here we close the fd and re-install a new file on this fd later. >> sys_close does all cleanup. >> But, for instance, if we allocate new fd later, normally it will reuse >> the just freed fds by sys_close, is there issue here? > Pavel, Huawei, > Could we come to a conclusion on this patch series please? For the previous 3 patches, i am OK except that i don't think inline is needed explicitly for non-performance critical function. For this patch, didn't check the fs code. > >
Don't know why previous mail get messed. On 7/10/2015 10:50 PM, Pavel Boldin wrote: Xie, Regarding the patches: 1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall. sys_close does extra cleanup than the replaced coe. My concern is, for example, sys_close will mark the fd as next-to-be-allocated fd. Will there be issue when we allocate a new fd, because it will be allocated starting from the value of next-to-be-allocted-fd? I think kernel willn't blindly use that value, but not sure. 2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem. Yes, that is exactly what the old code does. 3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code. I did a rough check. Maybe i miss something. I see it calls fd_install on a newly allocated fd. That is exactly what i want to replace the current code with. Currently we allcoate eventfd in user space and install a new file onto it through fd_install. Actually we don't need to allocate the eventfd in user space at all, what we should do is allocate a new fd in kernel, and install the file onto it. new_fd = get_unused_fd_flags(...) fd_install(new_fd, get_file(fp[i]) /huawei [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248 Pavel On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote: On 6/17/2015 11:24 PM, Thomas Monjalon wrote: > 2015-05-07 06:54, Xie, Huawei: >> On 4/16/2015 7:48 PM, Pavel Boldin wrote: >>> + /* Closing the source_fd */ >>> + ret = sys_close(eventfd_copy.source_fd); >> Pavel: >> Here we close the fd and re-install a new file on this fd later. >> sys_close does all cleanup. >> But, for instance, if we allocate new fd later, normally it will reuse >> the just freed fds by sys_close, is there issue here? > Pavel, Huawei, > Could we come to a conclusion on this patch series please? For the previous 3 patches, i am OK except that i don't think inline is needed explicitly for non-performance critical function. For this patch, didn't check the fs code. > >
2015-07-10 15:42, Xie, Huawei: > Don't know why previous mail get messed. Same problem with this one. It's maybe because you send HTML mail which is wrongly translated in plain text. > On 7/10/2015 10:50 PM, Pavel Boldin wrote: > Xie, > > Regarding the patches: > 1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall. > > sys_close does extra cleanup than the replaced coe. My concern is, for example, sys_close will mark the fd as next-to-be-allocated fd. Will there be issue when we allocate a new fd, because it will be allocated starting from the value of next-to-be-allocted-fd? I think kernel willn't blindly use that value, but not sure. > > 2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem. > > Yes, that is exactly what the old code does. > 3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code. > > I did a rough check. Maybe i miss something. I see it calls fd_install on a newly allocated fd. That is exactly what i want to replace the current code with. > Currently we allcoate eventfd in user space and install a new file onto it through fd_install. Actually we don't need to allocate the eventfd in user space at all, what we should do is allocate a new fd in kernel, and install the file onto it. > > new_fd = get_unused_fd_flags(...) > fd_install(new_fd, get_file(fp[i]) > > /huawei > > [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248 > > Pavel > > On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote: > On 6/17/2015 11:24 PM, Thomas Monjalon wrote: > > 2015-05-07 06:54, Xie, Huawei: > >> On 4/16/2015 7:48 PM, Pavel Boldin wrote: > >>> + /* Closing the source_fd */ > >>> + ret = sys_close(eventfd_copy.source_fd); > >> Pavel: > >> Here we close the fd and re-install a new file on this fd later. > >> sys_close does all cleanup. > >> But, for instance, if we allocate new fd later, normally it will reuse > >> the just freed fds by sys_close, is there issue here? > > Pavel, Huawei, > > Could we come to a conclusion on this patch series please? > For the previous 3 patches, i am OK except that i don't think inline is > needed explicitly for non-performance critical function. > For this patch, didn't check the fs code. > > > > > > > >
On 7/10/2015 11:50 PM, Thomas Monjalon wrote: > 2015-07-10 15:42, Xie, Huawei: >> Don't know why previous mail get messed. > Same problem with this one. > It's maybe because you send HTML mail which is wrongly translated in plain text. Have something to to with Pavel? :). I remember the format usually get messed when i reply to his mail. Anyway, I changed delivery format in Thunderbird from "Auto detect" to "Plain text only". > >> On 7/10/2015 10:50 PM, Pavel Boldin wrote: >> Xie, >> >> Regarding the patches: >> 1. The replaced code in fourth patch is checked to be a copy-paste of the `sys_close` syscall. >> >> sys_close does extra cleanup than the replaced coe. My concern is, for example, sys_close will mark the fd as next-to-be-allocated fd. Will there be issue when we allocate a new fd, because it will be allocated starting from the value of next-to-be-allocted-fd? I think kernel willn't blindly use that value, but not sure. >> >> 2. It is not uncommon for the applications to close FD making it allocated for a different file. In our particular case the file is closed in the *source* process and *added* to a target process, so matching fds should not be the problem. >> >> Yes, that is exactly what the old code does. >> 3. There is an implementation of the exact same thing in the SCM_RIGHTS [1] that can be used as the reference code. >> >> I did a rough check. Maybe i miss something. I see it calls fd_install on a newly allocated fd. That is exactly what i want to replace the current code with. >> Currently we allcoate eventfd in user space and install a new file onto it through fd_install. Actually we don't need to allocate the eventfd in user space at all, what we should do is allocate a new fd in kernel, and install the file onto it. >> >> new_fd = get_unused_fd_flags(...) >> fd_install(new_fd, get_file(fp[i]) >> >> /huawei >> >> [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248 >> >> Pavel >> >> On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto:huawei.xie@intel.com>> wrote: >> On 6/17/2015 11:24 PM, Thomas Monjalon wrote: >>> 2015-05-07 06:54, Xie, Huawei: >>>> On 4/16/2015 7:48 PM, Pavel Boldin wrote: >>>>> + /* Closing the source_fd */ >>>>> + ret = sys_close(eventfd_copy.source_fd); >>>> Pavel: >>>> Here we close the fd and re-install a new file on this fd later. >>>> sys_close does all cleanup. >>>> But, for instance, if we allocate new fd later, normally it will reuse >>>> the just freed fds by sys_close, is there issue here? >>> Pavel, Huawei, >>> Could we come to a conclusion on this patch series please? >> For the previous 3 patches, i am OK except that i don't think inline is >> needed explicitly for non-performance critical function. >> For this patch, didn't check the fs code. >> >>> >> >> > > >
Xie, All, Please find my comments intermixed below. On Fri, Jul 10, 2015 at 6:42 PM, Xie, Huawei <huawei.xie@intel.com> wrote: > Don't know why previous mail get messed. > > On 7/10/2015 10:50 PM, Pavel Boldin wrote: > Xie, > > Regarding the patches: > 1. The replaced code in fourth patch is checked to be a copy-paste of the > `sys_close` syscall. > > sys_close does extra cleanup than the replaced coe. My concern is, for > example, sys_close will mark the fd as next-to-be-allocated fd. Will there > be issue when we allocate a new fd, because it will be allocated starting > from the value of next-to-be-allocted-fd? I think kernel willn't blindly > use that value, but not sure. > That is what applications do when call `close' libc function -- the freed FD is ready to be allocated again and it is OK for applications to reuse FDs. > > 2. It is not uncommon for the applications to close FD making it allocated > for a different file. In our particular case the file is closed in the > *source* process and *added* to a target process, so matching fds should > not be the problem. > > Yes, that is exactly what the old code does. > 3. There is an implementation of the exact same thing in the SCM_RIGHTS > [1] that can be used as the reference code. > > I did a rough check. Maybe i miss something. I see it calls fd_install on > a newly allocated fd. That is exactly what i want to replace the current > code with. > Currently we allcoate eventfd in user space and install a new file onto it > through fd_install. Actually we don't need to allocate the eventfd in user > space at all, what we should do is allocate a new fd in kernel, and install > the file onto it. > > new_fd = get_unused_fd_flags(...) > fd_install(new_fd, get_file(fp[i]) > Well, this requires changes from the user-space side so I prefer not to do it by myself at the moment, because I'm no expert in DPDK. I can provide with the updated patches though but I will require a lab to check that it works indeed. No comments below this line. Pavel > > /huawei > > [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248 > > Pavel > > On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com<mailto: > huawei.xie@intel.com>> wrote: > On 6/17/2015 11:24 PM, Thomas Monjalon wrote: > > 2015-05-07 06:54, Xie, Huawei: > >> On 4/16/2015 7:48 PM, Pavel Boldin wrote: > >>> + /* Closing the source_fd */ > >>> + ret = sys_close(eventfd_copy.source_fd); > >> Pavel: > >> Here we close the fd and re-install a new file on this fd later. > >> sys_close does all cleanup. > >> But, for instance, if we allocate new fd later, normally it will reuse > >> the just freed fds by sys_close, is there issue here? > > Pavel, Huawei, > > Could we come to a conclusion on this patch series please? > For the previous 3 patches, i am OK except that i don't think inline is > needed explicitly for non-performance critical function. > For this patch, didn't check the fs code. > > > > > > > > >
On 7/11/2015 11:08 PM, Pavel Boldin wrote: > Xie, All, > > Please find my comments intermixed below. > > On Fri, Jul 10, 2015 at 6:42 PM, Xie, Huawei <huawei.xie@intel.com > <mailto:huawei.xie@intel.com>> wrote: > > Don't know why previous mail get messed. > > On 7/10/2015 10:50 PM, Pavel Boldin wrote: > Xie, > > Regarding the patches: > 1. The replaced code in fourth patch is checked to be a copy-paste > of the `sys_close` syscall. > > sys_close does extra cleanup than the replaced coe. My concern is, > for example, sys_close will mark the fd as next-to-be-allocated > fd. Will there be issue when we allocate a new fd, because it will > be allocated starting from the value of next-to-be-allocted-fd? I > think kernel willn't blindly use that value, but not sure. > > > That is what applications do when call `close' libc function -- the > freed FD is ready to be allocated again and it is OK for applications > to reuse FDs. > Pavel: Maybe i don't make it clear. It is ok we call sys_close to close a fd, and then alloc_fd to reuse the previous fd, but the tricky thing for us is after sys_close, we call fd_install to install a file on the previous fd value. Consider the following code flow: 1. sys_close->__close_fd: close the fd and mark the value of fd as the next_fd 2. fd_install(fd, file): here we install the file onto just freed fd in our event_fd module. 3. In the later fd allocation call, new_fd = alloc_fd(...), fd = files->next_fd Is it possible we get the previous fd value, while it is still being used? If it is, then there is issue here. so both the following two are straightforward. 1) sys_close->allocate_fd 2) As in our previous code, we did some internal cleanup on our old fd, and call fd_install to install the fd with a new file. > > 2. It is not uncommon for the applications to close FD making it > allocated for a different file. In our particular case the file is > closed in the *source* process and *added* to a target process, so > matching fds should not be the problem. > > Yes, that is exactly what the old code does. > 3. There is an implementation of the exact same thing in the > SCM_RIGHTS [1] that can be used as the reference code. > > I did a rough check. Maybe i miss something. I see it calls > fd_install on a newly allocated fd. That is exactly what i want to > replace the current code with. > Currently we allcoate eventfd in user space and install a new file > onto it through fd_install. Actually we don't need to allocate the > eventfd in user space at all, what we should do is allocate a new > fd in kernel, and install the file onto it. > > new_fd = get_unused_fd_flags(...) > fd_install(new_fd, get_file(fp[i]) > > > Well, this requires changes from the user-space side so I prefer not > to do it by myself at the moment, because I'm no expert in DPDK. I can > provide with the updated patches though but I will require a lab to > check that it works indeed. > Thanks. So if we aren't sure about this patch, is it ok we apply the previous three patches first? > No comments below this line. > > Pavel > > > > /huawei > > [1] https://github.com/torvalds/linux/blob/master/net/core/scm.c#L248 > > Pavel > > On Fri, Jul 10, 2015 at 5:27 PM, Xie, Huawei <huawei.xie@intel.com > <mailto:huawei.xie@intel.com><mailto:huawei.xie@intel.com > <mailto:huawei.xie@intel.com>>> wrote: > On 6/17/2015 11:24 PM, Thomas Monjalon wrote: > > 2015-05-07 06:54, Xie, Huawei: > >> On 4/16/2015 7:48 PM, Pavel Boldin wrote: > >>> + /* Closing the source_fd */ > >>> + ret = sys_close(eventfd_copy.source_fd); > >> Pavel: > >> Here we close the fd and re-install a new file on this fd later. > >> sys_close does all cleanup. > >> But, for instance, if we allocate new fd later, normally it > will reuse > >> the just freed fds by sys_close, is there issue here? > > Pavel, Huawei, > > Could we come to a conclusion on this patch series please? > For the previous 3 patches, i am OK except that i don't think > inline is > needed explicitly for non-performance critical function. > For this patch, didn't check the fs code. > > > > > > > > >
Xie, I think it is OK to merge first three patches at the moment. I'm going to implement a new scheme in a different ioctl soon. Pavel
diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c index 0a06594..9bc52a3 100644 --- a/lib/librte_vhost/eventfd_link/eventfd_link.c +++ b/lib/librte_vhost/eventfd_link/eventfd_link.c @@ -88,9 +88,8 @@ eventfd_link_ioctl_copy(unsigned long arg) { void __user *argp = (void __user *) arg; struct task_struct *task_target = NULL; - struct file *file; - struct files_struct *files; - struct fdtable *fdt; + struct file *target_file; + struct files_struct *target_files; struct eventfd_copy eventfd_copy; long ret = -EFAULT; @@ -109,51 +108,27 @@ eventfd_link_ioctl_copy(unsigned long arg) goto out; } - ret = -ESTALE; - files = get_files_struct(current); - if (files == NULL) { - pr_info("Failed to get current files struct\n"); - goto out_task; - } - - ret = -EBADF; - file = fget_from_files(files, eventfd_copy.source_fd); - - if (file == NULL) { - pr_info("Failed to get fd %d from source\n", - eventfd_copy.source_fd); - put_files_struct(files); + /* Closing the source_fd */ + ret = sys_close(eventfd_copy.source_fd); + if (ret) goto out_task; - } - - /* - * Release the existing eventfd in the source process - */ - spin_lock(&files->file_lock); - fput(file); - filp_close(file, files); - fdt = files_fdtable(files); - fdt->fd[eventfd_copy.source_fd] = NULL; - spin_unlock(&files->file_lock); - - put_files_struct(files); + ret = -ESTALE; /* * Find the file struct associated with the target fd. */ - ret = -ESTALE; - files = get_files_struct(task_target); - if (files == NULL) { + target_files = get_files_struct(task_target); + if (target_files == NULL) { pr_info("Failed to get target files struct\n"); goto out_task; } ret = -EBADF; - file = fget_from_files(files, eventfd_copy.target_fd); - put_files_struct(files); + target_file = fget_from_files(target_files, eventfd_copy.target_fd); + put_files_struct(target_files); - if (file == NULL) { + if (target_file == NULL) { pr_info("Failed to get fd %d from target\n", eventfd_copy.target_fd); goto out_task; @@ -164,7 +139,7 @@ eventfd_link_ioctl_copy(unsigned long arg) * file desciptor of the source process, */ - fd_install(eventfd_copy.source_fd, file); + fd_install(eventfd_copy.source_fd, target_file); ret = 0; out_task: