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

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

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.

>
>