[dpdk-dev,v5,4/5] vhost: eventfd_link: replace copy-pasted sys_close

  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(-)
  

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