Hi Krzysztof,
On 2025-03-18 at 10:39:41 GMT, Krzysztof Karas wrote:
> Hi Krzysztof,
>
> > > - /*
> > > - * While we should be able allocate everything without any flag
> > > - * restrictions, if we consider I915_BO_ALLOC_CONTIGUOUS then we are
> > > - * actually limited to the largest power-of-two for the region size i.e
> > > - * max_order, due to the inner workings of the buddy allocator. So make
> > > - * sure that does indeed hold true.
> > > - */
> > > -
> > > obj = igt_object_create(mem, &objects, size, I915_BO_ALLOC_CONTIGUOUS);
> > > - if (!IS_ERR(obj)) {
> > > - pr_err("%s too large contiguous allocation was not rejected\n",
> > > + if (!IS_ERR(obj) && !is_contiguous(obj)) {
> > > + pr_err("%s large allocation was not contiguous\n",
> > > __func__);
> > > err = -EINVAL;
> > > goto out_close;
> > > }
> > >
> >
> > If I understand the test correctly, the goal of the part that you're
> > changing is to see if an attempt at allocating more memory than
> > max_order is properly rejected. Since the allocations are more granular
> > now (not only limited to powers of two), and size doesn't get rounded up
> > to the higher power of two, we should be able to allocate 'size'
> > exactly. Meaning we lose the intended functionality of the test (check
> > if we can't allocate too big of an object), because we're not allocating
> > too big of an object anymore.
>
> Since the allocator is a lot more lenient now, we could focus on
> getting a contiguous object instead.
>
I don't think it was the intention of that part of the test, but that's
a valid thing to test against nonetheless.
> > I guess a check for contiguousness does not hurt, but the test behavior
> > is fundamentally different here. Maybe manually rounding size up to the
> > nearest larger power of two would be a better idea here?
>
> Before changes were made to the allocator we knew that there
> was a corner case with rounding size to the power of two. Now,
> we know that the allocator will take in any appropriate size and
> give us a valid object (correct me if I'm wrong here) - if that
> is the case, then this should just be a check if we fail on a
> size that is too large (unless this is covered by some other
> test).
>
That's what I meant with rounding up to the power of two - make an
object intentionally larger than we can accomodate so we can check if
that gets rejected. I suggested using that rounding up so we replicate
the previous behavior, but I guess it could as well be size + 1 or size
* 2 or any larger value; the point is to verify that the driver doesn't
allow allocs larger than expected.
> >
> > > + if (!IS_ERR(obj))
> > > + close_objects(mem, &objects);
> > > +
>
> This code would obfuscate the original purpose of this test and
> just pass regardless of successful or failed object allocation.
>
(that's a part of the original patch not my reply)
If this part of the test will morph into a contiguousness check, we need
to deallocate for the other check, so those two lines must stay. If that
part keeps being a rejection check, just tweaked to comply with the new
allocator behavior, then the deallocation will be in the error path like
before the change.
> > > obj = igt_object_create(mem, &objects, rounddown_pow_of_two(size),
> > > I915_BO_ALLOC_CONTIGUOUS);
> > > if (IS_ERR(obj)) {
> >
> > I'll paste some more lines from that test here:
> >
> > obj = igt_object_create(mem, &objects, rounddown_pow_of_two(size),
> > I915_BO_ALLOC_CONTIGUOUS);
> > if (IS_ERR(obj)) {
> > pr_err("%s largest possible contiguous allocation failed\n",
> > __func__);
> > err = PTR_ERR(obj);
> > goto out_close;
> > }
> >
> > This is the next check - we see if the largest possible allocation
> > (according to the old logic, it would be 'size' rounded down to the
> > largest smaller or equal power of two) _does_ go through. The success of
> > this particular check isn't affected by the allocator changes, but since
> > rounddown_pow_of_two(size) is not the largest possible allocation
> > anymore, maybe it's better to change this too (e.g. drop the rounddown
> > function). This way we keep the intended test behavior here as well.
> > I suppose this is still in scope of the patch.
>
> Yes, this would be a bit different than the original behavior,
> but we'd be sure to fail object allocation. If no other test
> checks this condition then this test could do that here.
>
I mean, if this test was doing that, I don't think we should patch that
out for no reason.
IMO we need to tweak those two checks to check for the same thing
functionally but keep the new allocator behavior in mind. Then
contiguousness is a good thing to check, but I think it should be added
as a third check so we don't lose either of the other two.
Thanks
Krzysztof
> Best Regards,
> Krzysztof
