Re: [PATCH 0/3] nohz: Convert tick dependency mask to atomic_t

[Ingo Molnar <mingo@kernel.org>]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Tue, Mar 29, 2016 at 4:44 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > Harmonizing thread_info::flags does not look easy, given how much assembly code
> > accesses this field.
> 
> It might not be too bad.
> 
> For 32-bit architectures (which is still most of them), it's just a
> 
>    unsigned int/long -> atomic_t
> 
> and for 64-bit architectures you end up with three choices:
> 
>  - it's already 32-bit (alpha, ia64, x86):
> 
>         unsigned int -> atomic_t
> 
>  - little-endian long:
> 
>         atomic_t flags
>         unsigned int padding;
> 
>  - big-endian long (only powerpc? Maybe there's a big-endian MIPS still?)
> 
>         unsigned int padding;
>         atomic_t flags;

Hm, that indeed sounds fairly nice and doable - I thought some architectures do 
have a task flag above bit 31, but that does not appear to be so ...

Right now we seem to have 27 bits defined in include/linux/sched.h, with 5 more 
bits left for the future. Here's their current usage histogram in the kernel 
source:

  PF_KTHREAD                    : 68
  PF_MEMALLOC                   : 65
  PF_EXITING                    : 49
  PF_RANDOMIZE                  : 20
  PF_VCPU                       : 18
  PF_FREEZER_SKIP               : 15
  PF_SUPERPRIV                  : 14
  PF_FSTRANS                    : 14
  PF_NOFREEZE                   : 13
  PF_WQ_WORKER                  : 12
  PF_SWAPWRITE                  : 12
  PF_MEMALLOC_NOIO              : 11
  PF_FROZEN                     : 11
  PF_NO_SETAFFINITY             : 9
  PF_LESS_THROTTLE              : 8
  PF_USED_MATH                  : 7
  PF_SUSPEND_TASK               : 7
  PF_KSWAPD                     : 7
  PF_FORKNOEXEC                 : 7
  PF_NPROC_EXCEEDED             : 6
  PF_MCE_PROCESS                : 6
  PF_MCE_EARLY                  : 6
  PF_USED_ASYNC                 : 5
  PF_SIGNALED                   : 5
  PF_EXITPIDONE                 : 5
  PF_DUMPCORE                   : 5
  PF_MUTEX_TESTER               : 1

1)

PF_MUTEX_TESTER could be gotten rid of straight away as it appears to be unused.

2)

I'd also rename the lot while touching every usage site: the PF_ 'process flag' 
namespace currently collides with:

 - the PF_ 'page flag' namespace

 - the PF_ 'protocol family' constants in the networking code

... all of which makes grepping and code reading a bit harder than it should be, 
IMHO.

Calling them 'process' flags is a misnomer anyway, these are fundamentally per 
task flags.

All in one, having them named TF_ would work for me. TF_ is a mostly unused 
namespace in generic code right now, and it would rhyme well with the existing 
TIF_ (thread_info flag) namespace.

( I guess ATF_ for 'atomic task flag' would work as well, except that the acronym
  sounds too much like a well-known government agency. Plus I guess the ASS
  acronym principle applies as well. )

3)

We could also rename the flag itself to __flags, for the following five purposes:

 - to make sure there's no lingering unconverted usage, especially in assembly
   code that tends to drop types and go by names only.

 - and to push people towards using accessors (task_flag(), set_task_flag(), 
   etc.), not the raw field.

 - accessor conversion could precede the type conversion. I.e. the new accessors
   could work on the old type as well.

 - accessors would also make it easier to extend the type to atomic64_t in the
   future, should we ever run out of 32 task flags.

 - accessors would make it easier to do per arch conversion as well.

So this:

	if (current->flags & PF_KTHREAD)

would look like this:

	if (atomic_read(&current->__task_flags) & TF_KTHREAD)

Or rather, we'd use obviously named accessors:

	if (task_flag(current, TF_KTHREAD))

plus:

	set_task_flag(current, TF_KTHREAD);

et al.

How does this sound?

Thanks,

	Ingo