Re: [PATCH 27/38] drm/i915: Introduce the i915_user_extension_method

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx