Hi,
On 11/01/2017 23:59, Andy Shevchenko wrote:
On Wed, Jan 11, 2017 at 1:58 PM, Tvrtko Ursulin <tursulin@xxxxxxxxxxx> wrote:
Since the scatterlist length field is an unsigned int, make
sure that sg_alloc_table_from_pages does not overflow it while
coallescing pages to a single entry.
/*
+ * Since the above length field is an unsigned int, below we define the maximum
+ * lenght in bytes that can be stored in one scatterlist entry.
length
+ */
+#define SCATTERLIST_MAX_SEGMENT (0xfffff000)
Shouldn't be calculated from PAGE_SIZE (PAGE bits, etc)?
Yep, and at the same time I would potentially change the name to be
consistent with the other defines in the file as Joonas suggested.
Something like:
#define SG_MAX_SEGMENT (UINT_MAX & PAGE_MASK)
But would need a better name since SG_MAX_SEGMENT*S* already exists and
means something else. If we can't come up with a better name then leave
it as SCATTERLIST_MAX_SEGMENT?
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -402,9 +403,16 @@ int sg_alloc_table_from_pages(struct sg_table *sgt,
/* compute number of contiguous chunks */
chunks = 1;
- for (i = 1; i < n_pages; ++i)
- if (page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1)
+ seg_len = PAGE_SIZE;
+ for (i = 1; i < n_pages; ++i) {
+ if (seg_len >= max_segment ||
+ page_to_pfn(pages[i]) != page_to_pfn(pages[i - 1]) + 1) {
++chunks;
+ seg_len = PAGE_SIZE;
+ } else {
+ seg_len += PAGE_SIZE;
+ }
+ }
Wouldn't be following looks more readable?
seg_len = 0;
// Are compilers so stupid doing calculation per iteration in for-conditional?
// for (i = 0; i + 1 < n_pages; i++) ?
I didn't get what you meant here?
for (i = 1; i < n_pages; ++i) {
seg_len += PAGE_SIZE;
if (seg_len >= max_segment || page_to_pfn(pages[i]) !=
page_to_pfn(pages[i - 1]) + 1) {
++chunks;
seg_len = PAGE_SIZE;
}
}
Tried it in my unit tester but it doesn't work for all scenarios, guess
there is a subtle bug somewhere. I don't find it that unreadable so
would prefer to leave it since it works.
Perhaps while() or do-while() will increase readability even more, but
I didn't check.
/* look for the end of the current chunk */
+ seg_len = PAGE_SIZE;
for (j = cur_page + 1; j < n_pages; ++j)
- if (page_to_pfn(pages[j]) !=
+ if (seg_len >= max_segment ||
+ page_to_pfn(pages[j]) !=
page_to_pfn(pages[j - 1]) + 1)
break;
+ else
+ seg_len += PAGE_SIZE;
Something similar here (didn't you get warning from checkpath about
curly braces?).
It didn't, but agreed that I should have added them. Will do.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
