Re: [PATCH v3 6/9] sched: Kill select_task_rq()'s sd_flag parameter

[Valentin Schneider <valentin.schneider@arm.com>]

On 16/04/20 11:47, Peter Zijlstra wrote:
> On Thu, Apr 16, 2020 at 09:42:36AM +0200, Vincent Guittot wrote:
>> On Wed, 15 Apr 2020 at 23:05, Valentin Schneider
>> > @@ -6622,13 +6622,25 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>> >   * preempt must be disabled.
>> >   */
>> >  static int
>> > +select_task_rq_fair(struct task_struct *p, int prev_cpu, int wake_flags)
>> >  {
>> > +       int sync = (wake_flags & WF_SYNC) && !(current->flags & PF_EXITING);
>> >         struct sched_domain *tmp, *sd = NULL;
>> >         int cpu = smp_processor_id();
>> >         int new_cpu = prev_cpu;
>> >         int want_affine = 0;
>> > -       int sync = (wake_flags & WF_SYNC) && !(current->flags & PF_EXITING);
>> > +       int sd_flag;
>> > +
>> > +       switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
>>
>> You remove a function parameter, which was directly set with the right
>> flag, but then you add a switch case to recreate this sd_flag
>> internally. Not sure we can say that it's real benefit
>>
>> > +       case WF_TTWU:
>> > +               sd_flag = SD_BALANCE_WAKE;
>> > +               break;
>> > +       case WF_FORK:
>> > +               sd_flag = SD_BALANCE_FORK;
>> > +               break;
>> > +       default:
>> > +               sd_flag = SD_BALANCE_EXEC;
>> > +       }
>
> Agreed, that's a bit yuck, how about something like so instead:
>

Aligning the SD_* and WF_* flags sure makes it simpler, I just wasn't
daring enough to do that. I suppose we'll want some BUILD_BUG_ON() checks
just for good measure.

Also, it doesn't directly solve the switch case of 9/9, but I may get out
of it with some hackery such as what I suggested in my reply to Vincent.

>
> --- a/include/linux/sched/topology.h
> +++ b/include/linux/sched/topology.h
> @@ -11,10 +11,12 @@
>   */
>  #ifdef CONFIG_SMP
>
> +/* First nibble of SD_flag is shared with WF_flag */
>  #define SD_BALANCE_NEWIDLE	0x0001	/* Balance when about to become idle */
>  #define SD_BALANCE_EXEC		0x0002	/* Balance on exec */
>  #define SD_BALANCE_FORK		0x0004	/* Balance on fork, clone */
>  #define SD_BALANCE_WAKE		0x0008  /* Balance on wakeup */
> +
>  #define SD_WAKE_AFFINE		0x0010	/* Wake task to waking CPU */
>  #define SD_ASYM_CPUCAPACITY	0x0020  /* Domain members have different CPU capacities */
>  #define SD_SHARE_CPUCAPACITY	0x0040	/* Domain members share CPU capacity */
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6635,16 +6635,8 @@ select_task_rq_fair(struct task_struct *
>       int want_affine = 0;
>       int sd_flag;
>
> -	switch (wake_flags & (WF_TTWU | WF_FORK | WF_EXEC)) {
> -	case WF_TTWU:
> -		sd_flag = SD_BALANCE_WAKE;
> -		break;
> -	case WF_FORK:
> -		sd_flag = SD_BALANCE_FORK;
> -		break;
> -	default:
> -		sd_flag = SD_BALANCE_EXEC;
> -	}
> +	/* SD_flags and WF_flags share the first nibble */
> +	sd_flag = wake_flags & 0xf;
>
>       if (sd_flag & SD_BALANCE_WAKE) {
>               record_wakee(p);
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -1685,11 +1685,12 @@ static inline int task_on_rq_migrating(s
>  /*
>   * Wake flags
>   */
> -#define WF_SYNC			0x01		/* Waker goes to sleep after wakeup */
> -#define WF_TTWU                 0x02            /* Regular task wakeup */
> -#define WF_FORK			0x04		/* Child wakeup after fork */
> -#define WF_EXEC			0x08		/* "Fake" wakeup at exec */
> -#define WF_MIGRATED		0x10		/* Internal use, task got migrated */
> +#define WF_EXEC			0x02	/* SD_BALANCE_EXEC */
> +#define WF_FORK			0x04	/* SD_BALANCE_FORK */
> +#define WF_TTWU			0x08	/* SD_BALANCE_WAKE */
> +
> +#define WF_SYNC			0x10	/* Waker goes to sleep after wakeup */
> +#define WF_MIGRATED		0x20	/* Internal use, task got migrated */
>
>  /*
>   * To aid in avoiding the subversion of "niceness" due to uneven distribution