Hi

On Mon, May 4, 2015 at 4:46 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:

...

The magic auth tokens we have are a simple map from cyclic IDs to drm_file objects. Remove all the old bulk of code and replace it with a simple, direct IDR.

The previous behavior is kept. Especially calling authmagic multiple times on the same magic results in EINVAL except on the first call. The only difference in behavior is that we never allocate IDs multiple times, even if they were already authenticated. To trigger that, you need 2^31 open DRM file descriptions at the same time, though.

Diff-stat: 5 files changed, 45 insertions(+), 157 deletions(-)

Signed-off-by: David Herrmann dh.herrmann@gmail.com

Just one quibble,

...

@@ -189,10 +80,17 @@ int drm_authmagic(struct drm_device *dev, void *data, struct drm_file *file;

  DRM_DEBUG("%u\n", auth->magic);

• if ((file = drm_find_file(file_priv->master, auth->magic))) {
  

• if (auth->magic >= INT_MAX)
  

•         return -EINVAL;
  

The idr upper bound is INT_MAX [inclusive].

...

• mutex_lock(&dev->struct_mutex);
  

• file = idr_find(&file_priv->master->magic_map, auth->magic);
  

But given that it will return NULL anyway, do you really want to short-circuit the erroneous lookup, and just leave it to the idr to validate it itself?

Indeed, I can just change it to "auth->magic > INT_MAX". Fixed!

So... How efficient is the cyclic idr for a long running system that has accumulated several hundred thousand stale magics?

Maybe an igt to create a couple of billion shortlived clients (with overlapping lifetimes) and check the behaviour?

I'm not sure this is an interesting benchmark. I mean, you usually have a dozen DRM clients max, anyway. So the only interesting detail is whether the cyclic behavior causes the IDR tree to degenerate. But in that case, we should either fix IDR or see whether we can avoid the cyclic behavior.

Given that I use idr_replace(idr, NULL, id) on auth, instead of an immediate idr_remove(), we can just use idr_alloc() instead of idr_alloc_cyclic(). However, idr_alloc_cyclic() seems to work fine for inotify, so it should be fine for auth-magics, I guess (and auth-magics aren't part of any fast-path, right?).

Anyway, I can ping Tejun and ask whether the cyclic IDR has any visible downsides compared to normal allocation. He should know.

Thanks! David

Hi

On Mon, May 4, 2015 at 5:11 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

On Mon, May 04, 2015 at 05:02:37PM +0200, David Herrmann wrote:

...

Hi

On Mon, May 4, 2015 at 4:46 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

...

On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:

...

The magic auth tokens we have are a simple map from cyclic IDs to drm_file objects. Remove all the old bulk of code and replace it with a simple, direct IDR.

The previous behavior is kept. Especially calling authmagic multiple times on the same magic results in EINVAL except on the first call. The only difference in behavior is that we never allocate IDs multiple times, even if they were already authenticated. To trigger that, you need 2^31 open DRM file descriptions at the same time, though.

Diff-stat: 5 files changed, 45 insertions(+), 157 deletions(-)

Signed-off-by: David Herrmann dh.herrmann@gmail.com

Just one quibble,

...

@@ -189,10 +80,17 @@ int drm_authmagic(struct drm_device *dev, void *data, struct drm_file *file;

  DRM_DEBUG("%u\n", auth->magic);

• if ((file = drm_find_file(file_priv->master, auth->magic))) {
  

• if (auth->magic >= INT_MAX)
  

•         return -EINVAL;
  

The idr upper bound is INT_MAX [inclusive].

...

• mutex_lock(&dev->struct_mutex);
  

• file = idr_find(&file_priv->master->magic_map, auth->magic);
  

But given that it will return NULL anyway, do you really want to short-circuit the erroneous lookup, and just leave it to the idr to validate it itself?

Indeed, I can just change it to "auth->magic > INT_MAX". Fixed!

...

So... How efficient is the cyclic idr for a long running system that has accumulated several hundred thousand stale magics?

Maybe an igt to create a couple of billion shortlived clients (with overlapping lifetimes) and check the behaviour?

I'm not sure this is an interesting benchmark. I mean, you usually have a dozen DRM clients max, anyway. So the only interesting detail is whether the cyclic behavior causes the IDR tree to degenerate. But in that case, we should either fix IDR or see whether we can avoid the cyclic behavior.

I was actually more concerned about what happens if we end up with 2 billion stale clients - do we get 2 billion entries? Can we even allocate that many? I suspect we end up with a DoS.

Ehh, the magic-entries are dropped on close(). So to actually keep 2 billion entries, you also need 2 billion "struct file*", "struct drm_file*", ...

The idr_alloc_cyclic() does _not_ mark old entries as "used". All it does is try to remember new IDs in a cyclic order. On wrap-around, old IDs will get re-used.

Thanks David

Hi

On Mon, May 4, 2015 at 5:25 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

On Mon, May 04, 2015 at 05:14:51PM +0200, David Herrmann wrote:

...

Hi

On Mon, May 4, 2015 at 5:11 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

...

On Mon, May 04, 2015 at 05:02:37PM +0200, David Herrmann wrote:

...

Hi

On Mon, May 4, 2015 at 4:46 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

...

On Mon, May 04, 2015 at 04:05:13PM +0200, David Herrmann wrote:

...

The magic auth tokens we have are a simple map from cyclic IDs to drm_file objects. Remove all the old bulk of code and replace it with a simple, direct IDR.

The previous behavior is kept. Especially calling authmagic multiple times on the same magic results in EINVAL except on the first call. The only difference in behavior is that we never allocate IDs multiple times, even if they were already authenticated. To trigger that, you need 2^31 open DRM file descriptions at the same time, though.

Diff-stat: 5 files changed, 45 insertions(+), 157 deletions(-)

Signed-off-by: David Herrmann dh.herrmann@gmail.com

Just one quibble,

...

@@ -189,10 +80,17 @@ int drm_authmagic(struct drm_device *dev, void *data, struct drm_file *file;

  DRM_DEBUG("%u\n", auth->magic);

• if ((file = drm_find_file(file_priv->master, auth->magic))) {
  

• if (auth->magic >= INT_MAX)
  

•         return -EINVAL;
  

The idr upper bound is INT_MAX [inclusive].

...

• mutex_lock(&dev->struct_mutex);
  

• file = idr_find(&file_priv->master->magic_map, auth->magic);
  

But given that it will return NULL anyway, do you really want to short-circuit the erroneous lookup, and just leave it to the idr to validate it itself?

Indeed, I can just change it to "auth->magic > INT_MAX". Fixed!

...

So... How efficient is the cyclic idr for a long running system that has accumulated several hundred thousand stale magics?

Maybe an igt to create a couple of billion shortlived clients (with overlapping lifetimes) and check the behaviour?

I'm not sure this is an interesting benchmark. I mean, you usually have a dozen DRM clients max, anyway. So the only interesting detail is whether the cyclic behavior causes the IDR tree to degenerate. But in that case, we should either fix IDR or see whether we can avoid the cyclic behavior.

I was actually more concerned about what happens if we end up with 2 billion stale clients - do we get 2 billion entries? Can we even allocate that many? I suspect we end up with a DoS.

Ehh, the magic-entries are dropped on close(). So to actually keep 2 billion entries, you also need 2 billion "struct file*", "struct drm_file*", ...

The idr_alloc_cyclic() does _not_ mark old entries as "used". All it does is try to remember new IDs in a cyclic order. On wrap-around, old IDs will get re-used.

But the layers are only freed if all below them are empty (iiui). Basically, I am not entirely familar with how idr will cope on a long running system, and would like some elaboration (and a test for future reference!).

IDR uses a tree of idr_layer objects (basically an optimized suffix tree on the binary ID). Tree depth is increased dynamically if a layer runs full. Max depths of layers is 4. Each layer manages 256 children (leafs store the void* data, nodes store pointers to sub-layers). Size of a layer is roughly 2k on 64bit.

If the ID-range is small and packed, the storage is used optimally. But a sparse ID-range might occupy one layer per ID in worst case. Lookup speed should be negligible as we are limited to a depth of 4 layers. However, memory consumption might get huge if the IDs are spread even. But this really depends on the allocation scheme.

I'm not sure how to write a benchmark for this. I mean, I can easily craft one that causes the IDR to degenerate, but it requires us to keep huge numbers of files open. But this fact makes IDR rather suboptimal for cyclic allocations, so I switched to idr_alloc() now. This guarantees tight/packed ID ranges and user-space cannot degenerate the layers, anymore. That is, unless you open more than 256 FDs on a device in parallel, we're fine with a single IDR layer; always. This should work fine, right?

Thanks David

The magic auth tokens we have are a simple map from cyclic IDs to drm_file objects. Remove all the old bulk of code and replace it with a simple, direct IDR.

The previous behavior is kept. Especially calling authmagic multiple times on the same magic results in EINVAL except on the first call. The only difference in behavior is that we never allocate IDs multiple times as long as a client has its FD open.

Diff-stat: 5 files changed, 42 insertions(+), 157 deletions(-)

Signed-off-by: David Herrmann dh.herrmann@gmail.com Reviewed-by: Chris Wilson chris@chris-wilson.co.uk --- v3: - drop redundant "magic > INT_MAX" check

v2: - Fix return code of GetMagic() - Use non-cyclic IDR allocator - fix off-by-one in "magic > INT_MAX" sanity check

drivers/gpu/drm/drm_auth.c | 175 +++++++++-------------------------------- drivers/gpu/drm/drm_drv.c | 12 +-- drivers/gpu/drm/drm_fops.c | 7 +- drivers/gpu/drm/drm_internal.h | 1 - include/drm/drmP.h | 4 +- 5 files changed, 42 insertions(+), 157 deletions(-)

diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 8a37524..50d0baa 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -1,11 +1,3 @@ -/** - * \file drm_auth.c - * IOCTLs for authentication - * - * \author Rickard E. (Rik) Faith faith@valinux.com - * \author Gareth Hughes gareth@valinux.com - */ - /* * Created: Tue Feb 2 08:37:54 1999 by faith@valinux.com * @@ -13,6 +5,9 @@ * Copyright 2000 VA Linux Systems, Inc., Sunnyvale, California. * All Rights Reserved. * + * Author Rickard E. (Rik) Faith faith@valinux.com + * Author Gareth Hughes gareth@valinux.com + * * 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 @@ -36,151 +31,47 @@ #include <drm/drmP.h> #include "drm_internal.h"

-struct drm_magic_entry { - struct drm_hash_item hash_item; - struct drm_file *priv; -}; - -/** - * Find the file with the given magic number. - * - * \param dev DRM device. - * \param magic magic number. - * - * Searches in drm_device::magiclist within all files with the same hash key - * the one with matching magic number, while holding the drm_device::struct_mutex - * lock. - */ -static struct drm_file *drm_find_file(struct drm_master *master, drm_magic_t magic) -{ - struct drm_file *retval = NULL; - struct drm_magic_entry *pt; - struct drm_hash_item *hash; - struct drm_device *dev = master->minor->dev; - - mutex_lock(&dev->struct_mutex); - if (!drm_ht_find_item(&master->magiclist, (unsigned long)magic, &hash)) { - pt = drm_hash_entry(hash, struct drm_magic_entry, hash_item); - retval = pt->priv; - } - mutex_unlock(&dev->struct_mutex); - return retval; -} - -/** - * Adds a magic number. - * - * \param dev DRM device. - * \param priv file private data. - * \param magic magic number. - * - * Creates a drm_magic_entry structure and appends to the linked list - * associated the magic number hash key in drm_device::magiclist, while holding - * the drm_device::struct_mutex lock. - */ -static int drm_add_magic(struct drm_master *master, struct drm_file *priv, - drm_magic_t magic) -{ - struct drm_magic_entry *entry; - struct drm_device *dev = master->minor->dev; - DRM_DEBUG("%d\n", magic); - - entry = kzalloc(sizeof(*entry), GFP_KERNEL); - if (!entry) - return -ENOMEM; - entry->priv = priv; - entry->hash_item.key = (unsigned long)magic; - mutex_lock(&dev->struct_mutex); - drm_ht_insert_item(&master->magiclist, &entry->hash_item); - mutex_unlock(&dev->struct_mutex); - - return 0; -} - -/** - * Remove a magic number. - * - * \param dev DRM device. - * \param magic magic number. - * - * Searches and unlinks the entry in drm_device::magiclist with the magic - * number hash key, while holding the drm_device::struct_mutex lock. - */ -int drm_remove_magic(struct drm_master *master, drm_magic_t magic) -{ - struct drm_magic_entry *pt; - struct drm_hash_item *hash; - struct drm_device *dev = master->minor->dev; - - DRM_DEBUG("%d\n", magic); - - mutex_lock(&dev->struct_mutex); - if (drm_ht_find_item(&master->magiclist, (unsigned long)magic, &hash)) { - mutex_unlock(&dev->struct_mutex); - return -EINVAL; - } - pt = drm_hash_entry(hash, struct drm_magic_entry, hash_item); - drm_ht_remove_item(&master->magiclist, hash); - mutex_unlock(&dev->struct_mutex); - - kfree(pt); - - return 0; -} - /** - * Get a unique magic number (ioctl). + * drm_getmagic - Get unique magic of a client + * @dev: DRM device to operate on + * @data: ioctl data containing the drm_auth object + * @file_priv: DRM file that performs the operation * - * \param inode device inode. - * \param file_priv DRM file private. - * \param cmd command. - * \param arg pointer to a resulting drm_auth structure. - * \return zero on success, or a negative number on failure. + * This looks up the unique magic of the passed client and returns it. If the + * client did not have a magic assigned, yet, a new one is registered. The magic + * is stored in the passed drm_auth object. * - * If there is a magic number in drm_file::magic then use it, otherwise - * searches an unique non-zero magic number and add it associating it with \p - * file_priv. - * This ioctl needs protection by the drm_global_mutex, which protects - * struct drm_file::magic and struct drm_magic_entry::priv. + * Returns: 0 on success, negative error code on failure. */ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) { - static drm_magic_t sequence = 0; - static DEFINE_SPINLOCK(lock); struct drm_auth *auth = data; + int ret = 0;

- /* Find unique magic */ - if (file_priv->magic) { - auth->magic = file_priv->magic; - } else { - do { - spin_lock(&lock); - if (!sequence) - ++sequence; /* reserve 0 */ - auth->magic = sequence++; - spin_unlock(&lock); - } while (drm_find_file(file_priv->master, auth->magic)); - file_priv->magic = auth->magic; - drm_add_magic(file_priv->master, file_priv, auth->magic); + mutex_lock(&dev->struct_mutex); + if (!file_priv->magic) { + ret = idr_alloc(&file_priv->master->magic_map, file_priv, + 1, 0, GFP_KERNEL); + if (ret >= 0) + file_priv->magic = ret; } + auth->magic = file_priv->magic; + mutex_unlock(&dev->struct_mutex);

DRM_DEBUG("%u\n", auth->magic);

- return 0; + return ret < 0 ? ret : 0; }

/** - * Authenticate with a magic. + * drm_authmagic - Authenticate client with a magic + * @dev: DRM device to operate on + * @data: ioctl data containing the drm_auth object + * @file_priv: DRM file that performs the operation * - * \param inode device inode. - * \param file_priv DRM file private. - * \param cmd command. - * \param arg pointer to a drm_auth structure. - * \return zero if authentication successed, or a negative number otherwise. + * This looks up a DRM client by the passed magic and authenticates it. * - * Checks if \p file_priv is associated with the magic number passed in \arg. - * This ioctl needs protection by the drm_global_mutex, which protects - * struct drm_file::magic and struct drm_magic_entry::priv. + * Returns: 0 on success, negative error code on failure. */ int drm_authmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) @@ -189,10 +80,14 @@ int drm_authmagic(struct drm_device *dev, void *data, struct drm_file *file;

DRM_DEBUG("%u\n", auth->magic); - if ((file = drm_find_file(file_priv->master, auth->magic))) { + + mutex_lock(&dev->struct_mutex); + file = idr_find(&file_priv->master->magic_map, auth->magic); + if (file) { file->authenticated = 1; - drm_remove_magic(file_priv->master, auth->magic); - return 0; + idr_replace(&file_priv->master->magic_map, NULL, auth->magic); } - return -EINVAL; + mutex_unlock(&dev->struct_mutex); + + return file ? 0 : -EINVAL; } diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 26ed9fe..88b594c 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -92,8 +92,6 @@ void drm_ut_debug_printk(const char *function_name, const char *format, ...) } EXPORT_SYMBOL(drm_ut_debug_printk);

-#define DRM_MAGIC_HASH_ORDER 4 /**< Size of key hash table. Must be power of 2. */ - struct drm_master *drm_master_create(struct drm_minor *minor) { struct drm_master *master; @@ -105,10 +103,7 @@ struct drm_master *drm_master_create(struct drm_minor *minor) kref_init(&master->refcount); spin_lock_init(&master->lock.spinlock); init_waitqueue_head(&master->lock.lock_queue); - if (drm_ht_create(&master->magiclist, DRM_MAGIC_HASH_ORDER)) { - kfree(master); - return NULL; - } + idr_init(&master->magic_map); master->minor = minor;

return master; @@ -143,10 +138,9 @@ static void drm_master_destroy(struct kref *kref) master->unique = NULL; master->unique_len = 0; } - - drm_ht_remove(&master->magiclist); - mutex_unlock(&dev->struct_mutex); + + idr_destroy(&master->magic_map); kfree(master); }

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 076dd60..0f6a5c8 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -380,6 +380,8 @@ int drm_release(struct inode *inode, struct file *filp)

mutex_lock(&dev->struct_mutex); list_del(&file_priv->lhead); + if (file_priv->magic) + idr_remove(&file_priv->master->magic_map, file_priv->magic); mutex_unlock(&dev->struct_mutex);

if (dev->driver->preclose) @@ -394,11 +396,6 @@ int drm_release(struct inode *inode, struct file *filp) (long)old_encode_dev(file_priv->minor->kdev->devt), dev->open_count);

- /* Release any auth tokens that might point to this file_priv, - (do that under the drm_global_mutex) */ - if (file_priv->magic) - (void) drm_remove_magic(file_priv->master, file_priv->magic); - /* if the master has gone away we can't do anything with the lock */ if (file_priv->minor->master) drm_master_release(dev, filp); diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 12a61d7..059af01 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -69,7 +69,6 @@ int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_authmagic(struct drm_device *dev, void *data, struct drm_file *file_priv); -int drm_remove_magic(struct drm_master *master, drm_magic_t magic);

/* drm_sysfs.c */ extern struct class *drm_class; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 80be07a..2f9d605 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -355,7 +355,7 @@ struct drm_lock_data { * @minor: Link back to minor char device we are master for. Immutable. * @unique: Unique identifier: e.g. busid. Protected by drm_global_mutex. * @unique_len: Length of unique field. Protected by drm_global_mutex. - * @magiclist: Hash of used authentication tokens. Protected by struct_mutex. + * @magic_map: Map of used authentication tokens. Protected by struct_mutex. * @lock: DRI lock information. * @driver_priv: Pointer to driver-private information. */ @@ -364,7 +364,7 @@ struct drm_master { struct drm_minor *minor; char *unique; int unique_len; - struct drm_open_hash magiclist; + struct idr magic_map; struct drm_lock_data lock; void *driver_priv; };