Re: [PATCH v2 5/5] tools/testing/scatterlist: Test new __sg_alloc_table_from_pages

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/09/2017 11:48, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2017-09-05 11:24:03)
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

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@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx
---
  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

I lifted it frim an existing makefile. I think this means no one was interested in building tests while doing a cross compile.

+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"

Again, I lifted the approach from one of the existing tests. It might be beneficial to have a local copy when debugging, but it is probably very marginal and both approaches look OK.

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

Hm, how do you think descending pages could be coalesced? By re-arranging the pages? But that would break everything, do I don't get it.

moment, 0, 2, 1, would be a good addition to the above set.

Is there any value in checking overflows in this interface?

Overflows as in size > num_pages * PAGE_SIZE as passed in to __sg_alloc_table_from_pages ? It is not checked in the implementation at the moment and it looks it is harmless.

There is one test above which checks for underflow (size < num_pages * PAGE_SIZE).

Lgtm, throw in a couple of inverted positions,
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>

Will do, thanks!

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux