On Tue, Jan 15, 2019 at 10:15:04AM +0000, Patrick Bellasi wrote:

I am confuzled by this thing..  so uclamp_se already has a value,bucket,
which per the prior code is the effective one.

Now; I think I see why you want another value; you need the second to
store the original value for when the system limits change and we must
re-evaluate.

So why are you not adding something like:

	unsigned int orig_value : bits_per(SCHED_CAPACITY_SCALE);

+unsigned int sysctl_sched_uclamp_util_min;

+unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;

+static inline void
+uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
+		     unsigned int *clamp_value, unsigned int *bucket_id)
+{
+	/* Task specific clamp value */
+	*clamp_value = p->uclamp[clamp_id].value;
+	*bucket_id = p->uclamp[clamp_id].bucket_id;
+
+	/* System default restriction */
+	if (unlikely(*clamp_value < uclamp_default[UCLAMP_MIN].value ||
+		     *clamp_value > uclamp_default[UCLAMP_MAX].value)) {
+		/* Keep it simple: unconditionally enforce system defaults */
+		*clamp_value = uclamp_default[clamp_id].value;
+		*bucket_id = uclamp_default[clamp_id].bucket_id;
+	}
+}

That would then turn into something like:

	unsigned int high = READ_ONCE(sysctl_sched_uclamp_util_max);
	unsigned int low  = READ_ONCE(sysctl_sched_uclamp_util_min);

	uclamp_se->orig_value = value;
	uclamp_se->value = clamp(value, low, high);

And then determine bucket_id based on value.

+int sched_uclamp_handler(struct ctl_table *table, int write,
+			 void __user *buffer, size_t *lenp,
+			 loff_t *ppos)
+{
+	int old_min, old_max;
+	int result = 0;
+
+	mutex_lock(&uclamp_mutex);
+
+	old_min = sysctl_sched_uclamp_util_min;
+	old_max = sysctl_sched_uclamp_util_max;
+
+	result = proc_dointvec(table, write, buffer, lenp, ppos);
+	if (result)
+		goto undo;
+	if (!write)
+		goto done;
+
+	if (sysctl_sched_uclamp_util_min > sysctl_sched_uclamp_util_max ||
+	    sysctl_sched_uclamp_util_max > SCHED_CAPACITY_SCALE) {
+		result = -EINVAL;
+		goto undo;
+	}
+
+	if (old_min != sysctl_sched_uclamp_util_min) {
+		uclamp_bucket_inc(NULL, &uclamp_default[UCLAMP_MIN],
+				  UCLAMP_MIN, sysctl_sched_uclamp_util_min);
+	}
+	if (old_max != sysctl_sched_uclamp_util_max) {
+		uclamp_bucket_inc(NULL, &uclamp_default[UCLAMP_MAX],
+				  UCLAMP_MAX, sysctl_sched_uclamp_util_max);
+	}

Should you not update all tasks?