| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.DEB.2.11.1505152030400.4225@nanos>
Date: Fri, 15 May 2015 21:18:34 +0200 (CEST)
From: Thomas Gleixner <tglx@...utronix.de>
To: Vikas Shivappa <vikas.shivappa@...ux.intel.com>
cc: vikas.shivappa@...el.com, x86@...nel.org,
linux-kernel@...r.kernel.org, hpa@...or.com, mingo@...nel.org,
tj@...nel.org, peterz@...radead.org, matt.fleming@...el.com,
will.auld@...el.com, peter.zijlstra@...el.com,
h.peter.anvin@...el.com, kanaka.d.juvva@...el.com,
mtosatti@...hat.com
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists