Messages in this thread | | | Subject | Re: [Intel-gfx] [PATCH v4] lib/scatterlist: Avoid potential scatterlist entry overflow | From | Tvrtko Ursulin <> | Date | Mon, 16 Jan 2017 10:05:58 +0000 |
| |
On 13/01/2017 22:23, Andy Shevchenko wrote: >>>> @@ -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? > > Why do we start from 1? I see here two micro (?) optimizations: > 1) starting from 1 on believe that compiler dumb enough to every time > do a calculation in condition;
The existing code starts from 1 because the pfn condition looks up page i - 1. I don't feel there is a need to change that as well.
> 2) ++i instead of i++, but this is just matter of style, it's not a c++.
Note that I haven't changed the existing code in this respect. I am happy to change it though.
>>> 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. > > Last seems has to be > seg_len = 0;
Oh right, of course. Your suggestion generates a tiny bit smaller binary so I am happy to change that as well. I'll resend the patch hopefully today.
Regards,
Tvrtko
| |