From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751196AbdAQVxt (ORCPT <rfc822;w@1wt.eu>);
        Tue, 17 Jan 2017 16:53:49 -0500
Received: from mx2.suse.de ([195.135.220.15]:52468 "EHLO mx2.suse.de"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1750924AbdAQVxs (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 17 Jan 2017 16:53:48 -0500
Date: Tue, 17 Jan 2017 21:03:39 +0100
From: Michal Hocko <mhocko@kernel.org>
To: "Chen, Tim C" <tim.c.chen@intel.com>
Cc: "Huang, Ying" <ying.huang@intel.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Hansen, Dave" <dave.hansen@intel.com>,
        "ak@linux.intel.com" <ak@linux.intel.com>,
        "Lu, Aaron" <aaron.lu@intel.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Hugh Dickins <hughd@google.com>, Shaohua Li <shli@kernel.org>,
        Minchan Kim <minchan@kernel.org>, Rik van Riel <riel@redhat.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Hillf Danton <hillf.zj@alibaba-inc.com>,
        Christian Borntraeger <borntraeger@de.ibm.com>,
        Jonathan Corbet <corbet@lwn.net>
Subject: Re: [Update][PATCH v5 7/9] mm/swap: Add cache for swap slots
 allocation
Message-ID: <20170117200338.GA26217@dhcp22.suse.cz>
References: <cover.1484082593.git.tim.c.chen@linux.intel.com>
 <35de301a4eaa8daa2977de6e987f2c154385eb66.1484082593.git.tim.c.chen@linux.intel.com>
 <87tw8ymm2z.fsf_-_@yhuang-dev.intel.com>
 <20170117101631.GG19699@dhcp22.suse.cz>
 <045D8A5597B93E4EBEDDCBF1FC15F50935C9F523@fmsmsx104.amr.corp.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <045D8A5597B93E4EBEDDCBF1FC15F50935C9F523@fmsmsx104.amr.corp.intel.com>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue 17-01-17 17:24:15, Chen, Tim C wrote:
> > > +	/*
> > > +	 * Preemption need to be turned on here, because we may sleep
> > > +	 * in refill_swap_slots_cache().  But it is safe, because
> > > +	 * accesses to the per-CPU data structure are protected by a
> > > +	 * mutex.
> > > +	 */
> > 
> > the comment doesn't really explain why it is safe. THere are other users
> > which are not using the lock. E.g. just look at free_swap_slot above.
> > How can
> > 	cache->slots_ret[cache->n_ret++] = entry; be safe wrt.
> > 	pentry = &cache->slots[cache->cur++];
> > 	entry = *pentry;
> > 
> > Both of them might touch the same slot, no? Btw. I would rather prefer this
> > would be a follow up fix with the trace and the detailed explanation.
> > 
> 
> The cache->slots_ret  is protected by cache->free_lock and cache->slots is
> protected by cache->free_lock.

Ohh, I have misread those names and considered them the same thing.
Sorry about the confusion. I will look at code more deeply tomorrow.

> They are two separate structures, one for
> caching the slots returned and one for caching the slots allocated.  So
> they do no touch the same slots.  We'll update the comments so it is clearer.

That would be really appreciated.

-- 
Michal Hocko
SUSE Labs