Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753177AbdIFMLV (ORCPT ); Wed, 6 Sep 2017 08:11:21 -0400 Received: from mga14.intel.com ([192.55.52.115]:45030 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752943AbdIFMLT (ORCPT ); Wed, 6 Sep 2017 08:11:19 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,483,1498546800"; d="scan'208";a="1215421258" Subject: Re: [Intel-gfx] [PATCH v2 5/5] tools/testing/scatterlist: Test new __sg_alloc_table_from_pages To: Chris Wilson , Tvrtko Ursulin , Intel-gfx@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org References: <20170731185512.20010-5-tvrtko.ursulin@linux.intel.com> <20170905102403.18463-1-tvrtko.ursulin@linux.intel.com> <150469489418.28581.10979037821117823587@mail.alporthouse.com> From: Tvrtko Ursulin Message-ID: <005d683d-9f72-cb01-152f-4ccef0bbfd8b@linux.intel.com> Date: Wed, 6 Sep 2017 13:10:57 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <150469489418.28581.10979037821117823587@mail.alporthouse.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5570 Lines: 153 On 06/09/2017 11:48, Chris Wilson wrote: > 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 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 >> +#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 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 Will do, thanks! Regards, Tvrtko