Messages in this thread | | | Date | Fri, 15 May 2015 21:18:34 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 2/7] x86/intel_rdt: Adds support for Class of service management |
| |
On Mon, 11 May 2015, Vikas Shivappa wrote: > arch/x86/include/asm/intel_rdt.h | 38 +++++++++++++ > arch/x86/kernel/cpu/intel_rdt.c | 112 +++++++++++++++++++++++++++++++++++++-- > include/linux/cgroup_subsys.h | 4 ++
Where is the Documentation for the new cgroup subsystem?
> +struct rdt_subsys_info { > + /* Clos Bitmap to keep track of available CLOSids.*/
If you want to document your struct members, please use KernelDoc formatting, so tools can pick it up as well.
> + unsigned long *closmap; > +}; > + > +struct intel_rdt { > + struct cgroup_subsys_state css;
Ditto
> + /* Class of service for the cgroup.*/ > + unsigned int clos; > +}; > + > +struct clos_cbm_map { > + unsigned long cache_mask; > + unsigned int clos_refcnt; > +};
> +/* > + * ccmap maintains 1:1 mapping between CLOSid and cache bitmask. > + */ > +static struct clos_cbm_map *ccmap; > +static struct rdt_subsys_info rdtss_info; > +static DEFINE_MUTEX(rdt_group_mutex); > +struct intel_rdt rdt_root_group; > + > +static void intel_rdt_free_closid(unsigned int clos) > +{ > + lockdep_assert_held(&rdt_group_mutex); > + > + clear_bit(clos, rdtss_info.closmap); > +} > + > +static void __clos_get(unsigned int closid)
What's the purpose of these double underscores?
> +{ > + struct clos_cbm_map *ccm = &ccmap[closid]; > + > + lockdep_assert_held(&rdt_group_mutex);
Do we really need all these lockdep asserts for a handfull of call sites?
> + ccm->clos_refcnt += 1;
What's wrong with:
ccmap[closid].clos_refcnt++;
Hmm?
> +} > + > +static void __clos_put(unsigned int closid) > +{ > + struct clos_cbm_map *ccm = &ccmap[closid]; > + > + lockdep_assert_held(&rdt_group_mutex); > + WARN_ON(!ccm->clos_refcnt);
So we have a warning but we do not act on it.
> + > + ccm->clos_refcnt -= 1; > + if (!ccm->clos_refcnt)
You can do that in a single line w/o the pointer magic. We want easy to read and small code rather than pointlessly blown up helper functions which just eat screen space.
> + intel_rdt_free_closid(closid); > +} > + > +static struct cgroup_subsys_state * > +intel_rdt_css_alloc(struct cgroup_subsys_state *parent_css) > +{ > + struct intel_rdt *parent = css_rdt(parent_css); > + struct intel_rdt *ir; > + > + /* > + * Cannot return failure on systems with no Cache Allocation > + * as the cgroup_init does not handle failures gracefully.
This comment is blatantly wrong. 5 lines further down you return -ENOMEM. Now what?
> + */ > + if (!parent) > + return &rdt_root_group.css; > + > + ir = kzalloc(sizeof(struct intel_rdt), GFP_KERNEL); > + if (!ir) > + return ERR_PTR(-ENOMEM); > + > + mutex_lock(&rdt_group_mutex); > + ir->clos = parent->clos; > + __clos_get(ir->clos); > + mutex_unlock(&rdt_group_mutex); > + > + return &ir->css; > +} > + > +static void intel_rdt_css_free(struct cgroup_subsys_state *css) > +{ > + struct intel_rdt *ir = css_rdt(css); > + > + mutex_lock(&rdt_group_mutex); > + __clos_put(ir->clos); > + kfree(ir); > + mutex_unlock(&rdt_group_mutex); > +} > > static int __init intel_rdt_late_init(void) > { > struct cpuinfo_x86 *c = &boot_cpu_data; > - int maxid, max_cbm_len; > + static struct clos_cbm_map *ccm; > + int maxid, max_cbm_len, err = 0; > + size_t sizeb; > > - if (!cpu_has(c, X86_FEATURE_CAT_L3)) > + if (!cpu_has(c, X86_FEATURE_CAT_L3)) { > + rdt_root_group.css.ss->disabled = 1; > return -ENODEV; > - > + } > maxid = c->x86_rdt_max_closid; > max_cbm_len = c->x86_rdt_max_cbm_len; > > + sizeb = BITS_TO_LONGS(maxid) * sizeof(long); > + rdtss_info.closmap = kzalloc(sizeb, GFP_KERNEL);
What's the point of this bitmap? In later patches you have a restriction to 64bits and the SDM says that CBM_LEN is limited to 32.
So where is the point for allocating a bitmap?
> + if (!rdtss_info.closmap) { > + err = -ENOMEM; > + goto out_err; > + } > + > + sizeb = maxid * sizeof(struct clos_cbm_map); > + ccmap = kzalloc(sizeb, GFP_KERNEL); > + if (!ccmap) { > + kfree(rdtss_info.closmap); > + err = -ENOMEM; > + goto out_err;
What's wrong with return -ENOMEM? We only use goto if we have to cleanup stuff, certainly not for just returning err.
> + } > + > + set_bit(0, rdtss_info.closmap); > + rdt_root_group.clos = 0; > + ccm = &ccmap[0]; > + bitmap_set(&ccm->cache_mask, 0, max_cbm_len); > + ccm->clos_refcnt = 1; > + > pr_info("Max bitmask length:%u,Max ClosIds: %u\n", max_cbm_len, maxid);
We surely do not want to sprinkle these all over dmesg.
+out_err:
- return 0; + return err;
Thanks,
tglx
| |