From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751183AbdAPKGS (ORCPT ); Mon, 16 Jan 2017 05:06:18 -0500 Received: from mga06.intel.com ([134.134.136.31]:47351 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750849AbdAPKGR (ORCPT ); Mon, 16 Jan 2017 05:06:17 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,238,1477983600"; d="scan'208";a="213851162" Subject: Re: [Intel-gfx] [PATCH v4] lib/scatterlist: Avoid potential scatterlist entry overflow To: Andy Shevchenko References: <1484125238-2539-2-git-send-email-tvrtko.ursulin@linux.intel.com> <1484135939-20988-1-git-send-email-tvrtko.ursulin@linux.intel.com> <05250677-7414-a4bd-8645-96da6a8b2df6@linux.intel.com> Cc: Tvrtko Ursulin , Masahiro Yamada , Intel-gfx@lists.freedesktop.org, "linux-kernel@vger.kernel.org" , Joonas Lahtinen From: Tvrtko Ursulin Message-ID: <2eab0dc7-9e1d-ae4c-ef6a-2bca0644f339@linux.intel.com> Date: Mon, 16 Jan 2017 10:05:58 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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