Re: [patch 9/9] mm: keep page cache radix tree nodes in check

[Minchan Kim <minchan@kernel.org>]

On Thu, Jan 23, 2014 at 02:22:12PM -0500, Johannes Weiner wrote:
> On Thu, Jan 23, 2014 at 02:20:14PM +0900, Minchan Kim wrote:
> > On Wed, Jan 22, 2014 at 01:42:17PM -0500, Johannes Weiner wrote:
> > > On Mon, Jan 13, 2014 at 04:39:47PM +0900, Minchan Kim wrote:
> > > > On Fri, Jan 10, 2014 at 01:10:43PM -0500, Johannes Weiner wrote:
> > > > > @@ -123,9 +129,39 @@ static void page_cache_tree_delete(struct address_space *mapping,
> > > > >  		 * same time and miss a shadow entry.
> > > > >  		 */
> > > > >  		smp_wmb();
> > > > > -	} else
> > > > > -		radix_tree_delete(&mapping->page_tree, page->index);
> > > > > +	}
> > > > >  	mapping->nrpages--;
> > > > > +
> > > > > +	if (!node) {
> > > > > +		/* Clear direct pointer tags in root node */
> > > > > +		mapping->page_tree.gfp_mask &= __GFP_BITS_MASK;
> > > > > +		radix_tree_replace_slot(slot, shadow);
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	/* Clear tree tags for the removed page */
> > > > > +	index = page->index;
> > > > > +	offset = index & RADIX_TREE_MAP_MASK;
> > > > > +	for (tag = 0; tag < RADIX_TREE_MAX_TAGS; tag++) {
> > > > > +		if (test_bit(offset, node->tags[tag]))
> > > > > +			radix_tree_tag_clear(&mapping->page_tree, index, tag);
> > > > > +	}
> > > > > +
> > > > > +	/* Delete page, swap shadow entry */
> > > > > +	radix_tree_replace_slot(slot, shadow);
> > > > > +	node->count--;
> > > > > +	if (shadow)
> > > > > +		node->count += 1U << RADIX_TREE_COUNT_SHIFT;
> > > > 
> > > > Nitpick2:
> > > > It should be a function of workingset.c rather than exposing
> > > > RADIX_TREE_COUNT_SHIFT?
> > > > 
> > > > IMO, It would be better to provide some accessor functions here, too.
> > > 
> > > The shadow maintenance and node lifetime management are pretty
> > > interwoven to share branches and reduce instructions as these are
> > > common paths.  I don't see how this could result in cleaner code while
> > > keeping these advantages.
> > 
> > What I want is just put a inline accessor in somewhere like workingset.h
> > 
> > static inline void inc_shadow_entry(struct radix_tree_node *node)
> > {
> >     node->count += 1U << RADIX_TREE_COUNT_MASK;
> > }
> > 
> > So, anyone don't need to know that node->count upper bits present
> > count of shadow entry.
> 
> Okay, but then you have to cover lower bits as well, without explicit
> higher bit access it would be confusing to use the mask for lower
> bits.
> 
> Something like the following?

LGTM.

-- 
Kind regards,
Minchan Kim

--
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>