On 22/01/2019 10:47, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2019-01-22 09:31:31)
On 18/01/2019 14:00, Chris Wilson wrote:
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#include <linux/sched/signal.h>
+#include <linux/uaccess.h>
+#include <uapi/drm/i915_drm.h>
+
+#include "i915_user_extensions.h"
+
+int i915_user_extensions(struct i915_user_extension __user *ext,
+ const i915_user_extension_fn *tbl,
+ unsigned long count,
I would be tempted to limit the count to unsigned int. 4 billion
extensions should be enough for everyone. :)
I just picked the natural type, thinking having it the same width as
ARRAY_SIZE might save a few questions from semantic analysers.
+{
+ while (ext) {
+ int err;
+ u64 x;
+
+ cond_resched();
+ if (signal_pending(current))
+ return -EINTR;
What was your thinking behind this? It feels like, since here we are not
doing any explicit wait/sleeps, that at this level the code shouldn't
bother with it.
This ties into the discussion vvv
+ if (get_user(x, &ext->name))
+ return -EFAULT;
+
+ err = -EINVAL;
+ if (x < count && tbl[x])
+ err = tbl[x](ext, data);
+ if (err)
+ return err;
We talked about providing unwind on failure ie. option for destructor
call backs. You gave up on that?
The patch is simply called 'merde'. Yes, unwind on failure does not lend
itself to a simple tail call implementation :) And it doesn't lend
itself nicely to writing the stacked cleanup handlers either. (So I
stuck with the solution of just doing a single cleanup on failure, safe
in the knowledge that the most complicated case in this series is
trivial.)
Thinking about the issues with providing a stack for unwind, leads to
the nasty question of how big a stack exactly do we want to provide.
Limiting the chain is required for defense against misuse, but what
depth? For the chained parameter setup of a single shot context create,
we could easily be into the dozens of parameters and extensions blocks.
The extreme I've been contemplating is a multi-batch execbuf setup (with
all the fancy extensions for e.g. semi-privileged fancy register setup),
for that I've been thinking about how this interface would extend to
supporting many chained chunks. What makes a good upper bound for stack
depth? 32? 64? 512? Pick a number, it won't be enough for someone. (Now,
really passing that much information through an ioctl means our design
is broken ;)
So... The break on interrupt there was for the silly protection against
recursion, if it doesn't result in an invalid command.
Another reason the patch was called merde.
I think the chained API extension is very powerful. Being able to do
arbitrary things like a single-shot context creation ioctl and still be
able to redefine/extend the interface as needs demands, is compelling.
I did not think about unwind implementation details. :)
We could make the ABI double linked list? But the feeling is bad..
Or have some reasonable stack size and if overflows fall back to a very
inefficient lookup from root to walk backwards. Sounds okay for this use
case since we would never overflow even a small stack, and even the
inefficient walk on unwind would be a) fast and b) rare.
But since you mention stack sizes like 512 I wonder I did not quite
grasp how much you'd want to use extensions in the future. :)
+/*
+ * i915_user_extension: Base class for defining a chain of extensions
+ *
+ * Many interfaces need to grow over time. In most cases we can simply
+ * extend the struct and have userspace pass in more data. Another option,
+ * as demonstrated by Vulkan's approach to providing extensions for forward
+ * and backward compatibility, is to use a list of optional structs to
+ * provide those extra details.
+ *
+ * The key advantage to using an extension chain is that it allows us to
+ * redefine the interface more easily than an ever growing struct of
+ * increasing complexity, and for large parts of that interface to be
+ * entirely optional. The downside is more pointer chasing; chasing across
+ * the __user boundary with pointers encapsulated inside u64.
+ */
+struct i915_user_extension {
+ __u64 next_extension;
+ __u64 name;
s/name/id/ ?
I think name is common parlance for extensions/parameters? At least I've
been using it like it was :) And I was trying to retrospectively restrict
'id' for handles tracked by an idr! :)
I don't know if it is common, it is completely new to me. Maybe it is..
Or how about type?
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
