Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 17 Jun 2020 05:17:17 -0700
From:   Matthew Wilcox <willy@infradead.org>
To:     js1304@gmail.com
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@kernel.org>,
        Hugh Dickins <hughd@google.com>,
        Minchan Kim <minchan@kernel.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Mel Gorman <mgorman@techsingularity.net>, kernel-team@lge.com,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH v6 4/6] mm/swapcache: support to handle the exceptional
 entries in swapcache
Message-ID: <20200617121717.GI8681@bombadil.infradead.org>
References: <1592371583-30672-1-git-send-email-iamjoonsoo.kim@lge.com>
 <1592371583-30672-5-git-send-email-iamjoonsoo.kim@lge.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1592371583-30672-5-git-send-email-iamjoonsoo.kim@lge.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jun 17, 2020 at 02:26:21PM +0900, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> Swapcache doesn't handle the exceptional entries since there is no case

Don't call them exceptional entries.

The radix tree has/had the concecpt of exceptional entries.  The swapcache
doesn't use the radix tree any more, it uses the XArray.  The XArray
has value entries.

But you shouldn't call them value entries either; that's an XArray
concept.  The swap cache and indeed page cache use value entries to
implement shadow entries (they're also used to implement dax entries and
swap entries in the page cache).  So just call them shadow entries here.

I know there are still places which use the term 'nrexceptional' in
the kernel.  I just haven't got round to replacing them yet.  Please
don't add more.

> +void clear_shadow_from_swap_cache(int type, unsigned long begin,
> +				unsigned long end)
> +{
> +	unsigned long curr;
> +	void *old;
> +	swp_entry_t entry = swp_entry(type, begin);
> +	struct address_space *address_space = swap_address_space(entry);
> +	XA_STATE(xas, &address_space->i_pages, begin);
> +
> +retry:
> +	xa_lock_irq(&address_space->i_pages);
> +	for (curr = begin; curr <= end; curr++) {
> +		entry = swp_entry(type, curr);
> +		if (swap_address_space(entry) != address_space) {
> +			xa_unlock_irq(&address_space->i_pages);
> +			address_space = swap_address_space(entry);
> +			begin = curr;
> +			xas_set(&xas, begin);
> +			goto retry;
> +		}
> +
> +		old = xas_load(&xas);
> +		if (!xa_is_value(old))
> +			continue;
> +		xas_store(&xas, NULL);
> +		address_space->nrexceptional--;
> +		xas_next(&xas);
> +	}
> +	xa_unlock_irq(&address_space->i_pages);
> +}

This is a very clunky loop.  I'm not sure it's even right, given that
you change address space without changing the xas's address space.  How
about this?

	for (;;) {
		XA_STATE(xas, &address_space->i_pages, begin);
		unsigned long nr_shadows = 0;

		xas_lock_irq(&xas);
		xas_for_each(&xas, entry, end) {
			if (!xa_is_value(entry))
				continue;
			xas_store(&xas, NULL);
			nr_shadows++;
		}
		address_space->nr_exceptionals -= nr_shadows;
		xas_unlock_irq(&xas);

		if (xas.xa_index >= end)
			break;
		entry = swp_entry(type, xas.xa_index);
		address_space = swap_address_space(entry);
	}