[igt-dev] [PATCH i-g-t 1/1] lib/intel_allocator: Check validity of the file descriptor

[Petri Latvala]

On Thu, Jun 17, 2021 at 08:06:46AM +0200, Zbigniew Kempczyński wrote:
> On Wed, Jun 16, 2021 at 03:05:50PM +0300, Petri Latvala wrote:
> > On Wed, Jun 16, 2021 at 01:37:14PM +0200, Andrzej Turko wrote:
> > > All procedures for opening particular types of allocators
> > > query the gtt size. Even if the user has provided the
> > > boundaries of the address space managed by the allocator,
> > > the query is performed for the sake of validation.
> > > 
> > > gem_aperture_size() called with an invalid id returns a
> > > default value instead of failing. Thus, we need to
> > > additionally check whether the file descriptor is valid.
> > > 
> > > Signed-off-by: Andrzej Turko <andrzej.turko at linux.intel.com>
> > > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > ---
> > >  lib/intel_allocator.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/lib/intel_allocator.c b/lib/intel_allocator.c
> > > index 96f839d4b..c060e10c3 100644
> > > --- a/lib/intel_allocator.c
> > > +++ b/lib/intel_allocator.c
> > > @@ -166,6 +166,15 @@ static inline void map_entry_free_func(struct igt_map_entry *entry)
> > >  	free(entry->data);
> > >  }
> > >  
> > > +static bool can_report_gtt_size(int fd)
> > > +{
> > > +	struct drm_i915_gem_context_param p = {
> > > +		.param = I915_CONTEXT_PARAM_GTT_SIZE
> > > +	};
> > > +
> > > +	return (__gem_context_get_param(fd, &p) == 0);
> > > +}
> > > +
> > >  static uint64_t __handle_create(struct allocator *al)
> > >  {
> > >  	struct handle_entry *h = malloc(sizeof(*h));
> > > @@ -271,6 +280,8 @@ static struct intel_allocator *intel_allocator_create(int fd,
> > >  {
> > >  	struct intel_allocator *ial = NULL;
> > >  
> > > +	igt_assert(can_report_gtt_size(fd));
> > > +
> > 
> > Is igt_assert correct here (as opposed to igt_require)? Or in other
> > words, is inability to report gtt size a kernel bug or not?
> 
> In this case yes, we want to assert. Let me try to explain the scenario.
> 
> 
> 1. allocator thread acts for children, for example we got 
>    fd i915 == 3 and ctx == 0
> 
> 2. child 1 does: 
>    
>    ahnd = intel_allocator_open(i915 == 3, ctx == 0);
> 
>    -> allocator (inside simple) calls gem_aperture_size(i915)
>       to find out vm size - this works perfect
> 
> 3. child 2 does: 
>    
>    i915 = gem_reopen_driver(i915);
>    ahnd = intel_allocator_open(i915 == 5, ctx == 0);
> 
>    -> allocator does same what child 1, but gem_aperture_size(i915)
>       is called on invalid from allocator process handle. 
>    
> Current implementation of gem_aperture_size() calls 
> gem_global_aperture_size() what resets errno on error returning
> predefined 256MB size.
> 
> We want to avoid such mistakes in the future in allocator. Plan is:
> 
> 1. introduce igt_assert() on such case. Currently does nothing because
>    there's no test who does child 2 scenario (but soon will be).
> 
> 2. put failing test to api_intel_allocator.c (which exercise above 
>    child 2 scenario).
> 
> 3. fix allocator -> drop calling gem_aperture_size() on allocator
>    thread and move it to the caller (it has active fd to kernel).
>    Thats will drop allocator thread i915 fd dependency and this 
>    is what we want.
> 
> One scenario is we cannot handle - two or more children reopens 
> driver. In such case they may have same i915 fd so they starts
> sharing vm instead having separate. But we still have possibility
> to use (i915, vm) as a key so in such scenario we may use child
> pid as vmid, then each child will have separate vm for allocations.
> Or if child will call intel_allocator_init() it will totally 
> drop allocator thread arbitration -> this reinitializes allocator
> data within child so it comes standalone allocator.


Yeah assert sounds more proper then. Although the best would be an
igt_assert_igt_is_broken_dont_blame_the_kernel() but one must use what
one has...


-- 
Petri Latvala