MIME-Version: 1.0
Message-ID: <89702248-0c3f-465c-bc1f-2115a21c8c89@default>
Date: Fri, 31 Aug 2012 10:23:21 -0700 (PDT)
From: Dan Magenheimer <dan.magenheimer@oracle.com>
To: Konrad Wilk <konrad.wilk@oracle.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Hugh Dickins <hughd@google.com>
Subject: RE: [PATCH] frontswap: support exclusive gets if tmem backend is
 capable
References: <5557ec97-daa1-41a6-b3db-671f116ddc50@default>
 <20120831170814.GF18929@localhost.localdomain>
In-Reply-To: <20120831170814.GF18929@localhost.localdomain>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2290
Lines: 68

> From: Konrad Rzeszutek Wilk

Hi Konrad --

Thanks for the fast feedback!

> > +#define FRONTSWAP_HAS_EXCLUSIVE_GETS
> > +extern void frontswap_tmem_exclusive_gets(bool);
> 
> I don't think you need the #define here..

The #define is used by an ifdef in the backend to ensure
that it is using a version of frontswap that has this feature,
so avoids the need for the frontend (frontswap) and
the backend (e.g. zcache2) to merge in lockstep.

> > +EXPORT_SYMBOL(frontswap_tmem_exclusive_gets);
> 
> We got two of these now - the writethrough and this one. Merging
> them in one function and one flag might be better. So something like:
> static int frontswap_mode = 0;
>
> void frontswap_set_mode(int set_mode)
> {
> 	if (mode & (FRONTSWAP_WRITETH | FRONTSWAP_EXCLUS..)
> 		mode |= set_mode;
> }

IMHO, it's too soon to try to optimize this.  One or
both of these may go away.   Or the mode may become
more fine-grained in the future (e.g. to allow individual
gets to be exclusive).

So unless you object strongly, let's just leave this
as is for now and revisit in the future if more "modes"
are needed.
 
> ... and
> > +
> > +/*
> >   * Called when a swap device is swapon'd.
> >   */
> >  void __frontswap_init(unsigned type)
> > @@ -174,8 +190,13 @@ int __frontswap_load(struct page *page)
> >  	BUG_ON(sis == NULL);
> >  	if (frontswap_test(sis, offset))
> >  		ret = (*frontswap_ops.load)(type, offset, page);
> > -	if (ret == 0)
> > +	if (ret == 0) {
> >  		inc_frontswap_loads();
> > +		if (frontswap_tmem_exclusive_gets_enabled) {
> 
> For these perhaps use asm goto for optimization? Is this showing up in
> perf as a hotspot? The asm goto might be a bit too much.

This is definitely not a performance hotspot.  Frontswap code
only is ever executed in situations where a swap-to-disk would
otherwise have occurred.  And in this case, this code only
gets executed after the frontswap_test has confirmed that
tmem does already contain the page of data, in which case
there is thousands of cycles spent copying and/or decompressing.

Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/