Re: [PATCH 1/5] mm: Constify a lot of struct page arguments

[John Hubbard <jhubbard@nvidia.com>]

On 4/9/20 10:12 AM, Jason Gunthorpe wrote:
> On Thu, Apr 09, 2020 at 05:47:44PM +0300, Kirill A. Shutemov wrote:
>> On Thu, Apr 09, 2020 at 07:32:38AM -0700, Matthew Wilcox wrote:
>>> On Thu, Apr 09, 2020 at 05:28:51PM +0300, Kirill A. Shutemov wrote:
>>>>> We have a few places that do things like
>>>>>
>>>>> mm/filemap.c:           if (unlikely(compound_head(page)->mapping != mapping)) {
>>>>
>>>> Good point.
>>>>
>>>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>>>
>>> Darn, I hoped you'd have a better idea.  I feel quite ashamed of this patch.
>>
>> I had two ideas. Both awful.
>>
>>   - Rename compound_head() to __compound_head() or something and make it
>>     return void *. Then wrap it into a macro that would cast return type to
>>     type of the argument. It would allow to regain *some* type safety.
>>
>>   - Provide two implementations and use C11 _Generic() :P
>>     (bump GCC version requirements first)
> 
> I saw people using static_assert stuff in the kernel, maybe C11 is
> feasible now? It is 9 years old..


ohhh, if this patch is a reliable indicator of what const correctness looks
like in the kernel, then we're going to end up with a lot movement in that
direction, too. With that in mind, let's provide some counter-force, by:

1) Poking around at the C11 idea, just in case It Is Time. Because clearly
the C language dialect that we're using now is not *quite* up to doing const
correctness, so moving the language ahead is likely to give us the cleanest
solution. Better than writing our own language in macros, which yes, I
realize is a way to compensate. But, uggghhh. :)

2) Perhaps providing two routines with different names might be the answer
in this case? This avoids macros, still provides a const cast (which is
breaks the chain of const correctness, but on the other hand, we still get
most of the const goodness--the rest comes when the language supports it):

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 222f6f7b2bb3..71e2d07ddcf4 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -186,6 +186,11 @@ static inline struct page *compound_head(struct page *page)
         return page;
  }
  
+static inline const struct page *compound_head_const(struct page *page)
+{
+       return (const struct page*)compound_head(page);
+}
+
  static __always_inline int PageTail(struct page *page)
  {
         return READ_ONCE(page->compound_head) & 1;
diff --git a/mm/debug.c b/mm/debug.c
index 2189357f0987..9a1f37a80ed7 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -44,7 +44,7 @@ const struct trace_print_flags vmaflag_names[] = {
  
  void __dump_page(struct page *page, const char *reason)
  {
-       struct page *head = compound_head(page);
+       const struct page *head = compound_head_const(page);
         struct address_space *mapping;
         bool page_poisoned = PagePoisoned(page);
         bool compound = PageCompound(page);

...etc

thanks,
-- 
John Hubbard
NVIDIA