On Mon, 1 Dec 2008 01:18:09 +0000 (GMT) Hugh Dickins <hugh@xxxxxxxxxxx> wrote: > On Fri, 28 Nov 2008, Nick Piggin wrote: > > On Thu, Nov 27, 2008 at 06:14:18PM +0000, Hugh Dickins wrote: > > > On Thu, 27 Nov 2008, Nick Piggin wrote: > > > > On Thu, Nov 27, 2008 at 11:52:40AM +0200, Pekka Enberg wrote: > > > > > > - err = add_to_page_cache_lru(page, mapping, index, gfp_mask); > > > > > > + err = add_to_page_cache_lru(page, mapping, index, > > > > > > + (gfp_mask & (__GFP_FS|__GFP_IO|__GFP_WAIT|__GFP_HIGH))); > > > > > > > > > > Can we use GFP_RECLAIM_MASK here? I mean, surely we need to pass > > > > > __GFP_NOFAIL, for example, down to radix_tree_preload() et al? > > > > > > I certainly agree with Pekka's suggestion to use GFP_RECLAIM_MASK. > > > > > > > > > > > Updated patch. > > > > > > I'm not sure about it. I came here before 2.6.25, not yet got around > > > to submitting, I went in the opposite direction. What drove me was an > > > irritation at the growing number of & ~__GFP_HIGHMEMs (after adding a > > > couple myself in shmem.c). At the least, I think we ought to change > > > those to & GFP_RECLAIM_MASKs (it seems we don't have one, but can > > > imagine a block driver that wants GFP_DMA or GFP_DMA32 for pagecache: > > > there's no reason to alloc its kernel-internal data structures for DMA). > > > > > > My patch went the opposite direction to yours, in that I was pushing > > > down the GFP_RECLAIM_MASKing into lib/radix-tree.c and mm/memcontrol.c > > > (but that now doesn't kmalloc for itself, so no longer needs the mask). > > > > > > I'm not sure which way is the right way: you can say I'm inconsistent > > > not to push it down into slab/sleb/slib/slob/slub itself, but I've a > > > notion that someone might object to any extra intrns in their hotpaths. > > > > > > My design principle, I think, was to minimize the line length of > > > the maximum number of source lines: you may believe in other > > > design principles of higher value. > > > > I think logically it doesn't belong in those files. The problem comes > > purely from the pagecache layer because we're using gfp masks to ask > > for two different (and not quite compatible things). > > > > We're using it to specify the pagecache page's memory type, and also > > the allocation context for both the pagecache page, and any other > > supporting allocations required. > > > > I think it's much more logical to go into the pagecache layer. > > Fair enough, I can go along with that, and stomach the longer lines. > > I do think that we ought to change those &~__GFP_HIGHMEMs to > &GFP_RECLAIM_MASKs, but that doesn't have to be part of your patch. > > And it does make me think that Kamezawa-san's patch in mmotm > memcg-fix-gfp_mask-of-callers-of-charge.patch > which is changing the gfp arg to assorted mem_cgroup interfaces > from GFP_KERNEL to GFP_HIGHUSER_MOVABLE, is probably misguided: > that gfp arg is not telling them what zones of memory to use, > it's telling them the constraints if it reclaims memory. > It comes from the fact that memcg reclaims memory not because of memory shortage but of memory limit. "From which zone the memory should be reclaimed" is not problem. I used GFP_HIGHUSER_MOVABLE to show "reclaim from anyware" in explicit way. too bad ? Thanks, -Kame > I'd skip the churn and leave them as GFP_KERNEL - you could argue > that we should define another another name for that set of reclaim > constraints, but I think it would end up too much hair-splitting. > > > > Updating it quickly to 2.6.28-rc6, built but untested, here's mine. > > > I'm not saying it's the right approach and yours wrong, just please > > > consider it before deciding on which way to go. > > > > > > I've left in the hunk from fs/buffer.c in case you can decipher it, > > > I think that's what held me up from submitting: I've never worked > > > out since whether that change is a profound insight into reality > > > here, or just a blind attempt to reduce the line length. > > > > I don't see why you drop the mapping_gfp_mask from there... if we ever > > change the buffer layer to support HIGHMEM pages, we'd want to set the > > inode's mapping_gfp_mask to GFP_HIGHUSER rather than GFP_USER, wouldn't > > we? > > It was just a mysterious fragment, if it makes no sense to you either, > let's just forget it - thanks for looking. > > Hugh > -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
