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

["Xie, Huawei" <huawei.xie@intel.com>]

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.
>
>     >
>     >
>
>
>
>