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