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

[Daisuke Nishimura <d-nishimura@mtf.biglobe.ne.jp>]

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, 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>