linux-kernel - Re: [RFC][PATCH] fix swap entries is not reclaimed in proper way for memg v3.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20090426010658.c0fa3258.d-nishimura@mtf.biglobe.ne.jp>
Date:	Sun, 26 Apr 2009 01:06:58 +0900
From:	Daisuke Nishimura <d-nishimura@....biglobe.ne.jp>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc:	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"balbir@...ux.vnet.ibm.com" <balbir@...ux.vnet.ibm.com>,
	"hugh@...itas.com" <hugh@...itas.com>,
	d-nishimura@....biglobe.ne.jp,
	Daisuke Nishimura <nishimura@....nes.nec.co.jp>
Subject: Re: [RFC][PATCH] fix swap entries is not reclaimed in proper way
 for memg v3.

A few minor nitpicks :)

> > +static void memcg_fixup_stale_swapcache(struct work_struct *work)
> > +{
> > +	int pos = 0;
> > +	swp_entry_t entry;
> > +	struct page *page;
> > +	int forget, ret;
> > +
> > +	while (ssc.num) {
> > +		spin_lock(&ssc.lock);
> > +		pos = find_next_bit(ssc.usemap, STALE_ENTS, pos);
> > +		spin_unlock(&ssc.lock);
> > +
> > +		if (pos >= STALE_ENTS)
> > +			break;
> > +
> > +		entry = ssc.ents[pos];
> > +
> > +		forget = 1;
> > +		page = lookup_swap_cache(entry);
I think using find_get_page() would be better.
lookup_swap_cache() update swapcache_info.

> > +		if (page) {
> > +			lock_page(page);
> > +			ret = try_to_free_swap(page);
> > +			/* If it's still under I/O, don't forget it */
> > +			if (!ret && PageWriteback(page))
> > +				forget = 0;
> > +			unlock_page(page);
> I think we need page_cache_release().
> lookup_swap_cache() gets the page.
> 
> > +		}
> > +		if (forget) {
> > +			spin_lock(&ssc.lock);
> > +			clear_bit(pos, ssc.usemap);
> > +			ssc.num--;
> > +			if (ssc.num < STALE_ENTS/2)
> > +				ssc.congestion = 0;
> > +			spin_unlock(&ssc.lock);
> > +		}
> > +		pos++;
> > +	}
> > +	if (ssc.num) /* schedule me again */
> > +		schedule_delayed_work(&ssc.gc_work, HZ/10);
We can use schedule_ssc_gc() here.
(It should be defined before this, of course. And can be inlined.)

> "if (ssc.congestion)" would be better ?
> 
> > +	return;
> > +}
> > +
> 

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