lkml.org 
[lkml]   [2015]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/7] x86/intel_rdt: Add support for cache bit mask management


On Fri, 15 May 2015, Thomas Gleixner wrote:

> On Mon, 11 May 2015, Vikas Shivappa wrote:
>> @@ -35,6 +36,42 @@ static struct rdt_subsys_info rdtss_info;
>> static DEFINE_MUTEX(rdt_group_mutex);
>> struct intel_rdt rdt_root_group;
>>
>> +/*
>> + * Mask of CPUs for writing CBM values. We only need one per-socket.
>
> One mask? One CPU? One what? Please add comments which are
> understandable and useful for people who did NOT write that code.

when there is already code in the upstream code which has comments exactly
consistent - when that made sense , this should ?

/*
* Mask of CPUs for reading CQM values. We only need one per-socket.
*/
static cpumask_t cqm_cpumask;

>
>> + */
>> +static cpumask_t rdt_cpumask;
>
>> +static void __cpu_cbm_update(void *info)
>> +{
>> + unsigned int closid = *((unsigned int *)info);
>
> So all you want is to transfer closid, right? Why having a pointer
> instead of just doing
>
> on_each_cpu_mask(...., (void *) id);
>
> and
> id = (unsigned long) id;

will fix.

>
> Looks too simple and readable, right?
>
>> +/*
>> + * cbm_update_msrs() - Updates all the existing IA32_L3_MASK_n MSRs
>> + * which are one per CLOSid, except IA32_L3_MASK_0 on the current package.
>> + * @cpu : the cpu on which the mask is updated.
>> + */
>> +static inline void cbm_update_msrs(int cpu)
>> +{
>> + int maxid = boot_cpu_data.x86_rdt_max_closid;
>> + unsigned int i;
>> +
>> + if (WARN_ON(cpu != smp_processor_id()))
>> + return;
>> +
>> + for (i = 1; i < maxid; i++) {
>
> Lacks a comment why this starts with 1 and not with 0 as one would expect.

Its in the function comment just above - "except IA32_L3_MASK_0 on the current
package."

>
>> + if (ccmap[i].clos_refcnt)
>> + __cpu_cbm_update(&i);
>> + }
>> +}
>
> This is only used by the cpu hotplug code. So why does it not live
> close to it?

ok will fix

Because its more fun to search for functions than having
> them just in place?
>
>> +/*
>> + * intel_cache_alloc_cbm_write() - Validates and writes the
>> + * cache bit mask(cbm) to the IA32_L3_MASK_n
>> + * and also store the same in the ccmap.
>> + *
>> + * CLOSids are reused for cgroups which have same bitmask.
>> + * - This helps to use the scant CLOSids optimally.
>> + * - This also implies that at context switch write
>> + * to PQR-MSR is done only when a task with a
>> + * different bitmask is scheduled in.
>> + */
>> +static int intel_cache_alloc_cbm_write(struct cgroup_subsys_state *css,
>
> Can you please drop these intel_ prefixes? They provide no value and
> just eat space.

well , i got ample comments which wanted me to me to specify intel as this
feature is specific to intel.
the cache monitoring code has similar prefixes as well - is that pref
specific then ? I just dont want people to come back and ask to make it clear
that things are intel specific

>
>> + struct cftype *cft, u64 cbmvalue)
>> +{
>> + u32 max_cbm = boot_cpu_data.x86_rdt_max_cbm_len;
>> + struct intel_rdt *ir = css_rdt(css);
>> + unsigned long cache_mask, max_mask;
>> + unsigned long *cbm_tmp;
>> + unsigned int closid;
>> + ssize_t err = 0;
>> +
>> + if (ir == &rdt_root_group)
>> + return -EPERM;
>> + bitmap_set(&max_mask, 0, max_cbm);
>> +
>> + /*
>> + * Need global mutex as cbm write may allocate a closid.
>> + */
>> + mutex_lock(&rdt_group_mutex);
>> + bitmap_and(&cache_mask, (unsigned long *)&cbmvalue, &max_mask, max_cbm);
>> + cbm_tmp = &ccmap[ir->clos].cache_mask;
>> +
>> + if (bitmap_equal(&cache_mask, cbm_tmp, MAX_CBM_LENGTH))
>> + goto out;
>> +
>
> Sigh, what's the point of this bitmap shuffling? Nothing as far as I
> can tell, because you first set all bits of maxmask and then AND
> cbmvalue.

Again , this contraditcs your earlier comment where you asked me to use the bit
manipulation APIs to make it more *readable*. So do you want more readable now
or is that not clear now ?

https://lkml.org/lkml/2014/11/19/1145

+ /* Reset the least significant bit.*/
+ i = var & (var - 1);
+
+ if (i != (var & (var << 1)))
+ return false;

was replaced with the

if (bitmap_weight(&var, maxcbm) < min_bitmask_len)
return false;

first_bit = find_next_bit(&var, maxcbm, 0);
zero_bit = find_next_zero_bit(&var, maxcbm, first_bit);

if (find_next_bit(&var, maxcbm, zero_bit) < maxcbm)
return false;

3 lines replaced with more to make it more readable ! so does this come back
then ?

>
> All you achieve is, that you truncate cbmvalue to max_cbm bits. So if
> cbmvalue has any of the invalid bits set, you happily ignore that.

taskset ignores the extra bits. is that a problem here ?

>
> This whole bitmap usage is just making the code obfuscated.
>
> u64 max_mask = (1ULL << max_cbm) - 1;
>
> if (cbm_value & ~max_mask)
> return -EINVAL;
>
> if (cbm_value == (u64)ccmap[ir->clos].cache_mask)
> return 0;
>
> No bitmap_set, bitmap_and nothing. Simple and readable, right?
>
> The bitmap would only be necessary, IF max_cbm is > 64. But that's not
> the case and nothing in your code would survive ma_ cbm > 64. And as I
> said before CBM_LEN is 32bit for now and I serioulsy doubt, that it
> can ever go over 64bit simply because that would not fit into the mask
> MSRs.
>
> So can you please get rid of that bitmap nonsense including the
> dynamic allocation and use simple and readable code constructs?
>
> Just for the record:
>
> struct bla {
> u64 cbm_mask;
> };
>
> versus:
>
> struct bla {
> unsigned long *cbm_mask;
> };
>
> plus
>
> bla.cbm_mask = kzalloc(sizeof(u64));
>
> Is just utter waste of data and text space.
>
>> + /*
>> + * At this point we are sure to change the cache_mask.Hence release the
>> + * reference to the current CLOSid and try to get a reference for
>> + * a different CLOSid.
>> + */
>> + __clos_put(ir->clos);
>
> So you drop the reference, before you make sure that you can get a new one?

I was horrible , will add fix. thanks for pointing out .

>
>> + if (cbm_search(cache_mask, &closid)) {
>> + ir->clos = closid;
>> + __clos_get(closid);
>> + } else {
>> + err = intel_rdt_alloc_closid(ir);
>> + if (err)
>> + goto out;
>
> What happens if intel_rdt_alloc_closid() fails? ir->clos is unchanged,
> but you have no reference anymore. The next call to this function or
> anything else which calls __clos_put() will trigger the underflow
> warning and the warning will be completely useless because the
> wreckage happened before that.
>
>> +static inline bool intel_rdt_update_cpumask(int cpu)
>> +{
>> + int phys_id = topology_physical_package_id(cpu);
>> + struct cpumask *mask = &rdt_cpumask;
>> + int i;
>> +
>> + for_each_cpu(i, mask) {
>> + if (phys_id == topology_physical_package_id(i))
>> + return false;
>> + }
>
> You must be kidding.

the rapl and cqm use similar code. You want me to keep a seperate
package mask for this code which not would be that frequent at all ?

for c-state transitions only idle_notifier gets called - so this is only when a
new package is physically added. and we dont need anything for cache alloc for
idle transitions. not really frequent ?

You know how many packages are possible. What's
> wrong with having a bitmap of physical packages which have a CPU
> handling it?
>
> id = topology_physical_package_id(cpu);
> if (!__test_and_set_bit(id, &package_map)) {
> cpumask_set_cpu(cpu, &rdt_cpumask);
> cbm_update_msrs(cpu);
> }
>
> Four lines of code without loops and hoops. And here is a bitmap the
> proper data structure.
>
>> +static void intel_rdt_cpu_exit(unsigned int cpu)
>> +{
>> + int phys_id = topology_physical_package_id(cpu);
>> + int i;
>> +
>> + mutex_lock(&rdt_group_mutex);
>> + if (!cpumask_test_and_clear_cpu(cpu, &rdt_cpumask)) {
>> + mutex_unlock(&rdt_group_mutex);
>> + return;
>> + }
>> +
>> + for_each_online_cpu(i) {
>> + if (i == cpu)
>> + continue;
>> +
>> + if (phys_id == topology_physical_package_id(i)) {
>> + cpumask_set_cpu(i, &rdt_cpumask);
>> + break;
>> + }
>> + }
>
> There is a better way than iterating over all online cpus ....
>
>>
>> +static struct cftype rdt_files[] = {
>> + {
>> + .name = "cache_mask",
>> + .seq_show = intel_cache_alloc_cbm_read,
>> + .write_u64 = intel_cache_alloc_cbm_write,
>> + .mode = 0666,
>> + },
>> + { } /* terminate */
>> +};
>
> Where is the Documentation for that file? What's the content, format,
> error codes ....

Will add section to rdt.txt in Documentation/cgroups - also will move the
documentation patch before all these patches..

>
> Sigh.
>
> tglx
>


\
 
 \ /
  Last update: 2015-05-18 22:01    [W:0.131 / U:1.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site