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