From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1045512AbdDWNQq (ORCPT ); Sun, 23 Apr 2017 09:16:46 -0400 Received: from mga14.intel.com ([192.55.52.115]:52995 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1045484AbdDWNQj (ORCPT ); Sun, 23 Apr 2017 09:16:39 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,239,1488873600"; d="scan'208";a="1122610969" From: "Huang\, Ying" To: Tim Chen Cc: "Huang\, Ying" , Minchan Kim , Andrew Morton , , , Hugh Dickins , Shaohua Li , Rik van Riel Subject: Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free References: <20170407064901.25398-1-ying.huang@intel.com> <20170418045909.GA11015@bbox> <87y3uwrez0.fsf@yhuang-dev.intel.com> <20170420063834.GB3720@bbox> <874lxjim7m.fsf@yhuang-dev.intel.com> <87tw5idjv9.fsf@yhuang-dev.intel.com> <1492817351.3209.56.camel@linux.intel.com> Date: Sun, 23 Apr 2017 21:16:35 +0800 In-Reply-To: <1492817351.3209.56.camel@linux.intel.com> (Tim Chen's message of "Fri, 21 Apr 2017 16:29:11 -0700") Message-ID: <87pog3b6x8.fsf@yhuang-dev.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Tim Chen writes: > On Fri, 2017-04-21 at 20:29 +0800, Huang, Ying wrote: >> "Huang, Ying" writes: >> >> > >> > Minchan Kim writes: >> > >> > > >> > > On Wed, Apr 19, 2017 at 04:14:43PM +0800, Huang, Ying wrote: >> > > > >> > > > Minchan Kim writes: >> > > > >> > > > > >> > > > > Hi Huang, >> > > > > >> > > > > On Fri, Apr 07, 2017 at 02:49:01PM +0800, Huang, Ying wrote: >> > > > > > >> > > > > > From: Huang Ying >> > > > > > >> > > > > >  void swapcache_free_entries(swp_entry_t *entries, int n) >> > > > > >  { >> > > > > >   struct swap_info_struct *p, *prev; >> > > > > > @@ -1075,6 +1083,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n) >> > > > > >   >> > > > > >   prev = NULL; >> > > > > >   p = NULL; >> > > > > > + >> > > > > > + /* Sort swap entries by swap device, so each lock is only taken once. */ >> > > > > > + if (nr_swapfiles > 1) >> > > > > > + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL); >> > > > > Let's think on other cases. >> > > > > >> > > > > There are two swaps and they are configured by priority so a swap's usage >> > > > > would be zero unless other swap used up. In case of that, this sorting >> > > > > is pointless. >> > > > > >> > > > > As well, nr_swapfiles is never decreased so if we enable multiple >> > > > > swaps and then disable until a swap is remained, this sorting is >> > > > > pointelss, too. >> > > > > >> > > > > How about lazy sorting approach? IOW, if we found prev != p and, >> > > > > then we can sort it. >> > > > Yes.  That should be better.  I just don't know whether the added >> > > > complexity is necessary, given the array is short and sort is fast. >> > > Huh? >> > > >> > > 1. swapon /dev/XXX1 >> > > 2. swapon /dev/XXX2 >> > > 3. swapoff /dev/XXX2 >> > > 4. use only one swap >> > > 5. then, always pointless sort. >> > Yes.  In this situation we will do unnecessary sorting.  What I don't >> > know is whether the unnecessary sorting will hurt performance in real >> > life.  I can do some measurement. >> I tested the patch with 1 swap device and 1 process to eat memory >> (remove the "if (nr_swapfiles > 1)" for test).   > > It is possible that nr_swapfiles > 1 when we have only 1 swapfile due > to swapoff.  The nr_swapfiles never decrement on swapoff. > We will need to use another counter in alloc_swap_info and > swapoff to track the true number of swapfiles in use to have a fast path > that avoid the search and sort for the 1 swap case. Yes. That is a possible optimization. But it doesn't cover another use cases raised by Minchan (two swap device with different priority). So in general, we still need to check whether there are entries from multiple swap devices in the array. Given the cost of the checking code is really low, I think maybe we can just always use the checking code. Do you think so? Best Regards, Huang, Ying From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f199.google.com (mail-io0-f199.google.com [209.85.223.199]) by kanga.kvack.org (Postfix) with ESMTP id 83FF66B028D for ; Sun, 23 Apr 2017 09:16:39 -0400 (EDT) Received: by mail-io0-f199.google.com with SMTP id z63so187939786ioz.23 for ; Sun, 23 Apr 2017 06:16:39 -0700 (PDT) Received: from mga05.intel.com (mga05.intel.com. [192.55.52.43]) by mx.google.com with ESMTPS id d90si15750051pfk.351.2017.04.23.06.16.38 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 23 Apr 2017 06:16:38 -0700 (PDT) From: "Huang\, Ying" Subject: Re: [PATCH -mm -v3] mm, swap: Sort swap entries before free References: <20170407064901.25398-1-ying.huang@intel.com> <20170418045909.GA11015@bbox> <87y3uwrez0.fsf@yhuang-dev.intel.com> <20170420063834.GB3720@bbox> <874lxjim7m.fsf@yhuang-dev.intel.com> <87tw5idjv9.fsf@yhuang-dev.intel.com> <1492817351.3209.56.camel@linux.intel.com> Date: Sun, 23 Apr 2017 21:16:35 +0800 In-Reply-To: <1492817351.3209.56.camel@linux.intel.com> (Tim Chen's message of "Fri, 21 Apr 2017 16:29:11 -0700") Message-ID: <87pog3b6x8.fsf@yhuang-dev.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: owner-linux-mm@kvack.org List-ID: To: Tim Chen Cc: "Huang, Ying" , Minchan Kim , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Hugh Dickins , Shaohua Li , Rik van Riel Tim Chen writes: > On Fri, 2017-04-21 at 20:29 +0800, Huang, Ying wrote: >> "Huang, Ying" writes: >> >> > >> > Minchan Kim writes: >> > >> > > >> > > On Wed, Apr 19, 2017 at 04:14:43PM +0800, Huang, Ying wrote: >> > > > >> > > > Minchan Kim writes: >> > > > >> > > > > >> > > > > Hi Huang, >> > > > > >> > > > > On Fri, Apr 07, 2017 at 02:49:01PM +0800, Huang, Ying wrote: >> > > > > > >> > > > > > From: Huang Ying >> > > > > > >> > > > > > A void swapcache_free_entries(swp_entry_t *entries, int n) >> > > > > > A { >> > > > > > A struct swap_info_struct *p, *prev; >> > > > > > @@ -1075,6 +1083,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n) >> > > > > > A >> > > > > > A prev = NULL; >> > > > > > A p = NULL; >> > > > > > + >> > > > > > + /* Sort swap entries by swap device, so each lock is only taken once. */ >> > > > > > + if (nr_swapfiles > 1) >> > > > > > + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL); >> > > > > Let's think on other cases. >> > > > > >> > > > > There are two swaps and they are configured by priority so a swap's usage >> > > > > would be zero unless other swap used up. In case of that, this sorting >> > > > > is pointless. >> > > > > >> > > > > As well, nr_swapfiles is never decreased so if we enable multiple >> > > > > swaps and then disable until a swap is remained, this sorting is >> > > > > pointelss, too. >> > > > > >> > > > > How about lazy sorting approach? IOW, if we found prev != p and, >> > > > > then we can sort it. >> > > > Yes.A A That should be better.A A I just don't know whether the added >> > > > complexity is necessary, given the array is short and sort is fast. >> > > Huh? >> > > >> > > 1. swapon /dev/XXX1 >> > > 2. swapon /dev/XXX2 >> > > 3. swapoff /dev/XXX2 >> > > 4. use only one swap >> > > 5. then, always pointless sort. >> > Yes.A A In this situation we will do unnecessary sorting.A A What I don't >> > know is whether the unnecessary sorting will hurt performance in real >> > life.A A I can do some measurement. >> I tested the patch with 1 swap device and 1 process to eat memory >> (remove the "if (nr_swapfiles > 1)" for test).A A > > It is possible that nr_swapfiles > 1 when we have only 1 swapfile due > to swapoff. A The nr_swapfiles never decrement on swapoff. > We will need to use another counter in alloc_swap_info and > swapoff to track the true number of swapfiles in use to have a fast path > that avoid the search and sort for the 1 swap case. Yes. That is a possible optimization. But it doesn't cover another use cases raised by Minchan (two swap device with different priority). So in general, we still need to check whether there are entries from multiple swap devices in the array. Given the cost of the checking code is really low, I think maybe we can just always use the checking code. Do you think so? Best Regards, Huang, Ying -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org