[PATCH 1/2] nouveau: make nouveau importing global buffers completely thread-safe, with tests

[PATCH 1/2] nouveau: make nouveau importing global buffers completely thread-safe, with tests

[Maarten Lankhorst]

While I've closed off most races in a previous patch, a small race still existed
where importing then unreffing cound cause an invalid bo. Add a test for this case.

Racing sequence fixed:

- thread 1 releases bo, refcount drops to zero, blocks on acquiring nvdev->lock.
- thread 2 increases refcount to 1.
- thread 2 decreases refcount to zero, blocks on acquiring nvdev->lock.

At this point the 2 threads will clean up the same bo.

How is this fixed? When thread 2 increases refcount to 1 it removes
the bo from the list, and creates a new bo. The original thread
will notice refcount was increased to 1 and skip deletion from list
and closing the handle.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
---
 configure.ac              |   1 +
 nouveau/nouveau.c         |  69 +++++++++++------------
 tests/Makefile.am         |   4 ++
 tests/nouveau/.gitignore  |   1 +
 tests/nouveau/Makefile.am |  15 +++++
 tests/nouveau/threaded.c  | 140 ++++++++++++++++++++++++++++++++++++++++++++++
 xf86atomic.h              |   6 +-
 7 files changed, 198 insertions(+), 38 deletions(-)
 create mode 100644 tests/nouveau/.gitignore
 create mode 100644 tests/nouveau/Makefile.am
 create mode 100644 tests/nouveau/threaded.c

diff --git a/configure.ac b/configure.ac
index 8afee83..6dc5044 100644
--- a/configure.ac
+++ b/configure.ac
@@ -441,6 +441,7 @@ AC_CONFIG_FILES([
 	tests/vbltest/Makefile
 	tests/exynos/Makefile
 	tests/tegra/Makefile
+	tests/nouveau/Makefile
 	man/Makefile
 	libdrm.pc])
 AC_OUTPUT
diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
index c6c153a..1c723b9 100644
--- a/nouveau/nouveau.c
+++ b/nouveau/nouveau.c
@@ -351,29 +351,18 @@ nouveau_bo_del(struct nouveau_bo *bo)
 
 	pthread_mutex_lock(&nvdev->lock);
 	if (nvbo->name) {
-		if (atomic_read(&nvbo->refcnt)) {
+		if (atomic_read(&nvbo->refcnt) == 0) {
+			DRMLISTDEL(&nvbo->head);
 			/*
-			 * bo has been revived by a race with
-			 * nouveau_bo_prime_handle_ref, or nouveau_bo_name_ref.
-			 *
-			 * In theory there's still a race possible with
-			 * nouveau_bo_wrap, but when using this function
-			 * the lifetime of the handle is probably already
-			 * handled in another way. If there are races
-			 * you're probably using nouveau_bo_wrap wrong.
+			 * This bo has to be closed with the lock held because
+			 * gem handles are not refcounted. If a shared bo is
+			 * closed and re-opened in another thread a race
+			 * against DRM_IOCTL_GEM_OPEN or drmPrimeFDToHandle
+			 * might cause the bo to be closed accidentally while
+			 * re-importing.
 			 */
-			pthread_mutex_unlock(&nvdev->lock);
-			return;
+			drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
 		}
-		DRMLISTDEL(&nvbo->head);
-		/*
-		 * This bo has to be closed with the lock held because gem
-		 * handles are not refcounted. If a shared bo is closed and
-		 * re-opened in another thread a race against
-		 * DRM_IOCTL_GEM_OPEN or drmPrimeFDToHandle might cause the
-		 * bo to be closed accidentally while re-importing.
-		 */
-		drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
 		pthread_mutex_unlock(&nvdev->lock);
 	} else {
 		DRMLISTDEL(&nvbo->head);
@@ -418,7 +407,7 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
 
 static int
 nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
-		       struct nouveau_bo **pbo)
+		       struct nouveau_bo **pbo, int name)
 {
 	struct nouveau_device_priv *nvdev = nouveau_device(dev);
 	struct drm_nouveau_gem_info req = { .handle = handle };
@@ -427,8 +416,24 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
 
 	DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {
 		if (nvbo->base.handle == handle) {
-			*pbo = NULL;
-			nouveau_bo_ref(&nvbo->base, pbo);
+			if (atomic_inc_return(&nvbo->refcnt) == 1) {
+				/*
+				 * Uh oh, this bo is dead and someone else
+				 * will free it, but because refcnt is
+				 * now non-zero fortunately they won't
+				 * call the ioctl to close the bo.
+				 *
+				 * Remove this bo from the list so other
+				 * calls to nouveau_bo_wrap_locked will
+				 * see our replacement nvbo.
+				 */
+				DRMLISTDEL(&nvbo->head);
+				if (!name)
+					name = nvbo->name;
+				break;
+			}
+
+			*pbo = &nvbo->base;
 			return 0;
 		}
 	}
@@ -443,6 +448,7 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
 		atomic_set(&nvbo->refcnt, 1);
 		nvbo->base.device = dev;
 		abi16_bo_info(&nvbo->base, &req);
+		nvbo->name = name;
 		DRMLISTADD(&nvbo->head, &nvdev->bo_list);
 		*pbo = &nvbo->base;
 		return 0;
@@ -458,7 +464,7 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t handle,
 	struct nouveau_device_priv *nvdev = nouveau_device(dev);
 	int ret;
 	pthread_mutex_lock(&nvdev->lock);
-	ret = nouveau_bo_wrap_locked(dev, handle, pbo);
+	ret = nouveau_bo_wrap_locked(dev, handle, pbo, 0);
 	pthread_mutex_unlock(&nvdev->lock);
 	return ret;
 }
@@ -468,24 +474,13 @@ nouveau_bo_name_ref(struct nouveau_device *dev, uint32_t name,
 		    struct nouveau_bo **pbo)
 {
 	struct nouveau_device_priv *nvdev = nouveau_device(dev);
-	struct nouveau_bo_priv *nvbo;
 	struct drm_gem_open req = { .name = name };
 	int ret;
 
 	pthread_mutex_lock(&nvdev->lock);
-	DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {
-		if (nvbo->name == name) {
-			*pbo = NULL;
-			nouveau_bo_ref(&nvbo->base, pbo);
-			pthread_mutex_unlock(&nvdev->lock);
-			return 0;
-		}
-	}
-
 	ret = drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req);
 	if (ret == 0) {
-		ret = nouveau_bo_wrap_locked(dev, req.handle, pbo);
-		nouveau_bo((*pbo))->name = name;
+		ret = nouveau_bo_wrap_locked(dev, req.handle, pbo, name);
 	}
 
 	pthread_mutex_unlock(&nvdev->lock);
@@ -537,7 +532,7 @@ nouveau_bo_prime_handle_ref(struct nouveau_device *dev, int prime_fd,
 	pthread_mutex_lock(&nvdev->lock);
 	ret = drmPrimeFDToHandle(dev->fd, prime_fd, &handle);
 	if (ret == 0) {
-		ret = nouveau_bo_wrap_locked(dev, handle, bo);
+		ret = nouveau_bo_wrap_locked(dev, handle, bo, 0);
 		if (!ret) {
 			struct nouveau_bo_priv *nvbo = nouveau_bo(*bo);
 			if (!nvbo->name) {
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 37b8d3a..38e4094 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -28,6 +28,10 @@ if HAVE_TEGRA
 SUBDIRS += tegra
 endif
 
+if HAVE_NOUVEAU
+SUBDIRS += nouveau
+endif
+
 if HAVE_LIBUDEV
 
 check_LTLIBRARIES = libdrmtest.la
diff --git a/tests/nouveau/.gitignore b/tests/nouveau/.gitignore
new file mode 100644
index 0000000..837bfb9
--- /dev/null
+++ b/tests/nouveau/.gitignore
@@ -0,0 +1 @@
+threaded
diff --git a/tests/nouveau/Makefile.am b/tests/nouveau/Makefile.am
new file mode 100644
index 0000000..8e3392e
--- /dev/null
+++ b/tests/nouveau/Makefile.am
@@ -0,0 +1,15 @@
+AM_CPPFLAGS = \
+	-I$(top_srcdir)/include/drm \
+	-I$(top_srcdir)/nouveau \
+	-I$(top_srcdir)
+
+AM_CFLAGS = -Wall -Werror
+AM_LDFLAGS = -pthread
+
+LDADD = -ldl \
+	../../nouveau/libdrm_nouveau.la \
+	../../libdrm.la
+
+noinst_PROGRAMS = \
+	threaded
+
diff --git a/tests/nouveau/threaded.c b/tests/nouveau/threaded.c
new file mode 100644
index 0000000..490a7fa
--- /dev/null
+++ b/tests/nouveau/threaded.c
@@ -0,0 +1,140 @@
+/*
+ * Copyright © 2015 Canonical Ltd. (Maarten Lankhorst)
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#ifdef HAVE_CONFIG_H
+#  include "config.h"
+#endif
+
+#include <sys/ioctl.h>
+#include <dlfcn.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+#include <pthread.h>
+
+#include "xf86drm.h"
+#include "nouveau.h"
+
+static const char default_device[] = "/dev/dri/renderD128";
+
+static typeof(ioctl) *old_ioctl;
+static int failed;
+
+static int import_fd;
+
+int ioctl(int fd, unsigned long request, ...)
+{
+	va_list va;
+	int ret;
+	void *arg;
+
+	va_start(va, request);
+	arg = va_arg(va, void *);
+	ret = old_ioctl(fd, request, arg);
+	va_end(va);
+
+	if (ret < 0 && request == DRM_IOCTL_GEM_CLOSE && errno == EINVAL)
+		failed = 1;
+
+	return ret;
+}
+
+static void *
+openclose(void *dev)
+{
+	struct nouveau_device *nvdev = dev;
+	struct nouveau_bo *bo = NULL;
+	int i;
+
+	for (i = 0; i < 100000; ++i) {
+		if (!nouveau_bo_prime_handle_ref(nvdev, import_fd, &bo))
+			nouveau_bo_ref(NULL, &bo);
+	}
+	return NULL;
+}
+
+int main(int argc, char *argv[])
+{
+	drmVersionPtr version;
+	const char *device;
+	int err, fd, fd2;
+	struct nouveau_device *nvdev, *nvdev2;
+	struct nouveau_bo *bo;
+
+	old_ioctl = dlsym(RTLD_NEXT, "ioctl");
+
+	pthread_t t1, t2;
+
+	if (argc < 2)
+		device = default_device;
+	else
+		device = argv[1];
+
+	fd = open(device, O_RDWR);
+	fd2 = open(device, O_RDWR);
+	if (fd < 0 || fd2 < 0)
+		return 1;
+
+	version = drmGetVersion(fd);
+	if (version) {
+		printf("Version: %d.%d.%d\n", version->version_major,
+		       version->version_minor, version->version_patchlevel);
+		printf("  Name: %s\n", version->name);
+		printf("  Date: %s\n", version->date);
+		printf("  Description: %s\n", version->desc);
+
+		drmFreeVersion(version);
+	}
+
+	err = nouveau_device_wrap(fd, 0, &nvdev);
+	if (!err)
+		err = nouveau_device_wrap(fd2, 0, &nvdev2);
+	if (err < 0)
+		return 1;
+
+	err = nouveau_bo_new(nvdev2, NOUVEAU_BO_GART, 0, 4096, NULL, &bo);
+	if (!err)
+		err = nouveau_bo_set_prime(bo, &import_fd);
+
+	if (!err) {
+		pthread_create(&t1, NULL, openclose, nvdev);
+		pthread_create(&t2, NULL, openclose, nvdev);
+	}
+
+	pthread_join(t1, NULL);
+	pthread_join(t2, NULL);
+
+	close(import_fd);
+	nouveau_bo_ref(NULL, &bo);
+
+	nouveau_device_del(&nvdev2);
+	nouveau_device_del(&nvdev);
+	close(fd2);
+	close(fd);
+
+	if (failed)
+		fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed with EINVAL,\n"
+				"race in opening/closing bo is likely.\n");
+
+	return failed;
+}
diff --git a/xf86atomic.h b/xf86atomic.h
index 8c4b696..66a8d9a 100644
--- a/xf86atomic.h
+++ b/xf86atomic.h
@@ -49,7 +49,8 @@ typedef struct {
 # define atomic_read(x) ((x)->atomic)
 # define atomic_set(x, val) ((x)->atomic = (val))
 # define atomic_inc(x) ((void) __sync_fetch_and_add (&(x)->atomic, 1))
-# define atomic_dec_and_test(x) (__sync_fetch_and_add (&(x)->atomic, -1) == 1)
+# define atomic_inc_return(x) (__sync_add_and_fetch (&(x)->atomic, 1))
+# define atomic_dec_and_test(x) (__sync_add_and_fetch (&(x)->atomic, -1) == 0)
 # define atomic_add(x, v) ((void) __sync_add_and_fetch(&(x)->atomic, (v)))
 # define atomic_dec(x, v) ((void) __sync_sub_and_fetch(&(x)->atomic, (v)))
 # define atomic_cmpxchg(x, oldv, newv) __sync_val_compare_and_swap (&(x)->atomic, oldv, newv)
@@ -68,6 +69,7 @@ typedef struct {
 # define atomic_read(x) AO_load_full(&(x)->atomic)
 # define atomic_set(x, val) AO_store_full(&(x)->atomic, (val))
 # define atomic_inc(x) ((void) AO_fetch_and_add1_full(&(x)->atomic))
+# define atomic_inc_return(x) (AO_fetch_and_add1_full(&(x)->atomic) + 1)
 # define atomic_add(x, v) ((void) AO_fetch_and_add_full(&(x)->atomic, (v)))
 # define atomic_dec(x, v) ((void) AO_fetch_and_add_full(&(x)->atomic, -(v)))
 # define atomic_dec_and_test(x) (AO_fetch_and_sub1_full(&(x)->atomic) == 1)
@@ -91,6 +93,7 @@ typedef struct { LIBDRM_ATOMIC_TYPE atomic; } atomic_t;
 # define atomic_read(x) (int) ((x)->atomic)
 # define atomic_set(x, val) ((x)->atomic = (LIBDRM_ATOMIC_TYPE)(val))
 # define atomic_inc(x) (atomic_inc_uint (&(x)->atomic))
+# define atomic_inc_return (atomic_inc_uint_nv(&(x)->atomic))
 # define atomic_dec_and_test(x) (atomic_dec_uint_nv(&(x)->atomic) == 0)
 # define atomic_add(x, v) (atomic_add_int(&(x)->atomic, (v)))
 # define atomic_dec(x, v) (atomic_add_int(&(x)->atomic, -(v)))
@@ -112,3 +115,4 @@ static inline int atomic_add_unless(atomic_t *v, int add, int unless)
 }
 
 #endif
+
-- 
2.3.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Maarten Lankhorst]

Only add wrapped bo's and bo's that have been exported through flink or dma-buf.
This avoids a lock in the common case, and decreases traversal needed for importing
a dma-buf or flink.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
---
 nouveau/nouveau.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
index 1c723b9..d411523 100644
--- a/nouveau/nouveau.c
+++ b/nouveau/nouveau.c
@@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
 	struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
 	struct drm_gem_close req = { bo->handle };
 
-	pthread_mutex_lock(&nvdev->lock);
-	if (nvbo->name) {
+	if (nvbo->head.next) {
+		pthread_mutex_lock(&nvdev->lock);
 		if (atomic_read(&nvbo->refcnt) == 0) {
 			DRMLISTDEL(&nvbo->head);
 			/*
@@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
 		}
 		pthread_mutex_unlock(&nvdev->lock);
 	} else {
-		DRMLISTDEL(&nvbo->head);
-		pthread_mutex_unlock(&nvdev->lock);
 		drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
 	}
 	if (bo->map)
@@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
 	       uint64_t size, union nouveau_bo_config *config,
 	       struct nouveau_bo **pbo)
 {
-	struct nouveau_device_priv *nvdev = nouveau_device(dev);
 	struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
 	struct nouveau_bo *bo = &nvbo->base;
 	int ret;
@@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
 		return ret;
 	}
 
-	pthread_mutex_lock(&nvdev->lock);
-	DRMLISTADD(&nvbo->head, &nvdev->bo_list);
-	pthread_mutex_unlock(&nvdev->lock);
-
 	*pbo = bo;
 	return 0;
 }
@@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
 	return -ENOMEM;
 }
 
+static void
+nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
+{
+	if (!nvbo->head.next) {
+		struct nouveau_device_priv *nvdev = nouveau_device(nvbo->base.device);
+		pthread_mutex_lock(&nvdev->lock);
+		if (!nvbo->head.next)
+			DRMLISTADD(&nvbo->head, &nvdev->bo_list);
+		pthread_mutex_unlock(&nvdev->lock);
+	}
+}
+
 drm_public int
 nouveau_bo_wrap(struct nouveau_device *dev, uint32_t handle,
 		struct nouveau_bo **pbo)
@@ -494,13 +499,17 @@ nouveau_bo_name_get(struct nouveau_bo *bo, uint32_t *name)
 	struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
 
 	*name = nvbo->name;
-	if (!*name || *name == ~0U) {
+	if (!*name) {
 		int ret = drmIoctl(bo->device->fd, DRM_IOCTL_GEM_FLINK, &req);
+		struct nouveau_device_priv *nvdev = nouveau_device(bo->device);
+
 		if (ret) {
 			*name = 0;
 			return ret;
 		}
 		nvbo->name = *name = req.name;
+
+		nouveau_bo_make_global(nvbo);
 	}
 	return 0;
 }
@@ -533,16 +542,6 @@ nouveau_bo_prime_handle_ref(struct nouveau_device *dev, int prime_fd,
 	ret = drmPrimeFDToHandle(dev->fd, prime_fd, &handle);
 	if (ret == 0) {
 		ret = nouveau_bo_wrap_locked(dev, handle, bo, 0);
-		if (!ret) {
-			struct nouveau_bo_priv *nvbo = nouveau_bo(*bo);
-			if (!nvbo->name) {
-				/*
-				 * XXX: Force locked DRM_IOCTL_GEM_CLOSE
-				 * to rule out race conditions
-				 */
-				nvbo->name = ~0;
-			}
-		}
 	}
 	pthread_mutex_unlock(&nvdev->lock);
 	return ret;
@@ -557,8 +556,8 @@ nouveau_bo_set_prime(struct nouveau_bo *bo, int *prime_fd)
 	ret = drmPrimeHandleToFD(bo->device->fd, nvbo->base.handle, DRM_CLOEXEC, prime_fd);
 	if (ret)
 		return ret;
-	if (!nvbo->name)
-		nvbo->name = ~0;
+
+	nouveau_bo_make_global(nvbo);
 	return 0;
 }
 
@@ -578,8 +577,8 @@ nouveau_bo_wait(struct nouveau_bo *bo, uint32_t access,
 	if (push && push->channel)
 		nouveau_pushbuf_kick(push, push->channel);
 
-	if (!nvbo->name && !(nvbo->access & NOUVEAU_BO_WR) &&
-			   !(      access & NOUVEAU_BO_WR))
+	if (!nvbo->head.next && !(nvbo->access & NOUVEAU_BO_WR) &&
+				!(access & NOUVEAU_BO_WR))
 		return 0;
 
 	req.handle = bo->handle;
-- 
2.3.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Emil Velikov]

On 24 February 2015 at 09:01, Maarten Lankhorst
<maarten.lankhorst@ubuntu.com> wrote:
> Only add wrapped bo's and bo's that have been exported through flink or dma-buf.
> This avoids a lock in the common case, and decreases traversal needed for importing
> a dma-buf or flink.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
> ---
>  nouveau/nouveau.c | 47 +++++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> index 1c723b9..d411523 100644
> --- a/nouveau/nouveau.c
> +++ b/nouveau/nouveau.c
> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
>         struct drm_gem_close req = { bo->handle };
>
> -       pthread_mutex_lock(&nvdev->lock);
> -       if (nvbo->name) {
> +       if (nvbo->head.next) {
> +               pthread_mutex_lock(&nvdev->lock);
>                 if (atomic_read(&nvbo->refcnt) == 0) {
>                         DRMLISTDEL(&nvbo->head);
>                         /*
> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
>                 }
>                 pthread_mutex_unlock(&nvdev->lock);
>         } else {
> -               DRMLISTDEL(&nvbo->head);
> -               pthread_mutex_unlock(&nvdev->lock);
>                 drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>         }
>         if (bo->map)
> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
>                uint64_t size, union nouveau_bo_config *config,
>                struct nouveau_bo **pbo)
>  {
> -       struct nouveau_device_priv *nvdev = nouveau_device(dev);
>         struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
>         struct nouveau_bo *bo = &nvbo->base;
>         int ret;
> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
>                 return ret;
>         }
>
> -       pthread_mutex_lock(&nvdev->lock);
> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
> -       pthread_mutex_unlock(&nvdev->lock);
> -
>         *pbo = bo;
>         return 0;
>  }
> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
>         return -ENOMEM;
>  }
>
> +static void
> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
> +{
> +       if (!nvbo->head.next) {
> +               struct nouveau_device_priv *nvdev = nouveau_device(nvbo->base.device);
> +               pthread_mutex_lock(&nvdev->lock);
> +               if (!nvbo->head.next)
Something looks a bit strange here. The list is modified under a lock,
so there should be no need for the second if above. Did you have
something else in mind ?

-Emil
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] nouveau: make nouveau importing global buffers completely thread-safe, with tests

[Emil Velikov]

On 24 February 2015 at 09:01, Maarten Lankhorst
<maarten.lankhorst@ubuntu.com> wrote:
> While I've closed off most races in a previous patch, a small race still existed
> where importing then unreffing cound cause an invalid bo. Add a test for this case.
>
> Racing sequence fixed:
>
> - thread 1 releases bo, refcount drops to zero, blocks on acquiring nvdev->lock.
> - thread 2 increases refcount to 1.
> - thread 2 decreases refcount to zero, blocks on acquiring nvdev->lock.
>
> At this point the 2 threads will clean up the same bo.
>
> How is this fixed? When thread 2 increases refcount to 1 it removes
> the bo from the list, and creates a new bo. The original thread
> will notice refcount was increased to 1 and skip deletion from list
> and closing the handle.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
> ---
>  configure.ac              |   1 +
>  nouveau/nouveau.c         |  69 +++++++++++------------
>  tests/Makefile.am         |   4 ++
>  tests/nouveau/.gitignore  |   1 +
>  tests/nouveau/Makefile.am |  15 +++++
>  tests/nouveau/threaded.c  | 140 ++++++++++++++++++++++++++++++++++++++++++++++
>  xf86atomic.h              |   6 +-
>  7 files changed, 198 insertions(+), 38 deletions(-)
>  create mode 100644 tests/nouveau/.gitignore
>  create mode 100644 tests/nouveau/Makefile.am
>  create mode 100644 tests/nouveau/threaded.c
>
> diff --git a/configure.ac b/configure.ac
> index 8afee83..6dc5044 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -441,6 +441,7 @@ AC_CONFIG_FILES([
>         tests/vbltest/Makefile
>         tests/exynos/Makefile
>         tests/tegra/Makefile
> +       tests/nouveau/Makefile
>         man/Makefile
>         libdrm.pc])
>  AC_OUTPUT
> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> index c6c153a..1c723b9 100644
> --- a/nouveau/nouveau.c
> +++ b/nouveau/nouveau.c
> @@ -351,29 +351,18 @@ nouveau_bo_del(struct nouveau_bo *bo)
>
>         pthread_mutex_lock(&nvdev->lock);
>         if (nvbo->name) {
> -               if (atomic_read(&nvbo->refcnt)) {
> +               if (atomic_read(&nvbo->refcnt) == 0) {
> +                       DRMLISTDEL(&nvbo->head);
>                         /*
> -                        * bo has been revived by a race with
> -                        * nouveau_bo_prime_handle_ref, or nouveau_bo_name_ref.
> -                        *
> -                        * In theory there's still a race possible with
> -                        * nouveau_bo_wrap, but when using this function
> -                        * the lifetime of the handle is probably already
> -                        * handled in another way. If there are races
> -                        * you're probably using nouveau_bo_wrap wrong.
> +                        * This bo has to be closed with the lock held because
> +                        * gem handles are not refcounted. If a shared bo is
> +                        * closed and re-opened in another thread a race
> +                        * against DRM_IOCTL_GEM_OPEN or drmPrimeFDToHandle
> +                        * might cause the bo to be closed accidentally while
> +                        * re-importing.
>                          */
> -                       pthread_mutex_unlock(&nvdev->lock);
> -                       return;
> +                       drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>                 }
> -               DRMLISTDEL(&nvbo->head);
> -               /*
> -                * This bo has to be closed with the lock held because gem
> -                * handles are not refcounted. If a shared bo is closed and
> -                * re-opened in another thread a race against
> -                * DRM_IOCTL_GEM_OPEN or drmPrimeFDToHandle might cause the
> -                * bo to be closed accidentally while re-importing.
> -                */
> -               drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>                 pthread_mutex_unlock(&nvdev->lock);
>         } else {
>                 DRMLISTDEL(&nvbo->head);
> @@ -418,7 +407,7 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
>
>  static int
>  nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
> -                      struct nouveau_bo **pbo)
> +                      struct nouveau_bo **pbo, int name)
>  {
>         struct nouveau_device_priv *nvdev = nouveau_device(dev);
>         struct drm_nouveau_gem_info req = { .handle = handle };
> @@ -427,8 +416,24 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
>
>         DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {
>                 if (nvbo->base.handle == handle) {
> -                       *pbo = NULL;
> -                       nouveau_bo_ref(&nvbo->base, pbo);
> +                       if (atomic_inc_return(&nvbo->refcnt) == 1) {
> +                               /*
> +                                * Uh oh, this bo is dead and someone else
> +                                * will free it, but because refcnt is
> +                                * now non-zero fortunately they won't
> +                                * call the ioctl to close the bo.
> +                                *
> +                                * Remove this bo from the list so other
> +                                * calls to nouveau_bo_wrap_locked will
> +                                * see our replacement nvbo.
> +                                */
> +                               DRMLISTDEL(&nvbo->head);
> +                               if (!name)
> +                                       name = nvbo->name;
> +                               break;
> +                       }
> +
> +                       *pbo = &nvbo->base;
>                         return 0;
>                 }
>         }
> @@ -443,6 +448,7 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
>                 atomic_set(&nvbo->refcnt, 1);
>                 nvbo->base.device = dev;
>                 abi16_bo_info(&nvbo->base, &req);
> +               nvbo->name = name;
>                 DRMLISTADD(&nvbo->head, &nvdev->bo_list);
>                 *pbo = &nvbo->base;
>                 return 0;
> @@ -458,7 +464,7 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t handle,
>         struct nouveau_device_priv *nvdev = nouveau_device(dev);
>         int ret;
>         pthread_mutex_lock(&nvdev->lock);
> -       ret = nouveau_bo_wrap_locked(dev, handle, pbo);
> +       ret = nouveau_bo_wrap_locked(dev, handle, pbo, 0);
>         pthread_mutex_unlock(&nvdev->lock);
>         return ret;
>  }
> @@ -468,24 +474,13 @@ nouveau_bo_name_ref(struct nouveau_device *dev, uint32_t name,
>                     struct nouveau_bo **pbo)
>  {
>         struct nouveau_device_priv *nvdev = nouveau_device(dev);
> -       struct nouveau_bo_priv *nvbo;
>         struct drm_gem_open req = { .name = name };
>         int ret;
>
>         pthread_mutex_lock(&nvdev->lock);
> -       DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {
> -               if (nvbo->name == name) {
> -                       *pbo = NULL;
> -                       nouveau_bo_ref(&nvbo->base, pbo);
> -                       pthread_mutex_unlock(&nvdev->lock);
> -                       return 0;
> -               }
> -       }
> -
>         ret = drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req);
>         if (ret == 0) {
> -               ret = nouveau_bo_wrap_locked(dev, req.handle, pbo);
> -               nouveau_bo((*pbo))->name = name;
> +               ret = nouveau_bo_wrap_locked(dev, req.handle, pbo, name);
>         }
>
>         pthread_mutex_unlock(&nvdev->lock);
> @@ -537,7 +532,7 @@ nouveau_bo_prime_handle_ref(struct nouveau_device *dev, int prime_fd,
>         pthread_mutex_lock(&nvdev->lock);
>         ret = drmPrimeFDToHandle(dev->fd, prime_fd, &handle);
>         if (ret == 0) {
> -               ret = nouveau_bo_wrap_locked(dev, handle, bo);
> +               ret = nouveau_bo_wrap_locked(dev, handle, bo, 0);
>                 if (!ret) {
>                         struct nouveau_bo_priv *nvbo = nouveau_bo(*bo);
>                         if (!nvbo->name) {
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 37b8d3a..38e4094 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -28,6 +28,10 @@ if HAVE_TEGRA
>  SUBDIRS += tegra
>  endif
>
> +if HAVE_NOUVEAU
> +SUBDIRS += nouveau
> +endif
> +
>  if HAVE_LIBUDEV
>
>  check_LTLIBRARIES = libdrmtest.la
> diff --git a/tests/nouveau/.gitignore b/tests/nouveau/.gitignore
> new file mode 100644
> index 0000000..837bfb9
> --- /dev/null
> +++ b/tests/nouveau/.gitignore
> @@ -0,0 +1 @@
> +threaded
> diff --git a/tests/nouveau/Makefile.am b/tests/nouveau/Makefile.am
> new file mode 100644
> index 0000000..8e3392e
> --- /dev/null
> +++ b/tests/nouveau/Makefile.am
> @@ -0,0 +1,15 @@
> +AM_CPPFLAGS = \
> +       -I$(top_srcdir)/include/drm \
> +       -I$(top_srcdir)/nouveau \
> +       -I$(top_srcdir)
> +
> +AM_CFLAGS = -Wall -Werror
Please use $(WARN_CFLAGS)

> +AM_LDFLAGS = -pthread
> +
I assume that adding -lpthread to the LDADD below will be better, but
I'm not 100% sure.

> +LDADD = -ldl \
> +       ../../nouveau/libdrm_nouveau.la \
> +       ../../libdrm.la
> +
Move -ldl at the bottom of the list so that it doesn't bite us in the future.

> +noinst_PROGRAMS = \
> +       threaded
If you want you can add this to the installed tests, and/or make check

> +
> diff --git a/tests/nouveau/threaded.c b/tests/nouveau/threaded.c
> new file mode 100644
> index 0000000..490a7fa
> --- /dev/null
> +++ b/tests/nouveau/threaded.c
> @@ -0,0 +1,140 @@
> +/*
> + * Copyright © 2015 Canonical Ltd. (Maarten Lankhorst)
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#  include "config.h"
> +#endif
> +
> +#include <sys/ioctl.h>
> +#include <dlfcn.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <pthread.h>
> +
> +#include "xf86drm.h"
> +#include "nouveau.h"
> +
> +static const char default_device[] = "/dev/dri/renderD128";
> +
Reuse the defines in xf86drm.h ?

> +static typeof(ioctl) *old_ioctl;
> +static int failed;
> +
> +static int import_fd;
> +
> +int ioctl(int fd, unsigned long request, ...)
> +{
> +       va_list va;
> +       int ret;
> +       void *arg;
> +
> +       va_start(va, request);
> +       arg = va_arg(va, void *);
> +       ret = old_ioctl(fd, request, arg);
> +       va_end(va);
> +
> +       if (ret < 0 && request == DRM_IOCTL_GEM_CLOSE && errno == EINVAL)
> +               failed = 1;
> +
> +       return ret;
> +}
> +
> +static void *
> +openclose(void *dev)
> +{
> +       struct nouveau_device *nvdev = dev;
> +       struct nouveau_bo *bo = NULL;
> +       int i;
> +
> +       for (i = 0; i < 100000; ++i) {
> +               if (!nouveau_bo_prime_handle_ref(nvdev, import_fd, &bo))
> +                       nouveau_bo_ref(NULL, &bo);
> +       }
> +       return NULL;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +       drmVersionPtr version;
> +       const char *device;
> +       int err, fd, fd2;
> +       struct nouveau_device *nvdev, *nvdev2;
> +       struct nouveau_bo *bo;
> +
> +       old_ioctl = dlsym(RTLD_NEXT, "ioctl");
> +
> +       pthread_t t1, t2;
> +
> +       if (argc < 2)
> +               device = default_device;
> +       else
> +               device = argv[1];
> +
> +       fd = open(device, O_RDWR);
> +       fd2 = open(device, O_RDWR);
> +       if (fd < 0 || fd2 < 0)
> +               return 1;
> +
> +       version = drmGetVersion(fd);
> +       if (version) {
> +               printf("Version: %d.%d.%d\n", version->version_major,
> +                      version->version_minor, version->version_patchlevel);
> +               printf("  Name: %s\n", version->name);
> +               printf("  Date: %s\n", version->date);
> +               printf("  Description: %s\n", version->desc);
> +
> +               drmFreeVersion(version);
> +       }
> +
> +       err = nouveau_device_wrap(fd, 0, &nvdev);
> +       if (!err)
> +               err = nouveau_device_wrap(fd2, 0, &nvdev2);
> +       if (err < 0)
> +               return 1;
> +
> +       err = nouveau_bo_new(nvdev2, NOUVEAU_BO_GART, 0, 4096, NULL, &bo);
> +       if (!err)
> +               err = nouveau_bo_set_prime(bo, &import_fd);
> +
> +       if (!err) {
> +               pthread_create(&t1, NULL, openclose, nvdev);
> +               pthread_create(&t2, NULL, openclose, nvdev);
> +       }
> +
> +       pthread_join(t1, NULL);
> +       pthread_join(t2, NULL);
> +
> +       close(import_fd);
> +       nouveau_bo_ref(NULL, &bo);
> +
> +       nouveau_device_del(&nvdev2);
> +       nouveau_device_del(&nvdev);
> +       close(fd2);
> +       close(fd);
> +
> +       if (failed)
> +               fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed with EINVAL,\n"
> +                               "race in opening/closing bo is likely.\n");
> +
> +       return failed;
> +}
> diff --git a/xf86atomic.h b/xf86atomic.h
> index 8c4b696..66a8d9a 100644
> --- a/xf86atomic.h
> +++ b/xf86atomic.h
> @@ -49,7 +49,8 @@ typedef struct {
>  # define atomic_read(x) ((x)->atomic)
>  # define atomic_set(x, val) ((x)->atomic = (val))
>  # define atomic_inc(x) ((void) __sync_fetch_and_add (&(x)->atomic, 1))
> -# define atomic_dec_and_test(x) (__sync_fetch_and_add (&(x)->atomic, -1) == 1)
> +# define atomic_inc_return(x) (__sync_add_and_fetch (&(x)->atomic, 1))
> +# define atomic_dec_and_test(x) (__sync_add_and_fetch (&(x)->atomic, -1) == 0)
The atomic_dec_and_test change seems like unrelated bugfix. Split it
out perhaps ?
Introduction of atomic_inc_return could/should be a separate commit as well.

>  # define atomic_add(x, v) ((void) __sync_add_and_fetch(&(x)->atomic, (v)))
>  # define atomic_dec(x, v) ((void) __sync_sub_and_fetch(&(x)->atomic, (v)))
>  # define atomic_cmpxchg(x, oldv, newv) __sync_val_compare_and_swap (&(x)->atomic, oldv, newv)
> @@ -68,6 +69,7 @@ typedef struct {
>  # define atomic_read(x) AO_load_full(&(x)->atomic)
>  # define atomic_set(x, val) AO_store_full(&(x)->atomic, (val))
>  # define atomic_inc(x) ((void) AO_fetch_and_add1_full(&(x)->atomic))
> +# define atomic_inc_return(x) (AO_fetch_and_add1_full(&(x)->atomic) + 1)
>  # define atomic_add(x, v) ((void) AO_fetch_and_add_full(&(x)->atomic, (v)))
>  # define atomic_dec(x, v) ((void) AO_fetch_and_add_full(&(x)->atomic, -(v)))
>  # define atomic_dec_and_test(x) (AO_fetch_and_sub1_full(&(x)->atomic) == 1)
> @@ -91,6 +93,7 @@ typedef struct { LIBDRM_ATOMIC_TYPE atomic; } atomic_t;
>  # define atomic_read(x) (int) ((x)->atomic)
>  # define atomic_set(x, val) ((x)->atomic = (LIBDRM_ATOMIC_TYPE)(val))
>  # define atomic_inc(x) (atomic_inc_uint (&(x)->atomic))
> +# define atomic_inc_return (atomic_inc_uint_nv(&(x)->atomic))
>  # define atomic_dec_and_test(x) (atomic_dec_uint_nv(&(x)->atomic) == 0)
>  # define atomic_add(x, v) (atomic_add_int(&(x)->atomic, (v)))
>  # define atomic_dec(x, v) (atomic_add_int(&(x)->atomic, -(v)))
> @@ -112,3 +115,4 @@ static inline int atomic_add_unless(atomic_t *v, int add, int unless)
>  }
>
>  #endif
> +
Extra white-space.

IMHO sending the series for dri-devel might be nice, for at least the
xf86atomic.h changes.

Cheers
Emil

P.S. Bless you dude for going through the lovely experience to fix this.
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Maarten Lankhorst]

Op 25-02-15 om 15:11 schreef Emil Velikov:
> On 24 February 2015 at 09:01, Maarten Lankhorst
> <maarten.lankhorst@ubuntu.com> wrote:
>> Only add wrapped bo's and bo's that have been exported through flink or dma-buf.
>> This avoids a lock in the common case, and decreases traversal needed for importing
>> a dma-buf or flink.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
>> ---
>>  nouveau/nouveau.c | 47 +++++++++++++++++++++++------------------------
>>  1 file changed, 23 insertions(+), 24 deletions(-)
>>
>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>> index 1c723b9..d411523 100644
>> --- a/nouveau/nouveau.c
>> +++ b/nouveau/nouveau.c
>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
>>         struct drm_gem_close req = { bo->handle };
>>
>> -       pthread_mutex_lock(&nvdev->lock);
>> -       if (nvbo->name) {
>> +       if (nvbo->head.next) {
>> +               pthread_mutex_lock(&nvdev->lock);
>>                 if (atomic_read(&nvbo->refcnt) == 0) {
>>                         DRMLISTDEL(&nvbo->head);
>>                         /*
>> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>                 }
>>                 pthread_mutex_unlock(&nvdev->lock);
>>         } else {
>> -               DRMLISTDEL(&nvbo->head);
>> -               pthread_mutex_unlock(&nvdev->lock);
>>                 drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>>         }
>>         if (bo->map)
>> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
>>                uint64_t size, union nouveau_bo_config *config,
>>                struct nouveau_bo **pbo)
>>  {
>> -       struct nouveau_device_priv *nvdev = nouveau_device(dev);
>>         struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
>>         struct nouveau_bo *bo = &nvbo->base;
>>         int ret;
>> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
>>                 return ret;
>>         }
>>
>> -       pthread_mutex_lock(&nvdev->lock);
>> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
>> -       pthread_mutex_unlock(&nvdev->lock);
>> -
>>         *pbo = bo;
>>         return 0;
>>  }
>> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
>>         return -ENOMEM;
>>  }
>>
>> +static void
>> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
>> +{
>> +       if (!nvbo->head.next) {
>> +               struct nouveau_device_priv *nvdev = nouveau_device(nvbo->base.device);
>> +               pthread_mutex_lock(&nvdev->lock);
>> +               if (!nvbo->head.next)
> Something looks a bit strange here. The list is modified under a lock,
> so there should be no need for the second if above. Did you have
> something else in mind ?
The unlocked if check could be removed, but checking is cheaper than locking. :P
I guess the bo_make_global call is not particularly sensitive, so removing's fine with me.

~Maarten
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Patrick Baggett]

[-- Attachment #1.1: Type: text/plain, Size: 3598 bytes --]

On Wed, Feb 25, 2015 at 8:59 AM, Maarten Lankhorst <
maarten.lankhorst-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> wrote:

> Op 25-02-15 om 15:11 schreef Emil Velikov:
> > On 24 February 2015 at 09:01, Maarten Lankhorst
> > <maarten.lankhorst-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> wrote:
> >> Only add wrapped bo's and bo's that have been exported through flink or
> dma-buf.
> >> This avoids a lock in the common case, and decreases traversal needed
> for importing
> >> a dma-buf or flink.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  nouveau/nouveau.c | 47 +++++++++++++++++++++++------------------------
> >>  1 file changed, 23 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> >> index 1c723b9..d411523 100644
> >> --- a/nouveau/nouveau.c
> >> +++ b/nouveau/nouveau.c
> >> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
> >>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
> >>         struct drm_gem_close req = { bo->handle };
> >>
> >> -       pthread_mutex_lock(&nvdev->lock);
> >> -       if (nvbo->name) {
> >> +       if (nvbo->head.next) {
> >> +               pthread_mutex_lock(&nvdev->lock);
> >>                 if (atomic_read(&nvbo->refcnt) == 0) {
> >>                         DRMLISTDEL(&nvbo->head);
> >>                         /*
> >> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
> >>                 }
> >>                 pthread_mutex_unlock(&nvdev->lock);
> >>         } else {
> >> -               DRMLISTDEL(&nvbo->head);
> >> -               pthread_mutex_unlock(&nvdev->lock);
> >>                 drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
> >>         }
> >>         if (bo->map)
> >> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t
> flags, uint32_t align,
> >>                uint64_t size, union nouveau_bo_config *config,
> >>                struct nouveau_bo **pbo)
> >>  {
> >> -       struct nouveau_device_priv *nvdev = nouveau_device(dev);
> >>         struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
> >>         struct nouveau_bo *bo = &nvbo->base;
> >>         int ret;
> >> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev,
> uint32_t flags, uint32_t align,
> >>                 return ret;
> >>         }
> >>
> >> -       pthread_mutex_lock(&nvdev->lock);
> >> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
> >> -       pthread_mutex_unlock(&nvdev->lock);
> >> -
> >>         *pbo = bo;
> >>         return 0;
> >>  }
> >> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev,
> uint32_t handle,
> >>         return -ENOMEM;
> >>  }
> >>
> >> +static void
> >> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
> >> +{
> >> +       if (!nvbo->head.next) {
> >> +               struct nouveau_device_priv *nvdev =
> nouveau_device(nvbo->base.device);
> >> +               pthread_mutex_lock(&nvdev->lock);
> >> +               if (!nvbo->head.next)
>
I guess the bo_make_global call is not particularly sensitive, so
> removing's fine with me.
>
I would be worried about the duplicated check. It seems like a "smart"
compiler could cache the value of "nvbo->head.next" (unless marked as
volatile), rendering the second if() useless. If the field is marked
volatile, then of course, this does not apply.

-Patrick

>
> ~Maarten
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
>

[-- Attachment #1.2: Type: text/html, Size: 5534 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Maarten Lankhorst]

Op 25-02-15 om 16:04 schreef Patrick Baggett:
> On Wed, Feb 25, 2015 at 8:59 AM, Maarten Lankhorst <
> maarten.lankhorst@ubuntu.com> wrote:
>
>> Op 25-02-15 om 15:11 schreef Emil Velikov:
>>> On 24 February 2015 at 09:01, Maarten Lankhorst
>>> <maarten.lankhorst@ubuntu.com> wrote:
>>>> Only add wrapped bo's and bo's that have been exported through flink or
>> dma-buf.
>>>> This avoids a lock in the common case, and decreases traversal needed
>> for importing
>>>> a dma-buf or flink.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
>>>> ---
>>>>  nouveau/nouveau.c | 47 +++++++++++++++++++++++------------------------
>>>>  1 file changed, 23 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>>>> index 1c723b9..d411523 100644
>>>> --- a/nouveau/nouveau.c
>>>> +++ b/nouveau/nouveau.c
>>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>>>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
>>>>         struct drm_gem_close req = { bo->handle };
>>>>
>>>> -       pthread_mutex_lock(&nvdev->lock);
>>>> -       if (nvbo->name) {
>>>> +       if (nvbo->head.next) {
>>>> +               pthread_mutex_lock(&nvdev->lock);
>>>>                 if (atomic_read(&nvbo->refcnt) == 0) {
>>>>                         DRMLISTDEL(&nvbo->head);
>>>>                         /*
>>>> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>>>                 }
>>>>                 pthread_mutex_unlock(&nvdev->lock);
>>>>         } else {
>>>> -               DRMLISTDEL(&nvbo->head);
>>>> -               pthread_mutex_unlock(&nvdev->lock);
>>>>                 drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>>>>         }
>>>>         if (bo->map)
>>>> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t
>> flags, uint32_t align,
>>>>                uint64_t size, union nouveau_bo_config *config,
>>>>                struct nouveau_bo **pbo)
>>>>  {
>>>> -       struct nouveau_device_priv *nvdev = nouveau_device(dev);
>>>>         struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
>>>>         struct nouveau_bo *bo = &nvbo->base;
>>>>         int ret;
>>>> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev,
>> uint32_t flags, uint32_t align,
>>>>                 return ret;
>>>>         }
>>>>
>>>> -       pthread_mutex_lock(&nvdev->lock);
>>>> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
>>>> -       pthread_mutex_unlock(&nvdev->lock);
>>>> -
>>>>         *pbo = bo;
>>>>         return 0;
>>>>  }
>>>> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev,
>> uint32_t handle,
>>>>         return -ENOMEM;
>>>>  }
>>>>
>>>> +static void
>>>> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
>>>> +{
>>>> +       if (!nvbo->head.next) {
>>>> +               struct nouveau_device_priv *nvdev =
>> nouveau_device(nvbo->base.device);
>>>> +               pthread_mutex_lock(&nvdev->lock);
>>>> +               if (!nvbo->head.next)
> I guess the bo_make_global call is not particularly sensitive, so
>> removing's fine with me.
>>
> I would be worried about the duplicated check. It seems like a "smart"
> compiler could cache the value of "nvbo->head.next" (unless marked as
> volatile), rendering the second if() useless. If the field is marked
> volatile, then of course, this does not apply.
>
In that case I would be worried about a compiler that ignores the acquire semantics of pthread_mutex_lock..

~Maarten

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Patrick Baggett]

[-- Attachment #1.1: Type: text/plain, Size: 4547 bytes --]

On Wed, Feb 25, 2015 at 9:07 AM, Maarten Lankhorst <
maarten.lankhorst-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> wrote:

> Op 25-02-15 om 16:04 schreef Patrick Baggett:
> > On Wed, Feb 25, 2015 at 8:59 AM, Maarten Lankhorst <
> > maarten.lankhorst-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> wrote:
> >
> >> Op 25-02-15 om 15:11 schreef Emil Velikov:
> >>> On 24 February 2015 at 09:01, Maarten Lankhorst
> >>> <maarten.lankhorst-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> wrote:
> >>>> Only add wrapped bo's and bo's that have been exported through flink
> or
> >> dma-buf.
> >>>> This avoids a lock in the common case, and decreases traversal needed
> >> for importing
> >>>> a dma-buf or flink.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> >>>> ---
> >>>>  nouveau/nouveau.c | 47
> +++++++++++++++++++++++------------------------
> >>>>  1 file changed, 23 insertions(+), 24 deletions(-)
> >>>>
> >>>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
> >>>> index 1c723b9..d411523 100644
> >>>> --- a/nouveau/nouveau.c
> >>>> +++ b/nouveau/nouveau.c
> >>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
> >>>>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
> >>>>         struct drm_gem_close req = { bo->handle };
> >>>>
> >>>> -       pthread_mutex_lock(&nvdev->lock);
> >>>> -       if (nvbo->name) {
> >>>> +       if (nvbo->head.next) {
> >>>> +               pthread_mutex_lock(&nvdev->lock);
> >>>>                 if (atomic_read(&nvbo->refcnt) == 0) {
> >>>>                         DRMLISTDEL(&nvbo->head);
> >>>>                         /*
> >>>> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
> >>>>                 }
> >>>>                 pthread_mutex_unlock(&nvdev->lock);
> >>>>         } else {
> >>>> -               DRMLISTDEL(&nvbo->head);
> >>>> -               pthread_mutex_unlock(&nvdev->lock);
> >>>>                 drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
> >>>>         }
> >>>>         if (bo->map)
> >>>> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev,
> uint32_t
> >> flags, uint32_t align,
> >>>>                uint64_t size, union nouveau_bo_config *config,
> >>>>                struct nouveau_bo **pbo)
> >>>>  {
> >>>> -       struct nouveau_device_priv *nvdev = nouveau_device(dev);
> >>>>         struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
> >>>>         struct nouveau_bo *bo = &nvbo->base;
> >>>>         int ret;
> >>>> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev,
> >> uint32_t flags, uint32_t align,
> >>>>                 return ret;
> >>>>         }
> >>>>
> >>>> -       pthread_mutex_lock(&nvdev->lock);
> >>>> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
> >>>> -       pthread_mutex_unlock(&nvdev->lock);
> >>>> -
> >>>>         *pbo = bo;
> >>>>         return 0;
> >>>>  }
> >>>> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device
> *dev,
> >> uint32_t handle,
> >>>>         return -ENOMEM;
> >>>>  }
> >>>>
> >>>> +static void
> >>>> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
> >>>> +{
> >>>> +       if (!nvbo->head.next) {
> >>>> +               struct nouveau_device_priv *nvdev =
> >> nouveau_device(nvbo->base.device);
> >>>> +               pthread_mutex_lock(&nvdev->lock);
> >>>> +               if (!nvbo->head.next)
> > I guess the bo_make_global call is not particularly sensitive, so
> >> removing's fine with me.
> >>
> > I would be worried about the duplicated check. It seems like a "smart"
> > compiler could cache the value of "nvbo->head.next" (unless marked as
> > volatile), rendering the second if() useless. If the field is marked
> > volatile, then of course, this does not apply.
> >
> In that case I would be worried about a compiler that ignores the acquire
> semantics of pthread_mutex_lock..
>

This isn't about the acquire semantics here, because you're reading it on
both sides of the lock. The compiler need not reorder the reads at all,
merely just save the value of the first read. Unless the compiler knows
that the value can change after the lock is acquired, it can simply cache
the result. Consider the nearly identical transformation:

const int condition = (!nvbo->head.next);
if(condition){
   pthread_mutex_lock(...);
    if(condition){ //redundant, already can assert it is true
        ...

I just wouldn't risk it. The compiler probably makes this transformation
already, but even if it doesn't, it *can*, and that's a bad thing IMO.


Patrick

>
> ~Maarten
>
>

[-- Attachment #1.2: Type: text/html, Size: 7011 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Maarten Lankhorst]

Op 25-02-15 om 16:28 schreef Patrick Baggett:
> On Wed, Feb 25, 2015 at 9:07 AM, Maarten Lankhorst <
> maarten.lankhorst@ubuntu.com> wrote:
>
>> Op 25-02-15 om 16:04 schreef Patrick Baggett:
>>> On Wed, Feb 25, 2015 at 8:59 AM, Maarten Lankhorst <
>>> maarten.lankhorst@ubuntu.com> wrote:
>>>
>>>> Op 25-02-15 om 15:11 schreef Emil Velikov:
>>>>> On 24 February 2015 at 09:01, Maarten Lankhorst
>>>>> <maarten.lankhorst@ubuntu.com> wrote:
>>>>>> Only add wrapped bo's and bo's that have been exported through flink
>> or
>>>> dma-buf.
>>>>>> This avoids a lock in the common case, and decreases traversal needed
>>>> for importing
>>>>>> a dma-buf or flink.
>>>>>>
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
>>>>>> ---
>>>>>>  nouveau/nouveau.c | 47
>> +++++++++++++++++++++++------------------------
>>>>>>  1 file changed, 23 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>>>>>> index 1c723b9..d411523 100644
>>>>>> --- a/nouveau/nouveau.c
>>>>>> +++ b/nouveau/nouveau.c
>>>>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>>>>>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
>>>>>>         struct drm_gem_close req = { bo->handle };
>>>>>>
>>>>>> -       pthread_mutex_lock(&nvdev->lock);
>>>>>> -       if (nvbo->name) {
>>>>>> +       if (nvbo->head.next) {
>>>>>> +               pthread_mutex_lock(&nvdev->lock);
>>>>>>                 if (atomic_read(&nvbo->refcnt) == 0) {
>>>>>>                         DRMLISTDEL(&nvbo->head);
>>>>>>                         /*
>>>>>> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>>>>>                 }
>>>>>>                 pthread_mutex_unlock(&nvdev->lock);
>>>>>>         } else {
>>>>>> -               DRMLISTDEL(&nvbo->head);
>>>>>> -               pthread_mutex_unlock(&nvdev->lock);
>>>>>>                 drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>>>>>>         }
>>>>>>         if (bo->map)
>>>>>> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev,
>> uint32_t
>>>> flags, uint32_t align,
>>>>>>                uint64_t size, union nouveau_bo_config *config,
>>>>>>                struct nouveau_bo **pbo)
>>>>>>  {
>>>>>> -       struct nouveau_device_priv *nvdev = nouveau_device(dev);
>>>>>>         struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
>>>>>>         struct nouveau_bo *bo = &nvbo->base;
>>>>>>         int ret;
>>>>>> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev,
>>>> uint32_t flags, uint32_t align,
>>>>>>                 return ret;
>>>>>>         }
>>>>>>
>>>>>> -       pthread_mutex_lock(&nvdev->lock);
>>>>>> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
>>>>>> -       pthread_mutex_unlock(&nvdev->lock);
>>>>>> -
>>>>>>         *pbo = bo;
>>>>>>         return 0;
>>>>>>  }
>>>>>> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device
>> *dev,
>>>> uint32_t handle,
>>>>>>         return -ENOMEM;
>>>>>>  }
>>>>>>
>>>>>> +static void
>>>>>> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
>>>>>> +{
>>>>>> +       if (!nvbo->head.next) {
>>>>>> +               struct nouveau_device_priv *nvdev =
>>>> nouveau_device(nvbo->base.device);
>>>>>> +               pthread_mutex_lock(&nvdev->lock);
>>>>>> +               if (!nvbo->head.next)
>>> I guess the bo_make_global call is not particularly sensitive, so
>>>> removing's fine with me.
>>>>
>>> I would be worried about the duplicated check. It seems like a "smart"
>>> compiler could cache the value of "nvbo->head.next" (unless marked as
>>> volatile), rendering the second if() useless. If the field is marked
>>> volatile, then of course, this does not apply.
>>>
>> In that case I would be worried about a compiler that ignores the acquire
>> semantics of pthread_mutex_lock..
>>
> This isn't about the acquire semantics here, because you're reading it on
> both sides of the lock. The compiler need not reorder the reads at all,
> merely just save the value of the first read. Unless the compiler knows
> that the value can change after the lock is acquired, it can simply cache
> the result. Consider the nearly identical transformation:
>
> const int condition = (!nvbo->head.next);
> if(condition){
>    pthread_mutex_lock(...);
>     if(condition){ //redundant, already can assert it is true
>         ...
>
So you're saying a compiler can optimize:

- statement with memory access
- read memory barrier
- statement with memory access

To drop the second statement? I would worry about your definition of memory barrier then..

~Maarten

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Emil Velikov]

On 25 February 2015 at 15:43, Maarten Lankhorst
<maarten.lankhorst@ubuntu.com> wrote:
> Op 25-02-15 om 16:28 schreef Patrick Baggett:
>> On Wed, Feb 25, 2015 at 9:07 AM, Maarten Lankhorst <
>> maarten.lankhorst@ubuntu.com> wrote:
>>
>>> Op 25-02-15 om 16:04 schreef Patrick Baggett:
>>>> On Wed, Feb 25, 2015 at 8:59 AM, Maarten Lankhorst <
>>>> maarten.lankhorst@ubuntu.com> wrote:
>>>>
>>>>> Op 25-02-15 om 15:11 schreef Emil Velikov:
>>>>>> On 24 February 2015 at 09:01, Maarten Lankhorst
>>>>>> <maarten.lankhorst@ubuntu.com> wrote:
>>>>>>> Only add wrapped bo's and bo's that have been exported through flink
>>> or
>>>>> dma-buf.
>>>>>>> This avoids a lock in the common case, and decreases traversal needed
>>>>> for importing
>>>>>>> a dma-buf or flink.
>>>>>>>
>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
>>>>>>> ---
>>>>>>>  nouveau/nouveau.c | 47
>>> +++++++++++++++++++++++------------------------
>>>>>>>  1 file changed, 23 insertions(+), 24 deletions(-)
>>>>>>>
>>>>>>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>>>>>>> index 1c723b9..d411523 100644
>>>>>>> --- a/nouveau/nouveau.c
>>>>>>> +++ b/nouveau/nouveau.c
>>>>>>> @@ -349,8 +349,8 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>>>>>>         struct nouveau_bo_priv *nvbo = nouveau_bo(bo);
>>>>>>>         struct drm_gem_close req = { bo->handle };
>>>>>>>
>>>>>>> -       pthread_mutex_lock(&nvdev->lock);
>>>>>>> -       if (nvbo->name) {
>>>>>>> +       if (nvbo->head.next) {
>>>>>>> +               pthread_mutex_lock(&nvdev->lock);
>>>>>>>                 if (atomic_read(&nvbo->refcnt) == 0) {
>>>>>>>                         DRMLISTDEL(&nvbo->head);
>>>>>>>                         /*
>>>>>>> @@ -365,8 +365,6 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>>>>>>                 }
>>>>>>>                 pthread_mutex_unlock(&nvdev->lock);
>>>>>>>         } else {
>>>>>>> -               DRMLISTDEL(&nvbo->head);
>>>>>>> -               pthread_mutex_unlock(&nvdev->lock);
>>>>>>>                 drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>>>>>>>         }
>>>>>>>         if (bo->map)
>>>>>>> @@ -379,7 +377,6 @@ nouveau_bo_new(struct nouveau_device *dev,
>>> uint32_t
>>>>> flags, uint32_t align,
>>>>>>>                uint64_t size, union nouveau_bo_config *config,
>>>>>>>                struct nouveau_bo **pbo)
>>>>>>>  {
>>>>>>> -       struct nouveau_device_priv *nvdev = nouveau_device(dev);
>>>>>>>         struct nouveau_bo_priv *nvbo = calloc(1, sizeof(*nvbo));
>>>>>>>         struct nouveau_bo *bo = &nvbo->base;
>>>>>>>         int ret;
>>>>>>> @@ -397,10 +394,6 @@ nouveau_bo_new(struct nouveau_device *dev,
>>>>> uint32_t flags, uint32_t align,
>>>>>>>                 return ret;
>>>>>>>         }
>>>>>>>
>>>>>>> -       pthread_mutex_lock(&nvdev->lock);
>>>>>>> -       DRMLISTADD(&nvbo->head, &nvdev->bo_list);
>>>>>>> -       pthread_mutex_unlock(&nvdev->lock);
>>>>>>> -
>>>>>>>         *pbo = bo;
>>>>>>>         return 0;
>>>>>>>  }
>>>>>>> @@ -457,6 +450,18 @@ nouveau_bo_wrap_locked(struct nouveau_device
>>> *dev,
>>>>> uint32_t handle,
>>>>>>>         return -ENOMEM;
>>>>>>>  }
>>>>>>>
>>>>>>> +static void
>>>>>>> +nouveau_bo_make_global(struct nouveau_bo_priv *nvbo)
>>>>>>> +{
>>>>>>> +       if (!nvbo->head.next) {
>>>>>>> +               struct nouveau_device_priv *nvdev =
>>>>> nouveau_device(nvbo->base.device);
>>>>>>> +               pthread_mutex_lock(&nvdev->lock);
>>>>>>> +               if (!nvbo->head.next)
>>>> I guess the bo_make_global call is not particularly sensitive, so
>>>>> removing's fine with me.
>>>>>
>>>> I would be worried about the duplicated check. It seems like a "smart"
>>>> compiler could cache the value of "nvbo->head.next" (unless marked as
>>>> volatile), rendering the second if() useless. If the field is marked
>>>> volatile, then of course, this does not apply.
>>>>
>>> In that case I would be worried about a compiler that ignores the acquire
>>> semantics of pthread_mutex_lock..
>>>
>> This isn't about the acquire semantics here, because you're reading it on
>> both sides of the lock. The compiler need not reorder the reads at all,
>> merely just save the value of the first read. Unless the compiler knows
>> that the value can change after the lock is acquired, it can simply cache
>> the result. Consider the nearly identical transformation:
>>
>> const int condition = (!nvbo->head.next);
>> if(condition){
>>    pthread_mutex_lock(...);
>>     if(condition){ //redundant, already can assert it is true
>>         ...
>>
> So you're saying a compiler can optimize:
>
> - statement with memory access
> - read memory barrier
> - statement with memory access
>
> To drop the second statement? I would worry about your definition of memory barrier then..
>
Patrick thanks for elaborating my concern in a more complete manner.

Maarten, I would assume that the overhead of dropping the first if
statement will be negligible wrt the time spent debugging when the
compiler does the silly thing. Afaict even with a single if, your
patch does reduce the time spend in libdrm_nouveau quite a bit.

That said, it's up-to you and others to draw the final call.

Cheers,
Emil
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Patrick Baggett]

[-- Attachment #1.1: Type: text/plain, Size: 2140 bytes --]

So you're saying a compiler can optimize:

>
> - statement with memory access
> - read memory barrier
> - statement with memory access
>
> To drop the second statement? I would worry about your definition of
> memory barrier then..
>
This is tricky, but I think you're mixing up the general case with the
actual code you have. In general, you are pointing to this example:

- Read memory location 1 (asm level)
- RMB
- Read memory location 2 (asm level)

In this case, you are precisely correct, the compiler will not reorder the
reads, and the CPU won't either.

What you actually have is this:
- Access memory location 1 (C code)
- RMB
- Acesss memory location 1 (C code)

I say "access" here because looking at the C code, not assembly. You have
this:

read nvbo->head.next
RMB
read nvbo->head.next

This only works if you can guarantee that the compiler will actually
generate a MOV instruction for the second read. If the variable isn't
marked as "volatile", the compiler has no requirement to. Barriers control
ordering and when a variable can be loaded, but you've already loaded it.
You need to reload the variable, but you haven't shown the compiler a
reason why it should. "Volatile" is a reason why.

I tested this code on gcc:

if(globalFlag) {
    math...
    if(globalFlag) {
        more math
    }
}

It only tests once. If you replace "math" with printf(), it retests the
value of globalFlag. Printf(), which has absolutely no barrier semantics at
all. In other words, GCC conservatively will assume any function call will
require a reload because it could modify anything. The code will work as-is.

Obviously, pthread_mutex_lock() does not modify the value of
`nvbo->head.next`. Once GCC knows that (and maybe other compilers already
do), perhaps with LTO, it doesn't have to generate a second load. If you
ever switch to an inlined lock or different compiler that understands that,
your code will break. I suggest you simply do the right thing the first
time and cast the pointer to volatile since you're treating the value as if
it is volatile (i.e. changing outside of the scope of the compiler's
knowledge).

Patrick

[-- Attachment #1.2: Type: text/html, Size: 2972 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Ilia Mirkin]

pthread_mutex_lock had *better* imply a compiler barrier across which
code can't be moved... which is very different from the printf case
where it might have done it due to register pressure or who knows
what.

If code like

x = *a;
pthread_mutex_lock or unlock or __memory_barrier()
y = *a;

doesn't cause a to get loaded twice, then the compiler's in serious
trouble. Basically functions like pthread_mutex_lock imply that all
memory is changed to the compiler, and thus need to be reloaded.

  -ilia


On Wed, Feb 25, 2015 at 11:28 AM, Patrick Baggett
<baggett.patrick@gmail.com> wrote:
> So you're saying a compiler can optimize:
>>
>>
>> - statement with memory access
>> - read memory barrier
>> - statement with memory access
>>
>> To drop the second statement? I would worry about your definition of
>> memory barrier then..
>
> This is tricky, but I think you're mixing up the general case with the
> actual code you have. In general, you are pointing to this example:
>
> - Read memory location 1 (asm level)
> - RMB
> - Read memory location 2 (asm level)
>
> In this case, you are precisely correct, the compiler will not reorder the
> reads, and the CPU won't either.
>
> What you actually have is this:
> - Access memory location 1 (C code)
> - RMB
> - Acesss memory location 1 (C code)
>
> I say "access" here because looking at the C code, not assembly. You have
> this:
>
> read nvbo->head.next
> RMB
> read nvbo->head.next
>
> This only works if you can guarantee that the compiler will actually
> generate a MOV instruction for the second read. If the variable isn't marked
> as "volatile", the compiler has no requirement to. Barriers control ordering
> and when a variable can be loaded, but you've already loaded it. You need to
> reload the variable, but you haven't shown the compiler a reason why it
> should. "Volatile" is a reason why.
>
> I tested this code on gcc:
>
> if(globalFlag) {
>     math...
>     if(globalFlag) {
>         more math
>     }
> }
>
> It only tests once. If you replace "math" with printf(), it retests the
> value of globalFlag. Printf(), which has absolutely no barrier semantics at
> all. In other words, GCC conservatively will assume any function call will
> require a reload because it could modify anything. The code will work as-is.
>
> Obviously, pthread_mutex_lock() does not modify the value of
> `nvbo->head.next`. Once GCC knows that (and maybe other compilers already
> do), perhaps with LTO, it doesn't have to generate a second load. If you
> ever switch to an inlined lock or different compiler that understands that,
> your code will break. I suggest you simply do the right thing the first time
> and cast the pointer to volatile since you're treating the value as if it is
> volatile (i.e. changing outside of the scope of the compiler's knowledge).
>
> Patrick
>
>
>
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Patrick Baggett]

[-- Attachment #1.1: Type: text/plain, Size: 1657 bytes --]

On Wed, Feb 25, 2015 at 10:35 AM, Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org> wrote:

> pthread_mutex_lock had *better* imply a compiler barrier across which
> code can't be moved... which is very different from the printf case
> where it might have done it due to register pressure or who knows
> what.
>
In the dummy function, register pressure was certainly not an issue, but
point taken. Still, a compiler barrier prevents reordering like:

x = *a;
y = *a;
pthread_mutex_lock()

This changes the external visibility ordering and is certainly NOT ok.


However, I contend that it doesn't stop:

const int loaded = *a;
x = loaded;
pthread_mutex_lock();
y = loaded;

Because this doesn't change the external visibility behavior, it just
changes whether a value is reloaded. It doesn't break the ordering of a
memory barrier at all.


> If code like
>
> x = *a;
> pthread_mutex_lock or unlock or __memory_barrier()
> y = *a;
>
> doesn't cause a to get loaded twice, then the compiler's in serious
> trouble. Basically functions like pthread_mutex_lock imply that all
> memory is changed to the compiler, and thus need to be reloaded.
>
> Well, I've said before and I might be alone, but I disagree with you. The
compiler is under no requirement to reload (*a) because a lock was changed.
It does, but it doesn't have to. It's fine if you guys don't want to change
it. It may never be a problem with gcc.

This is the definition of pthread_mutex_lock() in glibc. There aren't any
magic hints that this invalidates memory:

extern int pthread_mutex_lock (pthread_mutex_t *__mutex)
__THROWNL __nonnull ((1));

THOWNL is attribute((nothrow)).

[-- Attachment #1.2: Type: text/html, Size: 2538 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Ilia Mirkin]

On Wed, Feb 25, 2015 at 11:59 AM, Patrick Baggett
<baggett.patrick@gmail.com> wrote:
>> If code like
>>
>> x = *a;
>> pthread_mutex_lock or unlock or __memory_barrier()
>> y = *a;
>>
>> doesn't cause a to get loaded twice, then the compiler's in serious
>> trouble. Basically functions like pthread_mutex_lock imply that all
>> memory is changed to the compiler, and thus need to be reloaded.
>>
> Well, I've said before and I might be alone, but I disagree with you. The
> compiler is under no requirement to reload (*a) because a lock was changed.
> It does, but it doesn't have to. It's fine if you guys don't want to change
> it. It may never be a problem with gcc.
>
> This is the definition of pthread_mutex_lock() in glibc. There aren't any
> magic hints that this invalidates memory:
>
> extern int pthread_mutex_lock (pthread_mutex_t *__mutex)
> __THROWNL __nonnull ((1));
>
> THOWNL is attribute((nothrow)).

Hm, this is actually a little worrying. Maarten, thoughts? I would
have assumed there'd be a __attribute__((some_magic_thing)) in there.

  -ilia
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Maarten Lankhorst]

Hey,

On 25-02-15 18:05, Ilia Mirkin wrote:
> On Wed, Feb 25, 2015 at 11:59 AM, Patrick Baggett
> <baggett.patrick@gmail.com> wrote:
>>> If code like
>>>
>>> x = *a;
>>> pthread_mutex_lock or unlock or __memory_barrier()
>>> y = *a;
>>>
>>> doesn't cause a to get loaded twice, then the compiler's in serious
>>> trouble. Basically functions like pthread_mutex_lock imply that all
>>> memory is changed to the compiler, and thus need to be reloaded.
>>>
>> Well, I've said before and I might be alone, but I disagree with you. The
>> compiler is under no requirement to reload (*a) because a lock was changed.
>> It does, but it doesn't have to. It's fine if you guys don't want to change
>> it. It may never be a problem with gcc.
>>
>> This is the definition of pthread_mutex_lock() in glibc. There aren't any
>> magic hints that this invalidates memory:
>>
>> extern int pthread_mutex_lock (pthread_mutex_t *__mutex)
>> __THROWNL __nonnull ((1));
>>
>> THOWNL is attribute((nothrow)).
> 
> Hm, this is actually a little worrying. Maarten, thoughts? I would
> have assumed there'd be a __attribute__((some_magic_thing)) in there.
In general things don't get optimized across function calls, except in case of inlinable functions.

And for compiler attributes it's the opposite,__attribute__((const)) and __attribute((pure)) can be used to indicate some kind of safety to optimize across functions.

https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html

~Maarten
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Patrick Baggett]

[-- Attachment #1.1: Type: text/plain, Size: 557 bytes --]

>
>
> In general things don't get optimized across function calls, except in
> case of inlinable functions.
>
> And for compiler attributes it's the opposite,__attribute__((const)) and
> __attribute((pure)) can be used to indicate some kind of safety to optimize
> across functions.
>
> https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
>
> This is true, but LTO increases the compiler's ability to make these sorts
of optimizations across function calls and even C source file boundaries
without you needing to explicitly mark functions as such.

[-- Attachment #1.2: Type: text/html, Size: 1027 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Maarten Lankhorst]

On 25-02-15 18:26, Patrick Baggett wrote:
>>
>>
>> In general things don't get optimized across function calls, except in
>> case of inlinable functions.
>>
>> And for compiler attributes it's the opposite,__attribute__((const)) and
>> __attribute((pure)) can be used to indicate some kind of safety to optimize
>> across functions.
>>
>> https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
>>
>> This is true, but LTO increases the compiler's ability to make these sorts
> of optimizations across function calls and even C source file boundaries
> without you needing to explicitly mark functions as such.
Even if pthread_mutex_lock was completely inlined there would still be a asm volatile(("" ::: "memory")) in there acting as a complete memory barrier to the compiler.

Create a function called dummy.c, abusing the fact that gcc can't handle pointers well so it won't get reduced to a constant return value:
int x;
int *px = &x;

int main() {
    if (*px == 1)
        return 1;
    asm volatile("" ::: "memory");
    if (*px == 1)
        return 1;

    return -1;
}
Now compile with gcc test.c -O3 -fwhole-program, and run objdump -d a.out:
  400400:       83 3d 49 0c 20 00 01    cmpl   $0x1,0x200c49(%rip)        # 601050 <x>
  400407:       74 09                   je     400412 <main+0x12>
  400409:       83 3d 40 0c 20 00 01    cmpl   $0x1,0x200c40(%rip)        # 601050 <x>
  400410:       75 06                   jne    400418 <main+0x18>
  400412:       b8 01 00 00 00          mov    $0x1,%eax
  400417:       c3                      retq   
  400418:       83 c8 ff                or     $0xffffffff,%eax
  40041b:       c3                      retq   

Hey my second check didn't get compiled away.. magic.

And to show that a random function call does the same, replace the barrier with random():

0000000000400440 <main>:
  400440:       83 3d 09 0c 20 00 01    cmpl   $0x1,0x200c09(%rip)        # 601050 <x>
  400447:       74 1b                   je     400464 <main+0x24>
  400449:       50                      push   %rax
  40044a:       31 c0                   xor    %eax,%eax
  40044c:       e8 df ff ff ff          callq  400430 <random@plt>
  400451:       83 3d f8 0b 20 00 01    cmpl   $0x1,0x200bf8(%rip)        # 601050 <x>
  400458:       b8 01 00 00 00          mov    $0x1,%eax
  40045d:       75 0b                   jne    40046a <main+0x2a>
  40045f:       48 83 c4 08             add    $0x8,%rsp
  400463:       c3                      retq   
  400464:       b8 01 00 00 00          mov    $0x1,%eax
  400469:       c3                      retq   
  40046a:       83 c8 ff                or     $0xffffffff,%eax
  40046d:       eb f0                   jmp    40045f <main+0x1f>

And just to be thorough, showing what happens without function call or barrier:
0000000000400400 <main>:
  400400:       8b 05 4a 0c 20 00       mov    0x200c4a(%rip),%eax        # 601050 <x>
  400406:       ba ff ff ff ff          mov    $0xffffffff,%edx
  40040b:       83 f8 01                cmp    $0x1,%eax
  40040e:       0f 45 c2                cmovne %edx,%eax
  400411:       c3                      retq   

~Maarten
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 2/2] nouveau: Do not add most bo's to the global bo list.

[Patrick Baggett]

[-- Attachment #1.1: Type: text/plain, Size: 1294 bytes --]

On Wed, Feb 25, 2015 at 11:55 AM, Maarten Lankhorst <
maarten.lankhorst-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:

>
>
> On 25-02-15 18:26, Patrick Baggett wrote:
> >>
> >>
> >> In general things don't get optimized across function calls, except in
> >> case of inlinable functions.
> >>
> >> And for compiler attributes it's the opposite,__attribute__((const)) and
> >> __attribute((pure)) can be used to indicate some kind of safety to
> optimize
> >> across functions.
> >>
> >> https://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
> >>
> >> This is true, but LTO increases the compiler's ability to make these
> sorts
> > of optimizations across function calls and even C source file boundaries
> > without you needing to explicitly mark functions as such.
> Even if pthread_mutex_lock was completely inlined there would still be a
> asm volatile(("" ::: "memory")) in there acting as a complete memory
> barrier to the compiler.
>

I agree with your conclusion and I agree then that the patch can stand
as-is without any changes I proposed. I don't really think "memory barrier"
is the right word though, not that it matters. It's more like "all writes
need to finish and all *previous reads are* *invalid*". Anyways, you've
definitely proven to me that the solution is fine.

[-- Attachment #1.2: Type: text/html, Size: 1980 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] nouveau: make nouveau importing global buffers completely thread-safe, with tests

[Maarten Lankhorst]

On 25-02-15 15:14, Emil Velikov wrote:
> On 24 February 2015 at 09:01, Maarten Lankhorst
> <maarten.lankhorst@ubuntu.com> wrote:
>> While I've closed off most races in a previous patch, a small race still existed
>> where importing then unreffing cound cause an invalid bo. Add a test for this case.
>>
>> Racing sequence fixed:
>>
>> - thread 1 releases bo, refcount drops to zero, blocks on acquiring nvdev->lock.
>> - thread 2 increases refcount to 1.
>> - thread 2 decreases refcount to zero, blocks on acquiring nvdev->lock.
>>
>> At this point the 2 threads will clean up the same bo.
>>
>> How is this fixed? When thread 2 increases refcount to 1 it removes
>> the bo from the list, and creates a new bo. The original thread
>> will notice refcount was increased to 1 and skip deletion from list
>> and closing the handle.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
>> ---
>>  configure.ac              |   1 +
>>  nouveau/nouveau.c         |  69 +++++++++++------------
>>  tests/Makefile.am         |   4 ++
>>  tests/nouveau/.gitignore  |   1 +
>>  tests/nouveau/Makefile.am |  15 +++++
>>  tests/nouveau/threaded.c  | 140 ++++++++++++++++++++++++++++++++++++++++++++++
>>  xf86atomic.h              |   6 +-
>>  7 files changed, 198 insertions(+), 38 deletions(-)
>>  create mode 100644 tests/nouveau/.gitignore
>>  create mode 100644 tests/nouveau/Makefile.am
>>  create mode 100644 tests/nouveau/threaded.c
>>
>> diff --git a/configure.ac b/configure.ac
>> index 8afee83..6dc5044 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -441,6 +441,7 @@ AC_CONFIG_FILES([
>>         tests/vbltest/Makefile
>>         tests/exynos/Makefile
>>         tests/tegra/Makefile
>> +       tests/nouveau/Makefile
>>         man/Makefile
>>         libdrm.pc])
>>  AC_OUTPUT
>> diff --git a/nouveau/nouveau.c b/nouveau/nouveau.c
>> index c6c153a..1c723b9 100644
>> --- a/nouveau/nouveau.c
>> +++ b/nouveau/nouveau.c
>> @@ -351,29 +351,18 @@ nouveau_bo_del(struct nouveau_bo *bo)
>>
>>         pthread_mutex_lock(&nvdev->lock);
>>         if (nvbo->name) {
>> -               if (atomic_read(&nvbo->refcnt)) {
>> +               if (atomic_read(&nvbo->refcnt) == 0) {
>> +                       DRMLISTDEL(&nvbo->head);
>>                         /*
>> -                        * bo has been revived by a race with
>> -                        * nouveau_bo_prime_handle_ref, or nouveau_bo_name_ref.
>> -                        *
>> -                        * In theory there's still a race possible with
>> -                        * nouveau_bo_wrap, but when using this function
>> -                        * the lifetime of the handle is probably already
>> -                        * handled in another way. If there are races
>> -                        * you're probably using nouveau_bo_wrap wrong.
>> +                        * This bo has to be closed with the lock held because
>> +                        * gem handles are not refcounted. If a shared bo is
>> +                        * closed and re-opened in another thread a race
>> +                        * against DRM_IOCTL_GEM_OPEN or drmPrimeFDToHandle
>> +                        * might cause the bo to be closed accidentally while
>> +                        * re-importing.
>>                          */
>> -                       pthread_mutex_unlock(&nvdev->lock);
>> -                       return;
>> +                       drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>>                 }
>> -               DRMLISTDEL(&nvbo->head);
>> -               /*
>> -                * This bo has to be closed with the lock held because gem
>> -                * handles are not refcounted. If a shared bo is closed and
>> -                * re-opened in another thread a race against
>> -                * DRM_IOCTL_GEM_OPEN or drmPrimeFDToHandle might cause the
>> -                * bo to be closed accidentally while re-importing.
>> -                */
>> -               drmIoctl(bo->device->fd, DRM_IOCTL_GEM_CLOSE, &req);
>>                 pthread_mutex_unlock(&nvdev->lock);
>>         } else {
>>                 DRMLISTDEL(&nvbo->head);
>> @@ -418,7 +407,7 @@ nouveau_bo_new(struct nouveau_device *dev, uint32_t flags, uint32_t align,
>>
>>  static int
>>  nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
>> -                      struct nouveau_bo **pbo)
>> +                      struct nouveau_bo **pbo, int name)
>>  {
>>         struct nouveau_device_priv *nvdev = nouveau_device(dev);
>>         struct drm_nouveau_gem_info req = { .handle = handle };
>> @@ -427,8 +416,24 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
>>
>>         DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {
>>                 if (nvbo->base.handle == handle) {
>> -                       *pbo = NULL;
>> -                       nouveau_bo_ref(&nvbo->base, pbo);
>> +                       if (atomic_inc_return(&nvbo->refcnt) == 1) {
>> +                               /*
>> +                                * Uh oh, this bo is dead and someone else
>> +                                * will free it, but because refcnt is
>> +                                * now non-zero fortunately they won't
>> +                                * call the ioctl to close the bo.
>> +                                *
>> +                                * Remove this bo from the list so other
>> +                                * calls to nouveau_bo_wrap_locked will
>> +                                * see our replacement nvbo.
>> +                                */
>> +                               DRMLISTDEL(&nvbo->head);
>> +                               if (!name)
>> +                                       name = nvbo->name;
>> +                               break;
>> +                       }
>> +
>> +                       *pbo = &nvbo->base;
>>                         return 0;
>>                 }
>>         }
>> @@ -443,6 +448,7 @@ nouveau_bo_wrap_locked(struct nouveau_device *dev, uint32_t handle,
>>                 atomic_set(&nvbo->refcnt, 1);
>>                 nvbo->base.device = dev;
>>                 abi16_bo_info(&nvbo->base, &req);
>> +               nvbo->name = name;
>>                 DRMLISTADD(&nvbo->head, &nvdev->bo_list);
>>                 *pbo = &nvbo->base;
>>                 return 0;
>> @@ -458,7 +464,7 @@ nouveau_bo_wrap(struct nouveau_device *dev, uint32_t handle,
>>         struct nouveau_device_priv *nvdev = nouveau_device(dev);
>>         int ret;
>>         pthread_mutex_lock(&nvdev->lock);
>> -       ret = nouveau_bo_wrap_locked(dev, handle, pbo);
>> +       ret = nouveau_bo_wrap_locked(dev, handle, pbo, 0);
>>         pthread_mutex_unlock(&nvdev->lock);
>>         return ret;
>>  }
>> @@ -468,24 +474,13 @@ nouveau_bo_name_ref(struct nouveau_device *dev, uint32_t name,
>>                     struct nouveau_bo **pbo)
>>  {
>>         struct nouveau_device_priv *nvdev = nouveau_device(dev);
>> -       struct nouveau_bo_priv *nvbo;
>>         struct drm_gem_open req = { .name = name };
>>         int ret;
>>
>>         pthread_mutex_lock(&nvdev->lock);
>> -       DRMLISTFOREACHENTRY(nvbo, &nvdev->bo_list, head) {
>> -               if (nvbo->name == name) {
>> -                       *pbo = NULL;
>> -                       nouveau_bo_ref(&nvbo->base, pbo);
>> -                       pthread_mutex_unlock(&nvdev->lock);
>> -                       return 0;
>> -               }
>> -       }
>> -
>>         ret = drmIoctl(dev->fd, DRM_IOCTL_GEM_OPEN, &req);
>>         if (ret == 0) {
>> -               ret = nouveau_bo_wrap_locked(dev, req.handle, pbo);
>> -               nouveau_bo((*pbo))->name = name;
>> +               ret = nouveau_bo_wrap_locked(dev, req.handle, pbo, name);
>>         }
>>
>>         pthread_mutex_unlock(&nvdev->lock);
>> @@ -537,7 +532,7 @@ nouveau_bo_prime_handle_ref(struct nouveau_device *dev, int prime_fd,
>>         pthread_mutex_lock(&nvdev->lock);
>>         ret = drmPrimeFDToHandle(dev->fd, prime_fd, &handle);
>>         if (ret == 0) {
>> -               ret = nouveau_bo_wrap_locked(dev, handle, bo);
>> +               ret = nouveau_bo_wrap_locked(dev, handle, bo, 0);
>>                 if (!ret) {
>>                         struct nouveau_bo_priv *nvbo = nouveau_bo(*bo);
>>                         if (!nvbo->name) {
>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> index 37b8d3a..38e4094 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -28,6 +28,10 @@ if HAVE_TEGRA
>>  SUBDIRS += tegra
>>  endif
>>
>> +if HAVE_NOUVEAU
>> +SUBDIRS += nouveau
>> +endif
>> +
>>  if HAVE_LIBUDEV
>>
>>  check_LTLIBRARIES = libdrmtest.la
>> diff --git a/tests/nouveau/.gitignore b/tests/nouveau/.gitignore
>> new file mode 100644
>> index 0000000..837bfb9
>> --- /dev/null
>> +++ b/tests/nouveau/.gitignore
>> @@ -0,0 +1 @@
>> +threaded
>> diff --git a/tests/nouveau/Makefile.am b/tests/nouveau/Makefile.am
>> new file mode 100644
>> index 0000000..8e3392e
>> --- /dev/null
>> +++ b/tests/nouveau/Makefile.am
>> @@ -0,0 +1,15 @@
>> +AM_CPPFLAGS = \
>> +       -I$(top_srcdir)/include/drm \
>> +       -I$(top_srcdir)/nouveau \
>> +       -I$(top_srcdir)
>> +
>> +AM_CFLAGS = -Wall -Werror
> Please use $(WARN_CFLAGS)
> 
>> +AM_LDFLAGS = -pthread
>> +
> I assume that adding -lpthread to the LDADD below will be better, but
> I'm not 100% sure.
> 
>> +LDADD = -ldl \
>> +       ../../nouveau/libdrm_nouveau.la \
>> +       ../../libdrm.la
>> +
> Move -ldl at the bottom of the list so that it doesn't bite us in the future.
Ok might be useful indeed.

>> +noinst_PROGRAMS = \
>> +       threaded
> If you want you can add this to the installed tests, and/or make check
make check might fail if nouveau is not found or no render nodes are available, I'd have to add a skip then.

>> +
>> diff --git a/tests/nouveau/threaded.c b/tests/nouveau/threaded.c
>> new file mode 100644
>> index 0000000..490a7fa
>> --- /dev/null
>> +++ b/tests/nouveau/threaded.c
>> @@ -0,0 +1,140 @@
>> +/*
>> + * Copyright © 2015 Canonical Ltd. (Maarten Lankhorst)
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#  include "config.h"
>> +#endif
>> +
>> +#include <sys/ioctl.h>
>> +#include <dlfcn.h>
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <errno.h>
>> +#include <pthread.h>
>> +
>> +#include "xf86drm.h"
>> +#include "nouveau.h"
>> +
>> +static const char default_device[] = "/dev/dri/renderD128";
>> +
> Reuse the defines in xf86drm.h ?

Hmm, switching to drmOpenWithType("nouveau", NULL, DRM_NODE_RENDER) might be better..

>> +static typeof(ioctl) *old_ioctl;
>> +static int failed;
>> +
>> +static int import_fd;
>> +
>> +int ioctl(int fd, unsigned long request, ...)
>> +{
>> +       va_list va;
>> +       int ret;
>> +       void *arg;
>> +
>> +       va_start(va, request);
>> +       arg = va_arg(va, void *);
>> +       ret = old_ioctl(fd, request, arg);
>> +       va_end(va);
>> +
>> +       if (ret < 0 && request == DRM_IOCTL_GEM_CLOSE && errno == EINVAL)
>> +               failed = 1;
>> +
>> +       return ret;
>> +}
>> +
>> +static void *
>> +openclose(void *dev)
>> +{
>> +       struct nouveau_device *nvdev = dev;
>> +       struct nouveau_bo *bo = NULL;
>> +       int i;
>> +
>> +       for (i = 0; i < 100000; ++i) {
>> +               if (!nouveau_bo_prime_handle_ref(nvdev, import_fd, &bo))
>> +                       nouveau_bo_ref(NULL, &bo);
>> +       }
>> +       return NULL;
>> +}
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +       drmVersionPtr version;
>> +       const char *device;
>> +       int err, fd, fd2;
>> +       struct nouveau_device *nvdev, *nvdev2;
>> +       struct nouveau_bo *bo;
>> +
>> +       old_ioctl = dlsym(RTLD_NEXT, "ioctl");
>> +
>> +       pthread_t t1, t2;
>> +
>> +       if (argc < 2)
>> +               device = default_device;
>> +       else
>> +               device = argv[1];
>> +
>> +       fd = open(device, O_RDWR);
>> +       fd2 = open(device, O_RDWR);
>> +       if (fd < 0 || fd2 < 0)
>> +               return 1;
>> +
>> +       version = drmGetVersion(fd);
>> +       if (version) {
>> +               printf("Version: %d.%d.%d\n", version->version_major,
>> +                      version->version_minor, version->version_patchlevel);
>> +               printf("  Name: %s\n", version->name);
>> +               printf("  Date: %s\n", version->date);
>> +               printf("  Description: %s\n", version->desc);
>> +
>> +               drmFreeVersion(version);
>> +       }
>> +
>> +       err = nouveau_device_wrap(fd, 0, &nvdev);
>> +       if (!err)
>> +               err = nouveau_device_wrap(fd2, 0, &nvdev2);
>> +       if (err < 0)
>> +               return 1;
>> +
>> +       err = nouveau_bo_new(nvdev2, NOUVEAU_BO_GART, 0, 4096, NULL, &bo);
>> +       if (!err)
>> +               err = nouveau_bo_set_prime(bo, &import_fd);
>> +
>> +       if (!err) {
>> +               pthread_create(&t1, NULL, openclose, nvdev);
>> +               pthread_create(&t2, NULL, openclose, nvdev);
>> +       }
>> +
>> +       pthread_join(t1, NULL);
>> +       pthread_join(t2, NULL);
>> +
>> +       close(import_fd);
>> +       nouveau_bo_ref(NULL, &bo);
>> +
>> +       nouveau_device_del(&nvdev2);
>> +       nouveau_device_del(&nvdev);
>> +       close(fd2);
>> +       close(fd);
>> +
>> +       if (failed)
>> +               fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed with EINVAL,\n"
>> +                               "race in opening/closing bo is likely.\n");
>> +
>> +       return failed;
>> +}
>> diff --git a/xf86atomic.h b/xf86atomic.h
>> index 8c4b696..66a8d9a 100644
>> --- a/xf86atomic.h
>> +++ b/xf86atomic.h
>> @@ -49,7 +49,8 @@ typedef struct {
>>  # define atomic_read(x) ((x)->atomic)
>>  # define atomic_set(x, val) ((x)->atomic = (val))
>>  # define atomic_inc(x) ((void) __sync_fetch_and_add (&(x)->atomic, 1))
>> -# define atomic_dec_and_test(x) (__sync_fetch_and_add (&(x)->atomic, -1) == 1)
>> +# define atomic_inc_return(x) (__sync_add_and_fetch (&(x)->atomic, 1))
>> +# define atomic_dec_and_test(x) (__sync_add_and_fetch (&(x)->atomic, -1) == 0)
> The atomic_dec_and_test change seems like unrelated bugfix. Split it
> out perhaps ?
Not a bug fix, just swapping the order..
- atomic_fetch_and_add(-1) == 1
+ atomic_add_and_fetch(-1) == 0

> Introduction of atomic_inc_return could/should be a separate commit as well.



>>  # define atomic_add(x, v) ((void) __sync_add_and_fetch(&(x)->atomic, (v)))
>>  # define atomic_dec(x, v) ((void) __sync_sub_and_fetch(&(x)->atomic, (v)))
>>  # define atomic_cmpxchg(x, oldv, newv) __sync_val_compare_and_swap (&(x)->atomic, oldv, newv)
>> @@ -68,6 +69,7 @@ typedef struct {
>>  # define atomic_read(x) AO_load_full(&(x)->atomic)
>>  # define atomic_set(x, val) AO_store_full(&(x)->atomic, (val))
>>  # define atomic_inc(x) ((void) AO_fetch_and_add1_full(&(x)->atomic))
>> +# define atomic_inc_return(x) (AO_fetch_and_add1_full(&(x)->atomic) + 1)
>>  # define atomic_add(x, v) ((void) AO_fetch_and_add_full(&(x)->atomic, (v)))
>>  # define atomic_dec(x, v) ((void) AO_fetch_and_add_full(&(x)->atomic, -(v)))
>>  # define atomic_dec_and_test(x) (AO_fetch_and_sub1_full(&(x)->atomic) == 1)
>> @@ -91,6 +93,7 @@ typedef struct { LIBDRM_ATOMIC_TYPE atomic; } atomic_t;
>>  # define atomic_read(x) (int) ((x)->atomic)
>>  # define atomic_set(x, val) ((x)->atomic = (LIBDRM_ATOMIC_TYPE)(val))
>>  # define atomic_inc(x) (atomic_inc_uint (&(x)->atomic))
>> +# define atomic_inc_return (atomic_inc_uint_nv(&(x)->atomic))
>>  # define atomic_dec_and_test(x) (atomic_dec_uint_nv(&(x)->atomic) == 0)
>>  # define atomic_add(x, v) (atomic_add_int(&(x)->atomic, (v)))
>>  # define atomic_dec(x, v) (atomic_add_int(&(x)->atomic, -(v)))
>> @@ -112,3 +115,4 @@ static inline int atomic_add_unless(atomic_t *v, int add, int unless)
>>  }
>>
>>  #endif
>> +
> Extra white-space.
> 
> IMHO sending the series for dri-devel might be nice, for at least the
> xf86atomic.h changes.
Probably, not sure anyone will review it though, so I think just adding in a separate commit is fine.

> P.S. Bless you dude for going through the lovely experience to fix this.
Np. When I find some time I'll try nouveau in mesa next. :)

~Maarten
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 1/2] nouveau: make nouveau importing global buffers completely thread-safe, with tests

[Emil Velikov]

On 25 February 2015 at 21:59, Maarten Lankhorst
<maarten.lankhorst@canonical.com> wrote:
> On 25-02-15 15:14, Emil Velikov wrote:
>> On 24 February 2015 at 09:01, Maarten Lankhorst
>> <maarten.lankhorst@ubuntu.com> wrote:
...
>>> +static const char default_device[] = "/dev/dri/renderD128";
>>> +
>> Reuse the defines in xf86drm.h ?
>
> Hmm, switching to drmOpenWithType("nouveau", NULL, DRM_NODE_RENDER) might be better..
>
I saw that patch floating on the list, but did not know it landed already. Nice

...
>>> -# define atomic_dec_and_test(x) (__sync_fetch_and_add (&(x)->atomic, -1) == 1)
>>> +# define atomic_inc_return(x) (__sync_add_and_fetch (&(x)->atomic, 1))
>>> +# define atomic_dec_and_test(x) (__sync_add_and_fetch (&(x)->atomic, -1) == 0)
>> The atomic_dec_and_test change seems like unrelated bugfix. Split it
>> out perhaps ?
> Not a bug fix, just swapping the order..
> - atomic_fetch_and_add(-1) == 1
> + atomic_add_and_fetch(-1) == 0
>
Oops, I'm blind.

...
>> IMHO sending the series for dri-devel might be nice, for at least the
>> xf86atomic.h changes.
> Probably, not sure anyone will review it though, so I think just adding in a separate commit is fine.
>
If they don't then it's their problem. At least you've done the right thing :-)

Thanks
Emil
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau