[dpdk-dev,v5,3/4] vhost: using EVENTFD_COPY2
Commit Message
Signed-off-by: Pavel Boldin <pboldin@mirantis.com>
---
lib/librte_vhost/vhost_cuse/eventfd_copy.c | 54 ++++++++++++++++++----------
lib/librte_vhost/vhost_cuse/eventfd_copy.h | 6 ++++
lib/librte_vhost/vhost_cuse/vhost-net-cdev.c | 3 ++
3 files changed, 44 insertions(+), 19 deletions(-)
Comments
Thanks Pavel for this work.
This is what we think is the better implementation for eventfd proxy, in
our last review.
Could you add an additional patch to remove the old implementation?
Again, please run checkpatch.pl against your patch.
On 8/29/2015 2:51 AM, Pavel Boldin wrote:
[...]
> +
> +int
> +eventfd_init(void)
> +{
> + if (eventfd_link > 0)
0 could be valid fd. Change it to:
if (eventfd_link >= 0)
Change elsewhere if i miss it.
> +int
> +eventfd_free(void)
> +{
> + if (eventfd_link > 0)
same as above:
if (eventfd_link >= 0)
[...]
Xie,
Please find my comments intermixed below.
On Tue, Oct 20, 2015 at 12:52 PM, Xie, Huawei <huawei.xie@intel.com> wrote:
> Thanks Pavel for this work.
> This is what we think is the better implementation for eventfd proxy, in
> our last review.
> Could you add an additional patch to remove the old implementation?
>
I'm not really sure if we should do it -- imagine upgrading from one
version of DPDK to another.
Given the current implementation there is a backward compatibility.
>
> Again, please run checkpatch.pl against your patch.
>
Oops. Thanks for pointing out.
> On 8/29/2015 2:51 AM, Pavel Boldin wrote:
>
> [...]
> > +
> > +int
> > +eventfd_init(void)
> > +{
> > + if (eventfd_link > 0)
> 0 could be valid fd. Change it to:
>
Got it. Thanks.
> if (eventfd_link >= 0)
> Change elsewhere if i miss it.
> > +int
> > +eventfd_free(void)
> > +{
> > + if (eventfd_link > 0)
> same as above:
> if (eventfd_link >= 0)
>
> [...]
>
--
Sincerely,
Pavel
On 10/21/2015 8:16 PM, Pavel Boldin wrote:
> Xie,
>
> Please find my comments intermixed below.
>
> On Tue, Oct 20, 2015 at 12:52 PM, Xie, Huawei <huawei.xie@intel.com
> <mailto:huawei.xie@intel.com>> wrote:
>
> Thanks Pavel for this work.
> This is what we think is the better implementation for eventfd
> proxy, in
> our last review.
> Could you add an additional patch to remove the old implementation?
>
> I'm not really sure if we should do it -- imagine upgrading from one
> version of DPDK to another.
> Given the current implementation there is a backward compatibility.
I couldn't image the case any one would run old dpdk app with the new
dpdk module. However I am ok you leave it here, :), we could remove this
in next release.
Could you finish rebasing the patch before end of next week, otherwise
it will lose chance of being merged.
>
>
>
> Again, please run checkpatch.pl <http://checkpatch.pl> against
> your patch.
>
> Oops. Thanks for pointing out.
>
>
> On 8/29/2015 2:51 AM, Pavel Boldin wrote:
>
> [...]
> > +
> > +int
> > +eventfd_init(void)
> > +{
> > + if (eventfd_link > 0)
> 0 could be valid fd. Change it to:
>
> Got it. Thanks.
>
>
> if (eventfd_link >= 0)
> Change elsewhere if i miss it.
> > +int
> > +eventfd_free(void)
> > +{
> > + if (eventfd_link > 0)
> same as above:
> if (eventfd_link >= 0)
>
> [...]
>
>
> --
> Sincerely,
> Pavel
Huawei, Thomas,
Please find an updated patchset in the appropriate mail thread.
With best regards,
Pavel
On Mon, Oct 26, 2015 at 3:45 AM, Xie, Huawei <huawei.xie@intel.com> wrote:
> On 10/21/2015 8:16 PM, Pavel Boldin wrote:
> > Xie,
> >
> > Please find my comments intermixed below.
> >
> > On Tue, Oct 20, 2015 at 12:52 PM, Xie, Huawei <huawei.xie@intel.com
> > <mailto:huawei.xie@intel.com>> wrote:
> >
> > Thanks Pavel for this work.
> > This is what we think is the better implementation for eventfd
> > proxy, in
> > our last review.
> > Could you add an additional patch to remove the old implementation?
> >
> > I'm not really sure if we should do it -- imagine upgrading from one
> > version of DPDK to another.
> > Given the current implementation there is a backward compatibility.
> I couldn't image the case any one would run old dpdk app with the new
> dpdk module. However I am ok you leave it here, :), we could remove this
> in next release.
> Could you finish rebasing the patch before end of next week, otherwise
> it will lose chance of being merged.
> >
> >
> >
> > Again, please run checkpatch.pl <http://checkpatch.pl> against
> > your patch.
> >
> > Oops. Thanks for pointing out.
> >
> >
> > On 8/29/2015 2:51 AM, Pavel Boldin wrote:
> >
> > [...]
> > > +
> > > +int
> > > +eventfd_init(void)
> > > +{
> > > + if (eventfd_link > 0)
> > 0 could be valid fd. Change it to:
> >
> > Got it. Thanks.
> >
> >
> > if (eventfd_link >= 0)
> > Change elsewhere if i miss it.
> > > +int
> > > +eventfd_free(void)
> > > +{
> > > + if (eventfd_link > 0)
> > same as above:
> > if (eventfd_link >= 0)
> >
> > [...]
> >
> >
> > --
> > Sincerely,
> > Pavel
>
>
@@ -46,6 +46,32 @@
static const char eventfd_cdev[] = "/dev/eventfd-link";
+static int eventfd_link = -1;
+
+int
+eventfd_init(void)
+{
+ if (eventfd_link > 0)
+ return 0;
+
+ eventfd_link = open(eventfd_cdev, O_RDWR);
+ if (eventfd_link < 0) {
+ RTE_LOG(ERR, VHOST_CONFIG,
+ "eventfd_link module is not loaded\n");
+ return -1;
+ }
+
+ return 0;
+}
+
+int
+eventfd_free(void)
+{
+ if (eventfd_link > 0)
+ close(eventfd_link);
+ return 0;
+}
+
/*
* This function uses the eventfd_link kernel module to copy an eventfd file
* descriptor provided by QEMU in to our process space.
@@ -53,36 +79,26 @@ static const char eventfd_cdev[] = "/dev/eventfd-link";
int
eventfd_copy(int target_fd, int target_pid)
{
- int eventfd_link, ret;
- struct eventfd_copy eventfd_copy;
- int fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+ int ret;
+ struct eventfd_copy2 eventfd_copy2;
- if (fd == -1)
- return -1;
/* Open the character device to the kernel module. */
/* TODO: check this earlier rather than fail until VM boots! */
- eventfd_link = open(eventfd_cdev, O_RDWR);
- if (eventfd_link < 0) {
- RTE_LOG(ERR, VHOST_CONFIG,
- "eventfd_link module is not loaded\n");
- close(fd);
+ if (eventfd_init() < 0)
return -1;
- }
- eventfd_copy.source_fd = fd;
- eventfd_copy.target_fd = target_fd;
- eventfd_copy.target_pid = target_pid;
+ eventfd_copy2.fd = target_fd;
+ eventfd_copy2.pid = target_pid;
+ eventfd_copy2.flags = O_NONBLOCK | O_CLOEXEC;
/* Call the IOCTL to copy the eventfd. */
- ret = ioctl(eventfd_link, EVENTFD_COPY, &eventfd_copy);
- close(eventfd_link);
+ ret = ioctl(eventfd_link, EVENTFD_COPY2, &eventfd_copy2);
if (ret < 0) {
RTE_LOG(ERR, VHOST_CONFIG,
- "EVENTFD_COPY ioctl failed\n");
- close(fd);
+ "EVENTFD_COPY2 ioctl failed\n");
return -1;
}
- return fd;
+ return ret;
}
@@ -34,6 +34,12 @@
#define _EVENTFD_H
int
+eventfd_init(void);
+
+int
+eventfd_free(void);
+
+int
eventfd_copy(int target_fd, int target_pid);
#endif
@@ -373,6 +373,9 @@ rte_vhost_driver_register(const char *dev_name)
return -1;
}
+ if (eventfd_init() < 0)
+ return -1;
+
/*
* The device name is created. This is passed to QEMU so that it can
* register the device with our application.