Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 8BIT
To: Tvrtko Ursulin <tursulin@ursulin.net>, Intel-gfx@lists.freedesktop.org
From: Chris Wilson <chris@chris-wilson.co.uk>
In-Reply-To: <20170905102403.18463-1-tvrtko.ursulin@linux.intel.com>
Cc: tursulin@ursulin.net, "Tvrtko Ursulin" <tvrtko.ursulin@intel.com>,
        linux-kernel@vger.kernel.org
References: <20170731185512.20010-5-tvrtko.ursulin@linux.intel.com>
 <20170905102403.18463-1-tvrtko.ursulin@linux.intel.com>
Message-ID: <150469489418.28581.10979037821117823587@mail.alporthouse.com>
User-Agent: alot/0.3.6
Subject: Re: [PATCH v2 5/5] tools/testing/scatterlist: Test new
 __sg_alloc_table_from_pages
Date: Wed, 06 Sep 2017 11:48:14 +0100
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4628
Lines: 128

Quoting Tvrtko Ursulin (2017-09-05 11:24:03)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Exercise the new __sg_alloc_table_from_pages API (and through
> it also the old sg_alloc_table_from_pages), checking that the
> created table has the expected number of segments depending on
> the sequence of input pages and other conditions.
> 
> v2: Move to data driven for readability.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: linux-kernel@vger.kernel.org
> ---
>  tools/testing/scatterlist/Makefile   |  30 +++++++++
>  tools/testing/scatterlist/linux/mm.h | 125 +++++++++++++++++++++++++++++++++++
>  tools/testing/scatterlist/main.c     |  74 +++++++++++++++++++++
>  3 files changed, 229 insertions(+)
>  create mode 100644 tools/testing/scatterlist/Makefile
>  create mode 100644 tools/testing/scatterlist/linux/mm.h
>  create mode 100644 tools/testing/scatterlist/main.c
> 
> diff --git a/tools/testing/scatterlist/Makefile b/tools/testing/scatterlist/Makefile
> new file mode 100644
> index 000000000000..0867e0ef32d6
> --- /dev/null
> +++ b/tools/testing/scatterlist/Makefile
> @@ -0,0 +1,30 @@
> +CFLAGS += -I. -I../../include -g -O2 -Wall -fsanitize=address
> +LDFLAGS += -fsanitize=address
> +TARGETS = main
> +OFILES = main.o scatterlist.o
> +
> +ifeq ($(BUILD), 32)
> +        CFLAGS += -m32
> +        LDFLAGS += -m32
> +endif

Hmm, are there no HOST_CFLAGS? No. I wonder how menuconfig/xconfig get
compiled.

HOSTCC, HOSTCFLAGS.

hostprogs-y := main
always := $(hostprogs-y)

But nothing else seems to use HOSTCC in testing/selftests

> +targets: include $(TARGETS)
> +
> +main: $(OFILES)
> +
> +clean:
> +       $(RM) $(TARGETS) $(OFILES) scatterlist.c linux/scatterlist.h linux/highmem.h linux/kmemleak.h asm/io.h
> +       @rmdir asm
> +
> +scatterlist.c: ../../../lib/scatterlist.c
> +       @sed -e 's/^static //' -e 's/__always_inline //' -e 's/inline //' < $< > $@

I think I would have used

#define __always_inline inline
#include "../../../lib/scatterlist.c"

> diff --git a/tools/testing/scatterlist/main.c b/tools/testing/scatterlist/main.c
> new file mode 100644
> index 000000000000..8ca5c8703eb7
> --- /dev/null
> +++ b/tools/testing/scatterlist/main.c
> @@ -0,0 +1,74 @@
> +#include <stdio.h>
> +#include <assert.h>
> +
> +#include <linux/scatterlist.h>
> +
> +#define MAX_PAGES (64)
> +
> +static void set_pages(struct page **pages, const unsigned *array, unsigned num)
> +{
> +       unsigned int i;
> +
> +       assert(num < MAX_PAGES);
> +       for (i = 0; i < num; i++)
> +               pages[i] = (struct page *)(unsigned long)
> +                          ((1 + array[i]) * PAGE_SIZE);

pfn_to_page(PFN_BIAS + array[i]) ? Ah, that relies on headers. Ok.

> +}
> +
> +#define pfn(...) (unsigned []){ __VA_ARGS__ }
> +
> +int main(void)
> +{
> +       const unsigned int sgmax = SCATTERLIST_MAX_SEGMENT;
> +       struct test {
> +               int alloc_ret;
> +               unsigned num_pages;
> +               unsigned *pfn;
> +               unsigned size;
> +               unsigned int max_seg;
> +               unsigned int expected_segments;
> +       } *test, tests[] = {
> +               { -EINVAL, 1, pfn(0), PAGE_SIZE, PAGE_SIZE + 1, 1 },
> +               { -EINVAL, 1, pfn(0), PAGE_SIZE, 0, 1 },
> +               { -EINVAL, 1, pfn(0), PAGE_SIZE, sgmax + 1, 1 },
> +               { 0, 1, pfn(0), PAGE_SIZE, sgmax, 1 },
> +               { 0, 1, pfn(0), 1, sgmax, 1 },
> +               { 0, 2, pfn(0, 1), 2 * PAGE_SIZE, sgmax, 1 },
> +               { 0, 3, pfn(0, 1, 3), 3 * PAGE_SIZE, sgmax, 2 },
> +               { 0, 4, pfn(0, 1, 3, 4), 4 * PAGE_SIZE, sgmax, 2 },
> +               { 0, 5, pfn(0, 1, 3, 4, 5), 5 * PAGE_SIZE, sgmax, 2 },
> +               { 0, 5, pfn(0, 1, 3, 4, 6), 5 * PAGE_SIZE, sgmax, 3 },
> +               { 0, 5, pfn(0, 1, 2, 3, 4), 5 * PAGE_SIZE, sgmax, 1 },
> +               { 0, 5, pfn(0, 1, 2, 3, 4), 5 * PAGE_SIZE, 2 * PAGE_SIZE, 3 },
> +               { 0, 6, pfn(0, 1, 2, 3, 4, 5), 6 * PAGE_SIZE, 2 * PAGE_SIZE, 3 },
> +               { 0, 6, pfn(0, 2, 3, 4, 5, 6), 6 * PAGE_SIZE, 2 * PAGE_SIZE, 4 },
> +               { 0, 6, pfn(0, 1, 3, 4, 5, 6), 6 * PAGE_SIZE, 2 * PAGE_SIZE, 3 },

All ascending. Interesting challenge for 3,2,1,0; it can be coalesced,
we just don't. I wonder if we are missing some like that. But for the
moment, 0, 2, 1, would be a good addition to the above set.

Is there any value in checking overflows in this interface?

Lgtm, throw in a couple of inverted positions,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris