lkml.org 
[lkml]   [2011]   [Apr]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] mm: make read-only accessors take const parameters
    On Fri, 15 Apr 2011, Phil Carmody wrote:

    > Pulling out bits from flags and atomic_read() do not modify
    > anything, nor do we want to modify anything. We can extend that
    > insight to clients. This makes static code analysis easier.
    >
    > I'm not happy with the _ro bloat, but at least it doesn't change
    > the size of the generated code. An alternative would be a type-
    > less macro.
    >

    The only advantage I can see by doing this is that functions calling these
    helpers can mark their struct page * formals or automatic variables with
    const as well.

    That's only worthwhile if you have actual usecases where these newly-
    converted helpers generate more efficient code as a result of being able
    to be marked const themselves. If that's the case, then they should
    be proposed as an individual patch with both the caller and the helper
    being marked const at the same time.

    It doesn't really matter that these helpers are all inline since the
    qualifiers will still be enforced at compile time.

    > Also cleaned up some unnecessary (brackets).
    >

    These cleanups can be pushed through the trivial tree if you're
    interested, email Jiri Kosina <trivial@kernel.org>.

    > Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
    > ---
    > include/linux/mm.h | 27 +++++++++++++++++----------
    > include/linux/page-flags.h | 8 ++++----
    > 2 files changed, 21 insertions(+), 14 deletions(-)
    >
    > diff --git a/include/linux/mm.h b/include/linux/mm.h
    > index 692dbae..7134563 100644
    > --- a/include/linux/mm.h
    > +++ b/include/linux/mm.h
    > @@ -353,9 +353,16 @@ static inline struct page *compound_head(struct page *page)
    > return page;
    > }
    >
    > -static inline int page_count(struct page *page)
    > +static inline const struct page *compound_head_ro(const struct page *page)
    > {
    > - return atomic_read(&compound_head(page)->_count);
    > + if (unlikely(PageTail(page)))
    > + return page->first_page;
    > + return page;
    > +}
    > +
    > +static inline int page_count(const struct page *page)
    > +{
    > + return atomic_read(&compound_head_ro(page)->_count);
    > }
    >
    > static inline void get_page(struct page *page)

    Adding this excess code, however, is unnecessary since no caller of
    page_count() is optimized to use a const struct page * itself; if such an
    optimization actually exists, then it would need to be demonstrated with
    data before we'd want to add this extra function.

    If you'd like to propose a patch for the remainder of the
    "struct page *" -> "const struct page *" changes in this email, then
    there's no downside and could potentially be useful in the future for
    callers, so you can add my

    Acked-by: David Rientjes <rientjes@google.com>

    to such a patch.

    [ Please separate out the trivial changes by removing the brackets,
    though, and submit them to Jiri instead. ]

    > @@ -638,7 +645,7 @@ static inline pte_t maybe_mkwrite(pte_t pte, struct vm_area_struct *vma)
    > #define SECTIONS_MASK ((1UL << SECTIONS_WIDTH) - 1)
    > #define ZONEID_MASK ((1UL << ZONEID_SHIFT) - 1)
    >
    > -static inline enum zone_type page_zonenum(struct page *page)
    > +static inline enum zone_type page_zonenum(const struct page *page)
    > {
    > return (page->flags >> ZONES_PGSHIFT) & ZONES_MASK;
    > }
    > @@ -651,7 +658,7 @@ static inline enum zone_type page_zonenum(struct page *page)
    > * We guarantee only that it will return the same value for two
    > * combinable pages in a zone.
    > */
    > -static inline int page_zone_id(struct page *page)
    > +static inline int page_zone_id(const struct page *page)
    > {
    > return (page->flags >> ZONEID_PGSHIFT) & ZONEID_MASK;
    > }
    > @@ -786,7 +793,7 @@ static inline void *page_rmapping(struct page *page)
    > return (void *)((unsigned long)page->mapping & ~PAGE_MAPPING_FLAGS);
    > }
    >
    > -static inline int PageAnon(struct page *page)
    > +static inline int PageAnon(const struct page *page)
    > {
    > return ((unsigned long)page->mapping & PAGE_MAPPING_ANON) != 0;
    > }
    > @@ -809,20 +816,20 @@ static inline pgoff_t page_index(struct page *page)
    > */
    > static inline void reset_page_mapcount(struct page *page)
    > {
    > - atomic_set(&(page)->_mapcount, -1);
    > + atomic_set(&page->_mapcount, -1);
    > }
    >
    > -static inline int page_mapcount(struct page *page)
    > +static inline int page_mapcount(const struct page *page)
    > {
    > - return atomic_read(&(page)->_mapcount) + 1;
    > + return atomic_read(&page->_mapcount) + 1;
    > }
    >
    > /*
    > * Return true if this page is mapped into pagetables.
    > */
    > -static inline int page_mapped(struct page *page)
    > +static inline int page_mapped(const struct page *page)
    > {
    > - return atomic_read(&(page)->_mapcount) >= 0;
    > + return atomic_read(&page->_mapcount) >= 0;
    > }
    >
    > /*
    > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
    > index 811183d..7f8e553 100644
    > --- a/include/linux/page-flags.h
    > +++ b/include/linux/page-flags.h
    > @@ -135,7 +135,7 @@ enum pageflags {
    > * Macros to create function definitions for page flags
    > */
    > #define TESTPAGEFLAG(uname, lname) \
    > -static inline int Page##uname(struct page *page) \
    > +static inline int Page##uname(const struct page *page) \
    > { return test_bit(PG_##lname, &page->flags); }
    >
    > #define SETPAGEFLAG(uname, lname) \
    > @@ -173,7 +173,7 @@ static inline int __TestClearPage##uname(struct page *page) \
    > __SETPAGEFLAG(uname, lname) __CLEARPAGEFLAG(uname, lname)
    >
    > #define PAGEFLAG_FALSE(uname) \
    > -static inline int Page##uname(struct page *page) \
    > +static inline int Page##uname(const struct page *page) \
    > { return 0; }
    >
    > #define TESTSCFLAG(uname, lname) \
    > @@ -345,7 +345,7 @@ static inline void set_page_writeback(struct page *page)
    > __PAGEFLAG(Head, head) CLEARPAGEFLAG(Head, head)
    > __PAGEFLAG(Tail, tail)
    >
    > -static inline int PageCompound(struct page *page)
    > +static inline int PageCompound(const struct page *page)
    > {
    > return page->flags & ((1L << PG_head) | (1L << PG_tail));
    >
    > @@ -379,7 +379,7 @@ __PAGEFLAG(Head, compound)
    > */
    > #define PG_head_tail_mask ((1L << PG_compound) | (1L << PG_reclaim))
    >
    > -static inline int PageTail(struct page *page)
    > +static inline int PageTail(const struct page *page)
    > {
    > return ((page->flags & PG_head_tail_mask) == PG_head_tail_mask);
    > }


    \
     
     \ /
      Last update: 2011-04-17 01:51    [W:3.641 / U:0.676 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site