Re: [RFC PATCH 4/8] mm: Separate fault info out of 'struct vm_fault'

On Thu, Jan 14, 2021 at 10:01 AM Will Deacon [off-list ref] wrote:

Try to clean this up by splitting the immutable fault information out
into a new 'struct vm_fault_info' which is embedded in 'struct vm_fault'
and will later be made 'const'. The vast majority of this change was
performed with a coccinelle patch:

You may have a reason for doing it this way, but my reaction to this
was: "just make the new embedded struct unnamed".

Then you wouldn't need to do all the automated coccinelle changes.

Is there some reason you didn't do that, or just a "oh, I didn't think of it".

                Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Jan 14, 2021 at 10:16:13AM -0800, Linus Torvalds wrote:

On Thu, Jan 14, 2021 at 10:01 AM Will Deacon [off-list ref] wrote:

quoted

Try to clean this up by splitting the immutable fault information out
into a new 'struct vm_fault_info' which is embedded in 'struct vm_fault'
and will later be made 'const'. The vast majority of this change was
performed with a coccinelle patch:

You may have a reason for doing it this way, but my reaction to this
was: "just make the new embedded struct unnamed".

Then you wouldn't need to do all the automated coccinelle changes.

Is there some reason you didn't do that, or just a "oh, I didn't think of
it".

I tried that initially, e.g.

struct vm_fault {
	const struct {
		unsigned long address;
		...
	};
};

but I found that I had to make all of the members const to get it to work,
at which point the anonymous struct wasn't really adding anything. Did I
just botch the syntax?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Jan 14, 2021 at 11:00 AM Will Deacon [off-list ref] wrote:

I tried that initially, but I found that I had to make all of the
members const to get it to work, at which point the anonymous struct
wasn't really adding anything. Did I just botch the syntax?

I'm not sure what you tried. But this stupid test-case sure works for me:

    struct hello {
        const struct {
                unsigned long address;
        };
        unsigned int flags;
    };

    extern int fn(struct hello *);

    int test(void)
    {
        struct hello a = {
                .address = 1,
        };
        a.flags = 0;
        return fn(&a);
    }

and because "address" is in that unnamed constant struct, you can only
set it within that initializer, and cannot do

        a.address = 0;

without an error (the way you _can_ do "a.flags = 0").

I don't see naming the struct making a difference - apart from forcing
that big rename patch, of course.

But maybe we're talking about different issues?

            Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Jan 14, 2021 at 11:09:01AM -0800, Linus Torvalds wrote:

On Thu, Jan 14, 2021 at 11:00 AM Will Deacon [off-list ref] wrote:

quoted

I tried that initially, but I found that I had to make all of the
members const to get it to work, at which point the anonymous struct
wasn't really adding anything. Did I just botch the syntax?

I'm not sure what you tried. But this stupid test-case sure works for me:

    struct hello {
        const struct {
                unsigned long address;
        };
        unsigned int flags;
    };

    extern int fn(struct hello *);

    int test(void)
    {
        struct hello a = {
                .address = 1,
        };
        a.flags = 0;
        return fn(&a);
    }

and because "address" is in that unnamed constant struct, you can only
set it within that initializer, and cannot do

        a.address = 0;

without an error (the way you _can_ do "a.flags = 0").

I don't see naming the struct making a difference - apart from forcing
that big rename patch, of course.

But maybe we're talking about different issues?

Urgh...

We _are_ both on the same page, and your reply above had me thinking I've
lost the plot, so I went back to the start. Check out v5.11-rc3 and apply
this patch:

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ecdf8a8cd6ae..1eb950865450 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h

@@ -514,11 +514,14 @@ static inline bool fault_flag_allow_retry_first(unsigned int flags)
  * pgoff should be used in favour of virtual_address, if possible.
  */
 struct vm_fault {
-       struct vm_area_struct *vma;     /* Target VMA */
+       const struct {
+               struct vm_area_struct *vma;     /* Target VMA */
+               gfp_t gfp_mask;                 /* gfp mask to be used for allocations */
+               pgoff_t pgoff;                  /* Logical page offset based on vma */
+               unsigned long address;          /* Faulting virtual address */
+       };
+
        unsigned int flags;             /* FAULT_FLAG_xxx flags */
-       gfp_t gfp_mask;                 /* gfp mask to be used for allocations */
-       pgoff_t pgoff;                  /* Logical page offset based on vma */
-       unsigned long address;          /* Faulting virtual address */
        pmd_t *pmd;                     /* Pointer to pmd entry matching
                                         * the 'address' */
        pud_t *pud;                     /* Pointer to pud entry matching

Sure enough, an arm64 defconfig builds perfectly alright with that change,
but it really shouldn't. I'm using clang 11.0.5, so I had another go with
GCC 9.2.1 and bang:

mm/filemap.c: In function ‘filemap_map_pages’:
mm/filemap.c:2963:16: error: assignment of member ‘address’ in read-only object
 2963 |   vmf->address += (xas.xa_index - last_pgoff) << PAGE_SHIFT;
      |                ^~
make[1]: *** [scripts/Makefile.build:279: mm/filemap.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1805: mm] Error 2
make: *** Waiting for unfinished jobs....

Nick -- any clue what's happening here? We would like that const anonymous
struct to behave like a const struct member, as the alternative (naming the
thing) results in a lot of refactoring churn.

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Jan 14, 2021 at 11:41 AM Will Deacon [off-list ref] wrote:

Weird, looks like a bug to me in Clang, filed
https://bugs.llvm.org/show_bug.cgi?id=48755.
-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Jan 14, 2021 at 11:41 AM Will Deacon [off-list ref] wrote:

Sure enough, an arm64 defconfig builds perfectly alright with that change,
but it really shouldn't. I'm using clang 11.0.5, so I had another go with
GCC 9.2.1 and bang:

Ok, looks like a clang bug, but a reasonably benign one.

As long as we have sufficient coverage with gcc, we'll get error
reporting in a timely manner for any new incorrect assignments, so I
think we can do that constant anonymous struct even if it does mean
that clang might let some bad cases through (I personally use gcc for
build testing, and then clang for building my boot kernels, so I'd
catch anything x86-64 allmodconfig in my build tests).

And keeping it unnamed it would avoid a lot of noisy churn..

             Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Thu, Jan 14, 2021 at 01:11:12PM -0800, Linus Torvalds wrote:

On Thu, Jan 14, 2021 at 11:41 AM Will Deacon [off-list ref] wrote:

quoted

Sure enough, an arm64 defconfig builds perfectly alright with that change,
but it really shouldn't. I'm using clang 11.0.5, so I had another go with
GCC 9.2.1 and bang:

Ok, looks like a clang bug, but a reasonably benign one.

As long as we have sufficient coverage with gcc, we'll get error
reporting in a timely manner for any new incorrect assignments, so I
think we can do that constant anonymous struct even if it does mean
that clang might let some bad cases through (I personally use gcc for
build testing, and then clang for building my boot kernels, so I'd
catch anything x86-64 allmodconfig in my build tests).

And keeping it unnamed it would avoid a lot of noisy churn..

Hmm. The feedback on the clang bug suggests that GCC is the one in the
wrong here (although the argument is based on C11 and I haven't trawled
through the standards to see how this has evolved):

https://bugs.llvm.org/show_bug.cgi?id=48755#c1

There is at least some sympathy to generating a warning, so that might
be good enough. Otherwise, I suppose we can explicitly mark the fields
as 'const' but I won't jump to that immediately.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Jan 15, 2021 at 1:23 AM Will Deacon [off-list ref] wrote:

Hmm. The feedback on the clang bug suggests that GCC is the one in the
wrong here (although the argument is based on C11 and I haven't trawled
through the standards to see how this has evolved):

Oh well.

That writing is absolutely the _worst_ kind of weaselwording standards
language reading, trying to make excuses for bad behavior by basically
depending on "this language is unclear", and trying to say that the
buggy behavior is required by C11.

What a disappointment.

Absolutely nothing in the quoted C11 language says to me what that bug
entry claims it says.

The argument seems to hinge on

   "The members of an anonymous structure or union are considered to
be members of the containing structure or union"

and then it makes the completely uncalled-for leap that that means
that because it was "int" in the const struct, it must be "int" in the
containing structure too.

Which is complete BS, and doesn't follow logically _or_ grammatically.
It would be a "member of the containing structure" even with the
"const" qualifier, so the argument they make is just inane.

In fact, the _other_ sentence they quote clearly points to this being
just a clang bug:

 "A modifiable lvalue is [...] if it is a structure or union, does not
have any member (including, recursively, any member or element of all
contained aggregates or unions) with a const- qualified type"

and clearly this recursively is an element with a const-qualified
recursive struct.

Whatever. It's one of those "read the documentation squint-eyed to
avoid doing the right thing" arguments.

It's not worth arguing with people like that, and let's just ignore
the clang bug here.

            Linus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Fri, Jan 15, 2021 at 1:33 PM Linus Torvalds
[off-list ref] wrote:

On Fri, Jan 15, 2021 at 1:23 AM Will Deacon [off-list ref] wrote:

quoted

Hmm. The feedback on the clang bug suggests that GCC is the one in the
wrong here (although the argument is based on C11 and I haven't trawled
through the standards to see how this has evolved):

Oh well.

That writing is absolutely the _worst_ kind of weaselwording standards
language reading, trying to make excuses for bad behavior by basically
depending on "this language is unclear", and trying to say that the
buggy behavior is required by C11.

What a disappointment.

I don't really understand British humor either, but I assume that's
how the language lawyers throw shade on one anothers' standards.
Richard is both the WG21 spec editor (C++) and British, IIRC.
Apparently, there's a long conversion (behind closed doors; it's the
ISO way) going on in regards to the thread Richard has kicked off with
them (WG14; C).  Moreso on what should happen with the _Atomic
qualifier, assignments, and memcpy.  So it is still an important thing
to nail down the language spec.

Note there were also a lot of discussions lately on "where should the
volatile qualifier be allowed, or not."
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1152r0.html
https://www.youtube.com/watch?v=KJW_DLaVXIY
(2018? ok, maybe not lately.  Lately for C)

I view this similarly as "where should the const qualifier be allowed, or not."
-- 
Thanks,
~Nick Desaulniers

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel