Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit db2c520b authored by Patrick Bellasi's avatar Patrick Bellasi
Browse files

ANDROID: sched/tune: fix boost_group spin_lock re-initialization 



In:

  c5616f2f   ANDROID: sched/tune: Initialize raw_spin_lock
                in boosted_groups

a spin_lock is initialized on each CPU every time a boost_group is
activated (i.e. a cgroup created).

However, those spin_lock are already initialized at boot time by
schedtune_init_cgroups() which also set schedtune_initialized to true
thus enabling the tasks accounting done by schedtune_{en,de}queue_task().
This means that an already initialize and used spin_lock is wrongly
re-initialized thus potentially leading to a:
   BUG: spinlock already unlocked on CPU
in the (unlikely in AOSP) case we have a race between schedtune cgroups
creation and tasks enqueue/dequeue.

This probably happened because the fix provided by c5616f2f was just
the wrong cure for a different issues: the missing one-time
initialization of the per-CPU spinlocks in schedtune_init_cgroups().
All these fixes happened on v4.4 and have been forward ported to the
current v4.9 base.

Let's better fix this by:
- removing the not necessary spinlock re-initialization in:
  schedtune_boostgroup_init()
- add a new "valid" flag to better flag which boost_groups are currently
  used to track a valid cgroup.

This patch adds also a better documentation of the used data structures
and the locking strategy in use to synchronize fast and slow paths.

Change-Id: I3c2a256693b12b317373bbc032ed46e620f79ee8
Signed-off-by: default avatarPatrick Bellasi <patrick.bellasi@arm.com>
Reported-by: default avatarStanley Shih <stanley.shih@mstarsemi.com>

[
The first of the two fixes listed above has been already
merged by:

   commit f6bec4e8 ("Revert "ANDROID: sched/tune:
                         Initialize raw_spin_lock in boosted_groups")

which, in conjunction with:

   commit 751e5093 ("ANDROID: sched: tune:
                         Fix lacking spinlock initialization")

provides the correct initialization of the boostgroups spinlocks.

Let's keep the changelog as it is to better keep track of the original
intended fix as well as to better document the required locking strategy.
]
Signed-off-by: default avatarPatrick Bellasi <patrick.bellasi@arm.com>
parent eafebcab

kernel/sched/tune.c

+72 −1
Original line number Diff line number Diff line
@@ -108,6 +108,64 @@ __schedtune_accept_deltas(int nrg_delta, int cap_delta, 


/*

 * EAS scheduler tunables for task groups.

 *

 * When CGroup support is enabled, we have to synchronize two different

 * paths:

 *  - slow path: where CGroups are created/updated/removed

 *  - fast path: where tasks in a CGroups are accounted

 *

 * The slow path tracks (a limited number of) CGroups and maps each on a

 * "boost_group" index. The fastpath accounts tasks currently RUNNABLE on each

 * "boost_group".

 *

 * Once a new CGroup is created, a boost group idx is assigned and the

 * corresponding "boost_group" marked as valid on each CPU.

 * Once a CGroup is release, the corresponding "boost_group" is marked as

 * invalid on each CPU. The CPU boost value (boost_max) is aggregated by

 * considering only valid boost_groups with a non null tasks counter.

 *

 * .:: Locking strategy

 *

 * The fast path uses a spin lock for each CPU boost_group which protects the

 * tasks counter.

 *

 * The "valid" and "boost" values of each CPU boost_group is instead

 * protected by the RCU lock provided by the CGroups callbacks. Thus, only the

 * slow path can access and modify the boost_group attribtues of each CPU.

 * The fast path will catch up the most updated values at the next scheduling

 * event (i.e. enqueue/dequeue).

 *

 *                                                        |

 *                                             SLOW PATH  |   FAST PATH

 *                              CGroup add/update/remove  |   Scheduler enqueue/dequeue events

 *                                                        |

 *                                                        |

 *                                                        |     DEFINE_PER_CPU(struct boost_groups)

 *                                                        |     +--------------+----+---+----+----+

 *                                                        |     |  idle        |    |   |    |    |

 *                                                        |     |  boost_max   |    |   |    |    |

 *                                                        |  +---->lock        |    |   |    |    |

 *  struct schedtune                  allocated_groups    |  |  |  group[    ] |    |   |    |    |

 *  +------------------------------+         +-------+    |  |  +--+---------+-+----+---+----+----+

 *  | idx                          |         |       |    |  |     |  valid  |

 *  | boots / prefer_idle          |         |       |    |  |     |  boost  |

 *  | perf_{boost/constraints}_idx | <---------+(*)  |    |  |     |  tasks  | <------------+

 *  | css                          |         +-------+    |  |     +---------+              |

 *  +-+----------------------------+         |       |    |  |     |         |              |

 *    ^                                      |       |    |  |     |         |              |

 *    |                                      +-------+    |  |     +---------+              |

 *    |                                      |       |    |  |     |         |              |

 *    |                                      |       |    |  |     |         |              |

 *    |                                      +-------+    |  |     +---------+              |

 *    | zmalloc                              |       |    |  |     |         |              |

 *    |                                      |       |    |  |     |         |              |

 *    |                                      +-------+    |  |     +---------+              |

 *    +                              BOOSTGROUPS_COUNT    |  |     BOOSTGROUPS_COUNT        |

 *  schedtune_boostgroup_init()                           |  +                              |

 *                                                        |  schedtune_{en,de}queue_task()  |

 *                                                        |                                 +

 *                                                        |          schedtune_tasks_update()

 *                                                        |

 */



/* SchdTune tunables for a group of tasks */
@@ -226,6 +284,8 @@ struct boost_groups { 
	/* Maximum boost value for all RUNNABLE tasks on a CPU */

	int boost_max;

	struct {

		/* True when this boost group maps an actual cgroup */

		bool valid;

		/* The boost for tasks on that boost group */

		int boost;

		/* Count of RUNNABLE tasks on that boost group */
@@ -250,6 +310,11 @@ schedtune_cpu_update(int cpu) 
	/* The root boost group is always active */

	boost_max = bg->group[0].boost;

	for (idx = 1; idx < BOOSTGROUPS_COUNT; ++idx) {



		/* Ignore non boostgroups not mapping a cgroup */

		if (!bg->group[idx].valid)

			continue;



		/*

		 * A boost group affects a CPU only if it has

		 * RUNNABLE tasks on that CPU
@@ -259,6 +324,7 @@ schedtune_cpu_update(int cpu) 


		boost_max = max(boost_max, bg->group[idx].boost);

	}



	/* Ensures boost_max is non-negative when all cgroup boost values

	 * are neagtive. Avoids under-accounting of cpu capacity which may cause

	 * task stacking and frequency spikes.*/
@@ -278,6 +344,9 @@ schedtune_boostgroup_update(int idx, int boost) 
	for_each_possible_cpu(cpu) {

		bg = &per_cpu(cpu_boost_groups, cpu);



		/* CGroups are never associated to non active cgroups */

		BUG_ON(!bg->group[idx].valid);



		/*

		 * Keep track of current boost values to compute the per CPU

		 * maximum only when it has been affected by the new value of
@@ -642,7 +711,7 @@ schedtune_boostgroup_init(struct schedtune *st, int idx) 
	for_each_possible_cpu(cpu) {

		bg = &per_cpu(cpu_boost_groups, cpu);

		bg->group[idx].boost = 0;

		bg->group[idx].tasks = 0;

		bg->group[idx].valid = true;

	}



	/* Keep track of allocated boost groups */
@@ -697,6 +766,7 @@ schedtune_boostgroup_release(struct schedtune *st) 
	/* Reset per CPUs boost group support */

	for_each_possible_cpu(cpu) {

		bg = &per_cpu(cpu_boost_groups, cpu);

		bg->group[st->idx].valid = false;

		bg->group[st->idx].boost = 0;

	}
@@ -733,6 +803,7 @@ schedtune_init_cgroups(void) 
	for_each_possible_cpu(cpu) {

		bg = &per_cpu(cpu_boost_groups, cpu);

		memset(bg, 0, sizeof(struct boost_groups));

		bg->group[0].valid = true;

		raw_spin_lock_init(&bg->lock);

	}