[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <87fugneqp6.fsf@yhuang-dev.intel.com>
Date: Tue, 02 May 2017 14:08:21 +0800
From: "Huang\, Ying" <ying.huang@...el.com>
To: Minchan Kim <minchan@...nel.org>
Cc: "Huang\, Ying" <ying.huang@...el.com>,
Andrew Morton <akpm@...ux-foundation.org>,
<linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
Hugh Dickins <hughd@...gle.com>, Shaohua Li <shli@...nel.org>,
Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free
Minchan Kim <minchan@...nel.org> writes:
> Hi Huang,
>
> On Tue, May 02, 2017 at 01:35:24PM +0800, Huang, Ying wrote:
>> Hi, Minchan,
>>
>> Minchan Kim <minchan@...nel.org> writes:
>>
>> > On Fri, Apr 28, 2017 at 09:35:37PM +0800, Huang, Ying wrote:
>> >> In fact, during the test, I found the overhead of sort() is comparable
>> >> with the performance difference of adding likely()/unlikely() to the
>> >> "if" in the function.
>> >
>> > Huang,
>> >
>> > This discussion is started from your optimization code:
>> >
>> > if (nr_swapfiles > 1)
>> > sort();
>> >
>> > I don't have such fast machine so cannot test it. However, you added
>> > such optimization code in there so I guess it's *worth* to review so
>> > with spending my time, I pointed out what you are missing and
>> > suggested a idea to find a compromise.
>>
>> Sorry for wasting your time and Thanks a lot for your review and
>> suggestion!
>>
>> When I started talking this with you, I found there is some measurable
>> overhead of sort(). But later when I done more tests, I found the
>> measurable overhead is at the same level of likely()/unlikely() compiler
>> notation. So you help me to find that, Thanks again!
>>
>> > Now you are saying sort is so fast so no worth to add more logics
>> > to avoid the overhead?
>> > Then, please just drop that if condition part and instead, sort
>> > it unconditionally.
>>
>> Now, because we found the overhead of sort() is low, I suggest to put
>> minimal effort to avoid it. Like the original implementation,
>>
>> if (nr_swapfiles > 1)
>> sort();
>
> It might confuse someone in future and would make him/her send a patch
> to fix like we discussed. If the logic is not clear and doesn't have
> measureable overhead, just leave it which is more simple/clear.
Because the added code is minimal and cheap, I tend to keep it and add
some comments to avoid confusion. For example,
/*
* Although nr_swapfiles isn't absolute correct, but the overhead of sort()
* is so low that it isn't necessary to optimize further.
*/
>>
>> Or, we can make nr_swapfiles more correct as Tim suggested (tracking
>> the number of the swap devices during swap on/off).
>
> It might be better option but it's still hard to justify the patch
> because you said it's hard to measure. Such optimiztion patch should
> be from numbers.
OK.
Best Regards,
Huang, Ying
Powered by blists - more mailing lists