| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <005d683d-9f72-cb01-152f-4ccef0bbfd8b@linux.intel.com>
Date: Wed, 6 Sep 2017 13:10:57 +0100
From: Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
To: Chris Wilson <chris@...is-wilson.co.uk>,
Tvrtko Ursulin <tursulin@...ulin.net>,
Intel-gfx@...ts.freedesktop.org
Cc: linux-kernel@...r.kernel.org
Subject: Re: [Intel-gfx] [PATCH v2 5/5] tools/testing/scatterlist: Test new
__sg_alloc_table_from_pages
On 06/09/2017 11:48, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-09-05 11:24:03)
>> From: Tvrtko Ursulin <tvrtko.ursulin@...el.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@...el.com>
>> Cc: Chris Wilson <chris@...is-wilson.co.uk>
>> Cc: linux-kernel@...r.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@...is-wilson.co.uk>
Will do, thanks!
Regards,
Tvrtko
Powered by blists - more mailing lists