On Thu, Jan 12, 2017 at 10:56:42AM +0000, Tvrtko Ursulin wrote:
>
> On 11/01/2017 21:09, Chris Wilson wrote:
> >+static bool alloc_table(struct pfn_table *pt,
> >+ unsigned long count,
> >+ unsigned long max)
> >+{
> >+ struct scatterlist *sg;
> >+ unsigned long n, pfn;
> >+
> >+ if (sg_alloc_table(&pt->st, max,
> >+ GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN))
> >+ return false;
> >+
> >+ pt->start = PFN_BIAS;
> >+ pfn = pt->start;
> >+ sg = pt->st.sgl;
> >+ for (n = 0; n < count; n++) {
> >+ if (n)
> >+ sg = sg_next(sg);
> >+ sg_set_page(sg, pfn_to_page(pfn), (n + 1)*PAGE_SIZE, 0);
>
> For count = BIT(max_order) (== 1M) the last sg entry will have a
> size of 4GiB which would overflow sg->length, no? Especially with
> size + offset below. Unless for_each_prime_number stops below
> max_order. But where?
Stops at the last prime number less than 20, 19. A GEM_BUG_ON() would
clarify that.
>
> >+ pfn += n + 1;
>
> Please describe what kind of table and why you are building with a
> comment. If tests have no comments and do, on the first look, not
> fully obvious things, then it is a lot of effort in the future to
> work on fixes and/or maintain the code.
Creative block? If I start along those lines, I'll end up adding more
tests... Oh well.
> >+ }
> >+ sg_mark_end(sg);
> >+ pt->st.nents = n;
> >+ pt->end = pfn;
> >+
> >+ return true;
> >+}
> >+
> >+static int igt_sg_alloc(void *ignored)
> >+{
> >+ I915_SELFTEST_TIMEOUT(end_time);
> >+ const unsigned long max_order = 20; /* approximating a 4GiB object */
> >+ unsigned long prime;
> >+
> >+ for_each_prime_number(prime, max_order) {
> >+ unsigned long size = BIT(prime);
> >+ int offset;
> >+
> >+ for (offset = -1; offset <= 1; offset++) {
> >+ struct pfn_table pt;
> >+ int err;
> >+
> >+ if (!alloc_table(&pt, size + offset, size + offset))
> >+ return 0; /* out of memory, give up */
> >+
> >+ err = expect_pfn_sgtable(&pt, "sg_alloc_table");
> >+ sg_free_table(&pt.st);
> >+ if (err)
> >+ return err;
> >+
> >+ if (time_after(jiffies, end_time)) {
> >+ pr_warn("%s timed out: last table size %lu\n",
> >+ __func__, size + offset);
> >+ return 0;
> >+ }
>
> Maybe a fmt+vaarg timeout helper since I expect there'll be a lot of
> this. Like:
>
> if (i915_selftest_timeout(end_time, "fmt %s", arg))
> return 0;
>
> It is slightly tidier.
Nice.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
