On Thu, 20 Sep 2007 11:26:47 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

quoted text
> On Wed, 19 Sep 2007, KAMEZAWA Hiroyuki wrote:
> 
> > Any comments are welcome.
> 
> I am still a bit confused as to what the benefit of this is.
> 
Honestly, I have 3 purposes, 2 for readability/clarificaton and 1 for my trial.

1. Clarify page cache <-> inode relationship before *new concept of page cache*,
   yours or someone else's is introduced.

2. There are some places using PAGE_MAPPING_ANON directly. I don't want to see
   following line in .c file. 
   ==
   anon_vma = (struct anon_vma *)(mapping - PAGE_MAPPING_ANON);
   ==

3. I want to *try* page->mapping overriding... store  memory resource controller's   
   information in page->mapping. By this, memory controller doesn't enlarge sizeof
   struct page. (works well in my small test.)
   Before doing that, I have to hide page->mapping from direct access.


quoted text
> > +/*
> > + * On an anonymous page mapped into a user virtual memory area,
> > + * page->mapping points to its anon_vma, not to a struct address_space;
> > + * with the PAGE_MAPPING_ANON bit set to distinguish it.
> > + *
> > + * Please note that, confusingly, "page_mapping" refers to the inode
> > + * address_space which maps the page from disk; whereas "page_mapped"
> > + * refers to user virtual address space into which the page is mapped.
> > + */
> > +#define PAGE_MAPPING_ANON       1
> > +
> > +static inline bool PageAnon(struct page *page)
> 
> bool??? That is unusual?

This is my first experience of using bool in Linux kernel.. :)

I know bool is not very widely used in Linux now but I tried it because 
this function obviously returns yes or no, and C language supports bool as
_Bool now. If messy, I'll avoid using this in this time..


quoted text
> 
> > +static inline struct address_space *page_mapping_cache(struct page *page)
> > +{
> > +	if (!page->mapping || PageAnon(page))
> > +		return NULL;
> > +	return page->mapping;
> > +}
> 
> That is confusing.
> 
> if (PageAnon(page))
> 	return NULL;
> return page->mapping;
ok,

quoted text
> > +static inline struct address_space *page_mapping(struct page *page)
> > +{
> > +	struct address_space *mapping = page->mapping;
> > +
> > +	VM_BUG_ON(PageSlab(page));
> > +	if (unlikely(PageSwapCache(page)))
> > +		mapping = &swapper_space;
> > +#ifdef CONFIG_SLUB
> > +	else if (unlikely(PageSlab(page)))
> > +		mapping = NULL;
> > +#endif
> 
> The #ifdef does not exist in rc6-mm1. No need to reintroduce it.
> 
ok, thanks.

quoted text
> > +static inline bool
> > +is_page_consistent(struct page *page, struct address_space *mapping)
> > +{
> > +	struct address_space *check = page_mapping_cache(page);
> > +	return (check == mapping);
> > +}
> 
> Why do we need a special function? Why is it safer?
> 
For clarify meaning of compareing page_mapping_cache() with mapping.
Does this reduce readability ?

Thank you for comments.

Regards,
-Kame





-
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/