[PATCH 0/1] mm: make read-only accessors take const pointer parameters

[PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Phil Carmody]

Sending this one its own as it either becomes an enabler for further
related patches, or if nacked, shuts the door on them. Better to test
the water before investing too much time on such things.

Whilst following a few static code analysis warnings, it became clear
that either the tool (which I believe is considered practically state of
the art) was very dumb when sniffing into called functions, or that a
simple const flag would either help it not make the incorrect paranoid
assumptions that it did, or help me dismiss the report as a false
positive more quickly.

Of course, this is core core code, and shouldn't be diddled with lightly,
but it's because it's core code that it's an enabler.

Awaiting the judgement of the Solomons,
Cheers,
Phil

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm: make read-only accessors take const parameters

[Phil Carmody]

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.

Also cleaned up some unnecessary (brackets).

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)
@@ -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);
 }
-- 
1.7.2.rc1.37.gf8c40

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: make read-only accessors take const parameters

[Christoph Lameter]

On Fri, 15 Apr 2011, Phil Carmody wrote:

> +++ 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;
> +}

Can you make compound_head take a const pointer too to avoid this?

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: make read-only accessors take const parameters

[Phil Carmody]

On 15/04/11 09:51 -0500, ext Christoph Lameter wrote:
> On Fri, 15 Apr 2011, Phil Carmody wrote:
> 
> > +++ 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;
> > +}
> 
> Can you make compound_head take a const pointer too to avoid this?

Not in C, alas. As it returns what it's given I wouldn't want it to lie
about the type of what it returns, and some of its clients want it to
return something writeable.

The simplest macro would have multiple-evaluation issues:

#define compound_head(page) (PageTail(page) ? (page)->first_page : (page))

Not that there are any clients who would misuse that currently, but setting
traps isn't a good way to make things cleaner.

Phil

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: make read-only accessors take const parameters

[Artem Bityutskiy]

On Fri, 2011-04-15 at 19:07 +0300, Phil Carmody wrote:
> Not in C, alas. As it returns what it's given I wouldn't want it to
> lie
> about the type of what it returns, and some of its clients want it to
> return something writeable. 

I think this little lie is prettier than _ro variants of functions. I do
not think you'll go far with your 'const' quest if you start adding _ro
variants for different core functions, but if you just cast the return
pointer to non-const you might be more successful.

-- 
Best Regards,
Artem Bityutskiy (D?N?N?N?D 1/4  D?D,N?N?N?DoD,D1)

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: make read-only accessors take const parameters

[David Rientjes]

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);
>  }

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Andrea Arcangeli]

Hello Phil,

On Fri, Apr 15, 2011 at 12:56:16PM +0300, Phil Carmody wrote:
> 
> Sending this one its own as it either becomes an enabler for further
> related patches, or if nacked, shuts the door on them. Better to test
> the water before investing too much time on such things.
> 
> Whilst following a few static code analysis warnings, it became clear
> that either the tool (which I believe is considered practically state of
> the art) was very dumb when sniffing into called functions, or that a
> simple const flag would either help it not make the incorrect paranoid
> assumptions that it did, or help me dismiss the report as a false
> positive more quickly.
> 
> Of course, this is core core code, and shouldn't be diddled with lightly,
> but it's because it's core code that it's an enabler.
> 
> Awaiting the judgement of the Solomons,

What's the benefit of having it const other than shutdown the warnings
from the static code analysis? I doubt gcc can generate any better
output from this change because it's all inline anyway.

I guess the only chance this could help is if we call an extern
function and we read the pointer before and after the external call,
in that case gcc could assume the memory didn't change across the
extern function and just cache the value in callee-saved register
without having to re-read memory after the extern function
returns. But there isn't any extern function there...

I guess the static code analysis shouldn't suggest a const if it's all
inline and gcc has full visibility on everything that is done inside
those functions at build time.

But maybe I'm missing something gcc could do better with const that it
can't now.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Phil Carmody]

On 15/04/11 16:51 +0200, ext Andrea Arcangeli wrote:
> Hello Phil,
> 
> On Fri, Apr 15, 2011 at 12:56:16PM +0300, Phil Carmody wrote:
> > 
> > Sending this one its own as it either becomes an enabler for further
> > related patches, or if nacked, shuts the door on them. Better to test
> > the water before investing too much time on such things.
> > 
> > Whilst following a few static code analysis warnings, it became clear
> > that either the tool (which I believe is considered practically state of
> > the art) was very dumb when sniffing into called functions, or that a
> > simple const flag would either help it not make the incorrect paranoid
> > assumptions that it did, or help me dismiss the report as a false
> > positive more quickly.
> > 
> > Of course, this is core core code, and shouldn't be diddled with lightly,
> > but it's because it's core code that it's an enabler.
> > 
> > Awaiting the judgement of the Solomons,
> 
> What's the benefit of having it const other than shutdown the warnings
> from the static code analysis? I doubt gcc can generate any better
> output from this change because it's all inline anyway.

Yup, the only improvement occurs if there's an opaque layer between this
lower level code and a client that could benefit from making a const
assumption, and that opaque layer could also inherit/propagate the
constness.

> I guess the only chance this could help is if we call an extern
> function and we read the pointer before and after the external call,
> in that case gcc could assume the memory didn't change across the
> extern function and just cache the value in callee-saved register
> without having to re-read memory after the extern function
> returns. But there isn't any extern function there...

Yup, that direction's a dead end, but there is potential for clients of
clients. I'm unfortunately unable to find the example that prompted me
to look down this path, as it would depend on an as-yet-unwritten client
of these functions to propagate constness up another layer. It was
probably in FUSE, as that's the warning at the top of my screen
currently.

> I guess the static code analysis shouldn't suggest a const if it's all
> inline and gcc has full visibility on everything that is done inside
> those functions at build time.
> 
> But maybe I'm missing something gcc could do better with const that it
> can't now.

I think gcc itself is smart enough to have already concluded what it 
can and it will not immediately benefit the build from just this change.

I don't think the static analysis tools are as smart as gcc though, by
any means. GCC actually inlines, so everything is visible to it. The
static analysis tools only remember the subset of information that they
think is useful, and apparently 'didn't change anything, even though it 
could' isn't considered so useful.

I'm just glad this wasn't an insta-nack, as I am quite a fan of consts,
and hopefully something can be worked out.

Thanks for your input,
Phil

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Andrea Arcangeli]

On Fri, Apr 15, 2011 at 06:59:16PM +0300, Phil Carmody wrote:
> of these functions to propagate constness up another layer. It was
> probably in FUSE, as that's the warning at the top of my screen
> currently.

These function themselfs are inline too, so gcc already can see if
memory has been modified inside the inline function, so it shouldn't
provide an advantage. It would provide an advantage if page_count and
friends weren't inline.

> I think gcc itself is smart enough to have already concluded what it 
> can and it will not immediately benefit the build from just this change.

Hmm not sure... but I would hope it is smart enough already with
inline (it should never be worse to inline than encoding the whole
thing by hand in the caller, so skipping the function call
alltogether which then wouldn't require any const).

> I don't think the static analysis tools are as smart as gcc though, by
> any means. GCC actually inlines, so everything is visible to it. The
> static analysis tools only remember the subset of information that they
> think is useful, and apparently 'didn't change anything, even though it 
> could' isn't considered so useful.
> 
> I'm just glad this wasn't an insta-nack, as I am quite a fan of consts,
> and hopefully something can be worked out.

I'm not against it if it's from code strict point of view, I was
mostly trying to understand if this could have any impact, in which
case it wouldn't be false positive. I think it's a false positive if
gcc is as smart as I hope it to be. If we want it from coding style
reasons to keep the code more strict that's fine with me of course.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Artem Bityutskiy]

On Fri, 2011-04-15 at 18:09 +0200, Andrea Arcangeli wrote:
> On Fri, Apr 15, 2011 at 06:59:16PM +0300, Phil Carmody wrote:
> > of these functions to propagate constness up another layer. It was
> > probably in FUSE, as that's the warning at the top of my screen
> > currently.
> 
> These function themselfs are inline too, so gcc already can see if
> memory has been modified inside the inline function, so it shouldn't
> provide an advantage. It would provide an advantage if page_count and
> friends weren't inline.
> 
> > I think gcc itself is smart enough to have already concluded what it 
> > can and it will not immediately benefit the build from just this change.
> 
> Hmm not sure... but I would hope it is smart enough already with
> inline (it should never be worse to inline than encoding the whole
> thing by hand in the caller, so skipping the function call
> alltogether which then wouldn't require any const).
> 
> > I don't think the static analysis tools are as smart as gcc though, by
> > any means. GCC actually inlines, so everything is visible to it. The
> > static analysis tools only remember the subset of information that they
> > think is useful, and apparently 'didn't change anything, even though it 
> > could' isn't considered so useful.
> > 
> > I'm just glad this wasn't an insta-nack, as I am quite a fan of consts,
> > and hopefully something can be worked out.
> 
> I'm not against it if it's from code strict point of view, I was
> mostly trying to understand if this could have any impact, in which
> case it wouldn't be false positive. I think it's a false positive if
> gcc is as smart as I hope it to be. If we want it from coding style
> reasons to keep the code more strict that's fine with me of course.

I think it is good when small core functions like this are strict and
use 'const' whenever possible, even though 'const' is so imperfect in C.

Let me give an example from my own experience. I was writing code which
was using the kernel RB trees, and I was trying to be strict and use
'const' whenever possible. But because the core functions like 'rb_next'
do not have 'const' modifier, I could not use const in many many places
of my code, because gcc was yelling. And I was not very enthusiastic to
touch the RB-tree code that time.

So the outline is that when core functions are not strict, they force
the upper layers to not use 'const' so making the linux less strict
overall, and making gcc _potential_ to optimize less.

The kernel is large and complex, if if today we do not see any apparent
optimization out of this, to tomorrow when the code changes, new clients
come to the picture - we might get it!

Hence,

Acked-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

And

Thanks-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

P.S.: Phil, probably you've noticed my hint about the RB-trees? :-)

-- 
Best Regards,
Artem Bityutskiy (D?N?N?N?D 1/4  D?D,N?N?N?DoD,D1)

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Michal Nazarewicz]

On Wed, 20 Apr 2011 11:28:37 +0200, Artem Bityutskiy <dedekind1@gmail.com>  
wrote:
> I think it is good when small core functions like this are strict and
> use 'const' whenever possible, even though 'const' is so imperfect in C.
>
> Let me give an example from my own experience. I was writing code which
> was using the kernel RB trees, and I was trying to be strict and use
> 'const' whenever possible. But because the core functions like 'rb_next'
> do not have 'const' modifier, I could not use const in many many places
> of my code, because gcc was yelling. And I was not very enthusiastic to
> touch the RB-tree code that time.

The problem is that you end up with two sets of functions (one taking const
another taking non-const), a bunch of macros or a function that takes const
but returns non-const.  If we settle on anything I would probably vote for
the last option but the all are far from ideal.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Artem Bityutskiy]

On Wed, 2011-04-20 at 13:20 +0200, Michal Nazarewicz wrote:
> On Wed, 20 Apr 2011 11:28:37 +0200, Artem Bityutskiy <dedekind1@gmail.com>  
> wrote:
> > I think it is good when small core functions like this are strict and
> > use 'const' whenever possible, even though 'const' is so imperfect in C.
> >
> > Let me give an example from my own experience. I was writing code which
> > was using the kernel RB trees, and I was trying to be strict and use
> > 'const' whenever possible. But because the core functions like 'rb_next'
> > do not have 'const' modifier, I could not use const in many many places
> > of my code, because gcc was yelling. And I was not very enthusiastic to
> > touch the RB-tree code that time.
> 
> The problem is that you end up with two sets of functions (one taking const
> another taking non-const), a bunch of macros or a function that takes const
> but returns non-const.  If we settle on anything I would probably vote for
> the last option but the all are far from ideal.

I think it is fine to take const and return non-const. Yes, it is not
beautiful, but we could live with this.

-- 
Best Regards,
Artem Bityutskiy (D?N?N?N?D 1/4  D?D,N?N?N?DoD,D1)

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Michal Nazarewicz]

On Fri, 15 Apr 2011 17:59:16 +0200, Phil Carmody wrote:
> I'm just glad this wasn't an insta-nack, as I am quite a fan of
> consts, and hopefully something can be worked out.

I feel you man.  Unfortunately, I think that const, since it's an
after-thought, is not very usable in C.

For instance, as you've pointed in your patch, the "_ro" suffix
is sort of dumb, but without it compound_head would have to take
const and return non-const (like strchr() does) which is kinda
stupid as well.

What's more, because of lack of encapsulation, “const struct page”
only means that the object is const but thighs it points to aren't.
As such, const does not really play that well with structs anyway.

const is, in my opinion, one of those things C++ actually got
right (or close to right).

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michal "mina86" Nazarewicz    (o o)
ooo +-----<email/xmpp: mnazarewicz@google.com>-----ooO--(_)--Ooo--

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/1] mm: make read-only accessors take const pointer parameters

[Phil Carmody]

On 15/04/11 18:12 +0200, ext Michal Nazarewicz wrote:
> On Fri, 15 Apr 2011 17:59:16 +0200, Phil Carmody wrote:
>> I'm just glad this wasn't an insta-nack, as I am quite a fan of
>> consts, and hopefully something can be worked out.
>
> I feel you man.  Unfortunately, I think that const, since it's an
> after-thought, is not very usable in C.
>
> For instance, as you've pointed in your patch, the "_ro" suffix
> is sort of dumb, but without it compound_head would have to take
> const and return non-const (like strchr() does) which is kinda
> stupid as well.
>
> What's more, because of lack of encapsulation, a??const struct pagea??
> only means that the object is const but thighs it points to aren't.
> As such, const does not really play that well with structs anyway.

I'm very glad you've mentioned that point, I forgot to. I've taken
the view that in the absense of inside knowledge, const should be
inherited down all pointers. So not only will I not change you, but
I will not change anything you point to. No hidden side effects of
any kind. That reduces where it can be used, but is a much stronger
statement when it can be made.

> const is, in my opinion, one of those things C++ actually got
> right (or close to right).

I shouldn't be seen to agree with you on that, lest any fellow Nokians
notice that I've implied something positive about C++ after my rant on
our core chat channel a few days back. ;-)

Cheers,
Phil

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>