Re: [patch 2/8] mm: vmscan: disregard swappiness shortly beforegoing OOM

[Johannes Weiner]

On Thu, Dec 13, 2012 at 10:25:43PM +0000, Satoru Moriya wrote:
> 
> On 12/13/2012 11:05 AM, Michal Hocko wrote:> On Thu 13-12-12 16:29:59, Michal Hocko wrote:
> >> On Thu 13-12-12 10:34:20, Mel Gorman wrote:
> >>> On Wed, Dec 12, 2012 at 04:43:34PM -0500, Johannes Weiner wrote:
> >>>> When a reclaim scanner is doing its final scan before giving up and 
> >>>> there is swap space available, pay no attention to swappiness 
> >>>> preference anymore.  Just swap.
> >>>>
> >>>> Note that this change won't make too big of a difference for 
> >>>> general
> >>>> reclaim: anonymous pages are already force-scanned when there is 
> >>>> only very little file cache left, and there very likely isn't when 
> >>>> the reclaimer enters this final cycle.
> >>>>
> >>>> Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> >>>
> >>> Ok, I see the motivation for your patch but is the block inside 
> >>> still wrong for what you want? After your patch the block looks like 
> >>> this
> >>>
> >>>                 if (sc->priority || noswap) {
> >>>                         scan >>= sc->priority;
> >>>                         if (!scan && force_scan)
> >>>                                 scan = SWAP_CLUSTER_MAX;
> >>>                         scan = div64_u64(scan * fraction[file], denominator);
> >>>                 }
> >>>
> >>> if sc->priority == 0 and swappiness==0 then you enter this block but 
> >>> fraction[0] for anonymous pages will also be 0 and because of the 
> >>> ordering of statements there, scan will be
> >>>
> >>> scan = scan * 0 / denominator
> >>>
> >>> so you are still not reclaiming anonymous pages in the swappiness=0 
> >>> case. What did I miss?
> >>
> >> Yes, now that you have mentioned that I realized that it really 
> >> doesn't make any sense. fraction[0] is _always_ 0 for swappiness==0. 
> >> So we just made a bigger pressure on file LRUs. So this sounds like a 
> >> misuse of the swappiness. This all has been introduced with fe35004f 
> >> (mm: avoid swapping out with swappiness==0).
> >>
> >> I think that removing swappiness check make sense but I am not sure 
> >> it does what the changelog says. It should have said that checking 
> >> swappiness doesn't make any sense for small LRUs.
> >
> > Bahh, wait a moment. Now I remember why the check made sense 
> > especially for memcg.
> > It made "don't swap _at all_ for swappiness==0" for real - you are 
> > even willing to sacrifice OOM. Maybe this is OK for the global case 
> > because noswap would safe you here (assuming that there is no swap if 
> > somebody doesn't want to swap at all and swappiness doesn't play such 
> > a big role) but for memcg you really might want to prevent from 
> > swapping - not everybody has memcg swap extension enabled and swappiness is handy then.
> > So I am not sure this is actually what we want. Need to think about it.
> 
> I introduced swappiness check here with fe35004f because, in some
> cases, we prefer OOM to swap out pages to detect problems as soon
> as possible. Basically, we design the system not to swap out and
> so if it causes swapping, something goes wrong.

I might be missing something terribly obvious, but... why do you add
swap space to the system in the first place?  Or in case of cgroups,
why not set the memsw limit equal to the memory limit?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/