[Intel-gfx] [PATCH 27/38] drm/i915: Introduce the i915_user_extension_method

[Chris Wilson]

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