From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> To: Chris Wilson <chris@chris-wilson.co.uk>, Tvrtko Ursulin <tursulin@ursulin.net>, Intel-gfx@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH v2 5/5] tools/testing/scatterlist: Test new __sg_alloc_table_from_pages Date: Wed, 6 Sep 2017 13:10:57 +0100 [thread overview] Message-ID: <005d683d-9f72-cb01-152f-4ccef0bbfd8b@linux.intel.com> (raw) In-Reply-To: <150469489418.28581.10979037821117823587@mail.alporthouse.com> On 06/09/2017 11:48, Chris Wilson wrote: > 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 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@chris-wilson.co.uk> Will do, thanks! Regards, Tvrtko
WARNING: multiple messages have this Message-ID (diff)
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> To: Chris Wilson <chris@chris-wilson.co.uk>, Tvrtko Ursulin <tursulin@ursulin.net>, Intel-gfx@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 5/5] tools/testing/scatterlist: Test new __sg_alloc_table_from_pages Date: Wed, 6 Sep 2017 13:10:57 +0100 [thread overview] Message-ID: <005d683d-9f72-cb01-152f-4ccef0bbfd8b@linux.intel.com> (raw) In-Reply-To: <150469489418.28581.10979037821117823587@mail.alporthouse.com> On 06/09/2017 11:48, Chris Wilson wrote: > 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 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@chris-wilson.co.uk> Will do, thanks! Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2017-09-06 12:11 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-07-31 18:55 [PATCH 1/5] lib/scatterlist: Fix offset type in sg_alloc_table_from_pages Tvrtko Ursulin 2017-07-31 18:55 ` [PATCH 2/5] lib/scatterlist: Avoid potential scatterlist entry overflow Tvrtko Ursulin 2017-07-31 18:55 ` Tvrtko Ursulin 2017-08-03 9:13 ` [PATCH v6 " Tvrtko Ursulin 2017-08-03 9:13 ` Tvrtko Ursulin 2017-07-31 18:55 ` [PATCH 3/5] lib/scatterlist: Introduce and export __sg_alloc_table_from_pages Tvrtko Ursulin 2017-07-31 18:55 ` Tvrtko Ursulin 2017-08-03 9:13 ` [PATCH v7 " Tvrtko Ursulin 2017-08-03 9:13 ` Tvrtko Ursulin 2017-07-31 18:55 ` [PATCH 4/5] drm/i915: Use __sg_alloc_table_from_pages for userptr allocations Tvrtko Ursulin 2017-07-31 18:55 ` Tvrtko Ursulin 2017-08-03 9:14 ` [PATCH v8 " Tvrtko Ursulin 2017-08-03 9:14 ` Tvrtko Ursulin 2017-07-31 18:55 ` [PATCH 5/5] tools/testing/scatterlist: Test new __sg_alloc_table_from_pages Tvrtko Ursulin 2017-07-31 18:55 ` Tvrtko Ursulin 2017-09-05 10:24 ` [PATCH v2 " Tvrtko Ursulin 2017-09-06 10:48 ` Chris Wilson 2017-09-06 10:48 ` Chris Wilson 2017-09-06 12:10 ` Tvrtko Ursulin [this message] 2017-09-06 12:10 ` Tvrtko Ursulin 2017-09-06 12:39 ` [Intel-gfx] " Chris Wilson 2017-09-06 14:55 ` [PATCH v3 " Tvrtko Ursulin 2017-09-06 14:55 ` Tvrtko Ursulin 2017-07-31 19:25 ` ✓ Fi.CI.BAT: success for series starting with [1/5] lib/scatterlist: Fix offset type in sg_alloc_table_from_pages Patchwork 2017-08-03 9:41 ` ✓ Fi.CI.BAT: success for series starting with [1/5] lib/scatterlist: Fix offset type in sg_alloc_table_from_pages (rev4) Patchwork 2017-09-05 10:42 ` ✓ Fi.CI.BAT: success for series starting with [1/5] lib/scatterlist: Fix offset type in sg_alloc_table_from_pages (rev5) Patchwork 2017-09-05 11:54 ` ✓ Fi.CI.IGT: " Patchwork 2017-09-06 15:32 ` ✗ Fi.CI.BAT: failure for series starting with [1/5] lib/scatterlist: Fix offset type in sg_alloc_table_from_pages (rev6) Patchwork 2017-09-07 9:22 ` ✓ Fi.CI.BAT: success " Patchwork 2017-09-07 10:49 ` ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=005d683d-9f72-cb01-152f-4ccef0bbfd8b@linux.intel.com \ --to=tvrtko.ursulin@linux.intel.com \ --cc=Intel-gfx@lists.freedesktop.org \ --cc=chris@chris-wilson.co.uk \ --cc=linux-kernel@vger.kernel.org \ --cc=tursulin@ursulin.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.