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.
> > +/*
> > + * 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! :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
