lkml.org 
[lkml]   [2016]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 08/33] x86/intel_rdt: Add Class of service management
    On Thu, 8 Sep 2016, Fenghua Yu wrote:
    > @@ -0,0 +1,12 @@
    > +#ifndef _RDT_H_
    > +#define _RDT_H_

    The standard guard is

    _ASM_X86_RDT_H

    > +
    > +#ifdef CONFIG_INTEL_RDT

    What's the purpose of sticking a struct definition inside an ifdef?

    > +
    > +struct clos_cbm_table {
    > + unsigned long cbm;
    > + unsigned int clos_refcnt;

    Please proper align the struct members for readability sake

    unsigned long cbm;
    unsigned int clos_refcnt;

    Adding a kerneldoc comment above the struct explaining it would not hurting
    either.

    > +/*
    > + * cctable maintains 1:1 mapping between CLOSid and cache bitmask.
    > + */
    > +static struct clos_cbm_table *cctable;
    > +/*
    > + * closid availability bit map.
    > + */
    > +unsigned long *closmap;
    > +static DEFINE_MUTEX(rdtgroup_mutex);
    > +
    > +static inline void closid_get(u32 closid)
    > +{
    > + struct clos_cbm_table *cct = &cctable[closid];
    > +
    > + lockdep_assert_held(&rdtgroup_mutex);
    > +
    > + cct->clos_refcnt++;

    So the whole thing can be written with a single line at the single call
    site.

    cc_table[closid].clos_refcnt++;

    And that already has the lockdep_assert_held() check. While companies might
    pay based on line counts, kernel development asks for readable and sensible
    code.

    > +}
    > +
    > +static int closid_alloc(u32 *closid)
    > +{
    > + u32 maxid;
    > + u32 id;

    Please put variables with the same type into one line. There is no value in
    wasting screen estate.

    > +
    > + lockdep_assert_held(&rdtgroup_mutex);
    > +
    > + maxid = boot_cpu_data.x86_cache_max_closid;
    > + id = find_first_zero_bit(closmap, maxid);
    > + if (id == maxid)
    > + return -ENOSPC;
    > +
    > + set_bit(id, closmap);
    > + closid_get(id);
    > + *closid = id;
    > +
    > + return 0;
    > +}
    > +
    > +static inline void closid_free(u32 closid)
    > +{
    > + clear_bit(closid, closmap);
    > + cctable[closid].cbm = 0;
    > +}
    > +
    > +static void closid_put(u32 closid)
    > +{
    > + struct clos_cbm_table *cct = &cctable[closid];
    > +
    > + lockdep_assert_held(&rdtgroup_mutex);
    > + if (WARN_ON(!cct->clos_refcnt))
    > + return;
    > +
    > + if (!--cct->clos_refcnt)
    > + closid_free(closid);
    > +}
    >
    > static int __init intel_rdt_late_init(void)
    > {
    > struct cpuinfo_x86 *c = &boot_cpu_data;
    > + u32 maxid;
    > + int err = 0, size;
    >
    > if (!cpu_has(c, X86_FEATURE_CAT_L3))
    > return -ENODEV;
    >
    > - pr_info("Intel cache allocation detected\n");
    > + maxid = c->x86_cache_max_closid;
    >
    > - return 0;
    > + size = maxid * sizeof(struct clos_cbm_table);
    > + cctable = kzalloc(size, GFP_KERNEL);
    > + if (!cctable) {
    > + err = -ENOMEM;
    > + goto out_err;

    I told you that before: Using a goto just to return err is pointless and
    silly. What the hell is wrong with

    return -ENOMEM;
    ???

    > + }
    > +
    > + size = BITS_TO_LONGS(maxid) * sizeof(long);
    > + closmap = kzalloc(size, GFP_KERNEL);
    > + if (!closmap) {
    > + kfree(cctable);
    > + err = -ENOMEM;
    > + goto out_err;

    Groan.

    > + }
    > +
    > + pr_info("Intel cache allocation enabled\n");
    > +out_err:

    \
     
     \ /
      Last update: 2016-09-17 09:58    [W:4.116 / U:0.520 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site