Messages in this thread | | | Date | Sat, 24 Sep 2016 00:48:34 +0100 | From | Mark Rutland <> | Subject | Re: [PATCH] thread_info: use unsigned long for flags |
| |
[Adding Ingo, so this doesn't get lost -- please see the end of the mail]
On Fri, Sep 23, 2016 at 11:08:46AM -0700, Andy Lutomirski wrote: > On Fri, Sep 23, 2016 at 10:24 AM, Mark Rutland <mark.rutland@arm.com> wrote: > > The generic THREAD_INFO_IN_TASK definition of thread_info::flags is a > > u32, matching x86 prior to the introduction of THREAD_INFO_IN_TASK. > > > > However, common helpers like test_ti_thread_flag() implicitly assume > > that thread_info::flags has at least the size and alignment of unsigned > > long, and relying on padding and alignment provided by other elements of > > task_struct is somewhat fragile. Additionally, some architectures use > > more that 32 bits for thread_info::flags, and others may need to in > > future. > > > > With THREAD_INFO_IN_TASK, task struct follows thread_info with a long > > field, and thus we no longer save any space as we did back in commit > > affa219b60a11b32 ("x86: change thread_info's flag field back to 32 > > bits"). > > > > Given all this, it makes more sense for the generic thread_info::flags > > to be an unsigned long. Make it so. > > I have only one problem with this, and it's a general objection that's > mostly off topic: why the [expletive] do the arch-independent bitfield > helpers think in units of variable size? It's *absurd*, especially on > big-endian architectures. > > Now that that's out of my system, I think this patch is fine. > Big-endian arches that opt in will have to deal with it somehow, but I > don't see why making it 'unsigned long' is worse than anything else. > x86 is fine with this change.
FWIW, given <linux/thread_info.h> contains/uses the helpers mentioned above, BE arches *must* use unsigned long (or something of the same size) today, or they wouldn't work. In v4.8-rc7 that is the case:
$ ls -l arch | wc -l 33
# Note the above includes Kconfig, so there are 32 to consider
$ git grep -W 'struct thread_info {' -- arch/*/include | \ grep flags | grep 'unsigned long\s\+flags' | wc -l 29
$ git grep -W 'struct thread_info {' -- arch/*/include | \ grep flags | grep 'int\s\+flags' arch/alpha/include/asm/thread_info.h- unsigned int flags; /* low level flags */
$ git grep -W 'struct thread_info {' -- arch/*/include | \ grep flags | grep 'u32\s\+flags' arch/ia64/include/asm/thread_info.h- __u32 flags; /* thread_info flags (see TIF_*) */ arch/x86/include/asm/thread_info.h- __u32 flags; /* low level flags */
> Acked-by: Andy Lutomirski <luto@kernel.org> > > Ingo, can you apply this for 4.9 so that we can make this change > before other arches might start depending on the field being u32? > > --Andy
Thanks, Mark.
| |