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 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).
>
> > + 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.
> > 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.
Best Regards,
Krzysztof
