nouveau: make nouveau importing global buffers completely thread-safe, with tests

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.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@ubuntu.com>
Reviewed-By: Emil Velikov <emil.l.velikov@gmail.com>
main
Maarten Lankhorst 2015-02-26 11:54:03 +01:00 committed by Maarten Lankhorst
parent 7caa442e76
commit 5ea6f1c326
6 changed files with 210 additions and 37 deletions

View File

@ -454,6 +454,7 @@ AC_CONFIG_FILES([
tests/vbltest/Makefile
tests/exynos/Makefile
tests/tegra/Makefile
tests/nouveau/Makefile
man/Makefile
libdrm.pc])
AC_OUTPUT

View File

@ -351,29 +351,18 @@ nouveau_bo_del(struct nouveau_bo *bo)
pthread_mutex_lock(&nvdev->lock);
if (nvbo->name) {
if (atomic_read(&nvbo->refcnt)) {
/*
* 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.
*/
pthread_mutex_unlock(&nvdev->lock);
return;
}
if (atomic_read(&nvbo->refcnt) == 0) {
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.
* 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) {

View File

@ -31,6 +31,10 @@ check_PROGRAMS = \
dristat \
drmstat
if HAVE_NOUVEAU
SUBDIRS += nouveau
endif
if HAVE_LIBUDEV
check_LTLIBRARIES = libdrmtest.la

1
tests/nouveau/.gitignore vendored Normal file
View File

@ -0,0 +1 @@
threaded

16
tests/nouveau/Makefile.am Normal file
View File

@ -0,0 +1,16 @@
AM_CPPFLAGS = \
-I$(top_srcdir)/include/drm \
-I$(top_srcdir)/nouveau \
-I$(top_srcdir)
AM_CFLAGS = $(WARN_CFLAGS)
LDADD = \
../../nouveau/libdrm_nouveau.la \
../../libdrm.la \
-ldl -lpthread
TESTS = threaded
check_PROGRAMS = $(TESTS)

156
tests/nouveau/threaded.c Normal file
View File

@ -0,0 +1,156 @@
/*
* 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 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 = NULL;
int err, fd, fd2;
struct nouveau_device *nvdev, *nvdev2;
struct nouveau_bo *bo;
pthread_t t1, t2;
old_ioctl = dlsym(RTLD_NEXT, "ioctl");
if (argc < 2) {
fd = drmOpenWithType("nouveau", NULL, DRM_NODE_RENDER);
if (fd >= 0)
fd2 = drmOpenWithType("nouveau", NULL, DRM_NODE_RENDER);
} else {
device = argv[1];
fd = open(device, O_RDWR);
if (fd >= 0)
fd2 = open(device, O_RDWR);
else
fd2 = fd = -errno;
}
if (fd < 0) {
fprintf(stderr, "Opening nouveau render node failed with %i\n", fd);
return device ? -fd : 77;
}
if (fd2 < 0) {
fprintf(stderr, "Opening second nouveau render node failed with %i\n", -errno);
return errno;
}
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);
if (device) {
close(fd2);
close(fd);
} else {
drmClose(fd2);
drmClose(fd);
}
if (failed)
fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed with EINVAL,\n"
"race in opening/closing bo is likely.\n");
return failed;
}