From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752949AbdIFKsW convert rfc822-to-8bit (ORCPT ); Wed, 6 Sep 2017 06:48:22 -0400 Received: from mail.fireflyinternet.com ([109.228.58.192]:52925 "EHLO fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752979AbdIFKsS (ORCPT ); Wed, 6 Sep 2017 06:48:18 -0400 X-Default-Received-SPF: pass (skip=forwardok (res=PASS)) x-ip-name=78.156.65.138; Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT To: Tvrtko Ursulin , Intel-gfx@lists.freedesktop.org From: Chris Wilson In-Reply-To: <20170905102403.18463-1-tvrtko.ursulin@linux.intel.com> Cc: tursulin@ursulin.net, "Tvrtko Ursulin" , 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quoting Tvrtko Ursulin (2017-09-05 11:24:03) > From: Tvrtko Ursulin > > 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 > Cc: Chris Wilson > 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 > +#include > + > +#include > + > +#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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson 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 Message-ID: <150469489418.28581.10979037821117823587@mail.alporthouse.com> References: <20170731185512.20010-5-tvrtko.ursulin@linux.intel.com> <20170905102403.18463-1-tvrtko.ursulin@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Return-path: In-Reply-To: <20170905102403.18463-1-tvrtko.ursulin@linux.intel.com> Sender: linux-kernel-owner@vger.kernel.org To: Intel-gfx@lists.freedesktop.org Cc: tursulin@ursulin.net, Tvrtko Ursulin , linux-kernel@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org Quoting Tvrtko Ursulin (2017-09-05 11:24:03) > From: Tvrtko Ursulin > > 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 > Cc: Chris Wilson > 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 > +#include > + > +#include > + > +#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