diff mbox

[dpdk-dev,v5,3/4] vhost: using EVENTFD_COPY2

Message ID 1440787880-7079-3-git-send-email-pboldin@mirantis.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Pavel Boldin Aug. 28, 2015, 6:51 p.m. UTC
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

Huawei Xie Oct. 20, 2015, 9:52 a.m. UTC | #1
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)

[...]
Pavel Boldin Oct. 21, 2015, 12:16 p.m. UTC | #2
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
Huawei Xie Oct. 26, 2015, 1:45 a.m. UTC | #3
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
Pavel Boldin Oct. 28, 2015, 6:35 p.m. UTC | #4
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
>
>
diff mbox

Patch

diff --git a/lib/librte_vhost/vhost_cuse/eventfd_copy.c b/lib/librte_vhost/vhost_cuse/eventfd_copy.c
index 4d697a2..1625163 100644
--- a/lib/librte_vhost/vhost_cuse/eventfd_copy.c
+++ b/lib/librte_vhost/vhost_cuse/eventfd_copy.c
@@ -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;
 }
diff --git a/lib/librte_vhost/vhost_cuse/eventfd_copy.h b/lib/librte_vhost/vhost_cuse/eventfd_copy.h
index 19ae30d..5f446ca 100644
--- a/lib/librte_vhost/vhost_cuse/eventfd_copy.h
+++ b/lib/librte_vhost/vhost_cuse/eventfd_copy.h
@@ -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
diff --git a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
index 1ae7c49..ae7ad8d 100644
--- a/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
+++ b/lib/librte_vhost/vhost_cuse/vhost-net-cdev.c
@@ -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.