Re: [Intel-gfx] [PATCH v4] lib/scatterlist: Avoid potential scatterlist entry overflow

[Tvrtko Ursulin]

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