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!). -Chris