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.
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?).
Right, not concerned about speed here. Presuming no anomalous behaviour!
Anyway, I can ping Tejun and ask whether the cyclic IDR has any visible downsides compared to normal allocation. He should know.
Anyway, I just thought the test would be reasonably easy to write and useful to compare behaviour of long running systems - and helps to improve our test coverage just that little bit. -Chris